Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
From: Knut Omang <knut.omang@oracle.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kbuild@vger.kernel.org,
	Shuah Khan <shuah@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Michal Marek <michal.lkml@markovi.net>,
	Shreyans Devendra Doshi <0xinfosect0r@gmail.com>,
	Alan Maguire <alan.maguire@oracle.com>,
	Brendan Higgins <brendanhiggins@google.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Hidenori Yamaji <hidenori.yamaji@sony.com>,
	Frank Rowand <frowand.list@gmail.com>,
	Timothy Bird <Tim.Bird@sony.com>,
	Luis Chamberlain <mcgrof@kernel.org>,
	"Theodore Ts'o" <tytso@mit.edu>, Daniel Vetter <daniel@ffwll.ch>,
	Stephen Boyd <sboyd@kernel.org>
Subject: Re: [RFC 06/19] ktf: A simple debugfs interface to test results
Date: Wed, 14 Aug 2019 19:17:07 +0200
Message-ID: <a63bea757e02656a38463cc794da7da15273dd16.camel@oracle.com> (raw)
In-Reply-To: <20190813082152.GA17627@kroah.com>

On Tue, 2019-08-13 at 10:21 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 13, 2019 at 08:09:21AM +0200, Knut Omang wrote:
> > From: Alan Maguire <alan.maguire@oracle.com>
> > 
> > While test results is available via netlink from user space, sometimes
> > it may be useful to be able to access the results from the kernel as well,
> > for instance due to a crash. Make that possible via debugfs.
> > 
> > ktf_debugfs.h:   Support for creating a debugfs representation of test
> > 
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  tools/testing/selftests/ktf/kernel/ktf_debugfs.c | 356 ++++++++++++++++-
> >  tools/testing/selftests/ktf/kernel/ktf_debugfs.h |  34 ++-
> >  2 files changed, 390 insertions(+)
> >  create mode 100644 tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> >  create mode 100644 tools/testing/selftests/ktf/kernel/ktf_debugfs.h
> > 
> > diff --git a/tools/testing/selftests/ktf/kernel/ktf_debugfs.c b/tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> > new file mode 100644
> > index 0000000..a20fbd2
> > --- /dev/null
> > +++ b/tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> > @@ -0,0 +1,356 @@
> > +/*
> > + * Copyright (c) 2009, 2017, Oracle and/or its affiliates. All rights reserved.
> > + *    Author: Alan Maguire <alan.maguire@oracle.com>
> > + *
> > + * SPDX-License-Identifier: GPL-2.0
> 
> Has to be the first line of the file, did you run this through
> checkpatch?

Yes, the code has been subject to continuous integration which uses 
a version of my runchecks tool (https://lkml.org/lkml/2018/1/19/157)
to ensure that it is not possible to "forget" to run checkpatch 
(or sparse, smatch doc.check for that sake)

Ironically though I fell victim to my own tooling here,
as I postponed fixing the SPDX_LICENSE_TAG class of issues 
once that test appeared, while working on something else, 
and just forgot to re-enable it again..

> > +static int ktf_run_test_open(struct inode *inode, struct file *file)
> > +{
> > +	struct ktf_test *t;
> > +
> > +	if (!try_module_get(THIS_MODULE))
> > +		return -EIO;
> 
> This is an anti-pattern, and one guaranteed to not work properly.  NEVER
> do this.

Sorry, I didn't know this, and the origin is probably my responsibility.

I know the feeling of never being able to get rid of bad examples 
because they keep getting copied..

The pattern seemed to be widely used the first time I saw it, and although 
somewhat awkward, it seemed to be the standard way then, but as you know, 
my Infiniband driver (
https://github.com/oracle/linux-uek/blob/uek4/qu7/drivers/infiniband/hw/sif/sif_debug.c)
unfortunately never made it to the scrutiny of LKML, since the hardware project 
got cancelled.
The -EIO return value was also copied from merged kernel code back then.

I notice the discussion and your response here: 
http://linux-kernel.2935.n7.nabble.com/debugfs-and-module-unloading-td865175.html
I assume that means that protection against module unload while a debugfs file
is open is now safe.

On older kernels, having this code in place is far better than an unprotected 
debugfs entry/exit - I have tested it extensively in the past :-)

Back when I first used it, I had this cool set of polymorphic 
debugfs file code to list the set of active MRs, CQs, QPs, AHs etc 
that the whole infiniband driver, database and hardware teams loved 
so much that multiple users ended up using it in multiple windows 
from within watch for live observations of state changes, 
and often also running driver load/unloads for testing purposes.

I perfectly agree with you that reducing the hole for a race condition 
is generally a bad idea, but from the above mail thread 
it seems that's the only available choice for older kernels?

(I am asking because I still want to be able to support rather 
old kernels with the github version of KTF)

Anyway, great to know that a better solution now exists!

We'll fix the rest of the issues below as well for the next version..

Thanks!
Knut

> > +
> > +	t = (struct ktf_test *)inode->i_private;
> > +
> > +	return single_open(file, ktf_debugfs_run, t);
> > +}
> > +
> > +static int ktf_debugfs_release(struct inode *inode, struct file *file)
> > +{
> > +	module_put(THIS_MODULE);
> 
> Same here, not ok.
> 
> 
> > +	return single_release(inode, file);
> > +}
> > +
> > +static const struct file_operations ktf_run_test_fops = {
> > +	.open = ktf_run_test_open,
> > +	.read = seq_read,
> > +	.llseek = seq_lseek,
> > +	.release = ktf_debugfs_release,
> > +};
> > +
> > +static int ktf_results_test_open(struct inode *inode, struct file *file)
> > +{
> > +	struct ktf_test *t;
> > +
> > +	if (!try_module_get(THIS_MODULE))
> > +		return -EIO;
> 
> Nope!
> 
> And why -EIO?  That is not an io issue.

Agreed
> 
> > +void ktf_debugfs_create_test(struct ktf_test *t)
> > +{
> > +	struct ktf_case *testset = ktf_case_find(t->tclass);
> > +
> > +	if (!testset)
> > +		return;
> > +
> > +	memset(&t->debugfs, 0, sizeof(t->debugfs));
> > +
> > +	t->debugfs.debugfs_results_test =
> > +		debugfs_create_file(t->name, S_IFREG | 0444,
> > +				    testset->debugfs.debugfs_results_test,
> > +				 t, &ktf_results_test_fops);
> > +
> > +	if (t->debugfs.debugfs_results_test) {
> 
> How can that variable ever be NULL (hint, it can not.)
> 
> > +		t->debugfs.debugfs_run_test =
> > +			debugfs_create_file(t->name, S_IFREG | 0444,
> > +					    testset->debugfs.debugfs_run_test,
> > +				 t, &ktf_run_test_fops);
> > +		if (!t->debugfs.debugfs_run_test) {
> > +			_ktf_debugfs_destroy_test(t);
> > +		} else {
> > +			/* Take reference for test for debugfs */
> > +			ktf_test_get(t);
> > +		}
> > +	}
> 
> Never test the result of any debugfs call, you do not need to.  Just
> call it and move on, your code flow should NEVER be different with, or
> without, a successful debugfs call.
> 
> 
> > +static int ktf_run_testset_open(struct inode *inode, struct file *file)
> > +{
> > +	struct ktf_case *testset;
> > +
> > +	if (!try_module_get(THIS_MODULE))
> > +		return -EIO;
> 
> Again no.  I hate to know what code you copied this all from, as that
> code is very wrong.  Do you have a pointer to that code anywhere so we
> can fix that up?
> 
> > +
> > +	testset = (struct ktf_case *)inode->i_private;
> > +
> > +	return single_open(file, ktf_debugfs_run_all, testset);
> > +}
> > +
> > +static const struct file_operations ktf_run_testset_fops = {
> > +	.open = ktf_run_testset_open,
> > +	.read = seq_read,
> > +	.llseek = seq_lseek,
> > +	.release = ktf_debugfs_release,
> 
> If you really care about module references you should be setting the
> owner of the module here.
> 
> > +};
> > +
> > +static void _ktf_debugfs_destroy_testset(struct ktf_case *testset)
> > +{
> > +	debugfs_remove(testset->debugfs.debugfs_run_testset);
> > +	debugfs_remove(testset->debugfs.debugfs_run_test);
> > +	debugfs_remove(testset->debugfs.debugfs_results_testset);
> > +	debugfs_remove(testset->debugfs.debugfs_results_test);
> 
> Why not just recursivly remove the directory?  That way you do not have
> to keep track of any individual files.
> 
> 
> > +}
> > +
> > +void ktf_debugfs_create_testset(struct ktf_case *testset)
> > +{
> > +	char tests_subdir[KTF_DEBUGFS_NAMESZ];
> > +	const char *name = ktf_case_name(testset);
> > +
> > +	memset(&testset->debugfs, 0, sizeof(testset->debugfs));
> > +
> > +	/* First add /sys/kernel/debug/ktf/[results|run]/<testset> */
> > +	testset->debugfs.debugfs_results_testset =
> > +		debugfs_create_file(name, S_IFREG | 0444,
> > +				    ktf_debugfs_resultsdir,
> > +				 testset, &ktf_results_testset_fops);
> > +	if (!testset->debugfs.debugfs_results_testset)
> > +		goto err;
> 
> Again, can never happen, and again, do not do different things depending
> on the result of a debugfs call.
> 
> > +
> > +	testset->debugfs.debugfs_run_testset =
> > +		debugfs_create_file(name, S_IFREG | 0444,
> > +				    ktf_debugfs_rundir,
> > +				    testset, &ktf_run_testset_fops);
> > +	if (!testset->debugfs.debugfs_run_testset)
> > +		goto err;
> 
> Again, nope.
> 
> > +
> > +	/* Now add parent directories for individual test result/run tests
> > +	 * which live in
> > +	 * /sys/kernel/debug/ktf/[results|run]/<testset>-tests/<testname>
> > +	 */
> > +	(void)snprintf(tests_subdir, sizeof(tests_subdir), "%s%s",
> > +			name, KTF_DEBUGFS_TESTS_SUFFIX);
> 
> why (void)?
> 
> 
> > +
> > +	testset->debugfs.debugfs_results_test =
> > +		debugfs_create_dir(tests_subdir, ktf_debugfs_resultsdir);
> > +	if (!testset->debugfs.debugfs_results_test)
> > +		goto err;
> 
> nope :)
> 
> > +
> > +	testset->debugfs.debugfs_run_test =
> > +		debugfs_create_dir(tests_subdir, ktf_debugfs_rundir);
> > +	if (!testset->debugfs.debugfs_run_test)
> > +		goto err;
> 
> Nope :)
> 
> > +
> > +	/* Take reference count for testset.  One will do as we will always
> > +	 * free testset debugfs resources together.
> > +	 */
> > +	ktf_case_get(testset);
> > +	return;
> > +err:
> > +	_ktf_debugfs_destroy_testset(testset);
> > +}
> > +
> > +void ktf_debugfs_destroy_testset(struct ktf_case *testset)
> > +{
> > +	tlog(T_DEBUG, "Destroying debugfs testset %s", ktf_case_name(testset));
> > +	_ktf_debugfs_destroy_testset(testset);
> > +	/* Remove our debugfs reference cout to testset */
> > +	ktf_case_put(testset);
> > +}
> > +
> > +/* /sys/kernel/debug/ktf/coverage shows coverage statistics. */
> > +static int ktf_debugfs_cov(struct seq_file *seq, void *v)
> > +{
> > +	ktf_cov_seq_print(seq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ktf_cov_open(struct inode *inode, struct file *file)
> > +{
> > +	if (!try_module_get(THIS_MODULE))
> > +		return -EIO;
> 
> {sigh}  I'll stop reviewing now :)


> 
> thanks,
> 
> greg k-h





  reply index

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-13  6:09 [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree Knut Omang
2019-08-13  6:09 ` [RFC 01/19] kbuild: Fixes to rules for host-cshlib and host-cxxshlib Knut Omang
2019-08-13 14:01   ` Masahiro Yamada
2019-08-13 16:19     ` Knut Omang
2019-08-14  2:02       ` Masahiro Yamada
2019-08-14  5:50         ` Knut Omang
2019-08-14  5:52         ` Knut Omang
2019-08-14 12:52           ` Knut Omang
2019-08-21  1:47             ` Masahiro Yamada
2019-08-21  4:03               ` Knut Omang
2019-08-13  6:09 ` [RFC 02/19] ktf: Introduce the main part of the kernel side of ktf Knut Omang
2019-09-09  1:23   ` Brendan Higgins
2019-09-10  6:15     ` Knut Omang
2019-08-13  6:09 ` [RFC 03/19] ktf: Introduce a generic netlink protocol for test result communication Knut Omang
2019-09-09  1:28   ` Brendan Higgins
2019-09-10  6:30     ` Knut Omang
2019-08-13  6:09 ` [RFC 04/19] ktf: An implementation of a generic associative array container Knut Omang
2019-08-13  6:09 ` [RFC 05/19] ktf: Implementation of ktf support for overriding function entry and return Knut Omang
2019-08-13  6:09 ` [RFC 06/19] ktf: A simple debugfs interface to test results Knut Omang
2019-08-13  8:21   ` Greg Kroah-Hartman
2019-08-14 17:17     ` Knut Omang [this message]
2019-08-15  8:49       ` Greg Kroah-Hartman
2019-08-15 10:35         ` Knut Omang
2019-08-15 10:52           ` Greg Kroah-Hartman
2019-08-13  6:09 ` [RFC 07/19] ktf: Simple coverage support Knut Omang
2019-08-13  6:09 ` [RFC 08/19] ktf: Configurable context support for network info setup Knut Omang
2019-08-13  6:09 ` [RFC 09/19] ktf: resolve: A helper utility to aid in exposing private kernel symbols to KTF tests Knut Omang
2019-08-13  6:09 ` [RFC 10/19] ktf: Add documentation for Kernel Test Framework (KTF) Knut Omang
2019-08-13  6:09 ` [RFC 11/19] ktf: Add a small test suite with a few tests to test KTF itself Knut Omang
2019-08-13  6:09 ` [RFC 12/19] ktf: Main part of user land library for executing tests Knut Omang
2019-08-13  6:09 ` [RFC 13/19] ktf: Integration logic for running ktf tests from googletest Knut Omang
2019-08-13  6:09 ` [RFC 14/19] ktf: Internal debugging facilities Knut Omang
2019-08-13  6:09 ` [RFC 15/19] ktf: Some simple examples Knut Omang
2019-08-13  6:09 ` [RFC 16/19] ktf: Some user applications to run tests Knut Omang
2019-08-13  6:09 ` [RFC 17/19] ktf: Toplevel ktf Makefile/makefile includes and scripts to run from kselftest Knut Omang
2019-08-13  6:09 ` [RFC 18/19] kselftests: Enable building ktf Knut Omang
2019-08-13  6:09 ` [RFC 19/19] Documentation/dev-tools: Add index entry for KTF documentation Knut Omang
2019-08-13  8:10 ` [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree Brendan Higgins
2019-08-13  8:17 ` Brendan Higgins
2019-08-13 11:29   ` Knut Omang
2019-08-13 17:50     ` Brendan Higgins
2019-08-13  8:23 ` Greg Kroah-Hartman
2019-08-13  9:51   ` Knut Omang
2019-08-13 17:02     ` Brendan Higgins

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a63bea757e02656a38463cc794da7da15273dd16.camel@oracle.com \
    --to=knut.omang@oracle.com \
    --cc=0xinfosect0r@gmail.com \
    --cc=Tim.Bird@sony.com \
    --cc=alan.maguire@oracle.com \
    --cc=brendanhiggins@google.com \
    --cc=corbet@lwn.net \
    --cc=daniel@ffwll.ch \
    --cc=frowand.list@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hidenori.yamaji@sony.com \
    --cc=khilman@baylibre.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=michal.lkml@markovi.net \
    --cc=sboyd@kernel.org \
    --cc=shuah@kernel.org \
    --cc=tytso@mit.edu \
    --cc=yamada.masahiro@socionext.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org linux-kselftest@archiver.kernel.org
	public-inbox-index linux-kselftest


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/ public-inbox