All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v9 0/6] test_sysfs: add new selftest for sysfs
@ 2021-10-29 18:44 Luis Chamberlain
  2021-10-29 18:44 ` [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license Luis Chamberlain
                   ` (5 more replies)
  0 siblings, 6 replies; 36+ messages in thread
From: Luis Chamberlain @ 2021-10-29 18:44 UTC (permalink / raw)
  To: tj, gregkh, akpm, jeyu, shuah
  Cc: bvanassche, dan.j.williams, joe, tglx, mcgrof, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel

On this v9 I've dropped the generic sysfs deadlock fix given Ming Lei
has provided alternative fixes for the zram driver without incurring
a generic lock *and* we don't yet have full assessment of how wide
spread the deadlock case might be in the kernel. A full assessment
effort is still underway using Coccinelle with iteration support,
however that effort will take a bit more time to complete. We can
re-evaluate the value of a generic fix later after the assessment
is complete.

This series now just adds the test_sysfs selftest and failure injection
support for it on kernfs. The most valuable tests are those which
confirm that once a kernfs active reference is obtained with
kernfs_get_active() the pointers used there are still valid, and so
using sysfs ops *are* safe if we race against module removal. Likewise
it also confirms how module removal will *wait* for these ops to
complete if a kernfs node is already active.

This v9 series also addresses feedback mostly provided by Kees Cook and Greg.
I also made a few changes to the test_sysfs driver to account for changes in
the block layer. I also improved the kernfs failure injection tests with
documentation of how they work and to account for the real expected return
value of a write before the kernfs active reference is obtained. Upstream
commit 8e141f9eb803e ("block: drain file system I/O on del_gendisk") has
revealed that small minor induced delays on del_gendisk() can make a few
writes succeed if the delays used are small. So we clarify the logic of why
writes could either fail or succeed before the kernfs active reference is taken.

These changes also availble on this tree:

https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20211029-test-sysfs-v2

v9:
  * rebased onto linux-next tag next-20211029
  * add Reviewed-by tags for the SPDX change, and the drivers which
    get the tag for it
  * drop the generic sysfs deadlock fix for now as the scope of how
    wide spread the issue is still needs to be assessed
  * drop the zram patches as they are replaced by Ming Lei's fixes
  * drop already merged patches
  * try_module_get() docs: enhanced using feedback from Kees Cook. I
    extended the documention to make it clear that if proper care is not
    taken the use of this routine could crash the kernel.
  * kernfs: move failure injection knobs under /sys/kernel/debug/fail_kernfs
    as suggested by Kees Cook
  * kernfs: rename failure injection file to fault_inject.c as suggested
    by Kees Cook
  * kernfs: split up documentation of failure injection knobs as
    suggested by Kees Cook
  * kernfs: move the wait into debug call, and use a simple one liner
    may_wait() calls to make the changes much less intrusive and more
    readable  as suggested by Kees Cook 
  * kernfs: drop __func__ uses as suggested by Kees Cook
  * test_sysfs: use sizeof() instead of open coded 16 as suggested by
    Kees Cook
  * test_sysfs: use sysfs_emit as suggested by Kees Cook
  * test_sysfs: drop boiler place license as suggested by Greg KH
  * test_sysfs: use depends instead of select as suggested by Kees Cook
  * test_sysfs: drop #ifdefery as suggested by Kees Cook
  * test_sysfs: clarified that the use of a lock on rmmod which causes
    a deadlock is something drivers should avoid, and its why we leave
    the test disabled.
  * test_sysfs: now that device_add_disk() returns an error, use the
    new error return code, otherwise this is going to prevent us from
    eventually embracing __must_check() on that call on the block layer.
  * test_syfs: testdev_submit_bio() needed to change data types as now
    it returns void.
  * test_sysfs: enhance kernfs failure injection tests with documenation
    and correct the expected return value for writes

Luis Chamberlain (6):
  LICENSES: Add the copyleft-next-0.3.1 license
  testing: use the copyleft-next-0.3.1 SPDX tag
  selftests: add tests_sysfs module
  kernfs: add initial failure injection support
  test_sysfs: add support to use kernfs failure injection
  kernel/module: add documentation for try_module_get()

 .../fault-injection/fault-injection.rst       |   50 +
 LICENSES/dual/copyleft-next-0.3.1             |  237 +++
 MAINTAINERS                                   |    9 +-
 fs/kernfs/Makefile                            |    1 +
 fs/kernfs/fault_inject.c                      |   93 ++
 fs/kernfs/file.c                              |    9 +
 fs/kernfs/kernfs-internal.h                   |   70 +
 include/linux/kernfs.h                        |    5 +
 include/linux/module.h                        |   37 +-
 lib/Kconfig.debug                             |   23 +
 lib/Makefile                                  |    1 +
 lib/test_kmod.c                               |   12 +-
 lib/test_sysctl.c                             |   12 +-
 lib/test_sysfs.c                              |  913 +++++++++++
 tools/testing/selftests/kmod/kmod.sh          |   13 +-
 tools/testing/selftests/sysctl/sysctl.sh      |   12 +-
 tools/testing/selftests/sysfs/Makefile        |   12 +
 tools/testing/selftests/sysfs/config          |    5 +
 tools/testing/selftests/sysfs/settings        |    1 +
 tools/testing/selftests/sysfs/sysfs.sh        | 1411 +++++++++++++++++
 20 files changed, 2878 insertions(+), 48 deletions(-)
 create mode 100644 LICENSES/dual/copyleft-next-0.3.1
 create mode 100644 fs/kernfs/fault_inject.c
 create mode 100644 lib/test_sysfs.c
 create mode 100644 tools/testing/selftests/sysfs/Makefile
 create mode 100644 tools/testing/selftests/sysfs/config
 create mode 100644 tools/testing/selftests/sysfs/settings
 create mode 100755 tools/testing/selftests/sysfs/sysfs.sh

-- 
2.30.2


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2021-10-29 18:44 [PATCH v9 0/6] test_sysfs: add new selftest for sysfs Luis Chamberlain
@ 2021-10-29 18:44 ` Luis Chamberlain
  2022-05-23 21:10   ` Thomas Gleixner
                     ` (2 more replies)
  2021-10-29 18:44 ` [PATCH v9 2/6] testing: use the copyleft-next-0.3.1 SPDX tag Luis Chamberlain
                   ` (4 subsequent siblings)
  5 siblings, 3 replies; 36+ messages in thread
From: Luis Chamberlain @ 2021-10-29 18:44 UTC (permalink / raw)
  To: tj, gregkh, akpm, jeyu, shuah
  Cc: bvanassche, dan.j.williams, joe, tglx, mcgrof, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	Richard Fontana, copyleft-next, Ciaran Farrell,
	Christopher De Nicolo, Christoph Hellwig, Jonathan Corbet,
	Thorsten Leemhuis

Add the full text of the copyleft-next-0.3.1 license to the kernel
tree as well as the required tags for reference and tooling.
The license text was copied directly from the copyleft-next project's
git tree [0].

Discussion of using copyleft-next-0.3.1 on Linux started since June,
2016 [1]. In the end Linus' preference was to have drivers use
MODULE_LICENSE("GPL") to make it clear that the GPL applies when it
comes to Linux [2]. Additionally, even though copyleft-next-0.3.1 has
been found to be to be GPLv2 compatible by three attorneys at SUSE and
Redhat [3], to err on the side of caution we simply recommend to
always use the "OR" language for this license [4].

Even though it has been a goal of the project to be GPL-v2 compatible
to be certain in 2016 I asked for a clarification about what makes
copyleft-next GPLv2 compatible and also asked for a summary of
benefits. This prompted some small minor changes to make compatibility
even further clear and as of copyleft 0.3.1 compatibility should
be crystal clear [5].

The summary of why copyleft-next 0.3.1 is compatible with GPLv2
is explained as follows:

  Like GPLv2, copyleft-next requires distribution of derivative works
  ("Derived Works" in copyleft-next 0.3.x) to be under the same license.
  Ordinarily this would make the two licenses incompatible. However,
  copyleft-next 0.3.1 says: "If the Derived Work includes material
  licensed under the GPL, You may instead license the Derived Work under
  the GPL." "GPL" is defined to include GPLv2.

In practice this means copyleft-next code in Linux may be licensed
under the GPL2, however there are additional obvious gains for
bringing contributions from Linux outbound where copyleft-next is
preferred. A summary of benefits why projects outside of Linux might
prefer to use copyleft-next >= 0.3.1 over GPLv2:

o It is much shorter and simpler
o It has an explicit patent license grant, unlike GPLv2
o Its notice preservation conditions are clearer
o More free software/open source licenses are compatible
  with it (via section 4)
o The source code requirement triggered by binary distribution
  is much simpler in a procedural sense
o Recipients potentially have a contract claim against distributors
  who are noncompliant with the source code requirement
o There is a built-in inbound=outbound policy for upstream
  contributions (cf. Apache License 2.0 section 5)
o There are disincentives to engage in the controversial practice
  of copyleft/ proprietary dual-licensing
o In 15 years copyleft expires, which can be advantageous
  for legacy code
o There are explicit disincentives to bringing patent infringement
  claims accusing the licensed work of infringement (see 10b)
o There is a cure period for licensees who are not compliant
  with the license (there is no cure opportunity in GPLv2)
o copyleft-next has a 'built-in or-later' provision

The first driver submission to Linux under this dual strategy was
lib/test_sysctl.c through commit 9308f2f9e7f05 ("test_sysctl: add
dedicated proc sysctl test driver") merged in July 2017. Shortly after
that I also added test_kmod through commit d9c6a72d6fa29 ("kmod: add
test driver to stress test the module loader") in the same month. These
two drivers went in just a few months before the SPDX license practice
kicked in. In 2018 Kuno Woudt went through the process to get SPDX
identifiers for copyleft-next [6] [7]. Although there are SPDX tags
for copyleft-next-0.3.0, we only document use in Linux starting from
copyleft-next-0.3.1 which makes GPLv2 compatibility crystal clear.

This patch will let us update the two Linux selftest drivers in
subsequent patches with their respective SPDX license identifiers and
let us remove repetitive license boiler plate.

[0] https://github.com/copyleft-next/copyleft-next/blob/master/Releases/copyleft-next-0.3.1
[1] https://lore.kernel.org/lkml/1465929311-13509-1-git-send-email-mcgrof@kernel.org/
[2] https://lore.kernel.org/lkml/CA+55aFyhxcvD+q7tp+-yrSFDKfR0mOHgyEAe=f_94aKLsOu0Og@mail.gmail.com/
[3] https://lore.kernel.org/lkml/20170516232702.GL17314@wotan.suse.de/
[4] https://lkml.kernel.org/r/1495234558.7848.122.camel@linux.intel.com
[5] https://lists.fedorahosted.org/archives/list/copyleft-next@lists.fedorahosted.org/thread/JTGV56DDADWGKU7ZKTZA4DLXTGTLNJ57/#SQMDIKBRAVDOCT4UVNOOCRGBN2UJIKHZ
[6] https://spdx.org/licenses/copyleft-next-0.3.0.html
[7] https://spdx.org/licenses/copyleft-next-0.3.1.html

Cc: Goldwyn Rodrigues <rgoldwyn@suse.com>
Cc: Kuno Woudt <kuno@frob.nl>
Cc: Richard Fontana <fontana@sharpeleven.org>
Cc: copyleft-next@lists.fedorahosted.org
Cc: Ciaran Farrell <Ciaran.Farrell@suse.com>
Cc: Christopher De Nicolo <Christopher.DeNicolo@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Thorsten Leemhuis <linux@leemhuis.info>
Cc: Andrew Morton <akpm@linux-foundation.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 LICENSES/dual/copyleft-next-0.3.1 | 237 ++++++++++++++++++++++++++++++
 1 file changed, 237 insertions(+)
 create mode 100644 LICENSES/dual/copyleft-next-0.3.1

diff --git a/LICENSES/dual/copyleft-next-0.3.1 b/LICENSES/dual/copyleft-next-0.3.1
new file mode 100644
index 000000000000..086bcb74b478
--- /dev/null
+++ b/LICENSES/dual/copyleft-next-0.3.1
@@ -0,0 +1,237 @@
+Valid-License-Identifier: copyleft-next-0.3.1
+SPDX-URL: https://spdx.org/licenses/copyleft-next-0.3.1
+Usage-Guide:
+  This license can be used in code, it has been found to be GPLv2 compatible
+  by attorneys at Redhat and SUSE, however to air on the side of caution,
+  it's best to only use it together with a GPL2 compatible license using "OR".
+  To use the copyleft-next-0.3.1 license put the following SPDX tag/value
+  pair into a comment according to the placement guidelines in the
+  licensing rules documentation:
+    SPDX-License-Identifier: GPL-2.0 OR copyleft-next-0.3.1
+    SPDX-License-Identifier: GPL-2.0-only OR copyleft-next 0.3.1
+    SPDX-License-Identifier: GPL-2.0+ OR copyleft-next-0.3.1
+    SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
+License-Text:
+
+=======================================================================
+
+                      copyleft-next 0.3.1 ("this License")
+                            Release date: 2016-04-29
+
+1. License Grants; No Trademark License
+
+   Subject to the terms of this License, I grant You:
+
+   a) A non-exclusive, worldwide, perpetual, royalty-free, irrevocable
+      copyright license, to reproduce, Distribute, prepare derivative works
+      of, publicly perform and publicly display My Work.
+
+   b) A non-exclusive, worldwide, perpetual, royalty-free, irrevocable
+      patent license under Licensed Patents to make, have made, use, sell,
+      offer for sale, and import Covered Works.
+
+   This License does not grant any rights in My name, trademarks, service
+   marks, or logos.
+
+2. Distribution: General Conditions
+
+   You may Distribute Covered Works, provided that You (i) inform
+   recipients how they can obtain a copy of this License; (ii) satisfy the
+   applicable conditions of sections 3 through 6; and (iii) preserve all
+   Legal Notices contained in My Work (to the extent they remain
+   pertinent). "Legal Notices" means copyright notices, license notices,
+   license texts, and author attributions, but does not include logos,
+   other graphical images, trademarks or trademark legends.
+
+3. Conditions for Distributing Derived Works; Outbound GPL Compatibility
+
+   If You Distribute a Derived Work, You must license the entire Derived
+   Work as a whole under this License, with prominent notice of such
+   licensing. This condition may not be avoided through such means as
+   separate Distribution of portions of the Derived Work.
+
+   If the Derived Work includes material licensed under the GPL, You may
+   instead license the Derived Work under the GPL.
+   
+4. Condition Against Further Restrictions; Inbound License Compatibility
+
+   When Distributing a Covered Work, You may not impose further
+   restrictions on the exercise of rights in the Covered Work granted under
+   this License. This condition is not excused merely because such
+   restrictions result from Your compliance with conditions or obligations
+   extrinsic to this License (such as a court order or an agreement with a
+   third party).
+
+   However, You may Distribute a Covered Work incorporating material
+   governed by a license that is both OSI-Approved and FSF-Free as of the
+   release date of this License, provided that compliance with such
+   other license would not conflict with any conditions stated in other
+   sections of this License.
+
+5. Conditions for Distributing Object Code
+
+   You may Distribute an Object Code form of a Covered Work, provided that
+   you accompany the Object Code with a URL through which the Corresponding
+   Source is made available, at no charge, by some standard or customary
+   means of providing network access to source code.
+
+   If you Distribute the Object Code in a physical product or tangible
+   storage medium ("Product"), the Corresponding Source must be available
+   through such URL for two years from the date of Your most recent
+   Distribution of the Object Code in the Product. However, if the Product
+   itself contains or is accompanied by the Corresponding Source (made
+   available in a customarily accessible manner), You need not also comply
+   with the first paragraph of this section.
+
+   Each direct and indirect recipient of the Covered Work from You is an
+   intended third-party beneficiary of this License solely as to this
+   section 5, with the right to enforce its terms.
+
+6. Symmetrical Licensing Condition for Upstream Contributions
+
+   If You Distribute a work to Me specifically for inclusion in or
+   modification of a Covered Work (a "Patch"), and no explicit licensing
+   terms apply to the Patch, You license the Patch under this License, to
+   the extent of Your copyright in the Patch. This condition does not
+   negate the other conditions of this License, if applicable to the Patch.
+
+7. Nullification of Copyleft/Proprietary Dual Licensing
+
+   If I offer to license, for a fee, a Covered Work under terms other than
+   a license that is OSI-Approved or FSF-Free as of the release date of this
+   License or a numbered version of copyleft-next released by the
+   Copyleft-Next Project, then the license I grant You under section 1 is no
+   longer subject to the conditions in sections 3 through 5.
+
+8. Copyleft Sunset
+
+   The conditions in sections 3 through 5 no longer apply once fifteen
+   years have elapsed from the date of My first Distribution of My Work
+   under this License.
+
+9. Pass-Through
+
+   When You Distribute a Covered Work, the recipient automatically receives
+   a license to My Work from Me, subject to the terms of this License.
+
+10. Termination
+
+    Your license grants under section 1 are automatically terminated if You
+
+    a) fail to comply with the conditions of this License, unless You cure
+       such noncompliance within thirty days after becoming aware of it, or
+
+    b) initiate a patent infringement litigation claim (excluding
+       declaratory judgment actions, counterclaims, and cross-claims)
+       alleging that any part of My Work directly or indirectly infringes
+       any patent.
+
+    Termination of Your license grants extends to all copies of Covered
+    Works You subsequently obtain. Termination does not terminate the
+    rights of those who have received copies or rights from You subject to
+    this License.
+
+    To the extent permission to make copies of a Covered Work is necessary
+    merely for running it, such permission is not terminable.
+
+11. Later License Versions
+
+    The Copyleft-Next Project may release new versions of copyleft-next,
+    designated by a distinguishing version number ("Later Versions").
+    Unless I explicitly remove the option of Distributing Covered Works
+    under Later Versions, You may Distribute Covered Works under any Later
+    Version.
+
+** 12. No Warranty                                                       **
+**                                                                       **
+**     My Work is provided "as-is", without warranty. You bear the risk  **
+**     of using it. To the extent permitted by applicable law, each      **
+**     Distributor of My Work excludes the implied warranties of title,  **
+**     merchantability, fitness for a particular purpose and             **
+**     non-infringement.                                                 **
+
+** 13. Limitation of Liability                                           **
+**                                                                       **
+**     To the extent permitted by applicable law, in no event will any   **
+**     Distributor of My Work be liable to You for any damages           **
+**     whatsoever, whether direct, indirect, special, incidental, or     **
+**     consequential damages, whether arising under contract, tort       **
+**     (including negligence), or otherwise, even where the Distributor  **
+**     knew or should have known about the possibility of such damages.  **
+
+14. Severability
+
+    The invalidity or unenforceability of any provision of this License
+    does not affect the validity or enforceability of the remainder of
+    this License. Such provision is to be reformed to the minimum extent
+    necessary to make it valid and enforceable.
+
+15. Definitions
+
+    "Copyleft-Next Project" means the project that maintains the source
+    code repository at <https://github.com/copyleft-next/copyleft-next.git/>
+    as of the release date of this License.
+
+    "Corresponding Source" of a Covered Work in Object Code form means (i)
+    the Source Code form of the Covered Work; (ii) all scripts,
+    instructions and similar information that are reasonably necessary for
+    a skilled developer to generate such Object Code from the Source Code
+    provided under (i); and (iii) a list clearly identifying all Separate
+    Works (other than those provided in compliance with (ii)) that were
+    specifically used in building and (if applicable) installing the
+    Covered Work (for example, a specified proprietary compiler including
+    its version number). Corresponding Source must be machine-readable.
+
+    "Covered Work" means My Work or a Derived Work.
+
+    "Derived Work" means a work of authorship that copies from, modifies,
+    adapts, is based on, is a derivative work of, transforms, translates or
+    contains all or part of My Work, such that copyright permission is
+    required. The following are not Derived Works: (i) Mere Aggregation;
+    (ii) a mere reproduction of My Work; and (iii) if My Work fails to
+    explicitly state an expectation otherwise, a work that merely makes
+    reference to My Work.
+
+    "Distribute" means to distribute, transfer or make a copy available to
+    someone else, such that copyright permission is required.
+
+    "Distributor" means Me and anyone else who Distributes a Covered Work.
+
+    "FSF-Free" means classified as 'free' by the Free Software Foundation.
+
+    "GPL" means a version of the GNU General Public License or the GNU
+    Affero General Public License.
+
+    "I"/"Me"/"My" refers to the individual or legal entity that places My
+    Work under this License. "You"/"Your" refers to the individual or legal
+    entity exercising rights in My Work under this License. A legal entity
+    includes each entity that controls, is controlled by, or is under
+    common control with such legal entity. "Control" means (a) the power to
+    direct the actions of such legal entity, whether by contract or
+    otherwise, or (b) ownership of more than fifty percent of the
+    outstanding shares or beneficial ownership of such legal entity.
+
+    "Licensed Patents" means all patent claims licensable royalty-free by
+    Me, now or in the future, that are necessarily infringed by making,
+    using, or selling My Work, and excludes claims that would be infringed
+    only as a consequence of further modification of My Work.
+
+    "Mere Aggregation" means an aggregation of a Covered Work with a
+    Separate Work.
+
+    "My Work" means the particular work of authorship I license to You
+    under this License.
+
+    "Object Code" means any form of a work that is not Source Code.
+
+    "OSI-Approved" means approved as 'Open Source' by the Open Source
+    Initiative.
+
+    "Separate Work" means a work that is separate from and independent of a
+    particular Covered Work and is not by its nature an extension or
+    enhancement of the Covered Work, and/or a runtime library, standard
+    library or similar component that is used to generate an Object Code
+    form of a Covered Work.
+
+    "Source Code" means the preferred form of a work for making
+    modifications to it.
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v9 2/6] testing: use the copyleft-next-0.3.1 SPDX tag
  2021-10-29 18:44 [PATCH v9 0/6] test_sysfs: add new selftest for sysfs Luis Chamberlain
  2021-10-29 18:44 ` [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license Luis Chamberlain
@ 2021-10-29 18:44 ` Luis Chamberlain
  2021-10-29 18:44 ` [PATCH v9 3/6] selftests: add tests_sysfs module Luis Chamberlain
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2021-10-29 18:44 UTC (permalink / raw)
  To: tj, gregkh, akpm, jeyu, shuah
  Cc: bvanassche, dan.j.williams, joe, tglx, mcgrof, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	Richard Fontana, copyleft-next, Ciaran Farrell,
	Christopher De Nicolo, Christoph Hellwig, Jonathan Corbet,
	Thorsten Leemhuis

Two selftests drivers exist under the copyleft-next license.
These drivers were added prior to SPDX practice taking full swing
in the kernel. Now that we have an SPDX tag for copylef-next-0.3.1
documented, embrace it and remove the boiler plate.

Cc: Goldwyn Rodrigues <rgoldwyn@suse.com>
Cc: Kuno Woudt <kuno@frob.nl>
Cc: Richard Fontana <fontana@sharpeleven.org>
Cc: copyleft-next@lists.fedorahosted.org
Cc: Ciaran Farrell <Ciaran.Farrell@suse.com>
Cc: Christopher De Nicolo <Christopher.DeNicolo@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Thorsten Leemhuis <linux@leemhuis.info>
Cc: Andrew Morton <akpm@linux-foundation.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 lib/test_kmod.c                          | 12 +-----------
 lib/test_sysctl.c                        | 12 +-----------
 tools/testing/selftests/kmod/kmod.sh     | 13 +------------
 tools/testing/selftests/sysctl/sysctl.sh | 12 +-----------
 4 files changed, 4 insertions(+), 45 deletions(-)

diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index ce1589391413..d62afd89dc63 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -1,18 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
 /*
  * kmod stress test driver
  *
  * Copyright (C) 2017 Luis R. Rodriguez <mcgrof@kernel.org>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the Free
- * Software Foundation; either version 2 of the License, or at your option any
- * later version; or, when distributed separately from the Linux kernel or
- * when incorporated into other software packages, subject to the following
- * license:
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of copyleft-next (version 0.3.1 or later) as published
- * at http://copyleft-next.org/.
  */
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 3750323973f4..9e5bd10a930a 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -1,18 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
 /*
  * proc sysctl test driver
  *
  * Copyright (C) 2017 Luis R. Rodriguez <mcgrof@kernel.org>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the Free
- * Software Foundation; either version 2 of the License, or at your option any
- * later version; or, when distributed separately from the Linux kernel or
- * when incorporated into other software packages, subject to the following
- * license:
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of copyleft-next (version 0.3.1 or later) as published
- * at http://copyleft-next.org/.
  */
 
 /*
diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
index afd42387e8b2..7189715d7960 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -1,18 +1,7 @@
 #!/bin/bash
-#
+# SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
 # Copyright (C) 2017 Luis R. Rodriguez <mcgrof@kernel.org>
 #
-# This program is free software; you can redistribute it and/or modify it
-# under the terms of the GNU General Public License as published by the Free
-# Software Foundation; either version 2 of the License, or at your option any
-# later version; or, when distributed separately from the Linux kernel or
-# when incorporated into other software packages, subject to the following
-# license:
-#
-# This program is free software; you can redistribute it and/or modify it
-# under the terms of copyleft-next (version 0.3.1 or later) as published
-# at http://copyleft-next.org/.
-
 # This is a stress test script for kmod, the kernel module loader. It uses
 # test_kmod which exposes a series of knobs for the API for us so we can
 # tweak each test in userspace rather than in kernelspace.
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index 19515dcb7d04..2046c603a4d4 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -1,16 +1,6 @@
 #!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
 # Copyright (C) 2017 Luis R. Rodriguez <mcgrof@kernel.org>
-#
-# This program is free software; you can redistribute it and/or modify it
-# under the terms of the GNU General Public License as published by the Free
-# Software Foundation; either version 2 of the License, or at your option any
-# later version; or, when distributed separately from the Linux kernel or
-# when incorporated into other software packages, subject to the following
-# license:
-#
-# This program is free software; you can redistribute it and/or modify it
-# under the terms of copyleft-next (version 0.3.1 or later) as published
-# at http://copyleft-next.org/.
 
 # This performs a series tests against the proc sysctl interface.
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v9 3/6] selftests: add tests_sysfs module
  2021-10-29 18:44 [PATCH v9 0/6] test_sysfs: add new selftest for sysfs Luis Chamberlain
  2021-10-29 18:44 ` [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license Luis Chamberlain
  2021-10-29 18:44 ` [PATCH v9 2/6] testing: use the copyleft-next-0.3.1 SPDX tag Luis Chamberlain
@ 2021-10-29 18:44 ` Luis Chamberlain
  2021-12-03 15:29   ` Greg KH
  2021-10-29 18:44 ` [PATCH v9 4/6] kernfs: add initial failure injection support Luis Chamberlain
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 36+ messages in thread
From: Luis Chamberlain @ 2021-10-29 18:44 UTC (permalink / raw)
  To: tj, gregkh, akpm, jeyu, shuah
  Cc: bvanassche, dan.j.williams, joe, tglx, mcgrof, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel

This adds a new selftest module which can be used to test sysfs, which
would otherwise require using an existing driver. This lets us muck
with a template driver to test breaking things without affecting
system behaviour or requiring the dependencies of a real device
driver.

A series of 28 tests are added. Support for using two device types are
supported:

  * misc
  * block

Contrary to sysctls, sysfs requires a full write to happen at once, and
so we reduce the digit tests to single writes. Two main sysfs knobs are
provided for testing reading/storing, one which doesn't incur any
delays and another which can incur programmed delays. What locks are
held, if any, are configurable, at module load time, or through dynamic
configuration at run time.

Since sysfs is a technically filesystem, but a pseudo one, which
requires a kernel user, our test_sysfs module and respective test script
embraces fstests format for tests in the kernel ring bufffer. Likewise,
a scraper for kernel crashes is provided which matches what fstests does
as well.

Two tests are kept disabled as they are a demonstration of what not to
do as it can cause a deadlock with sysfs. These tests provides a mechanism
to easily show proof and demo how the deadlock can happen:

Demos the deadlock with a device specific lock
./tools/testing/selftests/sysfs/sysfs.sh -t 0027

Demos the deadlock with rtnl_lock()
./tools/testing/selftests/sysfs/sysfs.sh -t 0028

Drivers should *avoid* sharing a lock on rmmod and on sysfs ops.

This selftests will shortly be expanded upon with more tests which
require further kernel changes in order to provide better test
coverage.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 MAINTAINERS                            |    7 +
 lib/Kconfig.debug                      |   12 +
 lib/Makefile                           |    1 +
 lib/test_sysfs.c                       |  894 ++++++++++++++++++
 tools/testing/selftests/sysfs/Makefile |   12 +
 tools/testing/selftests/sysfs/config   |    2 +
 tools/testing/selftests/sysfs/settings |    1 +
 tools/testing/selftests/sysfs/sysfs.sh | 1197 ++++++++++++++++++++++++
 8 files changed, 2126 insertions(+)
 create mode 100644 lib/test_sysfs.c
 create mode 100644 tools/testing/selftests/sysfs/Makefile
 create mode 100644 tools/testing/selftests/sysfs/config
 create mode 100644 tools/testing/selftests/sysfs/settings
 create mode 100755 tools/testing/selftests/sysfs/sysfs.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index 5f87f622ac18..8b458c4dd577 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18393,6 +18393,13 @@ L:	linux-mmc@vger.kernel.org
 S:	Maintained
 F:	drivers/mmc/host/sdhci-pci-dwc-mshc.c
 
+SYSFS TEST DRIVER
+M:	Luis Chamberlain <mcgrof@kernel.org>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	lib/test_sysfs.c
+F:	tools/testing/selftests/sysfs/
+
 SYSTEM CONFIGURATION (SYSCON)
 M:	Lee Jones <lee.jones@linaro.org>
 M:	Arnd Bergmann <arnd@arndb.de>
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 44a6df361016..ec531b423c0e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2344,6 +2344,18 @@ config TEST_SYSCTL
 
 	  If unsure, say N.
 
+config TEST_SYSFS
+	tristate "sysfs test driver"
+	depends on SYSFS
+	depends on NET
+	depends on BLOCK
+	help
+	  This builds the "test_sysfs" module. This driver enables to test the
+	  sysfs file system safely without affecting production knobs which
+	  might alter system functionality.
+
+	  If unsure, say N.
+
 config BITFIELD_KUNIT
 	tristate "KUnit test bitfield functions at runtime"
 	depends on KUNIT
diff --git a/lib/Makefile b/lib/Makefile
index 364c23f15578..741c1be29781 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
 obj-$(CONFIG_TEST_BITOPS) += test_bitops.o
 CFLAGS_test_bitops.o += -Werror
 obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
+obj-$(CONFIG_TEST_SYSFS) += test_sysfs.o
 obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o
 obj-$(CONFIG_TEST_IDA) += test_ida.o
 obj-$(CONFIG_KASAN_KUNIT_TEST) += test_kasan.o
diff --git a/lib/test_sysfs.c b/lib/test_sysfs.c
new file mode 100644
index 000000000000..2a6ec072da60
--- /dev/null
+++ b/lib/test_sysfs.c
@@ -0,0 +1,894 @@
+// SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
+/*
+ * Copyright (C) 2021 Luis Chamberlain <mcgrof@kernel.org>
+ *
+ * sysfs test driver
+ *
+ * This module allows us to add race conditions which we can test for
+ * against the sysfs filesystem.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/async.h>
+#include <linux/delay.h>
+#include <linux/vmalloc.h>
+#include <linux/debugfs.h>
+#include <linux/rtnetlink.h>
+#include <linux/genhd.h>
+#include <linux/blkdev.h>
+
+static bool enable_lock;
+module_param(enable_lock, bool_enable_only, 0644);
+MODULE_PARM_DESC(enable_lock,
+		 "enable locking on reads / stores from the start");
+
+static bool enable_lock_on_rmmod;
+module_param(enable_lock_on_rmmod, bool_enable_only, 0644);
+MODULE_PARM_DESC(enable_lock_on_rmmod,
+		 "enable locking on rmmod");
+
+static bool use_rtnl_lock;
+module_param(use_rtnl_lock, bool_enable_only, 0644);
+MODULE_PARM_DESC(use_rtnl_lock,
+		 "use an rtnl_lock instead of the device mutex_lock");
+
+static unsigned int write_delay_msec_y = 500;
+module_param_named(write_delay_msec_y, write_delay_msec_y, uint, 0644);
+MODULE_PARM_DESC(write_delay_msec_y, "msec write delay for writes to y");
+
+static unsigned int test_devtype;
+module_param_named(devtype, test_devtype, uint, 0644);
+MODULE_PARM_DESC(devtype, "device type to register");
+
+static bool enable_busy_alloc;
+module_param(enable_busy_alloc, bool_enable_only, 0644);
+MODULE_PARM_DESC(enable_busy_alloc, "do a fake allocation during writes");
+
+static bool enable_debugfs;
+module_param(enable_debugfs, bool_enable_only, 0644);
+MODULE_PARM_DESC(enable_debugfs, "enable a few debugfs files");
+
+static bool enable_verbose_writes;
+module_param(enable_verbose_writes, bool_enable_only, 0644);
+MODULE_PARM_DESC(enable_debugfs, "enable stores to print verbose information");
+
+static unsigned int delay_rmmod_ms;
+module_param_named(delay_rmmod_ms, delay_rmmod_ms, uint, 0644);
+MODULE_PARM_DESC(delay_rmmod_ms, "if set how many ms to delay rmmod before device deletion");
+
+static bool enable_verbose_rmmod;
+module_param(enable_verbose_rmmod, bool_enable_only, 0644);
+MODULE_PARM_DESC(enable_verbose_rmmod, "enable verbose print messages on rmmod");
+
+static int sysfs_test_major;
+
+/**
+ * test_config - used for configuring how the sysfs test device will behave
+ *
+ * @enable_lock: if enabled a lock will be used when reading/storing variables
+ * @enable_lock_on_rmmod: if enabled a lock will be used when reading/storing
+ *	sysfs attributes, but it will also be used to lock on rmmod. This is
+ *	useful to test for a deadlock and should serve as an example of what
+ *	drivers should *not* do.
+ * @use_rtnl_lock: if enabled instead of configuration specific mutex, we'll
+ *	use the rtnl_lock. If your test case is modifying this on the fly
+ *	while doing other stores / reads, things will break as a lock can be
+ *	left contending. Best is that tests use this knob serially, without
+ *	allowing userspace to modify other knobs while this one changes.
+ * @write_delay_msec_y: the amount of delay to use when writing to y
+ * @enable_busy_alloc: if enabled we'll do a large allocation between
+ *	writes. We immediately free right away. We also schedule to give the
+ *	kernel some time to re-use any memory we don't need. This is intened
+ *	to mimic typical driver behaviour.
+ */
+struct test_config {
+	bool enable_lock;
+	bool enable_lock_on_rmmod;
+	bool use_rtnl_lock;
+	unsigned int write_delay_msec_y;
+	bool enable_busy_alloc;
+};
+
+/**
+ * enum sysfs_test_devtype - sysfs device type
+ * @TESTDEV_TYPE_MISC: misc device type
+ * @TESTDEV_TYPE_BLOCK: use a block device for the sysfs test device.
+ */
+enum sysfs_test_devtype {
+	TESTDEV_TYPE_MISC = 0,
+	TESTDEV_TYPE_BLOCK,
+};
+
+/**
+ * sysfs_test_device - test device to help test sysfs
+ *
+ * @devtype: the type of device to use
+ * @config: configuration for the test
+ * @config_mutex: protects configuration of test
+ * @misc_dev: we use a misc device under the hood
+ * @disk: represents a disk when used as a block device
+ * @dev: pointer to misc_dev's own struct device
+ * @dev_idx: unique ID for test device
+ * @x: variable we can use to test read / store
+ * @y: slow variable we can use to test read / store
+ */
+struct sysfs_test_device {
+	enum sysfs_test_devtype devtype;
+	struct test_config config;
+	struct mutex config_mutex;
+	struct miscdevice misc_dev;
+	struct gendisk *disk;
+	struct device *dev;
+	int dev_idx;
+	int x;
+	int y;
+};
+
+static struct sysfs_test_device *first_test_dev;
+
+static struct miscdevice *dev_to_misc_dev(struct device *dev)
+{
+	return dev_get_drvdata(dev);
+}
+
+static struct sysfs_test_device *misc_dev_to_test_dev(struct miscdevice *misc_dev)
+{
+	return container_of(misc_dev, struct sysfs_test_device, misc_dev);
+}
+
+static struct sysfs_test_device *devblock_to_test_dev(struct device *dev)
+{
+	return (struct sysfs_test_device *)dev_to_disk(dev)->private_data;
+}
+
+static struct sysfs_test_device *devmisc_to_testdev(struct device *dev)
+{
+	struct miscdevice *misc_dev;
+
+	misc_dev = dev_to_misc_dev(dev);
+	return misc_dev_to_test_dev(misc_dev);
+}
+
+static struct sysfs_test_device *dev_to_test_dev(struct device *dev)
+{
+	if (test_devtype == TESTDEV_TYPE_MISC)
+		return devmisc_to_testdev(dev);
+	else if (test_devtype == TESTDEV_TYPE_BLOCK)
+		return devblock_to_test_dev(dev);
+	return NULL;
+}
+
+static void test_dev_config_lock(struct sysfs_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+
+	if (config->enable_lock) {
+		if (config->use_rtnl_lock)
+			rtnl_lock();
+		else
+			mutex_lock(&test_dev->config_mutex);
+	}
+}
+
+static void test_dev_config_unlock(struct sysfs_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+
+	if (config->enable_lock) {
+		if (config->use_rtnl_lock)
+			rtnl_unlock();
+		else
+			mutex_unlock(&test_dev->config_mutex);
+	}
+}
+
+static void test_dev_config_lock_rmmod(struct sysfs_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+
+	if (config->enable_lock_on_rmmod)
+		test_dev_config_lock(test_dev);
+}
+
+static void test_dev_config_unlock_rmmod(struct sysfs_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+
+	if (config->enable_lock_on_rmmod)
+		test_dev_config_unlock(test_dev);
+}
+
+static void free_test_dev_sysfs(struct sysfs_test_device *test_dev)
+{
+	if (test_dev) {
+		kfree_const(test_dev->misc_dev.name);
+		test_dev->misc_dev.name = NULL;
+		kfree(test_dev);
+		test_dev = NULL;
+	}
+}
+
+static void test_sysfs_reset_vals(struct sysfs_test_device *test_dev)
+{
+	test_dev->x = 3;
+	test_dev->y = 4;
+}
+
+static ssize_t config_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int len = 0;
+
+	test_dev_config_lock(test_dev);
+
+	len += sysfs_emit_at(buf, len, "Configuration for: %s\n",
+			     dev_name(dev));
+	len += sysfs_emit_at(buf, len, "x:\t%d\n", test_dev->x);
+	len += sysfs_emit_at(buf, len, "y:\t%d\n", test_dev->y);
+	len += sysfs_emit_at(buf, len, "enable_lock:\t%s\n",
+			     config->enable_lock ? "true" : "false");
+	len += sysfs_emit_at(buf, len, "enable_lock_on_rmmmod:\t%s\n",
+			     config->enable_lock_on_rmmod ? "true" : "false");
+	len += sysfs_emit_at(buf, len, "use_rtnl_lock:\t%s\n",
+			     config->use_rtnl_lock ? "true" : "false");
+	len += sysfs_emit_at(buf, len, "write_delay_msec_y:\t%d\n",
+			     config->write_delay_msec_y);
+	len += sysfs_emit_at(buf, len, "enable_busy_alloc:\t%s\n",
+			     config->enable_busy_alloc ? "true" : "false");
+	len += sysfs_emit_at(buf, len, "enable_debugfs:\t%s\n",
+			     enable_debugfs ? "true" : "false");
+	len += sysfs_emit_at(buf, len, "enable_verbose_writes:\t%s\n",
+			     enable_verbose_writes ? "true" : "false");
+
+	test_dev_config_unlock(test_dev);
+
+	return len;
+}
+static DEVICE_ATTR_RO(config);
+
+static ssize_t reset_store(struct device *dev,
+			   struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	/*
+	 * We compromise and simplify this condition and do not use a lock
+	 * here as the lock type can change.
+	 */
+	config->enable_lock = false;
+	config->enable_lock_on_rmmod = false;
+	config->use_rtnl_lock = false;
+	config->enable_busy_alloc = false;
+	test_sysfs_reset_vals(test_dev);
+
+	dev_info(dev, "reset\n");
+
+	return count;
+}
+static DEVICE_ATTR_WO(reset);
+
+static void test_dev_busy_alloc(struct sysfs_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+	char *ignore;
+
+	if (!config->enable_busy_alloc)
+		return;
+
+	ignore = kzalloc(sizeof(struct sysfs_test_device) * 10, GFP_KERNEL);
+	kfree(ignore);
+
+	schedule();
+}
+
+static ssize_t test_dev_x_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	int ret;
+
+	test_dev_busy_alloc(test_dev);
+	test_dev_config_lock(test_dev);
+
+	ret = kstrtoint(buf, 10, &test_dev->x);
+	if (ret)
+		count = ret;
+
+	if (enable_verbose_writes)
+		dev_info(test_dev->dev, "wrote x = %d\n", test_dev->x);
+
+	test_dev_config_unlock(test_dev);
+
+	return count;
+}
+
+static ssize_t test_dev_x_show(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	int ret;
+
+	test_dev_config_lock(test_dev);
+	ret = sysfs_emit(buf, "%d\n", test_dev->x);
+	test_dev_config_unlock(test_dev);
+
+	return ret;
+}
+static DEVICE_ATTR_RW(test_dev_x);
+
+static ssize_t test_dev_y_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config;
+	int y;
+	int ret;
+
+	test_dev_busy_alloc(test_dev);
+	test_dev_config_lock(test_dev);
+
+	config = &test_dev->config;
+
+	ret = kstrtoint(buf, 10, &y);
+	if (ret)
+		count = ret;
+
+	msleep(config->write_delay_msec_y);
+	test_dev->y = test_dev->x + y + 7;
+
+	if (enable_verbose_writes)
+		dev_info(test_dev->dev, "wrote y = %d\n", test_dev->y);
+
+	test_dev_config_unlock(test_dev);
+
+	return count;
+}
+
+static ssize_t test_dev_y_show(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	int ret;
+
+	test_dev_config_lock(test_dev);
+	ret = sysfs_emit(buf, "%d\n", test_dev->y);
+	test_dev_config_unlock(test_dev);
+
+	return ret;
+}
+static DEVICE_ATTR_RW(test_dev_y);
+
+static ssize_t config_enable_lock_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int ret;
+	int val;
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	/*
+	 * We compromise for simplicty and do not lock when changing
+	 * locking configuration, with the assumption userspace tests
+	 * will know this.
+	 */
+	if (val)
+		config->enable_lock = true;
+	else
+		config->enable_lock = false;
+
+	return count;
+}
+
+static ssize_t config_enable_lock_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	ssize_t ret;
+
+	test_dev_config_lock(test_dev);
+	ret = sysfs_emit(buf, "%d\n", config->enable_lock);
+	test_dev_config_unlock(test_dev);
+
+	return ret;
+}
+static DEVICE_ATTR_RW(config_enable_lock);
+
+static ssize_t config_enable_lock_on_rmmod_store(struct device *dev,
+						 struct device_attribute *attr,
+						 const char *buf, size_t count)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int ret;
+	int val;
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	test_dev_config_lock(test_dev);
+	if (val)
+		config->enable_lock_on_rmmod = true;
+	else
+		config->enable_lock_on_rmmod = false;
+	test_dev_config_unlock(test_dev);
+
+	return count;
+}
+
+static ssize_t config_enable_lock_on_rmmod_show(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	ssize_t ret;
+
+	test_dev_config_lock(test_dev);
+	ret = sysfs_emit(buf, "%d\n", config->enable_lock_on_rmmod);
+	test_dev_config_unlock(test_dev);
+
+	return ret;
+}
+static DEVICE_ATTR_RW(config_enable_lock_on_rmmod);
+
+static ssize_t config_use_rtnl_lock_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf, size_t count)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int ret;
+	int val;
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	/*
+	 * We compromise and simplify this condition and do not use a lock
+	 * here as the lock type can change.
+	 */
+	if (val)
+		config->use_rtnl_lock = true;
+	else
+		config->use_rtnl_lock = false;
+
+	return count;
+}
+
+static ssize_t config_use_rtnl_lock_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return sysfs_emit(buf, "%d\n", config->use_rtnl_lock);
+}
+static DEVICE_ATTR_RW(config_use_rtnl_lock);
+
+static ssize_t config_write_delay_msec_y_store(struct device *dev,
+					       struct device_attribute *attr,
+					       const char *buf, size_t count)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int ret;
+	int val;
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	test_dev_config_lock(test_dev);
+	config->write_delay_msec_y = val;
+	test_dev_config_unlock(test_dev);
+
+	return count;
+}
+
+static ssize_t config_write_delay_msec_y_show(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return sysfs_emit(buf, "%d\n", config->write_delay_msec_y);
+}
+static DEVICE_ATTR_RW(config_write_delay_msec_y);
+
+static ssize_t config_enable_busy_alloc_store(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t count)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int ret;
+	int val;
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	test_dev_config_lock(test_dev);
+	config->enable_busy_alloc = val;
+	test_dev_config_unlock(test_dev);
+
+	return count;
+}
+
+static ssize_t config_enable_busy_alloc_show(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return sysfs_emit(buf, "%d\n", config->enable_busy_alloc);
+}
+static DEVICE_ATTR_RW(config_enable_busy_alloc);
+
+#define TEST_SYSFS_DEV_ATTR(name)		(&dev_attr_##name.attr)
+
+static struct attribute *test_dev_attrs[] = {
+	/* Generic driver knobs go here */
+	TEST_SYSFS_DEV_ATTR(config),
+	TEST_SYSFS_DEV_ATTR(reset),
+
+	/* These are used to test sysfs */
+	TEST_SYSFS_DEV_ATTR(test_dev_x),
+	TEST_SYSFS_DEV_ATTR(test_dev_y),
+
+	/*
+	 * These are configuration knobs to modify how we test sysfs when
+	 * doing reads / stores.
+	 */
+	TEST_SYSFS_DEV_ATTR(config_enable_lock),
+	TEST_SYSFS_DEV_ATTR(config_enable_lock_on_rmmod),
+	TEST_SYSFS_DEV_ATTR(config_use_rtnl_lock),
+	TEST_SYSFS_DEV_ATTR(config_write_delay_msec_y),
+	TEST_SYSFS_DEV_ATTR(config_enable_busy_alloc),
+
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(test_dev);
+
+static int sysfs_test_dev_alloc_miscdev(struct sysfs_test_device *test_dev)
+{
+	struct miscdevice *misc_dev;
+
+	misc_dev = &test_dev->misc_dev;
+	misc_dev->minor = MISC_DYNAMIC_MINOR;
+	misc_dev->name = kasprintf(GFP_KERNEL, "test_sysfs%d", test_dev->dev_idx);
+	if (!misc_dev->name) {
+		pr_err("Cannot alloc misc_dev->name\n");
+		return -ENOMEM;
+	}
+	misc_dev->groups = test_dev_groups;
+
+	return 0;
+}
+
+static int testdev_open(struct block_device *bdev, fmode_t mode)
+{
+	return -EINVAL;
+}
+
+static void testdev_submit_bio(struct bio *bio)
+{
+}
+
+static void testdev_slot_free_notify(struct block_device *bdev,
+				     unsigned long index)
+{
+}
+
+static int testdev_rw_page(struct block_device *bdev, sector_t sector,
+			   struct page *page, unsigned int op)
+{
+	return -EOPNOTSUPP;
+}
+
+static const struct block_device_operations sysfs_testdev_ops = {
+	.open = testdev_open,
+	.submit_bio = testdev_submit_bio,
+	.swap_slot_free_notify = testdev_slot_free_notify,
+	.rw_page = testdev_rw_page,
+	.owner = THIS_MODULE
+};
+
+static int sysfs_test_dev_alloc_blockdev(struct sysfs_test_device *test_dev)
+{
+	int ret = -ENOMEM;
+
+	test_dev->disk = blk_alloc_disk(NUMA_NO_NODE);
+	if (!test_dev->disk) {
+		pr_err("Error allocating disk structure for device %d\n",
+		       test_dev->dev_idx);
+		goto out;
+	}
+
+	test_dev->disk->major = sysfs_test_major;
+	test_dev->disk->first_minor = test_dev->dev_idx + 1;
+	test_dev->disk->fops = &sysfs_testdev_ops;
+	test_dev->disk->private_data = test_dev;
+	snprintf(test_dev->disk->disk_name, sizeof(test_dev->disk->disk_name),
+		 "test_sysfs%d", test_dev->dev_idx);
+	set_capacity(test_dev->disk, 0);
+	blk_queue_flag_set(QUEUE_FLAG_NONROT, test_dev->disk->queue);
+	blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, test_dev->disk->queue);
+	blk_queue_physical_block_size(test_dev->disk->queue, PAGE_SIZE);
+	blk_queue_max_discard_sectors(test_dev->disk->queue, UINT_MAX);
+	blk_queue_flag_set(QUEUE_FLAG_DISCARD, test_dev->disk->queue);
+
+	return 0;
+out:
+	return ret;
+}
+
+static struct sysfs_test_device *alloc_test_dev_sysfs(int idx)
+{
+	struct sysfs_test_device *test_dev;
+	int ret;
+
+	switch (test_devtype) {
+	case TESTDEV_TYPE_MISC:
+	       fallthrough;
+	case TESTDEV_TYPE_BLOCK:
+		break;
+	default:
+		return NULL;
+	}
+
+	test_dev = kzalloc(sizeof(struct sysfs_test_device), GFP_KERNEL);
+	if (!test_dev)
+		goto err_out;
+
+	mutex_init(&test_dev->config_mutex);
+	test_dev->dev_idx = idx;
+	test_dev->devtype = test_devtype;
+
+	if (test_dev->devtype == TESTDEV_TYPE_MISC) {
+		ret = sysfs_test_dev_alloc_miscdev(test_dev);
+		if (ret)
+			goto err_out_free;
+	} else if (test_dev->devtype == TESTDEV_TYPE_BLOCK) {
+		ret = sysfs_test_dev_alloc_blockdev(test_dev);
+		if (ret)
+			goto err_out_free;
+	}
+	return test_dev;
+
+err_out_free:
+	kfree(test_dev);
+	test_dev = NULL;
+err_out:
+	return NULL;
+}
+
+static int register_test_dev_sysfs_misc(struct sysfs_test_device *test_dev)
+{
+	int ret;
+
+	ret = misc_register(&test_dev->misc_dev);
+	if (ret)
+		return ret;
+
+	test_dev->dev = test_dev->misc_dev.this_device;
+
+	return 0;
+}
+
+static int register_test_dev_sysfs_block(struct sysfs_test_device *test_dev)
+{
+	int ret;
+
+	ret = device_add_disk(NULL, test_dev->disk, test_dev_groups);
+	if (ret) {
+		blk_cleanup_disk(test_dev->disk);
+		return ret;
+	}
+
+	test_dev->dev = disk_to_dev(test_dev->disk);
+
+	return 0;
+}
+
+static struct sysfs_test_device *register_test_dev_sysfs(void)
+{
+	struct sysfs_test_device *test_dev = NULL;
+	int ret;
+
+	test_dev = alloc_test_dev_sysfs(0);
+	if (!test_dev)
+		goto out;
+
+	if (test_dev->devtype == TESTDEV_TYPE_MISC) {
+		ret = register_test_dev_sysfs_misc(test_dev);
+		if (ret) {
+			pr_err("could not register misc device: %d\n", ret);
+			goto out_free_dev;
+		}
+	} else if (test_dev->devtype == TESTDEV_TYPE_BLOCK) {
+		ret = register_test_dev_sysfs_block(test_dev);
+		if (ret) {
+			pr_err("could not register block device: %d\n", ret);
+			goto out_free_dev;
+		}
+	}
+
+	dev_info(test_dev->dev, "interface ready\n");
+
+out:
+	return test_dev;
+out_free_dev:
+	free_test_dev_sysfs(test_dev);
+	return NULL;
+}
+
+static struct sysfs_test_device *register_test_dev_set_config(void)
+{
+	struct sysfs_test_device *test_dev;
+	struct test_config *config;
+
+	test_dev = register_test_dev_sysfs();
+	if (!test_dev)
+		return NULL;
+
+	config = &test_dev->config;
+
+	if (enable_lock)
+		config->enable_lock = true;
+	if (enable_lock_on_rmmod)
+		config->enable_lock_on_rmmod = true;
+	if (use_rtnl_lock)
+		config->use_rtnl_lock = true;
+	if (enable_busy_alloc)
+		config->enable_busy_alloc = true;
+
+	config->write_delay_msec_y = write_delay_msec_y;
+	test_sysfs_reset_vals(test_dev);
+
+	return test_dev;
+}
+
+static void unregister_test_dev_sysfs_misc(struct sysfs_test_device *test_dev)
+{
+	misc_deregister(&test_dev->misc_dev);
+}
+
+static void unregister_test_dev_sysfs_block(struct sysfs_test_device *test_dev)
+{
+	del_gendisk(test_dev->disk);
+	blk_cleanup_disk(test_dev->disk);
+}
+
+static void unregister_test_dev_sysfs(struct sysfs_test_device *test_dev)
+{
+	test_dev_config_lock_rmmod(test_dev);
+
+	dev_info(test_dev->dev, "removing interface\n");
+
+	if (test_dev->devtype == TESTDEV_TYPE_MISC)
+		unregister_test_dev_sysfs_misc(test_dev);
+	else if (test_dev->devtype == TESTDEV_TYPE_BLOCK)
+		unregister_test_dev_sysfs_block(test_dev);
+
+	test_dev_config_unlock_rmmod(test_dev);
+
+	free_test_dev_sysfs(test_dev);
+}
+
+static struct dentry *debugfs_dir;
+
+/* When read represents how many times we have reset the first_test_dev */
+static u8 reset_first_test_dev;
+
+static ssize_t read_reset_first_test_dev(struct file *file,
+					 char __user *user_buf,
+					 size_t count, loff_t *ppos)
+{
+	ssize_t len;
+	char buf[32];
+
+	reset_first_test_dev++;
+	len = sprintf(buf, "%d\n", reset_first_test_dev);
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t write_reset_first_test_dev(struct file *file,
+					  const char __user *user_buf,
+					  size_t count, loff_t *ppos)
+{
+	if (!try_module_get(THIS_MODULE))
+		return -ENODEV;
+
+	if (!first_test_dev) {
+		module_put(THIS_MODULE);
+		return -ENODEV;
+	}
+
+	dev_info(first_test_dev->dev, "going to reset first interface ...\n");
+
+	unregister_test_dev_sysfs(first_test_dev);
+	first_test_dev = register_test_dev_set_config();
+
+	dev_info(first_test_dev->dev, "first interface reset complete\n");
+
+	module_put(THIS_MODULE);
+
+	return count;
+}
+
+static const struct file_operations fops_reset_first_test_dev = {
+	.read = read_reset_first_test_dev,
+	.write = write_reset_first_test_dev,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
+static int __init test_sysfs_init(void)
+{
+	first_test_dev = register_test_dev_set_config();
+	if (!first_test_dev)
+		return -ENOMEM;
+
+	if (!enable_debugfs)
+		return 0;
+
+	debugfs_dir = debugfs_create_dir("test_sysfs", NULL);
+	if (!debugfs_dir) {
+		unregister_test_dev_sysfs(first_test_dev);
+		return -ENOMEM;
+	}
+
+	debugfs_create_file("reset_first_test_dev", 0600, debugfs_dir,
+			    NULL, &fops_reset_first_test_dev);
+	return 0;
+}
+module_init(test_sysfs_init);
+
+static void __exit test_sysfs_exit(void)
+{
+	if (enable_debugfs)
+		debugfs_remove(debugfs_dir);
+	if (delay_rmmod_ms)
+		msleep(delay_rmmod_ms);
+	unregister_test_dev_sysfs(first_test_dev);
+	if (enable_verbose_rmmod)
+		pr_info("unregister_test_dev_sysfs() completed\n");
+	first_test_dev = NULL;
+}
+module_exit(test_sysfs_exit);
+
+MODULE_AUTHOR("Luis Chamberlain <mcgrof@kernel.org>");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/sysfs/Makefile b/tools/testing/selftests/sysfs/Makefile
new file mode 100644
index 000000000000..fde99caa2338
--- /dev/null
+++ b/tools/testing/selftests/sysfs/Makefile
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0-only
+# Makefile for sysfs selftests.
+
+# No binaries, but make sure arg-less "make" doesn't trigger "run_tests".
+all:
+
+TEST_PROGS := sysfs.sh
+
+include ../lib.mk
+
+# Nothing to clean up.
+clean:
diff --git a/tools/testing/selftests/sysfs/config b/tools/testing/selftests/sysfs/config
new file mode 100644
index 000000000000..9196f452ecd5
--- /dev/null
+++ b/tools/testing/selftests/sysfs/config
@@ -0,0 +1,2 @@
+CONFIG_SYSFS=m
+CONFIG_TEST_SYSFS=m
diff --git a/tools/testing/selftests/sysfs/settings b/tools/testing/selftests/sysfs/settings
new file mode 100644
index 000000000000..ae36db2b3200
--- /dev/null
+++ b/tools/testing/selftests/sysfs/settings
@@ -0,0 +1 @@
+timeout=200
diff --git a/tools/testing/selftests/sysfs/sysfs.sh b/tools/testing/selftests/sysfs/sysfs.sh
new file mode 100755
index 000000000000..802651d78427
--- /dev/null
+++ b/tools/testing/selftests/sysfs/sysfs.sh
@@ -0,0 +1,1197 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (C) 2021 Luis Chamberlain <mcgrof@kernel.org>
+#
+# This performs a series tests against the sysfs filesystem.
+
+# Kselftest framework requirement - SKIP code is 4.
+ksft_skip=4
+
+TEST_NAME="sysfs"
+TEST_DRIVER="test_${TEST_NAME}"
+TEST_DIR=$(dirname $0)
+TEST_FILE=$(mktemp)
+
+# This represents
+#
+# TEST_ID:TEST_COUNT:ENABLED:TARGET
+#
+# TEST_ID: is the test id number
+# TEST_COUNT: number of times we should run the test
+# ENABLED: 1 if enabled, 0 otherwise
+# TARGET: test target file required on the test_sysfs module
+#
+# Once these are enabled please leave them as-is. Write your own test,
+# we have tons of space.
+ALL_TESTS="0001:3:1:test_dev_x:misc"
+ALL_TESTS="$ALL_TESTS 0002:3:1:test_dev_y:misc"
+ALL_TESTS="$ALL_TESTS 0003:3:1:test_dev_x:misc"
+ALL_TESTS="$ALL_TESTS 0004:3:1:test_dev_y:misc"
+ALL_TESTS="$ALL_TESTS 0005:1:1:test_dev_x:misc"
+ALL_TESTS="$ALL_TESTS 0006:1:1:test_dev_y:misc"
+ALL_TESTS="$ALL_TESTS 0007:1:1:test_dev_y:misc"
+ALL_TESTS="$ALL_TESTS 0008:1:1:test_dev_x:misc"
+ALL_TESTS="$ALL_TESTS 0009:1:1:test_dev_y:misc"
+ALL_TESTS="$ALL_TESTS 0010:1:1:test_dev_y:misc"
+ALL_TESTS="$ALL_TESTS 0011:1:1:test_dev_x:misc"
+ALL_TESTS="$ALL_TESTS 0012:1:1:test_dev_y:misc"
+ALL_TESTS="$ALL_TESTS 0013:1:1:test_dev_y:misc"
+ALL_TESTS="$ALL_TESTS 0014:3:1:test_dev_x:block" # block equivalent set
+ALL_TESTS="$ALL_TESTS 0015:3:1:test_dev_x:block"
+ALL_TESTS="$ALL_TESTS 0016:3:1:test_dev_x:block"
+ALL_TESTS="$ALL_TESTS 0017:3:1:test_dev_y:block"
+ALL_TESTS="$ALL_TESTS 0018:1:1:test_dev_x:block"
+ALL_TESTS="$ALL_TESTS 0019:1:1:test_dev_y:block"
+ALL_TESTS="$ALL_TESTS 0020:1:1:test_dev_y:block"
+ALL_TESTS="$ALL_TESTS 0021:1:1:test_dev_x:block"
+ALL_TESTS="$ALL_TESTS 0022:1:1:test_dev_y:block"
+ALL_TESTS="$ALL_TESTS 0023:1:1:test_dev_y:block"
+ALL_TESTS="$ALL_TESTS 0024:1:1:test_dev_x:block"
+ALL_TESTS="$ALL_TESTS 0025:1:1:test_dev_y:block"
+ALL_TESTS="$ALL_TESTS 0026:1:1:test_dev_y:block"
+ALL_TESTS="$ALL_TESTS 0027:1:0:test_dev_x:block" # deadlock test
+ALL_TESTS="$ALL_TESTS 0028:1:0:test_dev_x:block" # deadlock test with rntl_lock
+
+allow_user_defaults()
+{
+	if [ -z $DIR ]; then
+		case $TEST_DEV_TYPE in
+		misc)
+			DIR="/sys/devices/virtual/misc/${TEST_DRIVER}0"
+			;;
+		block)
+			DIR="/sys/devices/virtual/block/${TEST_DRIVER}0"
+			;;
+		*)
+			DIR="/sys/devices/virtual/misc/${TEST_DRIVER}0"
+			;;
+		esac
+	fi
+	case $TEST_DEV_TYPE in
+		misc)
+			MODPROBE_TESTDEV_TYPE=""
+			;;
+		block)
+			MODPROBE_TESTDEV_TYPE="devtype=1"
+			;;
+		*)
+			MODPROBE_TESTDEV_TYPE=""
+			;;
+	esac
+	if [ -z $SYSFS_DEBUGFS_DIR ]; then
+		SYSFS_DEBUGFS_DIR="/sys/kernel/debug/test_sysfs"
+	fi
+	if [ -z $PAGE_SIZE ]; then
+		PAGE_SIZE=$(getconf PAGESIZE)
+	fi
+	if [ -z $MAX_DIGITS ]; then
+		MAX_DIGITS=$(($PAGE_SIZE/8))
+	fi
+	if [ -z $INT_MAX ]; then
+		INT_MAX=$(getconf INT_MAX)
+	fi
+	if [ -z $UINT_MAX ]; then
+		UINT_MAX=$(getconf UINT_MAX)
+	fi
+}
+
+test_reqs()
+{
+	uid=$(id -u)
+	if [ $uid -ne 0 ]; then
+		echo $msg must be run as root >&2
+		exit $ksft_skip
+	fi
+
+	if ! which modprobe 2> /dev/null > /dev/null; then
+		echo "$0: You need modprobe installed" >&2
+		exit $ksft_skip
+	fi
+	if ! which getconf 2> /dev/null > /dev/null; then
+		echo "$0: You need getconf installed"
+		exit $ksft_skip
+	fi
+	if ! which diff 2> /dev/null > /dev/null; then
+		echo "$0: You need diff installed"
+		exit $ksft_skip
+	fi
+	if ! which perl 2> /dev/null > /dev/null; then
+		echo "$0: You need perl installed"
+		exit $ksft_skip
+	fi
+}
+
+call_modprobe()
+{
+	modprobe $TEST_DRIVER $MODPROBE_TESTDEV_TYPE $FIRST_MODPROBE_ARGS $MODPROBE_ARGS
+	return $?
+}
+
+modprobe_reset()
+{
+	modprobe -q -r $TEST_DRIVER
+	call_modprobe
+	return $?
+}
+
+modprobe_reset_enable_debugfs()
+{
+	FIRST_MODPROBE_ARGS="enable_debugfs=1"
+	modprobe_reset
+	unset FIRST_MODPROBE_ARGS
+}
+
+modprobe_reset_enable_lock_on_rmmod()
+{
+	FIRST_MODPROBE_ARGS="enable_lock=1 enable_lock_on_rmmod=1 enable_verbose_writes=1"
+	modprobe_reset
+	unset FIRST_MODPROBE_ARGS
+}
+
+modprobe_reset_enable_rtnl_lock_on_rmmod()
+{
+	FIRST_MODPROBE_ARGS="enable_lock=1 use_rtnl_lock=1 enable_lock_on_rmmod=1"
+	FIRST_MODPROBE_ARGS="$FIRST_MODPROBE_ARGS enable_verbose_writes=1"
+	modprobe_reset
+	unset FIRST_MODPROBE_ARGS
+}
+
+load_req_mod()
+{
+	modprobe_reset
+	if [ ! -d $DIR ]; then
+		if ! modprobe -q -n $TEST_DRIVER; then
+			echo "$0: module $TEST_DRIVER not found [SKIP]"
+			echo "You must set CONFIG_TEST_SYSFS=m in your kernel" >&2
+			exit $ksft_skip
+		fi
+		call_modprobe
+		if [ $? -ne 0 ]; then
+			echo "$0: modprobe $TEST_DRIVER failed."
+			exit
+		fi
+	fi
+}
+
+config_reset()
+{
+	if ! echo -n "1" >"$DIR"/reset; then
+		echo "$0: reset should have worked" >&2
+		exit 1
+	fi
+}
+
+debugfs_reset_first_test_dev_ignore_errors()
+{
+	echo -n "1" >"$SYSFS_DEBUGFS_DIR"/reset_first_test_dev
+}
+
+set_orig()
+{
+	if [[ ! -z $TARGET ]] && [[ ! -z $ORIG ]]; then
+		if [ -f ${TARGET} ]; then
+			echo "${ORIG}" > "${TARGET}"
+		fi
+	fi
+}
+
+set_test()
+{
+	echo "${TEST_STR}" > "${TARGET}"
+}
+
+set_test_ignore_errors()
+{
+	echo "${TEST_STR}" > "${TARGET}" 2> /dev/null
+}
+
+verify()
+{
+	local seen
+	seen=$(cat "$1")
+	target_short=$(basename $TARGET)
+	case $target_short in
+	test_dev_x)
+		if [ "${seen}" != "${TEST_STR}" ]; then
+			return 1
+		fi
+		;;
+	test_dev_y)
+		DIRNAME=$(dirname $1)
+		EXPECTED_RESULT=""
+		# If our target was the test file then what we write to it
+		# is the same as what that we expect when we read from it.
+		# When we write to test_dev_y directly though we expect
+		# a computed value which is driver specific.
+		if [[ "$DIRNAME" == "/tmp" ]]; then
+			let EXPECTED_RESULT="${TEST_STR}"
+		else
+			x=$(cat ${DIR}/test_dev_x)
+			let EXPECTED_RESULT="$x+${TEST_STR}+7"
+		fi
+
+		if [[ "${seen}" != "${EXPECTED_RESULT}" ]]; then
+			return 1
+		fi
+		;;
+	*)
+		echo "Unsupported target type update test script: $target_short"
+		exit 1
+	esac
+	return 0
+}
+
+verify_diff_w()
+{
+	echo "$TEST_STR" | diff -q -w -u - $1 > /dev/null
+	return $?
+}
+
+test_rc()
+{
+	if [[ $rc != 0 ]]; then
+		echo "Failed test, return value: $rc" >&2
+		exit $rc
+	fi
+}
+
+test_finish()
+{
+	set_orig
+	rm -f "${TEST_FILE}"
+
+	if [ ! -z ${old_strict} ]; then
+		echo ${old_strict} > ${WRITES_STRICT}
+	fi
+	exit $rc
+}
+
+# kernfs requires us to write everything we want in one shot because
+# There is no easy way for us to know if userspace is only doing a partial
+# write, so we don't support them. We expect the entire buffer to come on
+# the first write.  If you're writing a value, first read the file,
+# modify only the value you're changing, then write entire buffer back.
+# Since we are only testing digits we just full single writes and old stuff.
+# For more details, refer to kernfs_fop_write_iter().
+run_numerictests_single_write()
+{
+	echo "== Testing sysfs behavior against ${TARGET} =="
+
+	rc=0
+
+	echo -n "Writing test file ... "
+	echo "${TEST_STR}" > "${TEST_FILE}"
+	if ! verify "${TEST_FILE}"; then
+		echo "FAIL" >&2
+		exit 1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Checking the sysfs file is not set to test value ... "
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		exit 1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Writing to sysfs file from shell ... "
+	set_test
+	if ! verify "${TARGET}"; then
+		echo "FAIL" >&2
+		exit 1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Resetting sysfs file to original value ... "
+	set_orig
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		exit 1
+	else
+		echo "ok"
+	fi
+
+	# Now that we've validated the sanity of "set_test" and "set_orig",
+	# we can use those functions to set starting states before running
+	# specific behavioral tests.
+
+	echo -n "Writing to the entire sysfs file in a single write ... "
+	set_orig
+	dd if="${TEST_FILE}" of="${TARGET}" bs=4096 2>/dev/null
+	if ! verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Writing to the sysfs file with multiple long writes ... "
+	set_orig
+	(perl -e 'print "A" x 50;'; echo "${TEST_STR}") | \
+		dd of="${TARGET}" bs=50 2>/dev/null
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+}
+
+reset_vals()
+{
+	echo -n 3 > $DIR/test_dev_x
+	echo -n 4 > $DIR/test_dev_x
+}
+
+check_failure()
+{
+	echo -n "Testing that $1 fails as expected..."
+	reset_vals
+	TEST_STR="$1"
+	orig="$(cat $TARGET)"
+	echo -n "$TEST_STR" > $TARGET 2> /dev/null
+
+	# write should fail and $TARGET should retain its original value
+	if [ $? = 0 ] || [ "$(cat $TARGET)" != "$orig" ]; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+}
+
+load_modreqs()
+{
+	export TEST_DEV_TYPE=$(get_test_type $1)
+	unset DIR
+	allow_user_defaults
+	load_req_mod
+}
+
+target_exists()
+{
+	TARGET="${DIR}/$1"
+	TEST_ID="$2"
+
+	if [ ! -f ${TARGET} ] ; then
+		echo "Target for test $TEST_ID: $TARGET does not exist, skipping test ..."
+		return 0
+	fi
+	return 1
+}
+
+config_enable_lock()
+{
+	if ! echo -n 1 > $DIR/config_enable_lock; then
+		echo "$0: Unable to enable locks" >&2
+		exit 1
+	fi
+}
+
+config_write_delay_msec_y()
+{
+	if ! echo -n $1 > $DIR/config_write_delay_msec_y ; then
+		echo "$0: Unable to set write_delay_msec_y to $1" >&2
+		exit 1
+	fi
+}
+
+# Default filter for dmesg scanning.
+# Ignore lockdep complaining about its own bugginess when scanning dmesg
+# output, because we shouldn't be failing filesystem tests on account of
+# lockdep.
+_check_dmesg_filter()
+{
+	egrep -v -e "BUG: MAX_LOCKDEP_CHAIN_HLOCKS too low" \
+		-e "BUG: MAX_STACK_TRACE_ENTRIES too low"
+}
+
+check_dmesg()
+{
+	# filter out intentional WARNINGs or Oopses
+	local filter=${1:-_check_dmesg_filter}
+
+	_dmesg_since_test_start | $filter >$seqres.dmesg
+	egrep -q -e "kernel BUG at" \
+	     -e "WARNING:" \
+	     -e "\bBUG:" \
+	     -e "Oops:" \
+	     -e "possible recursive locking detected" \
+	     -e "Internal error" \
+	     -e "(INFO|ERR): suspicious RCU usage" \
+	     -e "INFO: possible circular locking dependency detected" \
+	     -e "general protection fault:" \
+	     -e "BUG .* remaining" \
+	     -e "UBSAN:" \
+	     $seqres.dmesg
+	if [ $? -eq 0 ]; then
+		echo "something found in dmesg (see $seqres.dmesg)"
+		return 1
+	else
+		if [ "$KEEP_DMESG" != "yes" ]; then
+			rm -f $seqres.dmesg
+		fi
+		return 0
+	fi
+}
+
+log_kernel_fstest_dmesg()
+{
+	export FSTYP="$1"
+	export seqnum="$FSTYP/$2"
+	export date_time=$(date +"%F %T")
+	echo "run fstests $seqnum at $date_time" > /dev/kmsg
+}
+
+modprobe_loop()
+{
+	while true; do
+		call_modprobe > /dev/null 2>&1
+		modprobe -r $TEST_DRIVER > /dev/null 2>&1
+	done > /dev/null 2>&1
+}
+
+write_loop()
+{
+	while true; do
+		set_test_ignore_errors > /dev/null 2>&1
+		TEST_STR=$(( $TEST_STR + 1 ))
+	done > /dev/null 2>&1
+}
+
+write_loop_reset()
+{
+	while true; do
+		set_test_ignore_errors > /dev/null 2>&1
+		debugfs_reset_first_test_dev_ignore_errors > /dev/null 2>&1
+	done > /dev/null 2>&1
+}
+
+write_loop_bg()
+{
+	BG_WRITES=1000 > /dev/null 2>&1
+	while true; do
+		for i in $(seq 1 $BG_WRITES); do
+			set_test_ignore_errors > /dev/null 2>&1 &
+			TEST_STR=$(( $TEST_STR + 1 ))
+		done > /dev/null 2>&1
+		wait
+	done > /dev/null 2>&1
+	wait
+}
+
+reset_loop()
+{
+	while true; do
+		debugfs_reset_first_test_dev_ignore_errors > /dev/null 2>&1
+	done > /dev/null 2>&1
+}
+
+kill_trigger_loop()
+{
+
+	local my_first_loop_pid=$1
+	local my_second_loop_pid=$2
+	local my_sleep_max=$3
+	local my_loop=0
+
+	while true; do
+		sleep 1
+		if [[ $my_loop -ge $my_sleep_max ]]; then
+			break
+		fi
+		let my_loop=$my_loop+1
+	done
+
+	kill -s TERM $my_first_loop_pid 2>&1 > /dev/null
+	kill -s TERM $my_second_loop_pid 2>&1 > /dev/null
+}
+
+_dmesg_since_test_start()
+{
+	# search the dmesg log of last run of $seqnum for possible failures
+	# use sed \cregexpc address type, since $seqnum contains "/"
+	dmesg | tac | sed -ne "0,\#run fstests $seqnum at $date_time#p" | tac
+}
+
+sysfs_test_0001()
+{
+	TARGET="${DIR}/$(get_test_target 0001)"
+	config_reset
+	reset_vals
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+
+	run_numerictests_single_write
+}
+
+sysfs_test_0002()
+{
+	TARGET="${DIR}/$(get_test_target 0002)"
+	config_reset
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+
+	run_numerictests_single_write
+}
+
+sysfs_test_0003()
+{
+	TARGET="${DIR}/$(get_test_target 0003)"
+	config_reset
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+
+	config_enable_lock
+
+	run_numerictests_single_write
+}
+
+sysfs_test_0004()
+{
+	TARGET="${DIR}/$(get_test_target 0004)"
+	config_reset
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+
+	config_enable_lock
+
+	run_numerictests_single_write
+}
+
+sysfs_test_0005()
+{
+	TARGET="${DIR}/$(get_test_target 0005)"
+	modprobe_reset
+	config_reset
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Loop writing x while loading/unloading the module... "
+
+	modprobe_loop &
+	modprobe_pid=$!
+
+	write_loop &
+	write_pid=$!
+
+	kill_trigger_loop $modprobe_pid $write_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+sysfs_test_0006()
+{
+	TARGET="${DIR}/$(get_test_target 0006)"
+	modprobe_reset
+	config_reset
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Loop writing y while loading/unloading the module... "
+	modprobe_loop &
+	modprobe_pid=$!
+
+	write_loop &
+	write_pid=$!
+
+	kill_trigger_loop $modprobe_pid $write_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+sysfs_test_0007()
+{
+	TARGET="${DIR}/$(get_test_target 0007)"
+	modprobe_reset
+	config_reset
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Loop writing y with a larger delay while loading/unloading the module... "
+
+	MODPROBE_ARGS="write_delay_msec_y=1500"
+	modprobe_loop > /dev/null 2>&1 &
+	modprobe_pid=$!
+	unset MODPROBE_ARGS
+
+	write_loop &
+	write_pid=$!
+
+	kill_trigger_loop $modprobe_pid $write_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+sysfs_test_0008()
+{
+	TARGET="${DIR}/$(get_test_target 0008)"
+	modprobe_reset
+	config_reset
+	reset_vals
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Loop busy writing x while loading/unloading the module... "
+
+	modprobe_loop > /dev/null 2>&1 &
+	modprobe_pid=$!
+
+	write_loop_bg > /dev/null 2>&1 &
+	write_pid=$!
+
+	kill_trigger_loop $modprobe_pid $write_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+sysfs_test_0009()
+{
+	TARGET="${DIR}/$(get_test_target 0009)"
+	modprobe_reset
+	config_reset
+	reset_vals
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Loop busy writing y while loading/unloading the module... "
+
+	modprobe_loop > /dev/null 2>&1 &
+	modprobe_pid=$!
+
+	write_loop_bg > /dev/null 2>&1 &
+	write_pid=$!
+
+	kill_trigger_loop $modprobe_pid $write_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+sysfs_test_0010()
+{
+	TARGET="${DIR}/$(get_test_target 0010)"
+	modprobe_reset
+	config_reset
+	reset_vals
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Loop busy writing y with a larger delay while loading/unloading the module... "
+	modprobe -q -r $TEST_DRIVER > /dev/null 2>&1
+
+	MODPROBE_ARGS="write_delay_msec_y=1500"
+	modprobe_loop > /dev/null 2>&1 &
+	modprobe_pid=$!
+	unset MODPROBE_ARGS
+
+	write_loop_bg > /dev/null 2>&1 &
+	write_pid=$!
+
+	kill_trigger_loop $modprobe_pid $write_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+sysfs_test_0011()
+{
+	TARGET="${DIR}/$(get_test_target 0011)"
+	modprobe_reset_enable_debugfs
+	config_reset
+	reset_vals
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Loop writing x and resetting ... "
+
+	write_loop > /dev/null 2>&1 &
+	write_pid=$!
+
+	reset_loop > /dev/null 2>&1 &
+	reset_pid=$!
+
+	kill_trigger_loop $write_pid $reset_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+sysfs_test_0012()
+{
+	TARGET="${DIR}/$(get_test_target 0012)"
+	modprobe_reset_enable_debugfs
+	config_reset
+	reset_vals
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Loop writing y and resetting ... "
+
+	write_loop > /dev/null 2>&1 &
+	write_pid=$!
+
+	reset_loop > /dev/null 2>&1 &
+	reset_pid=$!
+
+	kill_trigger_loop $write_pid $reset_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+sysfs_test_0013()
+{
+	TARGET="${DIR}/$(get_test_target 0013)"
+	modprobe_reset_enable_debugfs
+	config_reset
+	reset_vals
+	config_write_delay_msec_y 1500
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Loop writing y with a larger delay and resetting ... "
+
+	write_loop > /dev/null 2>&1 &
+	write_pid=$!
+
+	reset_loop > /dev/null 2>&1 &
+	reset_pid=$!
+
+	kill_trigger_loop $write_pid $reset_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+sysfs_test_0014()
+{
+	sysfs_test_0001
+}
+
+sysfs_test_0015()
+{
+	sysfs_test_0002
+}
+
+sysfs_test_0016()
+{
+	sysfs_test_0003
+}
+
+sysfs_test_0017()
+{
+	sysfs_test_0004
+}
+
+sysfs_test_0018()
+{
+	sysfs_test_0005
+}
+
+sysfs_test_0019()
+{
+	sysfs_test_0006
+}
+
+sysfs_test_0020()
+{
+	sysfs_test_0007
+}
+
+sysfs_test_0021()
+{
+	sysfs_test_0008
+}
+
+sysfs_test_0022()
+{
+	sysfs_test_0009
+}
+
+sysfs_test_0023()
+{
+	sysfs_test_0010
+}
+
+sysfs_test_0024()
+{
+	sysfs_test_0011
+}
+
+sysfs_test_0025()
+{
+	sysfs_test_0012
+}
+
+sysfs_test_0026()
+{
+	sysfs_test_0013
+}
+
+sysfs_test_0027()
+{
+	TARGET="${DIR}/$(get_test_target 0027)"
+	modprobe_reset_enable_lock_on_rmmod
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Test for possible rmmod deadlock while writing x ... "
+
+	write_loop > /dev/null 2>&1 &
+	write_pid=$!
+
+	MODPROBE_ARGS="enable_lock=1 enable_lock_on_rmmod=1 enable_verbose_writes=1"
+	modprobe_loop > /dev/null 2>&1 &
+	modprobe_pid=$!
+	unset MODPROBE_ARGS
+
+	kill_trigger_loop $modprobe_pid $write_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+sysfs_test_0028()
+{
+	TARGET="${DIR}/$(get_test_target 0028)"
+	modprobe_reset_enable_lock_on_rmmod
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+	WAIT_TIME=2
+
+	echo -n "Test for possible rmmod deadlock using rtnl_lock while writing x ... "
+
+	write_loop > /dev/null 2>&1 &
+	write_pid=$!
+
+	MODPROBE_ARGS="enable_lock=1 enable_lock_on_rmmod=1 use_rtnl_lock=1 enable_verbose_writes=1"
+	modprobe_loop > /dev/null 2>&1 &
+	modprobe_pid=$!
+	unset MODPROBE_ARGS
+
+	kill_trigger_loop $modprobe_pid $write_pid $WAIT_TIME > /dev/null 2>&1 &
+	kill_pid=$!
+
+	wait $kill_pid > /dev/null 2>&1
+
+	if [[ $? -eq 0 ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+test_gen_desc()
+{
+	echo -n "$1 x $(get_test_count $1)"
+}
+
+list_tests()
+{
+	echo "Test ID list:"
+	echo
+	echo "TEST_ID x NUM_TEST"
+	echo "TEST_ID:   Test ID"
+	echo "NUM_TESTS: Number of recommended times to run the test"
+	echo
+	echo "$(test_gen_desc 0001) - misc test writing x in different ways"
+	echo "$(test_gen_desc 0002) - misc test writing y in different ways"
+	echo "$(test_gen_desc 0003) - misc test writing x in different ways using a mutex lock"
+	echo "$(test_gen_desc 0004) - misc test writing y in different ways using a mutex lock"
+	echo "$(test_gen_desc 0005) - misc test writing x load and remove the test_sysfs module"
+	echo "$(test_gen_desc 0006) - misc writing y load and remove the test_sysfs module"
+	echo "$(test_gen_desc 0007) - misc test writing y larger delay, load, remove test_sysfs"
+	echo "$(test_gen_desc 0008) - misc test busy writing x remove test_sysfs module"
+	echo "$(test_gen_desc 0009) - misc test busy writing y remove the test_sysfs module"
+	echo "$(test_gen_desc 0010) - misc test busy writing y larger delay, remove test_sysfs"
+	echo "$(test_gen_desc 0011) - misc test writing x and resetting device"
+	echo "$(test_gen_desc 0012) - misc test writing y and resetting device"
+	echo "$(test_gen_desc 0013) - misc test writing y with a larger delay and resetting device"
+	echo "$(test_gen_desc 0014) - block test writing x in different ways"
+	echo "$(test_gen_desc 0015) - block test writing y in different ways"
+	echo "$(test_gen_desc 0016) - block test writing x in different ways using a mutex lock"
+	echo "$(test_gen_desc 0017) - block test writing y in different ways using a mutex lock"
+	echo "$(test_gen_desc 0018) - block test writing x load and remove the test_sysfs module"
+	echo "$(test_gen_desc 0019) - block test writing y load and remove the test_sysfs module"
+	echo "$(test_gen_desc 0020) - block test writing y larger delay, load, remove test_sysfs"
+	echo "$(test_gen_desc 0021) - block test busy writing x remove the test_sysfs module"
+	echo "$(test_gen_desc 0022) - block test busy writing y remove the test_sysfs module"
+	echo "$(test_gen_desc 0023) - block test busy writing y larger delay, remove test_sysfs"
+	echo "$(test_gen_desc 0024) - block test writing x and resetting device"
+	echo "$(test_gen_desc 0025) - block test writing y and resetting device"
+	echo "$(test_gen_desc 0026) - block test writing y larger delay and resetting device"
+	echo "$(test_gen_desc 0027) - test rmmod deadlock while writing x ... "
+	echo "$(test_gen_desc 0028) - test rmmod deadlock using rtnl_lock while writing x ..."
+}
+
+usage()
+{
+	NUM_TESTS=$(grep -o ' ' <<<"$ALL_TESTS" | grep -c .)
+	let NUM_TESTS=$NUM_TESTS+1
+	MAX_TEST=$(printf "%04d\n" $NUM_TESTS)
+	echo "Usage: $0 [ -t <4-number-digit> ] | [ -w <4-number-digit> ] |"
+	echo "		 [ -s <4-number-digit> ] | [ -c <4-number-digit> <test- count>"
+	echo "           [ all ] [ -h | --help ] [ -l ]"
+	echo ""
+	echo "Valid tests: 0001-$MAX_TEST"
+	echo ""
+	echo "    all     Runs all tests (default)"
+	echo "    -t      Run test ID the number amount of times is recommended"
+	echo "    -w      Watch test ID run until it runs into an error"
+	echo "    -c      Run test ID once"
+	echo "    -s      Run test ID x test-count number of times"
+	echo "    -l      List all test ID list"
+	echo " -h|--help  Help"
+	echo
+	echo "If an error every occurs execution will immediately terminate."
+	echo "If you are adding a new test try using -w <test-ID> first to"
+	echo "make sure the test passes a series of tests."
+	echo
+	echo Example uses:
+	echo
+	echo "$TEST_NAME.sh            -- executes all tests"
+	echo "$TEST_NAME.sh -t 0002    -- Executes test ID 0002 number of times is recomended"
+	echo "$TEST_NAME.sh -w 0002    -- Watch test ID 0002 run until an error occurs"
+	echo "$TEST_NAME.sh -s 0002    -- Run test ID 0002 once"
+	echo "$TEST_NAME.sh -c 0002 3  -- Run test ID 0002 three times"
+	echo
+	list_tests
+	exit 1
+}
+
+test_num()
+{
+	re='^[0-9]+$'
+	if ! [[ $1 =~ $re ]]; then
+		usage
+	fi
+}
+
+get_test_count()
+{
+	test_num $1
+	TEST_NUM=$(echo $1 | sed 's/^0*//')
+	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$TEST_NUM'}')
+	echo ${TEST_DATA} | awk -F":" '{print $2}'
+}
+
+get_test_enabled()
+{
+	test_num $1
+	TEST_NUM=$(echo $1 | sed 's/^0*//')
+	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$TEST_NUM'}')
+	echo ${TEST_DATA} | awk -F":" '{print $3}'
+}
+
+get_test_target()
+{
+	test_num $1
+	TEST_NUM=$(echo $1 | sed 's/^0*//')
+	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$TEST_NUM'}')
+	echo ${TEST_DATA} | awk -F":" '{print $4}'
+}
+
+get_test_type()
+{
+	test_num $1
+	TEST_NUM=$(echo $1 | sed 's/^0*//')
+	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$TEST_NUM'}')
+	echo ${TEST_DATA} | awk -F":" '{print $5}'
+}
+
+run_all_tests()
+{
+	for i in $ALL_TESTS ; do
+		TEST_ID=$(echo $i | awk -F":" '{print $1}')
+		ENABLED=$(get_test_enabled $TEST_ID)
+		TEST_COUNT=$(get_test_count $TEST_ID)
+		TEST_TARGET=$(get_test_target $TEST_ID)
+		if [[ $ENABLED -eq "1" ]]; then
+			test_case $TEST_ID $TEST_COUNT $TEST_TARGET
+		else
+			echo -n "Skipping test $TEST_ID as its disabled, likely "
+			echo "could crash your system ..."
+		fi
+	done
+}
+
+watch_log()
+{
+	if [ $# -ne 3 ]; then
+		clear
+	fi
+	echo "Running test: $2 - run #$1"
+}
+
+watch_case()
+{
+	i=0
+	while [ 1 ]; do
+		if [ $# -eq 1 ]; then
+			test_num $1
+			watch_log $i ${TEST_NAME}_test_$1
+			log_kernel_fstest_dmesg sysfs $1
+			RUN_TEST=${TEST_NAME}_test_$1
+			$RUN_TEST
+			check_dmesg
+			if [[ $? -ne 0 ]]; then
+				exit 1
+			fi
+		else
+			watch_log $i all
+			run_all_tests
+		fi
+		let i=$i+1
+	done
+}
+
+test_case()
+{
+	NUM_TESTS=$2
+
+	i=0
+
+	load_modreqs $1
+	if target_exists $3 $1; then
+		return
+	fi
+
+	while [[ $i -lt $NUM_TESTS ]]; do
+		test_num $1
+		watch_log $i ${TEST_NAME}_test_$1 noclear
+		log_kernel_fstest_dmesg sysfs $1
+		RUN_TEST=${TEST_NAME}_test_$1
+		$RUN_TEST
+		let i=$i+1
+	done
+	check_dmesg
+	if [[ $? -ne 0 ]]; then
+		exit 1
+	fi
+}
+
+parse_args()
+{
+	if [ $# -eq 0 ]; then
+		run_all_tests
+	else
+		if [[ "$1" = "all" ]]; then
+			run_all_tests
+		elif [[ "$1" = "-w" ]]; then
+			shift
+			watch_case $@
+		elif [[ "$1" = "-t" ]]; then
+			shift
+			test_num $1
+			test_case $1 $(get_test_count $1) $(get_test_target $1)
+			shift
+		elif [[ "$1" = "-c" ]]; then
+			shift
+			test_num $1
+			test_num $2
+			test_case $1 $2 $(get_test_target $1)
+			shift
+			shift
+		elif [[ "$1" = "-s" ]]; then
+			shift
+			test_case $1 1 $(get_test_target $1)
+			shift
+		elif [[ "$1" = "-l" ]]; then
+			list_tests
+			shift
+		elif [[ "$1" = "-h" || "$1" = "--help" ]]; then
+			usage
+		else
+			usage
+		fi
+	fi
+}
+
+test_reqs
+allow_user_defaults
+
+trap "test_finish" EXIT
+
+parse_args $@
+
+exit 0
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v9 4/6] kernfs: add initial failure injection support
  2021-10-29 18:44 [PATCH v9 0/6] test_sysfs: add new selftest for sysfs Luis Chamberlain
                   ` (2 preceding siblings ...)
  2021-10-29 18:44 ` [PATCH v9 3/6] selftests: add tests_sysfs module Luis Chamberlain
@ 2021-10-29 18:44 ` Luis Chamberlain
  2021-10-29 18:44 ` [PATCH v9 5/6] test_sysfs: add support to use kernfs failure injection Luis Chamberlain
  2021-10-29 18:45 ` [PATCH v9 6/6] kernel/module: add documentation for try_module_get() Luis Chamberlain
  5 siblings, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2021-10-29 18:44 UTC (permalink / raw)
  To: tj, gregkh, akpm, jeyu, shuah
  Cc: bvanassche, dan.j.williams, joe, tglx, mcgrof, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel

This adds initial failure injection support to kernfs. We start
off with debug knobs which when enabled allow test drivers, such as
test_sysfs, to then make use of these to try to force certain
difficult races to take place with a high degree of certainty.

This only adds runtime code *iff* the new bool CONFIG_FAIL_KERNFS_KNOBS is
enabled in your kernel. If you don't have this enabled this provides
no new functional. When CONFIG_FAIL_KERNFS_KNOBS is disabled the new
routine may_wait_kernfs_debug() ends up being transformed to a no-op
and so the compiler should optimize these out as dead code producing no
new effective binary changes.

We start off with enabling failure injections in kernfs by allowing us to
alter the way kernfs_fop_write_iter() behaves. We allow for the routine
kernfs_fop_write_iter() to wait for a certain condition in the kernel to
occur, after which it will sleep a configurable amount of time. This lets
kernfs users to time exactly when it want kernfs_fop_write_iter() to
complete, allowing for developing race conditions and test for correctness
in kernfs.

You'd boot with this enabled on your kernel command line:

fail_kernfs_fop_write_iter=1,100,0,1

The values are <interval,probability,size,times>, we don't care for
size, so for now we ignore it. The above ensures a failure will trigger
only once.

*How* we allow for this routine to change behaviour is left to knobs we
expose under debugfs:

 # ls -1 /sys/kernel/debug/fail_kernfs/config_fail_kernfs_fop_write_iter/
wait_after_active
wait_after_mutex
wait_at_start
wait_before_mutex

A debugfs entry also exists to allow us to sleep a configurabler amount
of time after the completion:

/sys/kernel/debug/fail_kernfs/sleep_after_wait_ms

These two sets of knobs allow us to construct races and demonstrate
how the kernfs active reference should suffice to protect against
races.

Enabling CONFIG_FAULT_INJECTION_DEBUG_FS enables us to configure the
different fault injection parameters for the new fail_kernfs_fop_write_iter
fault injection at run time:

ls -1 /sys/kernel/debug/fail_kernfs/fail_kernfs_fop_write_iter/
interval
probability
space
task-filter
times
verbose
verbose_ratelimit_burst
verbose_ratelimit_interval_ms

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 .../fault-injection/fault-injection.rst       | 50 ++++++++++
 MAINTAINERS                                   |  2 +-
 fs/kernfs/Makefile                            |  1 +
 fs/kernfs/fault_inject.c                      | 93 +++++++++++++++++++
 fs/kernfs/file.c                              |  9 ++
 fs/kernfs/kernfs-internal.h                   | 70 ++++++++++++++
 include/linux/kernfs.h                        |  5 +
 lib/Kconfig.debug                             | 10 ++
 8 files changed, 239 insertions(+), 1 deletion(-)
 create mode 100644 fs/kernfs/fault_inject.c

diff --git a/Documentation/fault-injection/fault-injection.rst b/Documentation/fault-injection/fault-injection.rst
index 4a25c5eb6f07..af05f285b441 100644
--- a/Documentation/fault-injection/fault-injection.rst
+++ b/Documentation/fault-injection/fault-injection.rst
@@ -28,6 +28,19 @@ Available fault injection capabilities
 
   injects kernel RPC client and server failures.
 
+- fail_kernfs_fop_write_iter
+
+  Allows for failures to be enabled inside kernfs_fop_write_iter(). Enabling
+  this does not immediately enable any errors to occur. You must configure
+  how you want this routine to fail or change behaviour by using the respective
+  debugfs knobs for it documented below under
+  /sys/kernel/debug/fail_kernfs/config_fail_kernfs_fop_write_iter/
+
+  If you enable CONFIG_FAULT_INJECTION_DEBUG_FS the fail_add_disk failure
+  injection parameters are placed under:
+
+  /sys/kernel/debug/kernfs/fail_kernfs_fop_write_iter/
+
 - fail_make_request
 
   injects disk IO errors on devices permitted by setting
@@ -155,6 +168,43 @@ configuration of fault-injection capabilities.
 	default is 'N', setting it to 'Y' will disable failure injections
 	when dealing with private (address space) futexes.
 
+- /sys/kernel/debug/fail_kernfs/sleep_after_wait_ms
+
+	Format: { digit-representing-milliseconds }
+
+  Configure how long to sleep in ms after a kernfs_debug_wait_completion
+  wait for completion. This completion is currently only used if any of the
+  waits are enabled under
+  /sys/kernel/debug/fail_kernfs/config_fail_kernfs_fop_write_iter/
+
+- /sys/kernel/debug/fail_kernfs/config_fail_kernfs_fop_write_iter/wait_after_active
+
+	Format: { 'Y' | 'N' }
+
+  Whether or not to issue a wait for completion after the kernfs_get_active()
+  completes on the kernfs_fop_write_iter() routine.
+
+- /sys/kernel/debug/fail_kernfs/config_fail_kernfs_fop_write_iter/wait_after_mutex
+
+	Format: { 'Y' | 'N' }
+
+  Whether or not to issue a wait for completion after the struct
+  kernfs_open_file mutex is taken on the routine kernfs_fop_write_iter().
+
+- /sys/kernel/debug/fail_kernfs/config_fail_kernfs_fop_write_iter/wait_at_start
+
+	Format: { 'Y' | 'N' }
+
+  Whether or not to issue a wait for completion after the very beginning of the
+  routine kernfs_fop_write_iter().
+
+- /sys/kernel/debug/fail_kernfs/config_fail_kernfs_fop_write_iter/wait_before_mutex
+
+	Format: { 'Y' | 'N' }
+
+  Whether or not to issue a wait for completion before the struct
+  kernfs_open_file mutex is taken on the routine kernfs_fop_write_iter().
+
 - /sys/kernel/debug/fail_sunrpc/ignore-client-disconnect:
 
 	Format: { 'Y' | 'N' }
diff --git a/MAINTAINERS b/MAINTAINERS
index 8b458c4dd577..61a89bab1d73 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10469,7 +10469,7 @@ M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 M:	Tejun Heo <tj@kernel.org>
 S:	Supported
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
-F:	fs/kernfs/
+F:	fs/kernfs/*
 F:	include/linux/kernfs.h
 
 KEXEC
diff --git a/fs/kernfs/Makefile b/fs/kernfs/Makefile
index 4ca54ff54c98..79ac7688b108 100644
--- a/fs/kernfs/Makefile
+++ b/fs/kernfs/Makefile
@@ -4,3 +4,4 @@
 #
 
 obj-y		:= mount.o inode.o dir.o file.o symlink.o
+obj-$(CONFIG_FAIL_KERNFS_KNOBS)    += fault_inject.o
diff --git a/fs/kernfs/fault_inject.c b/fs/kernfs/fault_inject.c
new file mode 100644
index 000000000000..f887f26646aa
--- /dev/null
+++ b/fs/kernfs/fault_inject.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/fault-inject.h>
+#include <linux/delay.h>
+
+#include "kernfs-internal.h"
+
+static DECLARE_FAULT_ATTR(fail_kernfs_fop_write_iter);
+struct kernfs_config_fail kernfs_config_fail;
+
+#define kernfs_config_fail(when) \
+	kernfs_config_fail.kernfs_fop_write_iter_fail.wait_ ## when
+
+#define kernfs_config_fail(when) \
+	kernfs_config_fail.kernfs_fop_write_iter_fail.wait_ ## when
+
+static int __init setup_fail_kernfs_fop_write_iter(char *str)
+{
+	return setup_fault_attr(&fail_kernfs_fop_write_iter, str);
+}
+
+__setup("fail_kernfs_fop_write_iter=", setup_fail_kernfs_fop_write_iter);
+
+struct dentry *kernfs_debugfs_root;
+struct dentry *config_fail_kernfs_fop_write_iter;
+
+static int __init kernfs_init_failure_injection(void)
+{
+	kernfs_config_fail.sleep_after_wait_ms = 100;
+	kernfs_debugfs_root = debugfs_create_dir("fail_kernfs", NULL);
+
+	fault_create_debugfs_attr("fail_kernfs_fop_write_iter",
+				  kernfs_debugfs_root, &fail_kernfs_fop_write_iter);
+
+	config_fail_kernfs_fop_write_iter =
+		debugfs_create_dir("config_fail_kernfs_fop_write_iter",
+				   kernfs_debugfs_root);
+
+	debugfs_create_u32("sleep_after_wait_ms", 0600,
+			   kernfs_debugfs_root,
+			   &kernfs_config_fail.sleep_after_wait_ms);
+
+	debugfs_create_bool("wait_at_start", 0600,
+			    config_fail_kernfs_fop_write_iter,
+			    &kernfs_config_fail(at_start));
+	debugfs_create_bool("wait_before_mutex", 0600,
+			    config_fail_kernfs_fop_write_iter,
+			    &kernfs_config_fail(before_mutex));
+	debugfs_create_bool("wait_after_mutex", 0600,
+			    config_fail_kernfs_fop_write_iter,
+			    &kernfs_config_fail(after_mutex));
+	debugfs_create_bool("wait_after_active", 0600,
+			    config_fail_kernfs_fop_write_iter,
+			    &kernfs_config_fail(after_active));
+	return 0;
+}
+late_initcall(kernfs_init_failure_injection);
+
+DECLARE_COMPLETION(kernfs_debug_wait_completion);
+EXPORT_SYMBOL_NS_GPL(kernfs_debug_wait_completion, KERNFS_DEBUG_PRIVATE);
+
+static void kernfs_debug_wait(void)
+{
+	unsigned long timeout;
+
+	timeout = wait_for_completion_timeout(&kernfs_debug_wait_completion,
+					      msecs_to_jiffies(3000));
+	if (!timeout)
+		pr_info("waiting for kernfs_debug_wait_completion timed out\n");
+	else
+		pr_info("received completion with time left on timeout %u ms\n",
+			jiffies_to_msecs(timeout));
+
+	/**
+	 * The goal is wait for an event, and *then* once we have
+	 * reached it, the other side will try to do something which
+	 * it thinks will break. So we must give it some time to do
+	 * that. The amount of time is configurable.
+	 */
+	msleep(kernfs_config_fail.sleep_after_wait_ms);
+	pr_info("wait for kernfs_debug_wait_completion ended\n");
+}
+
+void __kernfs_debug_should_wait_kernfs_fop_write_iter(bool evaluate)
+{
+	if (!evaluate)
+		return;
+
+	if (should_fail(&fail_kernfs_fop_write_iter, 0))
+		kernfs_debug_wait();
+}
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 60e2a86c535e..860fc8912520 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -259,6 +259,8 @@ static ssize_t kernfs_fop_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	const struct kernfs_ops *ops;
 	char *buf;
 
+	may_wait_kernfs_debug(kernfs_fop_write_iter, at_start);
+
 	if (of->atomic_write_len) {
 		if (len > of->atomic_write_len)
 			return -E2BIG;
@@ -280,17 +282,24 @@ static ssize_t kernfs_fop_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	}
 	buf[len] = '\0';	/* guarantee string termination */
 
+	may_wait_kernfs_debug(kernfs_fop_write_iter, before_mutex);
+
 	/*
 	 * @of->mutex nests outside active ref and is used both to ensure that
 	 * the ops aren't called concurrently for the same open file.
 	 */
 	mutex_lock(&of->mutex);
+
+	may_wait_kernfs_debug(kernfs_fop_write_iter, after_mutex);
+
 	if (!kernfs_get_active(of->kn)) {
 		mutex_unlock(&of->mutex);
 		len = -ENODEV;
 		goto out_free;
 	}
 
+	may_wait_kernfs_debug(kernfs_fop_write_iter, after_active);
+
 	ops = kernfs_ops(of->kn);
 	if (ops->write)
 		len = ops->write(of, buf, len, iocb->ki_pos);
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index f9cc912c31e1..278bd935982d 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -18,6 +18,7 @@
 
 #include <linux/kernfs.h>
 #include <linux/fs_context.h>
+#include <linux/stringify.h>
 
 struct kernfs_iattrs {
 	kuid_t			ia_uid;
@@ -147,4 +148,73 @@ void kernfs_drain_open_files(struct kernfs_node *kn);
  */
 extern const struct inode_operations kernfs_symlink_iops;
 
+/*
+ * failure-injection.c
+ */
+#ifdef CONFIG_FAIL_KERNFS_KNOBS
+
+/**
+ * struct kernfs_fop_write_iter_fail - how kernfs_fop_write_iter_fail fails
+ *
+ * This lets you configure what part of kernfs_fop_write_iter() should behave
+ * in a specific way to allow userspace to capture possible failures in
+ * kernfs. The wait knobs are allowed to let you design capture possible
+ * race conditions which would otherwise be difficult to reproduce. A
+ * secondary driver would tell kernfs's wait completion when it is done.
+ *
+ * The point to the wait completion failure injection tests are to confirm
+ * that the kernfs active refcount suffice to ensure other objects in other
+ * layers are also gauranteed to exist, even they are opaque to kernfs. This
+ * includes kobjects, devices, and other objects built on top of this, like
+ * the block layer when using sysfs block device attributes.
+ *
+ * @wait_at_start: waits for completion from a third party at the start of
+ *	the routine.
+ * @wait_before_mutex: waits for completion from a third party before we
+ *	are allowed to continue before the of->mutex is held.
+ * @wait_after_mutex: waits for completion from a third party after we
+ *	have held the of->mutex.
+ * @wait_after_active: waits for completion from a thid party after we
+ *	have refcounted the struct kernfs_node.
+ */
+struct kernfs_fop_write_iter_fail {
+	bool wait_at_start;
+	bool wait_before_mutex;
+	bool wait_after_mutex;
+	bool wait_after_active;
+};
+
+/**
+ * struct kernfs_config_fail - kernfs configuration for failure injection
+ *
+ * You can kernfs failure injection on boot, and in particular we currently
+ * only support failures for kernfs_fop_write_iter(). However, we don't
+ * want to always enable errors on this call when failure injection is enabled
+ * as this routine is used by many parts of the kernel for proper functionality.
+ * The compromise we make is we let userspace start enabling which parts it
+ * wants to fail after boot, if and only if failure injection has been enabled.
+ *
+ * @kernfs_fop_write_iter_fail: configuration for how we want to allow
+ *	for failure injection on kernfs_fop_write_iter()
+ * @sleep_after_wait_ms: how many ms to wait after completion is received.
+ */
+struct kernfs_config_fail {
+	struct kernfs_fop_write_iter_fail kernfs_fop_write_iter_fail;
+	u32 sleep_after_wait_ms;
+};
+
+extern struct kernfs_config_fail kernfs_config_fail;
+
+#define __kernfs_config_wait_var(func, when) \
+	(kernfs_config_fail.func  ##_fail.wait_  ##when)
+#define __kernfs_debug_should_wait_func_name(func) __kernfs_debug_should_wait_## func
+
+#define may_wait_kernfs_debug(func, when) \
+	__kernfs_debug_should_wait_func_name(func)(__kernfs_config_wait_var(func, when))
+void __kernfs_debug_should_wait_kernfs_fop_write_iter(bool evaluate);
+#else
+static inline void kernfs_init_failure_injection(void) {}
+#define may_wait_kernfs_debug(func, when) do { } while(0)
+#endif
+
 #endif	/* __KERNFS_INTERNAL_H */
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 3ccce6f24548..cd968ee2b503 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -411,6 +411,11 @@ void kernfs_init(void);
 
 struct kernfs_node *kernfs_find_and_get_node_by_id(struct kernfs_root *root,
 						   u64 id);
+
+#ifdef CONFIG_FAIL_KERNFS_KNOBS
+extern struct completion kernfs_debug_wait_completion;
+#endif
+
 #else	/* CONFIG_KERNFS */
 
 static inline enum kernfs_node_type kernfs_type(struct kernfs_node *kn)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ec531b423c0e..04d2c3f53d2a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1902,6 +1902,16 @@ config FAULT_INJECTION_USERCOPY
 	  Provides fault-injection capability to inject failures
 	  in usercopy functions (copy_from_user(), get_user(), ...).
 
+config FAIL_KERNFS_KNOBS
+	bool "Fault-injection support in kernfs"
+	depends on FAULT_INJECTION
+	help
+	  Provide fault-injection capability for kernfs. This only enables
+	  the error injection functionality. To use it you must configure which
+	  which path you want to trigger on error on using debugfs under
+	  /sys/kernel/debug/kernfs/config_fail_kernfs_fop_write_iter/. By
+	  default all of these are disabled.
+
 config FAIL_MAKE_REQUEST
 	bool "Fault-injection capability for disk IO"
 	depends on FAULT_INJECTION && BLOCK
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v9 5/6] test_sysfs: add support to use kernfs failure injection
  2021-10-29 18:44 [PATCH v9 0/6] test_sysfs: add new selftest for sysfs Luis Chamberlain
                   ` (3 preceding siblings ...)
  2021-10-29 18:44 ` [PATCH v9 4/6] kernfs: add initial failure injection support Luis Chamberlain
@ 2021-10-29 18:44 ` Luis Chamberlain
  2021-10-29 18:45 ` [PATCH v9 6/6] kernel/module: add documentation for try_module_get() Luis Chamberlain
  5 siblings, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2021-10-29 18:44 UTC (permalink / raw)
  To: tj, gregkh, akpm, jeyu, shuah
  Cc: bvanassche, dan.j.williams, joe, tglx, mcgrof, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel

This extends test_sysfs with support for using the failure injection
wait completion and knobs to force a few race conditions which
demonstrates that kernfs active reference protection is sufficient
for kobject / device protection at higher layers.

This adds 4 new tests which tries to remove the device attribute
store operation in 4 different situations:

  1) at the start of kernfs_kernfs_fop_write_iter()
  2) before the of->mutex is held in kernfs_kernfs_fop_write_iter()
  3) after the of->mutex is held in kernfs_kernfs_fop_write_iter()
  4) after the kernfs node active reference is taken with
     kernfs_get_active()

A write can fail or succeed before the kernfs node active reference
is obtained with kernfs_get_active(), and the reason is that the
del_gendisk() may happen before the write or after the write is
triggered. However, regardless of the delayed used, all writes are
gauranteed to succeed after kernfs_get_active(), and so del_gendisk()
must wait for any pending writes to complete. The fact that you cannot
remove the kernfs entry while the kenfs entry is active also implies that
a module that created the respective sysfs / kernfs entry *cannot*
be removed during a sysfs operation. Test number 32 provides us with
proof of this. If it were not true test #32 should crash.

No null dereferences are reproduced, even though this has been observed
in some complex testing cases [0]. If this issue really exists we should
have enough tools on the sysfs_test toolbox now to try to reproduce
this easily without having to poke around other drivers. It very likley
was the case that the issue reported [0] was possibly a side issue after
the first bug which was zram specific. This is why it is important to
isolate the issue and try to reproduce it in a generic form using the
test_sysfs driver.

[0] https://lkml.kernel.org/r/20210623215007.862787-1-mcgrof@kernel.org

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 lib/Kconfig.debug                      |   1 +
 lib/test_sysfs.c                       |  19 +++
 tools/testing/selftests/sysfs/config   |   3 +
 tools/testing/selftests/sysfs/sysfs.sh | 214 +++++++++++++++++++++++++
 4 files changed, 237 insertions(+)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 04d2c3f53d2a..ab3052277f23 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2359,6 +2359,7 @@ config TEST_SYSFS
 	depends on SYSFS
 	depends on NET
 	depends on BLOCK
+	depends on FAIL_KERNFS_KNOBS
 	help
 	  This builds the "test_sysfs" module. This driver enables to test the
 	  sysfs file system safely without affecting production knobs which
diff --git a/lib/test_sysfs.c b/lib/test_sysfs.c
index 2a6ec072da60..6918fddb1aed 100644
--- a/lib/test_sysfs.c
+++ b/lib/test_sysfs.c
@@ -25,6 +25,9 @@
 #include <linux/rtnetlink.h>
 #include <linux/genhd.h>
 #include <linux/blkdev.h>
+#include <linux/kernfs.h>
+
+MODULE_IMPORT_NS(KERNFS_DEBUG_PRIVATE);
 
 static bool enable_lock;
 module_param(enable_lock, bool_enable_only, 0644);
@@ -69,6 +72,11 @@ static bool enable_verbose_rmmod;
 module_param(enable_verbose_rmmod, bool_enable_only, 0644);
 MODULE_PARM_DESC(enable_verbose_rmmod, "enable verbose print messages on rmmod");
 
+static bool enable_completion_on_rmmod;
+module_param(enable_completion_on_rmmod, bool_enable_only, 0644);
+MODULE_PARM_DESC(enable_completion_on_rmmod,
+		 "enable sending a kernfs completion on rmmod");
+
 static int sysfs_test_major;
 
 /**
@@ -251,6 +259,8 @@ static ssize_t config_show(struct device *dev,
 			     enable_debugfs ? "true" : "false");
 	len += sysfs_emit_at(buf, len, "enable_verbose_writes:\t%s\n",
 			     enable_verbose_writes ? "true" : "false");
+	len += sysfs_emit_at(buf, len, "enable_completion_on_rmmod:\t%s\n",
+			enable_completion_on_rmmod ? "true" : "false");
 
 	test_dev_config_unlock(test_dev);
 
@@ -877,10 +887,19 @@ static int __init test_sysfs_init(void)
 }
 module_init(test_sysfs_init);
 
+/* The goal is to race our device removal with a pending kernfs -> store call */
+static void test_sysfs_kernfs_send_completion_rmmod(void)
+{
+	if (!enable_completion_on_rmmod)
+		return;
+	complete(&kernfs_debug_wait_completion);
+}
+
 static void __exit test_sysfs_exit(void)
 {
 	if (enable_debugfs)
 		debugfs_remove(debugfs_dir);
+	test_sysfs_kernfs_send_completion_rmmod();
 	if (delay_rmmod_ms)
 		msleep(delay_rmmod_ms);
 	unregister_test_dev_sysfs(first_test_dev);
diff --git a/tools/testing/selftests/sysfs/config b/tools/testing/selftests/sysfs/config
index 9196f452ecd5..2876a229f95b 100644
--- a/tools/testing/selftests/sysfs/config
+++ b/tools/testing/selftests/sysfs/config
@@ -1,2 +1,5 @@
 CONFIG_SYSFS=m
 CONFIG_TEST_SYSFS=m
+CONFIG_FAULT_INJECTION=y
+CONFIG_FAULT_INJECTION_DEBUG_FS=y
+CONFIG_FAIL_KERNFS_KNOBS=y
diff --git a/tools/testing/selftests/sysfs/sysfs.sh b/tools/testing/selftests/sysfs/sysfs.sh
index 802651d78427..84093ee653c6 100755
--- a/tools/testing/selftests/sysfs/sysfs.sh
+++ b/tools/testing/selftests/sysfs/sysfs.sh
@@ -51,6 +51,10 @@ ALL_TESTS="$ALL_TESTS 0025:1:1:test_dev_y:block"
 ALL_TESTS="$ALL_TESTS 0026:1:1:test_dev_y:block"
 ALL_TESTS="$ALL_TESTS 0027:1:0:test_dev_x:block" # deadlock test
 ALL_TESTS="$ALL_TESTS 0028:1:0:test_dev_x:block" # deadlock test with rntl_lock
+ALL_TESTS="$ALL_TESTS 0029:1:1:test_dev_x:block" # kernfs race removal of store
+ALL_TESTS="$ALL_TESTS 0030:1:1:test_dev_x:block" # kernfs race removal before mutex
+ALL_TESTS="$ALL_TESTS 0031:1:1:test_dev_x:block" # kernfs race removal after mutex
+ALL_TESTS="$ALL_TESTS 0032:1:1:test_dev_x:block" # kernfs race removal after active
 
 allow_user_defaults()
 {
@@ -81,6 +85,9 @@ allow_user_defaults()
 	if [ -z $SYSFS_DEBUGFS_DIR ]; then
 		SYSFS_DEBUGFS_DIR="/sys/kernel/debug/test_sysfs"
 	fi
+	if [ -z $KERNFS_DEBUGFS_DIR ]; then
+		KERNFS_DEBUGFS_DIR="/sys/kernel/debug/fail_kernfs"
+	fi
 	if [ -z $PAGE_SIZE ]; then
 		PAGE_SIZE=$(getconf PAGESIZE)
 	fi
@@ -156,6 +163,14 @@ modprobe_reset_enable_rtnl_lock_on_rmmod()
 	unset FIRST_MODPROBE_ARGS
 }
 
+modprobe_reset_enable_completion()
+{
+	FIRST_MODPROBE_ARGS="enable_completion_on_rmmod=1 enable_verbose_writes=1"
+	FIRST_MODPROBE_ARGS="$FIRST_MODPROBE_ARGS enable_verbose_rmmod=1 delay_rmmod_ms=0"
+	modprobe_reset
+	unset FIRST_MODPROBE_ARGS
+}
+
 load_req_mod()
 {
 	modprobe_reset
@@ -186,6 +201,63 @@ debugfs_reset_first_test_dev_ignore_errors()
 	echo -n "1" >"$SYSFS_DEBUGFS_DIR"/reset_first_test_dev
 }
 
+debugfs_kernfs_kernfs_fop_write_iter_exists()
+{
+	KNOB_DIR="${KERNFS_DEBUGFS_DIR}/config_fail_kernfs_fop_write_iter"
+	if [[ ! -d $KNOB_DIR ]]; then
+		echo "kernfs debugfs does not exist $KNOB_DIR"
+		return 0;
+	fi
+	KNOB_DEBUGFS="${KERNFS_DEBUGFS_DIR}/fail_kernfs_fop_write_iter"
+	if [[ ! -d $KNOB_DEBUGFS ]]; then
+		echo -n "kernfs debugfs for coniguring fail_kernfs_fop_write_iter "
+		echo "does not exist $KNOB_DIR"
+		return 0;
+	fi
+	return 1
+}
+
+debugfs_kernfs_kernfs_fop_write_iter_set_fail_once()
+{
+	KNOB_DEBUGFS="${KERNFS_DEBUGFS_DIR}/fail_kernfs_fop_write_iter"
+	echo 1 > $KNOB_DEBUGFS/interval
+	echo 100 > $KNOB_DEBUGFS/probability
+	echo 0 > $KNOB_DEBUGFS/space
+	# Disable verbose messages on the kernel ring buffer which may
+	# confuse developers with a kernel panic.
+	echo 0 > $KNOB_DEBUGFS/verbose
+
+	# Fail only once
+	echo 1 > $KNOB_DEBUGFS/times
+}
+
+debugfs_kernfs_kernfs_fop_write_iter_set_fail_never()
+{
+	KNOB_DEBUGFS="${KERNFS_DEBUGFS_DIR}/fail_kernfs_fop_write_iter"
+	echo 0 > $KNOB_DEBUGFS/times
+}
+
+debugfs_kernfs_set_wait_ms()
+{
+	SLEEP_AFTER_WAIT_MS="${KERNFS_DEBUGFS_DIR}/sleep_after_wait_ms"
+	echo $1 > $SLEEP_AFTER_WAIT_MS
+}
+
+debugfs_kernfs_disable_wait_kernfs_fop_write_iter()
+{
+	ENABLE_WAIT_KNOB="${KERNFS_DEBUGFS_DIR}/config_fail_kernfs_fop_write_iter/wait_"
+	for KNOB in ${ENABLE_WAIT_KNOB}*; do
+		echo 0 > $KNOB
+	done
+}
+
+debugfs_kernfs_enable_wait_kernfs_fop_write_iter()
+{
+	ENABLE_WAIT_KNOB="${KERNFS_DEBUGFS_DIR}/config_fail_kernfs_fop_write_iter/wait_$1"
+	echo -n "1" > $ENABLE_WAIT_KNOB
+	return $?
+}
+
 set_orig()
 {
 	if [[ ! -z $TARGET ]] && [[ ! -z $ORIG ]]; then
@@ -961,6 +1033,144 @@ sysfs_test_0028()
 	fi
 }
 
+sysfs_race_kernfs_kernfs_fop_write_iter()
+{
+	TARGET="${DIR}/$(get_test_target $1)"
+	WAIT_AT=$2
+	EXPECT_WRITE_RETURNS=$3
+	MSDELAY=$4
+
+	modprobe_reset_enable_completion
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+
+	echo -n "Test racing removal of sysfs store op with kernfs $WAIT_AT ... "
+
+	if debugfs_kernfs_kernfs_fop_write_iter_exists; then
+		echo -n "skipping test as CONFIG_FAIL_KERNFS_KNOBS "
+		echo " or CONFIG_FAULT_INJECTION_DEBUG_FS is disabled"
+		return $ksft_skip
+	fi
+
+	# Allow for failing the kernfs_kernfs_fop_write_iter call once,
+	# we'll provide exact context shortly afterwards.
+	debugfs_kernfs_kernfs_fop_write_iter_set_fail_once
+
+	# First disable all waits
+	debugfs_kernfs_disable_wait_kernfs_fop_write_iter
+
+	# Enable a wait_for_completion(&kernfs_debug_wait_completion) at the
+	# specified location inside the kernfs_fop_write_iter() routine
+	debugfs_kernfs_enable_wait_kernfs_fop_write_iter $WAIT_AT
+
+	# Configure kernfs so that after its wait_for_completion() it
+	# will msleep() this amount of time and schedule(). We figure this
+	# will be sufficient time to allow for our module removal to complete.
+	debugfs_kernfs_set_wait_ms $MSDELAY
+
+	# Now we trigger a kernfs write op, which will run kernfs_fop_write_iter,
+	# but will wait until our driver sends a respective completion
+	set_test_ignore_errors &
+	write_pid=$!
+
+	# At this point kernfs_fop_write_iter() hasn't run our op, its
+	# waiting for our completion at the specified time $WAIT_AT.
+	# We now remove our module which will send a
+	# complete(&kernfs_debug_wait_completion) right before we deregister
+	# our device and the sysfs device attributes are removed.
+	#
+	# After the completion is sent, the test_sysfs driver races with
+	# kernfs to do the device deregistration with the kernfs msleep
+	# and schedule(). This should mean we've forced trying to remove the
+	# module prior to allowing kernfs to run our store operation. If the
+	# race did happen we'll panic with a null dereference on the store op.
+	#
+	# If no race happens we should see no write operation triggered.
+	modprobe -r $TEST_DRIVER > /dev/null 2>&1
+
+	debugfs_kernfs_kernfs_fop_write_iter_set_fail_never
+
+	wait $write_pid
+
+	check_dmesg
+	if [[ $? -ne 0 ]]; then
+		echo "FAIL" >&2
+		exit 1
+	fi
+
+	# In cases where a write *can* fail or succeed, we don't care
+	# about the return code of the write, we just care we don't crash
+	# the kernel.
+	if [[ "$EXPECT_WRITE_RETURNS" == "2" ]]; then
+		echo "ok"
+		return
+	fi
+
+	if [[ $? -eq $EXPECT_WRITE_RETURNS ]]; then
+		echo "ok"
+	else
+		echo "FAIL" >&2
+	fi
+}
+
+# The test cases 0029-0032 race writes issued to sysfs files exposed by a
+# disk against del_gendisk() which removes these sysfs files. Each test case
+# forces the race to happen at different points in time where the kernfs is
+# processing a write, before the sysfs op gets called.
+#
+# The writes races against different parts of the kernfs_fop_write_iter().
+# A completion is sent by the test_sysfs driver on driver removal before
+# del_gendisk() is called so to *start* the race. The races vary by time,
+# specified in milliseconds.
+#
+# So for example test case 0029 will force the function kernfs_fop_write_iter()
+# to wait for completion *at the start* of that function. The completion is
+# issued by the test_sysfs driver on driver removal right before del_gendisk()
+# is called. However test_sysfs will also wait a configurable amount of
+# milliseconds before having del_gendisk() run. A long delay should ensure the
+# write completes.
+#
+# Test case 0030 will do the same but before mutex_lock(&of->mutex) is called
+# on kernfs_fop_write_iter(). And so on. Writes are only expected to *always*
+# succeed once kernfs_get_active() is called successfully. Before that a write
+# could succeed or fail, it will depend on what gets preempted / scheduled, and
+# so the only thing we can be sure of is we should not be crashing the kernel.
+# Before kernfs_get_active(), if an excessively long delay is used, then
+# del_gendisk() is expected to be delayed and so writes should work.
+sysfs_test_0029()
+{
+	for delay in 0 2 4 8 16 32 64 128 246 512 1024; do
+		echo "Delay-after-completion before del_gendisk(): $delay ms"
+		sysfs_race_kernfs_kernfs_fop_write_iter 0029 at_start 2 $delay
+	done
+}
+
+sysfs_test_0030()
+{
+	for delay in 0 2 4 8 16 32 64 128 246 512 1024; do
+		echo "Delay-after-completion before del_gendisk(): $delay ms"
+		sysfs_race_kernfs_kernfs_fop_write_iter 0030 before_mutex 2 $delay
+	done
+}
+
+sysfs_test_0031()
+{
+	for delay in 0 2 4 8 16 32 64 128 246 512 1024; do
+		echo "Delay-after-completion before del_gendisk(): $delay ms"
+		sysfs_race_kernfs_kernfs_fop_write_iter 0031 after_mutex 2 $delay
+	done
+}
+
+# A write only guaranteed to succeed *iff* a module removal happens *after*
+# the kernfs active reference is obtained with kernfs_get_active().
+sysfs_test_0032()
+{
+	for delay in 0 2 4 8 16 32 64 128 246 512 1024; do
+		echo "Delay-after-completion before del_gendisk(): $delay ms"
+		sysfs_race_kernfs_kernfs_fop_write_iter 0032 after_active 0 $delay
+	done
+}
+
 test_gen_desc()
 {
 	echo -n "$1 x $(get_test_count $1)"
@@ -1002,6 +1212,10 @@ list_tests()
 	echo "$(test_gen_desc 0026) - block test writing y larger delay and resetting device"
 	echo "$(test_gen_desc 0027) - test rmmod deadlock while writing x ... "
 	echo "$(test_gen_desc 0028) - test rmmod deadlock using rtnl_lock while writing x ..."
+	echo "$(test_gen_desc 0029) - racing removal of store op with kernfs at start"
+	echo "$(test_gen_desc 0030) - racing removal of store op with kernfs before mutex"
+	echo "$(test_gen_desc 0031) - racing removal of store op with kernfs after mutex"
+	echo "$(test_gen_desc 0032) - racing removal of store op with kernfs after active"
 }
 
 usage()
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v9 6/6] kernel/module: add documentation for try_module_get()
  2021-10-29 18:44 [PATCH v9 0/6] test_sysfs: add new selftest for sysfs Luis Chamberlain
                   ` (4 preceding siblings ...)
  2021-10-29 18:44 ` [PATCH v9 5/6] test_sysfs: add support to use kernfs failure injection Luis Chamberlain
@ 2021-10-29 18:45 ` Luis Chamberlain
  5 siblings, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2021-10-29 18:45 UTC (permalink / raw)
  To: tj, gregkh, akpm, jeyu, shuah
  Cc: bvanassche, dan.j.williams, joe, tglx, mcgrof, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel

There is quite a bit of tribal knowledge around proper use of
try_module_get() and that it must be used only in a context which
can ensure the module won't be gone during the operation. Document
this little bit of tribal knowledge.

I'm extending this tribal knowledge with new developments which it
seems some folks do not yet believe to be true: we can be sure a
module will exist during the lifetime of a sysfs file operation.
For proof, refer to test_sysfs test #32:

./tools/testing/selftests/sysfs/sysfs.sh -t 0032

Without this being true, the write would fail or worse,
a crash would happen, in this test. It does not.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 include/linux/module.h | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index c9f1200b2312..35c98e4196cb 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -609,10 +609,43 @@ void symbol_put_addr(void *addr);
    to handle the error case (which only happens with rmmod --wait). */
 extern void __module_get(struct module *module);
 
-/* This is the Right Way to get a module: if it fails, it's being removed,
- * so pretend it's not there. */
+/**
+ * try_module_get() - Take module refcount unless module is being removed
+ * @module: the module we should check for
+ *
+ * This can be used to try to bump the reference count of a module, so to
+ * prevent module removal. The reference count of a module is not allowed
+ * to be incremented if the module is already being removed.
+ *
+ * Care must be taken to ensure the module cannot be removed during the call to
+ * try_module_get() because otherwise use of this routine could crash the kernel
+ * when racing to remove a module. Proper care can be taken by having another
+ * entity other than the module itself increment the module reference count,
+ * or through some other means which guarantees the module could not be removed
+ * during an operation. An example of this later case is using try_module_get()
+ * in a sysfs file which the module created. The sysfs store / read file
+ * operations are gauranteed to exist through the use of kernfs's active
+ * reference (see kernfs_active()). If a sysfs file operation is being run,
+ * the module which created it must still exist as the module is in charge of
+ * removing the same sysfs file being used. A sysfs / kernfs file removal
+ * cannot happen unless the same file does not have an active reference.
+ *
+ * One of the real values to try_module_get() is the module_is_live() check
+ * which ensures this the caller of try_module_get() can yield to userspace
+ * module removal requests and fail whatever it was about to process.
+ *
+ * Returns true if the reference count was successfully incremented.
+ */
 extern bool try_module_get(struct module *module);
 
+/**
+ * module_put() - release a reference count to a module
+ * @module: the module we should release a reference count for
+ *
+ * If you successfully bump a reference count to a module with try_module_get(),
+ * when you are finished you must call module_put() to release that reference
+ * count.
+ */
 extern void module_put(struct module *module);
 
 #else /*!CONFIG_MODULE_UNLOAD*/
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 3/6] selftests: add tests_sysfs module
  2021-10-29 18:44 ` [PATCH v9 3/6] selftests: add tests_sysfs module Luis Chamberlain
@ 2021-12-03 15:29   ` Greg KH
  2021-12-09  1:48     ` Luis Chamberlain
  2022-05-22 14:37     ` Thomas Gleixner
  0 siblings, 2 replies; 36+ messages in thread
From: Greg KH @ 2021-12-03 15:29 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: tj, akpm, jeyu, shuah, bvanassche, dan.j.williams, joe, tglx,
	keescook, rostedt, minchan, linux-spdx, linux-doc, linux-block,
	linux-fsdevel, linux-kselftest, linux-kernel

On Fri, Oct 29, 2021 at 11:44:57AM -0700, Luis Chamberlain wrote:
> This adds a new selftest module which can be used to test sysfs, which
> would otherwise require using an existing driver. This lets us muck
> with a template driver to test breaking things without affecting
> system behaviour or requiring the dependencies of a real device
> driver.

Test sysfs "how"?  What exactly are you wanting to test?

I see lots of things in this code as examples of how to NOT use sysfs,
so are you testing my review cycles?  :)

> A series of 28 tests are added. Support for using two device types are
> supported:
> 
>   * misc
>   * block

So you are testing the misc and block sysfs apis from within the kernel?

> Contrary to sysctls, sysfs requires a full write to happen at once, and
> so we reduce the digit tests to single writes. Two main sysfs knobs are
> provided for testing reading/storing, one which doesn't incur any
> delays and another which can incur programmed delays. What locks are
> held, if any, are configurable, at module load time, or through dynamic
> configuration at run time.

I do not understand this paragraph at all.  What are you trying to say?

sysfs is a read/write api, yes.  That's all, nothing fancy.  What does
delays have to do with anything?

> Since sysfs is a technically filesystem, but a pseudo one, which
> requires a kernel user, our test_sysfs module and respective test script
> embraces fstests format for tests in the kernel ring bufffer. Likewise,
> a scraper for kernel crashes is provided which matches what fstests does
> as well.

What is crashing?

> Two tests are kept disabled as they are a demonstration of what not to
> do as it can cause a deadlock with sysfs. These tests provides a mechanism
> to easily show proof and demo how the deadlock can happen:

Yes you can do foolish things in sysfs, but why are you limiting
yourself to just 2 ways to shoot yourself in the foot?

Again, I do not understand the goal here at all.  What is this file for?

> Demos the deadlock with a device specific lock
> ./tools/testing/selftests/sysfs/sysfs.sh -t 0027
> 
> Demos the deadlock with rtnl_lock()
> ./tools/testing/selftests/sysfs/sysfs.sh -t 0028
> 
> Drivers should *avoid* sharing a lock on rmmod and on sysfs ops.
> 
> This selftests will shortly be expanded upon with more tests which
> require further kernel changes in order to provide better test
> coverage.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  MAINTAINERS                            |    7 +
>  lib/Kconfig.debug                      |   12 +
>  lib/Makefile                           |    1 +
>  lib/test_sysfs.c                       |  894 ++++++++++++++++++
>  tools/testing/selftests/sysfs/Makefile |   12 +
>  tools/testing/selftests/sysfs/config   |    2 +
>  tools/testing/selftests/sysfs/settings |    1 +
>  tools/testing/selftests/sysfs/sysfs.sh | 1197 ++++++++++++++++++++++++
>  8 files changed, 2126 insertions(+)
>  create mode 100644 lib/test_sysfs.c
>  create mode 100644 tools/testing/selftests/sysfs/Makefile
>  create mode 100644 tools/testing/selftests/sysfs/config
>  create mode 100644 tools/testing/selftests/sysfs/settings
>  create mode 100755 tools/testing/selftests/sysfs/sysfs.sh
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5f87f622ac18..8b458c4dd577 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18393,6 +18393,13 @@ L:	linux-mmc@vger.kernel.org
>  S:	Maintained
>  F:	drivers/mmc/host/sdhci-pci-dwc-mshc.c
>  
> +SYSFS TEST DRIVER
> +M:	Luis Chamberlain <mcgrof@kernel.org>
> +L:	linux-kernel@vger.kernel.org
> +S:	Maintained
> +F:	lib/test_sysfs.c
> +F:	tools/testing/selftests/sysfs/
> +
>  SYSTEM CONFIGURATION (SYSCON)
>  M:	Lee Jones <lee.jones@linaro.org>
>  M:	Arnd Bergmann <arnd@arndb.de>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 44a6df361016..ec531b423c0e 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2344,6 +2344,18 @@ config TEST_SYSCTL
>  
>  	  If unsure, say N.
>  
> +config TEST_SYSFS
> +	tristate "sysfs test driver"
> +	depends on SYSFS
> +	depends on NET
> +	depends on BLOCK
> +	help
> +	  This builds the "test_sysfs" module. This driver enables to test the
> +	  sysfs file system safely without affecting production knobs which
> +	  might alter system functionality.
> +
> +	  If unsure, say N.
> +
>  config BITFIELD_KUNIT
>  	tristate "KUnit test bitfield functions at runtime"
>  	depends on KUNIT
> diff --git a/lib/Makefile b/lib/Makefile
> index 364c23f15578..741c1be29781 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
>  obj-$(CONFIG_TEST_BITOPS) += test_bitops.o
>  CFLAGS_test_bitops.o += -Werror
>  obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
> +obj-$(CONFIG_TEST_SYSFS) += test_sysfs.o
>  obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o
>  obj-$(CONFIG_TEST_IDA) += test_ida.o
>  obj-$(CONFIG_KASAN_KUNIT_TEST) += test_kasan.o
> diff --git a/lib/test_sysfs.c b/lib/test_sysfs.c
> new file mode 100644
> index 000000000000..2a6ec072da60
> --- /dev/null
> +++ b/lib/test_sysfs.c
> @@ -0,0 +1,894 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1

Again, sorry, but no, I am going to object to this license as you are
only accessing a GPL-v2-only api.  Any other license on a file that
interacts with that, especially for core stuff like testing the
functionality of this code, needs to have that same license.  Sorry.

> +/*
> + * Copyright (C) 2021 Luis Chamberlain <mcgrof@kernel.org>
> + *
> + * sysfs test driver
> + *
> + * This module allows us to add race conditions which we can test for
> + * against the sysfs filesystem.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/async.h>
> +#include <linux/delay.h>
> +#include <linux/vmalloc.h>
> +#include <linux/debugfs.h>
> +#include <linux/rtnetlink.h>
> +#include <linux/genhd.h>
> +#include <linux/blkdev.h>
> +
> +static bool enable_lock;
> +module_param(enable_lock, bool_enable_only, 0644);
> +MODULE_PARM_DESC(enable_lock,
> +		 "enable locking on reads / stores from the start");

This isn't the 1990's why have module parameters when you are dealing
with sysfs?  :)


> +
> +static bool enable_lock_on_rmmod;
> +module_param(enable_lock_on_rmmod, bool_enable_only, 0644);
> +MODULE_PARM_DESC(enable_lock_on_rmmod,
> +		 "enable locking on rmmod");
> +
> +static bool use_rtnl_lock;
> +module_param(use_rtnl_lock, bool_enable_only, 0644);
> +MODULE_PARM_DESC(use_rtnl_lock,
> +		 "use an rtnl_lock instead of the device mutex_lock");
> +
> +static unsigned int write_delay_msec_y = 500;
> +module_param_named(write_delay_msec_y, write_delay_msec_y, uint, 0644);
> +MODULE_PARM_DESC(write_delay_msec_y, "msec write delay for writes to y");
> +
> +static unsigned int test_devtype;
> +module_param_named(devtype, test_devtype, uint, 0644);
> +MODULE_PARM_DESC(devtype, "device type to register");
> +
> +static bool enable_busy_alloc;
> +module_param(enable_busy_alloc, bool_enable_only, 0644);
> +MODULE_PARM_DESC(enable_busy_alloc, "do a fake allocation during writes");
> +
> +static bool enable_debugfs;
> +module_param(enable_debugfs, bool_enable_only, 0644);
> +MODULE_PARM_DESC(enable_debugfs, "enable a few debugfs files");
> +
> +static bool enable_verbose_writes;
> +module_param(enable_verbose_writes, bool_enable_only, 0644);
> +MODULE_PARM_DESC(enable_debugfs, "enable stores to print verbose information");
> +
> +static unsigned int delay_rmmod_ms;
> +module_param_named(delay_rmmod_ms, delay_rmmod_ms, uint, 0644);
> +MODULE_PARM_DESC(delay_rmmod_ms, "if set how many ms to delay rmmod before device deletion");
> +
> +static bool enable_verbose_rmmod;
> +module_param(enable_verbose_rmmod, bool_enable_only, 0644);
> +MODULE_PARM_DESC(enable_verbose_rmmod, "enable verbose print messages on rmmod");
> +
> +static int sysfs_test_major;
> +
> +/**
> + * test_config - used for configuring how the sysfs test device will behave
> + *
> + * @enable_lock: if enabled a lock will be used when reading/storing variables
> + * @enable_lock_on_rmmod: if enabled a lock will be used when reading/storing
> + *	sysfs attributes, but it will also be used to lock on rmmod. This is
> + *	useful to test for a deadlock and should serve as an example of what
> + *	drivers should *not* do.
> + * @use_rtnl_lock: if enabled instead of configuration specific mutex, we'll
> + *	use the rtnl_lock. If your test case is modifying this on the fly
> + *	while doing other stores / reads, things will break as a lock can be
> + *	left contending. Best is that tests use this knob serially, without
> + *	allowing userspace to modify other knobs while this one changes.

Why are any of these locks needed at all?  What are you trying to test?
How badly this code abuses locks and sysfs?

I do not understand how locking models matter to how the sysfs api works
at all.

Again, what exactly are you trying to test?  What portions of the sysfs
api?  I see misc and block device interactions here, those have
different apis and have nothing to do with sysfs other than they too
have sysfs interactions.

If you want to test the sysfs api, that's great, but that is not what
you are doing here, it's a mis/match of block/misc bad things.

> + * @write_delay_msec_y: the amount of delay to use when writing to y
> + * @enable_busy_alloc: if enabled we'll do a large allocation between
> + *	writes. We immediately free right away. We also schedule to give the
> + *	kernel some time to re-use any memory we don't need. This is intened
> + *	to mimic typical driver behaviour.
> + */
> +struct test_config {
> +	bool enable_lock;
> +	bool enable_lock_on_rmmod;
> +	bool use_rtnl_lock;
> +	unsigned int write_delay_msec_y;
> +	bool enable_busy_alloc;
> +};
> +
> +/**
> + * enum sysfs_test_devtype - sysfs device type
> + * @TESTDEV_TYPE_MISC: misc device type
> + * @TESTDEV_TYPE_BLOCK: use a block device for the sysfs test device.
> + */
> +enum sysfs_test_devtype {
> +	TESTDEV_TYPE_MISC = 0,
> +	TESTDEV_TYPE_BLOCK,
> +};
> +
> +/**
> + * sysfs_test_device - test device to help test sysfs
> + *
> + * @devtype: the type of device to use
> + * @config: configuration for the test
> + * @config_mutex: protects configuration of test
> + * @misc_dev: we use a misc device under the hood
> + * @disk: represents a disk when used as a block device
> + * @dev: pointer to misc_dev's own struct device
> + * @dev_idx: unique ID for test device
> + * @x: variable we can use to test read / store
> + * @y: slow variable we can use to test read / store
> + */
> +struct sysfs_test_device {
> +	enum sysfs_test_devtype devtype;
> +	struct test_config config;
> +	struct mutex config_mutex;
> +	struct miscdevice misc_dev;
> +	struct gendisk *disk;
> +	struct device *dev;

So you have one device that controls the lifecycle (misc_dev) and then
pointers to 2 others with different lifecycles?  That's odd.

And dev is not needed at all, please drop.


> +	int dev_idx;
> +	int x;
> +	int y;
> +};
> +
> +static struct sysfs_test_device *first_test_dev;
> +
> +static struct miscdevice *dev_to_misc_dev(struct device *dev)
> +{
> +	return dev_get_drvdata(dev);
> +}
> +
> +static struct sysfs_test_device *misc_dev_to_test_dev(struct miscdevice *misc_dev)
> +{
> +	return container_of(misc_dev, struct sysfs_test_device, misc_dev);
> +}
> +
> +static struct sysfs_test_device *devblock_to_test_dev(struct device *dev)
> +{
> +	return (struct sysfs_test_device *)dev_to_disk(dev)->private_data;
> +}
> +
> +static struct sysfs_test_device *devmisc_to_testdev(struct device *dev)
> +{
> +	struct miscdevice *misc_dev;
> +
> +	misc_dev = dev_to_misc_dev(dev);
> +	return misc_dev_to_test_dev(misc_dev);
> +}
> +
> +static struct sysfs_test_device *dev_to_test_dev(struct device *dev)
> +{
> +	if (test_devtype == TESTDEV_TYPE_MISC)
> +		return devmisc_to_testdev(dev);
> +	else if (test_devtype == TESTDEV_TYPE_BLOCK)
> +		return devblock_to_test_dev(dev);
> +	return NULL;
> +}
> +
> +static void test_dev_config_lock(struct sysfs_test_device *test_dev)
> +{
> +	struct test_config *config = &test_dev->config;
> +
> +	if (config->enable_lock) {
> +		if (config->use_rtnl_lock)
> +			rtnl_lock();
> +		else
> +			mutex_lock(&test_dev->config_mutex);
> +	}
> +}
> +
> +static void test_dev_config_unlock(struct sysfs_test_device *test_dev)
> +{
> +	struct test_config *config = &test_dev->config;
> +
> +	if (config->enable_lock) {
> +		if (config->use_rtnl_lock)
> +			rtnl_unlock();
> +		else
> +			mutex_unlock(&test_dev->config_mutex);
> +	}
> +}
> +
> +static void test_dev_config_lock_rmmod(struct sysfs_test_device *test_dev)
> +{
> +	struct test_config *config = &test_dev->config;
> +
> +	if (config->enable_lock_on_rmmod)
> +		test_dev_config_lock(test_dev);
> +}
> +
> +static void test_dev_config_unlock_rmmod(struct sysfs_test_device *test_dev)
> +{
> +	struct test_config *config = &test_dev->config;
> +
> +	if (config->enable_lock_on_rmmod)
> +		test_dev_config_unlock(test_dev);
> +}
> +
> +static void free_test_dev_sysfs(struct sysfs_test_device *test_dev)
> +{
> +	if (test_dev) {
> +		kfree_const(test_dev->misc_dev.name);
> +		test_dev->misc_dev.name = NULL;
> +		kfree(test_dev);
> +		test_dev = NULL;
> +	}
> +}
> +
> +static void test_sysfs_reset_vals(struct sysfs_test_device *test_dev)
> +{
> +	test_dev->x = 3;
> +	test_dev->y = 4;
> +}
> +
> +static ssize_t config_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +	int len = 0;
> +
> +	test_dev_config_lock(test_dev);
> +
> +	len += sysfs_emit_at(buf, len, "Configuration for: %s\n",
> +			     dev_name(dev));
> +	len += sysfs_emit_at(buf, len, "x:\t%d\n", test_dev->x);
> +	len += sysfs_emit_at(buf, len, "y:\t%d\n", test_dev->y);
> +	len += sysfs_emit_at(buf, len, "enable_lock:\t%s\n",
> +			     config->enable_lock ? "true" : "false");
> +	len += sysfs_emit_at(buf, len, "enable_lock_on_rmmmod:\t%s\n",
> +			     config->enable_lock_on_rmmod ? "true" : "false");
> +	len += sysfs_emit_at(buf, len, "use_rtnl_lock:\t%s\n",
> +			     config->use_rtnl_lock ? "true" : "false");
> +	len += sysfs_emit_at(buf, len, "write_delay_msec_y:\t%d\n",
> +			     config->write_delay_msec_y);
> +	len += sysfs_emit_at(buf, len, "enable_busy_alloc:\t%s\n",
> +			     config->enable_busy_alloc ? "true" : "false");
> +	len += sysfs_emit_at(buf, len, "enable_debugfs:\t%s\n",
> +			     enable_debugfs ? "true" : "false");
> +	len += sysfs_emit_at(buf, len, "enable_verbose_writes:\t%s\n",
> +			     enable_verbose_writes ? "true" : "false");

sysfs is one-value-per-file.  This is a huge violation of it and is not
allowed at all.  This function alone would cause me to reject it :(

Also, you are creating sysfs files, where are the Documentation/ABI/
entries?


> +
> +	test_dev_config_unlock(test_dev);
> +
> +	return len;
> +}
> +static DEVICE_ATTR_RO(config);
> +
> +static ssize_t reset_store(struct device *dev,
> +			   struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +
> +	/*
> +	 * We compromise and simplify this condition and do not use a lock
> +	 * here as the lock type can change.
> +	 */
> +	config->enable_lock = false;
> +	config->enable_lock_on_rmmod = false;
> +	config->use_rtnl_lock = false;
> +	config->enable_busy_alloc = false;
> +	test_sysfs_reset_vals(test_dev);

I do not understand how a lock matters here.

And you can accept any data at all?  That's not a valid test :(

> +
> +	dev_info(dev, "reset\n");
> +
> +	return count;
> +}
> +static DEVICE_ATTR_WO(reset);
> +
> +static void test_dev_busy_alloc(struct sysfs_test_device *test_dev)
> +{
> +	struct test_config *config = &test_dev->config;
> +	char *ignore;
> +
> +	if (!config->enable_busy_alloc)
> +		return;
> +
> +	ignore = kzalloc(sizeof(struct sysfs_test_device) * 10, GFP_KERNEL);
> +	kfree(ignore);
> +
> +	schedule();
> +}
> +
> +static ssize_t test_dev_x_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	int ret;
> +
> +	test_dev_busy_alloc(test_dev);
> +	test_dev_config_lock(test_dev);
> +
> +	ret = kstrtoint(buf, 10, &test_dev->x);
> +	if (ret)
> +		count = ret;
> +
> +	if (enable_verbose_writes)
> +		dev_info(test_dev->dev, "wrote x = %d\n", test_dev->x);
> +
> +	test_dev_config_unlock(test_dev);
> +
> +	return count;
> +}
> +
> +static ssize_t test_dev_x_show(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	int ret;
> +
> +	test_dev_config_lock(test_dev);
> +	ret = sysfs_emit(buf, "%d\n", test_dev->x);
> +	test_dev_config_unlock(test_dev);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(test_dev_x);
> +
> +static ssize_t test_dev_y_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config;
> +	int y;
> +	int ret;
> +
> +	test_dev_busy_alloc(test_dev);
> +	test_dev_config_lock(test_dev);
> +
> +	config = &test_dev->config;
> +
> +	ret = kstrtoint(buf, 10, &y);
> +	if (ret)
> +		count = ret;
> +
> +	msleep(config->write_delay_msec_y);
> +	test_dev->y = test_dev->x + y + 7;

What is "7" for?


> +
> +	if (enable_verbose_writes)
> +		dev_info(test_dev->dev, "wrote y = %d\n", test_dev->y);
> +
> +	test_dev_config_unlock(test_dev);
> +
> +	return count;
> +}
> +
> +static ssize_t test_dev_y_show(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	int ret;
> +
> +	test_dev_config_lock(test_dev);
> +	ret = sysfs_emit(buf, "%d\n", test_dev->y);
> +	test_dev_config_unlock(test_dev);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(test_dev_y);
> +
> +static ssize_t config_enable_lock_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t count)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +	int ret;
> +	int val;
> +
> +	ret = kstrtoint(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * We compromise for simplicty and do not lock when changing
> +	 * locking configuration, with the assumption userspace tests
> +	 * will know this.
> +	 */
> +	if (val)
> +		config->enable_lock = true;
> +	else
> +		config->enable_lock = false;
> +
> +	return count;
> +}
> +
> +static ssize_t config_enable_lock_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buf)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +	ssize_t ret;
> +
> +	test_dev_config_lock(test_dev);
> +	ret = sysfs_emit(buf, "%d\n", config->enable_lock);
> +	test_dev_config_unlock(test_dev);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(config_enable_lock);
> +
> +static ssize_t config_enable_lock_on_rmmod_store(struct device *dev,
> +						 struct device_attribute *attr,
> +						 const char *buf, size_t count)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +	int ret;
> +	int val;
> +
> +	ret = kstrtoint(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	test_dev_config_lock(test_dev);
> +	if (val)
> +		config->enable_lock_on_rmmod = true;
> +	else
> +		config->enable_lock_on_rmmod = false;
> +	test_dev_config_unlock(test_dev);
> +
> +	return count;
> +}
> +
> +static ssize_t config_enable_lock_on_rmmod_show(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +	ssize_t ret;
> +
> +	test_dev_config_lock(test_dev);
> +	ret = sysfs_emit(buf, "%d\n", config->enable_lock_on_rmmod);
> +	test_dev_config_unlock(test_dev);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(config_enable_lock_on_rmmod);
> +
> +static ssize_t config_use_rtnl_lock_store(struct device *dev,
> +					  struct device_attribute *attr,
> +					  const char *buf, size_t count)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +	int ret;
> +	int val;
> +
> +	ret = kstrtoint(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * We compromise and simplify this condition and do not use a lock
> +	 * here as the lock type can change.
> +	 */
> +	if (val)
> +		config->use_rtnl_lock = true;
> +	else
> +		config->use_rtnl_lock = false;
> +
> +	return count;
> +}
> +
> +static ssize_t config_use_rtnl_lock_show(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +
> +	return sysfs_emit(buf, "%d\n", config->use_rtnl_lock);
> +}
> +static DEVICE_ATTR_RW(config_use_rtnl_lock);
> +
> +static ssize_t config_write_delay_msec_y_store(struct device *dev,
> +					       struct device_attribute *attr,
> +					       const char *buf, size_t count)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +	int ret;
> +	int val;
> +
> +	ret = kstrtoint(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	test_dev_config_lock(test_dev);
> +	config->write_delay_msec_y = val;
> +	test_dev_config_unlock(test_dev);
> +
> +	return count;
> +}
> +
> +static ssize_t config_write_delay_msec_y_show(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +
> +	return sysfs_emit(buf, "%d\n", config->write_delay_msec_y);
> +}
> +static DEVICE_ATTR_RW(config_write_delay_msec_y);
> +
> +static ssize_t config_enable_busy_alloc_store(struct device *dev,
> +					      struct device_attribute *attr,
> +					      const char *buf, size_t count)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +	int ret;
> +	int val;
> +
> +	ret = kstrtoint(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	test_dev_config_lock(test_dev);
> +	config->enable_busy_alloc = val;
> +	test_dev_config_unlock(test_dev);
> +
> +	return count;
> +}
> +
> +static ssize_t config_enable_busy_alloc_show(struct device *dev,
> +					     struct device_attribute *attr,
> +					     char *buf)
> +{
> +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> +	struct test_config *config = &test_dev->config;
> +
> +	return sysfs_emit(buf, "%d\n", config->enable_busy_alloc);
> +}
> +static DEVICE_ATTR_RW(config_enable_busy_alloc);
> +
> +#define TEST_SYSFS_DEV_ATTR(name)		(&dev_attr_##name.attr)

Just spell it out please, no need for a macro.

> +
> +static struct attribute *test_dev_attrs[] = {
> +	/* Generic driver knobs go here */
> +	TEST_SYSFS_DEV_ATTR(config),
> +	TEST_SYSFS_DEV_ATTR(reset),
> +
> +	/* These are used to test sysfs */
> +	TEST_SYSFS_DEV_ATTR(test_dev_x),
> +	TEST_SYSFS_DEV_ATTR(test_dev_y),
> +
> +	/*
> +	 * These are configuration knobs to modify how we test sysfs when
> +	 * doing reads / stores.
> +	 */
> +	TEST_SYSFS_DEV_ATTR(config_enable_lock),
> +	TEST_SYSFS_DEV_ATTR(config_enable_lock_on_rmmod),
> +	TEST_SYSFS_DEV_ATTR(config_use_rtnl_lock),
> +	TEST_SYSFS_DEV_ATTR(config_write_delay_msec_y),
> +	TEST_SYSFS_DEV_ATTR(config_enable_busy_alloc),
> +
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(test_dev);

That's a weak "test" of how attribute groups work and how they can work.
It's pretty much the simplest way.  why?

> +static int sysfs_test_dev_alloc_miscdev(struct sysfs_test_device *test_dev)
> +{
> +	struct miscdevice *misc_dev;
> +
> +	misc_dev = &test_dev->misc_dev;
> +	misc_dev->minor = MISC_DYNAMIC_MINOR;
> +	misc_dev->name = kasprintf(GFP_KERNEL, "test_sysfs%d", test_dev->dev_idx);
> +	if (!misc_dev->name) {
> +		pr_err("Cannot alloc misc_dev->name\n");
> +		return -ENOMEM;
> +	}
> +	misc_dev->groups = test_dev_groups;
> +
> +	return 0;
> +}
> +
> +static int testdev_open(struct block_device *bdev, fmode_t mode)
> +{
> +	return -EINVAL;

Why?

> +}
> +
> +static void testdev_submit_bio(struct bio *bio)
> +{
> +}

Huh?

> +
> +static void testdev_slot_free_notify(struct block_device *bdev,
> +				     unsigned long index)
> +{
> +}

Why nothing?

> +
> +static int testdev_rw_page(struct block_device *bdev, sector_t sector,
> +			   struct page *page, unsigned int op)
> +{
> +	return -EOPNOTSUPP;
> +}

What is this doing?

> +
> +static const struct block_device_operations sysfs_testdev_ops = {
> +	.open = testdev_open,
> +	.submit_bio = testdev_submit_bio,
> +	.swap_slot_free_notify = testdev_slot_free_notify,
> +	.rw_page = testdev_rw_page,
> +	.owner = THIS_MODULE
> +};
> +
> +static int sysfs_test_dev_alloc_blockdev(struct sysfs_test_device *test_dev)
> +{
> +	int ret = -ENOMEM;
> +
> +	test_dev->disk = blk_alloc_disk(NUMA_NO_NODE);
> +	if (!test_dev->disk) {
> +		pr_err("Error allocating disk structure for device %d\n",
> +		       test_dev->dev_idx);
> +		goto out;
> +	}
> +
> +	test_dev->disk->major = sysfs_test_major;
> +	test_dev->disk->first_minor = test_dev->dev_idx + 1;
> +	test_dev->disk->fops = &sysfs_testdev_ops;
> +	test_dev->disk->private_data = test_dev;
> +	snprintf(test_dev->disk->disk_name, sizeof(test_dev->disk->disk_name),
> +		 "test_sysfs%d", test_dev->dev_idx);
> +	set_capacity(test_dev->disk, 0);
> +	blk_queue_flag_set(QUEUE_FLAG_NONROT, test_dev->disk->queue);
> +	blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, test_dev->disk->queue);
> +	blk_queue_physical_block_size(test_dev->disk->queue, PAGE_SIZE);
> +	blk_queue_max_discard_sectors(test_dev->disk->queue, UINT_MAX);
> +	blk_queue_flag_set(QUEUE_FLAG_DISCARD, test_dev->disk->queue);
> +
> +	return 0;
> +out:
> +	return ret;
> +}

What does this block device do?

> +
> +static struct sysfs_test_device *alloc_test_dev_sysfs(int idx)
> +{
> +	struct sysfs_test_device *test_dev;
> +	int ret;
> +
> +	switch (test_devtype) {
> +	case TESTDEV_TYPE_MISC:
> +	       fallthrough;
> +	case TESTDEV_TYPE_BLOCK:
> +		break;

That's the only 2 types you have, why test?

> +	default:
> +		return NULL;
> +	}
> +
> +	test_dev = kzalloc(sizeof(struct sysfs_test_device), GFP_KERNEL);
> +	if (!test_dev)
> +		goto err_out;
> +
> +	mutex_init(&test_dev->config_mutex);
> +	test_dev->dev_idx = idx;
> +	test_dev->devtype = test_devtype;
> +
> +	if (test_dev->devtype == TESTDEV_TYPE_MISC) {
> +		ret = sysfs_test_dev_alloc_miscdev(test_dev);
> +		if (ret)
> +			goto err_out_free;
> +	} else if (test_dev->devtype == TESTDEV_TYPE_BLOCK) {
> +		ret = sysfs_test_dev_alloc_blockdev(test_dev);
> +		if (ret)
> +			goto err_out_free;
> +	}
> +	return test_dev;
> +
> +err_out_free:
> +	kfree(test_dev);
> +	test_dev = NULL;
> +err_out:
> +	return NULL;
> +}
> +
> +static int register_test_dev_sysfs_misc(struct sysfs_test_device *test_dev)
> +{
> +	int ret;
> +
> +	ret = misc_register(&test_dev->misc_dev);
> +	if (ret)
> +		return ret;
> +
> +	test_dev->dev = test_dev->misc_dev.this_device;

Why are you messing with the internals of a misc device's struct device?
That's not for anyone to play with.

> +
> +	return 0;
> +}
> +
> +static int register_test_dev_sysfs_block(struct sysfs_test_device *test_dev)
> +{
> +	int ret;
> +
> +	ret = device_add_disk(NULL, test_dev->disk, test_dev_groups);
> +	if (ret) {
> +		blk_cleanup_disk(test_dev->disk);
> +		return ret;
> +	}
> +
> +	test_dev->dev = disk_to_dev(test_dev->disk);
> +
> +	return 0;
> +}
> +
> +static struct sysfs_test_device *register_test_dev_sysfs(void)
> +{
> +	struct sysfs_test_device *test_dev = NULL;
> +	int ret;
> +
> +	test_dev = alloc_test_dev_sysfs(0);
> +	if (!test_dev)
> +		goto out;
> +
> +	if (test_dev->devtype == TESTDEV_TYPE_MISC) {
> +		ret = register_test_dev_sysfs_misc(test_dev);
> +		if (ret) {
> +			pr_err("could not register misc device: %d\n", ret);
> +			goto out_free_dev;
> +		}
> +	} else if (test_dev->devtype == TESTDEV_TYPE_BLOCK) {
> +		ret = register_test_dev_sysfs_block(test_dev);
> +		if (ret) {
> +			pr_err("could not register block device: %d\n", ret);
> +			goto out_free_dev;
> +		}
> +	}
> +
> +	dev_info(test_dev->dev, "interface ready\n");
> +
> +out:
> +	return test_dev;
> +out_free_dev:
> +	free_test_dev_sysfs(test_dev);
> +	return NULL;
> +}
> +
> +static struct sysfs_test_device *register_test_dev_set_config(void)
> +{
> +	struct sysfs_test_device *test_dev;
> +	struct test_config *config;
> +
> +	test_dev = register_test_dev_sysfs();
> +	if (!test_dev)
> +		return NULL;
> +
> +	config = &test_dev->config;
> +
> +	if (enable_lock)
> +		config->enable_lock = true;
> +	if (enable_lock_on_rmmod)
> +		config->enable_lock_on_rmmod = true;
> +	if (use_rtnl_lock)
> +		config->use_rtnl_lock = true;
> +	if (enable_busy_alloc)
> +		config->enable_busy_alloc = true;
> +
> +	config->write_delay_msec_y = write_delay_msec_y;
> +	test_sysfs_reset_vals(test_dev);
> +
> +	return test_dev;
> +}
> +
> +static void unregister_test_dev_sysfs_misc(struct sysfs_test_device *test_dev)
> +{
> +	misc_deregister(&test_dev->misc_dev);
> +}
> +
> +static void unregister_test_dev_sysfs_block(struct sysfs_test_device *test_dev)
> +{
> +	del_gendisk(test_dev->disk);
> +	blk_cleanup_disk(test_dev->disk);
> +}
> +
> +static void unregister_test_dev_sysfs(struct sysfs_test_device *test_dev)
> +{
> +	test_dev_config_lock_rmmod(test_dev);
> +
> +	dev_info(test_dev->dev, "removing interface\n");
> +
> +	if (test_dev->devtype == TESTDEV_TYPE_MISC)
> +		unregister_test_dev_sysfs_misc(test_dev);
> +	else if (test_dev->devtype == TESTDEV_TYPE_BLOCK)
> +		unregister_test_dev_sysfs_block(test_dev);
> +
> +	test_dev_config_unlock_rmmod(test_dev);
> +
> +	free_test_dev_sysfs(test_dev);
> +}
> +
> +static struct dentry *debugfs_dir;

Why get debugfs involved?


> +
> +/* When read represents how many times we have reset the first_test_dev */
> +static u8 reset_first_test_dev;
> +
> +static ssize_t read_reset_first_test_dev(struct file *file,
> +					 char __user *user_buf,
> +					 size_t count, loff_t *ppos)
> +{
> +	ssize_t len;
> +	char buf[32];
> +
> +	reset_first_test_dev++;
> +	len = sprintf(buf, "%d\n", reset_first_test_dev);
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}
> +
> +static ssize_t write_reset_first_test_dev(struct file *file,
> +					  const char __user *user_buf,
> +					  size_t count, loff_t *ppos)
> +{
> +	if (!try_module_get(THIS_MODULE))

Totally racy, broken, and not allowed, sorry.


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 3/6] selftests: add tests_sysfs module
  2021-12-03 15:29   ` Greg KH
@ 2021-12-09  1:48     ` Luis Chamberlain
  2021-12-10 21:39       ` Luis Chamberlain
  2021-12-14 19:31       ` [copyleft-next] " Richard Fontana
  2022-05-22 14:37     ` Thomas Gleixner
  1 sibling, 2 replies; 36+ messages in thread
From: Luis Chamberlain @ 2021-12-09  1:48 UTC (permalink / raw)
  To: Greg KH, Petr Mladek
  Cc: tj, akpm, jeyu, shuah, bvanassche, dan.j.williams, joe, tglx,
	keescook, rostedt, minchan, linux-spdx, linux-doc, linux-block,
	linux-fsdevel, linux-kselftest, linux-kernel, copyleft-next

On Fri, Dec 03, 2021 at 04:29:02PM +0100, Greg KH wrote:
> On Fri, Oct 29, 2021 at 11:44:57AM -0700, Luis Chamberlain wrote:
> > This adds a new selftest module which can be used to test sysfs, which
> > would otherwise require using an existing driver. This lets us muck
> > with a template driver to test breaking things without affecting
> > system behaviour or requiring the dependencies of a real device
> > driver.
> 
> Test sysfs "how"?  What exactly are you wanting to test?

You can look at the 32 tests added after all patches applied.

> I see lots of things in this code as examples of how to NOT use sysfs,
> so are you testing my review cycles?  :)

You are exagerating, there are 32 tests there and only 2 tests deal
with a deadlock which we are not yet sure how widespread it could be.

> > A series of 28 tests are added. Support for using two device types are
> > supported:
> > 
> >   * misc
> >   * block
> 
> So you are testing the misc and block sysfs apis from within the kernel?

The reason for these two was to replicate typical driver setups and
environments.

> > Contrary to sysctls, sysfs requires a full write to happen at once, and
> > so we reduce the digit tests to single writes. Two main sysfs knobs are
> > provided for testing reading/storing, one which doesn't incur any
> > delays and another which can incur programmed delays. What locks are
> > held, if any, are configurable, at module load time, or through dynamic
> > configuration at run time.
> 
> I do not understand this paragraph at all.  What are you trying to say?
>
> sysfs is a read/write api, yes.  That's all, nothing fancy.  What does
> delays have to do with anything?

It is trying to clarify why tools/testing/selftests/sysctl/sysctl.sh
tests are different than    tools/testing/selftests/sysfs/sysfs.sh

> > Since sysfs is a technically filesystem, but a pseudo one, which
> > requires a kernel user, our test_sysfs module and respective test script
> > embraces fstests format for tests in the kernel ring bufffer. Likewise,
> > a scraper for kernel crashes is provided which matches what fstests does
> > as well.
> 
> What is crashing?

The kernel can crash. If it crashes then we want something to pick that up.

> > Two tests are kept disabled as they are a demonstration of what not to
> > do as it can cause a deadlock with sysfs. These tests provides a mechanism
> > to easily show proof and demo how the deadlock can happen:
> 
> Yes you can do foolish things in sysfs, but why are you limiting
> yourself to just 2 ways to shoot yourself in the foot?
>
> Again, I do not understand the goal here at all.  What is this file for?

Because clearly evidence is showing that *this* way to shoot yourself
in the foot was not clear and documented as not allowed and why. Petr
summarized the things which we need to document in a generic form:

http://lkml.kernel.org/r/YYFYFrnhwPiyOtst@alley

> > diff --git a/lib/test_sysfs.c b/lib/test_sysfs.c
> > new file mode 100644
> > index 000000000000..2a6ec072da60
> > --- /dev/null
> > +++ b/lib/test_sysfs.c
> > @@ -0,0 +1,894 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
> 
> Again, sorry, but no, I am going to object to this license as you are
> only accessing a GPL-v2-only api.  Any other license on a file that
> interacts with that, especially for core stuff like testing the
> functionality of this code, needs to have that same license.  Sorry.

Huh? The license is GPL-v2 compatible, and when used in the kernel the
GPLv2 applies.

Likewise, are you taking the position that permissively licensed code,
say BSD or ISC licensed code, cannot use EXPORT_SYMBOL_GPL() symbols?

> > +/*
> > + * Copyright (C) 2021 Luis Chamberlain <mcgrof@kernel.org>
> > + *
> > + * sysfs test driver
> > + *
> > + * This module allows us to add race conditions which we can test for
> > + * against the sysfs filesystem.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/init.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/printk.h>
> > +#include <linux/fs.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/async.h>
> > +#include <linux/delay.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/rtnetlink.h>
> > +#include <linux/genhd.h>
> > +#include <linux/blkdev.h>
> > +
> > +static bool enable_lock;
> > +module_param(enable_lock, bool_enable_only, 0644);
> > +MODULE_PARM_DESC(enable_lock,
> > +		 "enable locking on reads / stores from the start");
> 
> This isn't the 1990's why have module parameters when you are dealing
> with sysfs?  :)

It makes it easier for a test to specify its requirements from
the start. But yes these have respective device attributes too.

> > +static int sysfs_test_major;
> > +
> > +/**
> > + * test_config - used for configuring how the sysfs test device will behave
> > + *
> > + * @enable_lock: if enabled a lock will be used when reading/storing variables
> > + * @enable_lock_on_rmmod: if enabled a lock will be used when reading/storing
> > + *	sysfs attributes, but it will also be used to lock on rmmod. This is
> > + *	useful to test for a deadlock and should serve as an example of what
> > + *	drivers should *not* do.
> > + * @use_rtnl_lock: if enabled instead of configuration specific mutex, we'll
> > + *	use the rtnl_lock. If your test case is modifying this on the fly
> > + *	while doing other stores / reads, things will break as a lock can be
> > + *	left contending. Best is that tests use this knob serially, without
> > + *	allowing userspace to modify other knobs while this one changes.
> 
> Why are any of these locks needed at all?  What are you trying to test?
> How badly this code abuses locks and sysfs?
> 
> I do not understand how locking models matter to how the sysfs api works
> at all.

The rules summarized by Petr are things we need to document, this
demonstrates *why*.

> Again, what exactly are you trying to test?  What portions of the sysfs
> api?  I see misc and block device interactions here, those have
> different apis and have nothing to do with sysfs other than they too
> have sysfs interactions.

The way different misc / block devices *register* device attributes vary.
It is why both are used.

> If you want to test the sysfs api, that's great, but that is not what
> you are doing here, it's a mis/match of block/misc bad things.

After several tests, yes it does not matter which one you use.
But you are ignoring the tests mimic what we have been doing for
sysctls testing as well.

> > + * @write_delay_msec_y: the amount of delay to use when writing to y
> > + * @enable_busy_alloc: if enabled we'll do a large allocation between
> > + *	writes. We immediately free right away. We also schedule to give the
> > + *	kernel some time to re-use any memory we don't need. This is intened
> > + *	to mimic typical driver behaviour.
> > + */
> > +struct test_config {
> > +	bool enable_lock;
> > +	bool enable_lock_on_rmmod;
> > +	bool use_rtnl_lock;
> > +	unsigned int write_delay_msec_y;
> > +	bool enable_busy_alloc;
> > +};
> > +
> > +/**
> > + * enum sysfs_test_devtype - sysfs device type
> > + * @TESTDEV_TYPE_MISC: misc device type
> > + * @TESTDEV_TYPE_BLOCK: use a block device for the sysfs test device.
> > + */
> > +enum sysfs_test_devtype {
> > +	TESTDEV_TYPE_MISC = 0,
> > +	TESTDEV_TYPE_BLOCK,
> > +};
> > +
> > +/**
> > + * sysfs_test_device - test device to help test sysfs
> > + *
> > + * @devtype: the type of device to use
> > + * @config: configuration for the test
> > + * @config_mutex: protects configuration of test
> > + * @misc_dev: we use a misc device under the hood
> > + * @disk: represents a disk when used as a block device
> > + * @dev: pointer to misc_dev's own struct device
> > + * @dev_idx: unique ID for test device
> > + * @x: variable we can use to test read / store
> > + * @y: slow variable we can use to test read / store
> > + */
> > +struct sysfs_test_device {
> > +	enum sysfs_test_devtype devtype;
> > +	struct test_config config;
> > +	struct mutex config_mutex;
> > +	struct miscdevice misc_dev;
> > +	struct gendisk *disk;
> > +	struct device *dev;
> 
> So you have one device that controls the lifecycle (misc_dev) and then
> pointers to 2 others with different lifecycles?  That's odd.
> 
> And dev is not needed at all, please drop.

I use it to shortcut to the respective dev_to_test_dev() which can be
misc or block.

> > +	int dev_idx;
> > +	int x;
> > +	int y;
> > +};
> > +
> > +static struct sysfs_test_device *first_test_dev;
> > +
> > +static struct miscdevice *dev_to_misc_dev(struct device *dev)
> > +{
> > +	return dev_get_drvdata(dev);
> > +}
> > +
> > +static struct sysfs_test_device *misc_dev_to_test_dev(struct miscdevice *misc_dev)
> > +{
> > +	return container_of(misc_dev, struct sysfs_test_device, misc_dev);
> > +}
> > +
> > +static struct sysfs_test_device *devblock_to_test_dev(struct device *dev)
> > +{
> > +	return (struct sysfs_test_device *)dev_to_disk(dev)->private_data;
> > +}
> > +
> > +static struct sysfs_test_device *devmisc_to_testdev(struct device *dev)
> > +{
> > +	struct miscdevice *misc_dev;
> > +
> > +	misc_dev = dev_to_misc_dev(dev);
> > +	return misc_dev_to_test_dev(misc_dev);
> > +}
> > +
> > +static struct sysfs_test_device *dev_to_test_dev(struct device *dev)
> > +{
> > +	if (test_devtype == TESTDEV_TYPE_MISC)
> > +		return devmisc_to_testdev(dev);
> > +	else if (test_devtype == TESTDEV_TYPE_BLOCK)
> > +		return devblock_to_test_dev(dev);
> > +	return NULL;
> > +}
> > +
> > +static void test_dev_config_lock(struct sysfs_test_device *test_dev)
> > +{
> > +	struct test_config *config = &test_dev->config;
> > +
> > +	if (config->enable_lock) {
> > +		if (config->use_rtnl_lock)
> > +			rtnl_lock();
> > +		else
> > +			mutex_lock(&test_dev->config_mutex);
> > +	}
> > +}
> > +
> > +static void test_dev_config_unlock(struct sysfs_test_device *test_dev)
> > +{
> > +	struct test_config *config = &test_dev->config;
> > +
> > +	if (config->enable_lock) {
> > +		if (config->use_rtnl_lock)
> > +			rtnl_unlock();
> > +		else
> > +			mutex_unlock(&test_dev->config_mutex);
> > +	}
> > +}
> > +
> > +static void test_dev_config_lock_rmmod(struct sysfs_test_device *test_dev)
> > +{
> > +	struct test_config *config = &test_dev->config;
> > +
> > +	if (config->enable_lock_on_rmmod)
> > +		test_dev_config_lock(test_dev);
> > +}
> > +
> > +static void test_dev_config_unlock_rmmod(struct sysfs_test_device *test_dev)
> > +{
> > +	struct test_config *config = &test_dev->config;
> > +
> > +	if (config->enable_lock_on_rmmod)
> > +		test_dev_config_unlock(test_dev);
> > +}
> > +
> > +static void free_test_dev_sysfs(struct sysfs_test_device *test_dev)
> > +{
> > +	if (test_dev) {
> > +		kfree_const(test_dev->misc_dev.name);
> > +		test_dev->misc_dev.name = NULL;
> > +		kfree(test_dev);
> > +		test_dev = NULL;
> > +	}
> > +}
> > +
> > +static void test_sysfs_reset_vals(struct sysfs_test_device *test_dev)
> > +{
> > +	test_dev->x = 3;
> > +	test_dev->y = 4;
> > +}
> > +
> > +static ssize_t config_show(struct device *dev,
> > +			   struct device_attribute *attr,
> > +			   char *buf)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +	int len = 0;
> > +
> > +	test_dev_config_lock(test_dev);
> > +
> > +	len += sysfs_emit_at(buf, len, "Configuration for: %s\n",
> > +			     dev_name(dev));
> > +	len += sysfs_emit_at(buf, len, "x:\t%d\n", test_dev->x);
> > +	len += sysfs_emit_at(buf, len, "y:\t%d\n", test_dev->y);
> > +	len += sysfs_emit_at(buf, len, "enable_lock:\t%s\n",
> > +			     config->enable_lock ? "true" : "false");
> > +	len += sysfs_emit_at(buf, len, "enable_lock_on_rmmmod:\t%s\n",
> > +			     config->enable_lock_on_rmmod ? "true" : "false");
> > +	len += sysfs_emit_at(buf, len, "use_rtnl_lock:\t%s\n",
> > +			     config->use_rtnl_lock ? "true" : "false");
> > +	len += sysfs_emit_at(buf, len, "write_delay_msec_y:\t%d\n",
> > +			     config->write_delay_msec_y);
> > +	len += sysfs_emit_at(buf, len, "enable_busy_alloc:\t%s\n",
> > +			     config->enable_busy_alloc ? "true" : "false");
> > +	len += sysfs_emit_at(buf, len, "enable_debugfs:\t%s\n",
> > +			     enable_debugfs ? "true" : "false");
> > +	len += sysfs_emit_at(buf, len, "enable_verbose_writes:\t%s\n",
> > +			     enable_verbose_writes ? "true" : "false");
> 
> sysfs is one-value-per-file.  This is a huge violation of it and is not
> allowed at all.  This function alone would cause me to reject it :(

Huh? You make no sense. I am not allowing folks to input / write a full
config here to this one file, this is just a read-only trigger to output
the full configuration for the test. This makes it easy to read. And I
already have similar methodology for my other selftests.

> Also, you are creating sysfs files, where are the Documentation/ABI/
> entries?

These are selftests, there is no ABI. The ABI if you will *is* parity
with the selftest script. And those tests remain compatible with older
kernels.

> > +
> > +	test_dev_config_unlock(test_dev);
> > +
> > +	return len;
> > +}
> > +static DEVICE_ATTR_RO(config);
> > +
> > +static ssize_t reset_store(struct device *dev,
> > +			   struct device_attribute *attr,
> > +			   const char *buf, size_t count)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +
> > +	/*
> > +	 * We compromise and simplify this condition and do not use a lock
> > +	 * here as the lock type can change.
> > +	 */
> > +	config->enable_lock = false;
> > +	config->enable_lock_on_rmmod = false;
> > +	config->use_rtnl_lock = false;
> > +	config->enable_busy_alloc = false;
> > +	test_sysfs_reset_vals(test_dev);
> 
> I do not understand how a lock matters here.

Because you can technically race two scripts trying to configure
one knob, while this just tries to reset to defaults. So since
we are a selftest, and we design *how* we run the test we have
control over the fact that the way we design the test this racy
behaviour won't happen as we are not designing the test to
do that.

> And you can accept any data at all?  That's not a valid test :(

Who cares, yes, I am.

> > +
> > +	dev_info(dev, "reset\n");
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_WO(reset);
> > +
> > +static void test_dev_busy_alloc(struct sysfs_test_device *test_dev)
> > +{
> > +	struct test_config *config = &test_dev->config;
> > +	char *ignore;
> > +
> > +	if (!config->enable_busy_alloc)
> > +		return;
> > +
> > +	ignore = kzalloc(sizeof(struct sysfs_test_device) * 10, GFP_KERNEL);
> > +	kfree(ignore);
> > +
> > +	schedule();
> > +}
> > +
> > +static ssize_t test_dev_x_store(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf, size_t count)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	int ret;
> > +
> > +	test_dev_busy_alloc(test_dev);
> > +	test_dev_config_lock(test_dev);
> > +
> > +	ret = kstrtoint(buf, 10, &test_dev->x);
> > +	if (ret)
> > +		count = ret;
> > +
> > +	if (enable_verbose_writes)
> > +		dev_info(test_dev->dev, "wrote x = %d\n", test_dev->x);
> > +
> > +	test_dev_config_unlock(test_dev);
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t test_dev_x_show(struct device *dev,
> > +			       struct device_attribute *attr,
> > +			       char *buf)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	int ret;
> > +
> > +	test_dev_config_lock(test_dev);
> > +	ret = sysfs_emit(buf, "%d\n", test_dev->x);
> > +	test_dev_config_unlock(test_dev);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RW(test_dev_x);
> > +
> > +static ssize_t test_dev_y_store(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf, size_t count)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config;
> > +	int y;
> > +	int ret;
> > +
> > +	test_dev_busy_alloc(test_dev);
> > +	test_dev_config_lock(test_dev);
> > +
> > +	config = &test_dev->config;
> > +
> > +	ret = kstrtoint(buf, 10, &y);
> > +	if (ret)
> > +		count = ret;
> > +
> > +	msleep(config->write_delay_msec_y);
> > +	test_dev->y = test_dev->x + y + 7;
> 
> What is "7" for?

It does not matter what value, you just need to set a value
which userspace can know / expect for a test.

> > +
> > +	if (enable_verbose_writes)
> > +		dev_info(test_dev->dev, "wrote y = %d\n", test_dev->y);
> > +
> > +	test_dev_config_unlock(test_dev);
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t test_dev_y_show(struct device *dev,
> > +			       struct device_attribute *attr,
> > +			       char *buf)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	int ret;
> > +
> > +	test_dev_config_lock(test_dev);
> > +	ret = sysfs_emit(buf, "%d\n", test_dev->y);
> > +	test_dev_config_unlock(test_dev);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RW(test_dev_y);
> > +
> > +static ssize_t config_enable_lock_store(struct device *dev,
> > +					struct device_attribute *attr,
> > +					const char *buf, size_t count)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +	int ret;
> > +	int val;
> > +
> > +	ret = kstrtoint(buf, 10, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * We compromise for simplicty and do not lock when changing
> > +	 * locking configuration, with the assumption userspace tests
> > +	 * will know this.
> > +	 */
> > +	if (val)
> > +		config->enable_lock = true;
> > +	else
> > +		config->enable_lock = false;
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t config_enable_lock_show(struct device *dev,
> > +				       struct device_attribute *attr,
> > +				       char *buf)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +	ssize_t ret;
> > +
> > +	test_dev_config_lock(test_dev);
> > +	ret = sysfs_emit(buf, "%d\n", config->enable_lock);
> > +	test_dev_config_unlock(test_dev);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RW(config_enable_lock);
> > +
> > +static ssize_t config_enable_lock_on_rmmod_store(struct device *dev,
> > +						 struct device_attribute *attr,
> > +						 const char *buf, size_t count)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +	int ret;
> > +	int val;
> > +
> > +	ret = kstrtoint(buf, 10, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	test_dev_config_lock(test_dev);
> > +	if (val)
> > +		config->enable_lock_on_rmmod = true;
> > +	else
> > +		config->enable_lock_on_rmmod = false;
> > +	test_dev_config_unlock(test_dev);
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t config_enable_lock_on_rmmod_show(struct device *dev,
> > +						struct device_attribute *attr,
> > +						char *buf)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +	ssize_t ret;
> > +
> > +	test_dev_config_lock(test_dev);
> > +	ret = sysfs_emit(buf, "%d\n", config->enable_lock_on_rmmod);
> > +	test_dev_config_unlock(test_dev);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RW(config_enable_lock_on_rmmod);
> > +
> > +static ssize_t config_use_rtnl_lock_store(struct device *dev,
> > +					  struct device_attribute *attr,
> > +					  const char *buf, size_t count)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +	int ret;
> > +	int val;
> > +
> > +	ret = kstrtoint(buf, 10, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * We compromise and simplify this condition and do not use a lock
> > +	 * here as the lock type can change.
> > +	 */
> > +	if (val)
> > +		config->use_rtnl_lock = true;
> > +	else
> > +		config->use_rtnl_lock = false;
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t config_use_rtnl_lock_show(struct device *dev,
> > +					 struct device_attribute *attr,
> > +					 char *buf)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +
> > +	return sysfs_emit(buf, "%d\n", config->use_rtnl_lock);
> > +}
> > +static DEVICE_ATTR_RW(config_use_rtnl_lock);
> > +
> > +static ssize_t config_write_delay_msec_y_store(struct device *dev,
> > +					       struct device_attribute *attr,
> > +					       const char *buf, size_t count)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +	int ret;
> > +	int val;
> > +
> > +	ret = kstrtoint(buf, 10, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	test_dev_config_lock(test_dev);
> > +	config->write_delay_msec_y = val;
> > +	test_dev_config_unlock(test_dev);
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t config_write_delay_msec_y_show(struct device *dev,
> > +					      struct device_attribute *attr,
> > +					      char *buf)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +
> > +	return sysfs_emit(buf, "%d\n", config->write_delay_msec_y);
> > +}
> > +static DEVICE_ATTR_RW(config_write_delay_msec_y);
> > +
> > +static ssize_t config_enable_busy_alloc_store(struct device *dev,
> > +					      struct device_attribute *attr,
> > +					      const char *buf, size_t count)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +	int ret;
> > +	int val;
> > +
> > +	ret = kstrtoint(buf, 10, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	test_dev_config_lock(test_dev);
> > +	config->enable_busy_alloc = val;
> > +	test_dev_config_unlock(test_dev);
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t config_enable_busy_alloc_show(struct device *dev,
> > +					     struct device_attribute *attr,
> > +					     char *buf)
> > +{
> > +	struct sysfs_test_device *test_dev = dev_to_test_dev(dev);
> > +	struct test_config *config = &test_dev->config;
> > +
> > +	return sysfs_emit(buf, "%d\n", config->enable_busy_alloc);
> > +}
> > +static DEVICE_ATTR_RW(config_enable_busy_alloc);
> > +
> > +#define TEST_SYSFS_DEV_ATTR(name)		(&dev_attr_##name.attr)
> 
> Just spell it out please, no need for a macro.

I've been doing that for years for my other selftests.

> > +
> > +static struct attribute *test_dev_attrs[] = {
> > +	/* Generic driver knobs go here */
> > +	TEST_SYSFS_DEV_ATTR(config),
> > +	TEST_SYSFS_DEV_ATTR(reset),
> > +
> > +	/* These are used to test sysfs */
> > +	TEST_SYSFS_DEV_ATTR(test_dev_x),
> > +	TEST_SYSFS_DEV_ATTR(test_dev_y),
> > +
> > +	/*
> > +	 * These are configuration knobs to modify how we test sysfs when
> > +	 * doing reads / stores.
> > +	 */
> > +	TEST_SYSFS_DEV_ATTR(config_enable_lock),
> > +	TEST_SYSFS_DEV_ATTR(config_enable_lock_on_rmmod),
> > +	TEST_SYSFS_DEV_ATTR(config_use_rtnl_lock),
> > +	TEST_SYSFS_DEV_ATTR(config_write_delay_msec_y),
> > +	TEST_SYSFS_DEV_ATTR(config_enable_busy_alloc),
> > +
> > +	NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(test_dev);
> 
> That's a weak "test" of how attribute groups work and how they can work.
> It's pretty much the simplest way.  why?

I'm not testing attribute groups here yet in this selftest other than
this simple case. So far this selftests is testing reads/writes and
races.

We can later extend how we we define attribute groups and do crazy
things.

> > +static int sysfs_test_dev_alloc_miscdev(struct sysfs_test_device *test_dev)
> > +{
> > +	struct miscdevice *misc_dev;
> > +
> > +	misc_dev = &test_dev->misc_dev;
> > +	misc_dev->minor = MISC_DYNAMIC_MINOR;
> > +	misc_dev->name = kasprintf(GFP_KERNEL, "test_sysfs%d", test_dev->dev_idx);
> > +	if (!misc_dev->name) {
> > +		pr_err("Cannot alloc misc_dev->name\n");
> > +		return -ENOMEM;
> > +	}
> > +	misc_dev->groups = test_dev_groups;
> > +
> > +	return 0;
> > +}
> > +
> > +static int testdev_open(struct block_device *bdev, fmode_t mode)
> > +{
> > +	return -EINVAL;
> 
> Why?

Because we don't need it. We just want to borrow the way the block
layer registers attributes.

> > +}
> > +
> > +static void testdev_submit_bio(struct bio *bio)
> > +{
> > +}
> 
> Huh?

We don't need that either.
> 
> > +
> > +static void testdev_slot_free_notify(struct block_device *bdev,
> > +				     unsigned long index)
> > +{
> > +}
> 
> Why nothing?

And so one, becuase we just want to borrow the same path used
to register attributes from the block layer.

> 
> > +
> > +static int testdev_rw_page(struct block_device *bdev, sector_t sector,
> > +			   struct page *page, unsigned int op)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> 
> What is this doing?

Obviously nothing.

> > +
> > +static const struct block_device_operations sysfs_testdev_ops = {
> > +	.open = testdev_open,
> > +	.submit_bio = testdev_submit_bio,
> > +	.swap_slot_free_notify = testdev_slot_free_notify,
> > +	.rw_page = testdev_rw_page,
> > +	.owner = THIS_MODULE
> > +};
> > +
> > +static int sysfs_test_dev_alloc_blockdev(struct sysfs_test_device *test_dev)
> > +{
> > +	int ret = -ENOMEM;
> > +
> > +	test_dev->disk = blk_alloc_disk(NUMA_NO_NODE);
> > +	if (!test_dev->disk) {
> > +		pr_err("Error allocating disk structure for device %d\n",
> > +		       test_dev->dev_idx);
> > +		goto out;
> > +	}
> > +
> > +	test_dev->disk->major = sysfs_test_major;
> > +	test_dev->disk->first_minor = test_dev->dev_idx + 1;
> > +	test_dev->disk->fops = &sysfs_testdev_ops;
> > +	test_dev->disk->private_data = test_dev;
> > +	snprintf(test_dev->disk->disk_name, sizeof(test_dev->disk->disk_name),
> > +		 "test_sysfs%d", test_dev->dev_idx);
> > +	set_capacity(test_dev->disk, 0);
> > +	blk_queue_flag_set(QUEUE_FLAG_NONROT, test_dev->disk->queue);
> > +	blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, test_dev->disk->queue);
> > +	blk_queue_physical_block_size(test_dev->disk->queue, PAGE_SIZE);
> > +	blk_queue_max_discard_sectors(test_dev->disk->queue, UINT_MAX);
> > +	blk_queue_flag_set(QUEUE_FLAG_DISCARD, test_dev->disk->queue);
> > +
> > +	return 0;
> > +out:
> > +	return ret;
> > +}
> 
> What does this block device do?

Nothing. We are borrowing the way we register the sysfs attributes.

> > +
> > +static struct sysfs_test_device *alloc_test_dev_sysfs(int idx)
> > +{
> > +	struct sysfs_test_device *test_dev;
> > +	int ret;
> > +
> > +	switch (test_devtype) {
> > +	case TESTDEV_TYPE_MISC:
> > +	       fallthrough;
> > +	case TESTDEV_TYPE_BLOCK:
> > +		break;
> 
> That's the only 2 types you have, why test?

That suffices for now. This is only an initial set of tests. If we come
up with another driver odd case and want to try to see if we can
reproduce the issue in a clean form we can do it here by expanding on
the types.

> > +	default:
> > +		return NULL;
> > +	}
> > +
> > +	test_dev = kzalloc(sizeof(struct sysfs_test_device), GFP_KERNEL);
> > +	if (!test_dev)
> > +		goto err_out;
> > +
> > +	mutex_init(&test_dev->config_mutex);
> > +	test_dev->dev_idx = idx;
> > +	test_dev->devtype = test_devtype;
> > +
> > +	if (test_dev->devtype == TESTDEV_TYPE_MISC) {
> > +		ret = sysfs_test_dev_alloc_miscdev(test_dev);
> > +		if (ret)
> > +			goto err_out_free;
> > +	} else if (test_dev->devtype == TESTDEV_TYPE_BLOCK) {
> > +		ret = sysfs_test_dev_alloc_blockdev(test_dev);
> > +		if (ret)
> > +			goto err_out_free;
> > +	}
> > +	return test_dev;
> > +
> > +err_out_free:
> > +	kfree(test_dev);
> > +	test_dev = NULL;
> > +err_out:
> > +	return NULL;
> > +}
> > +
> > +static int register_test_dev_sysfs_misc(struct sysfs_test_device *test_dev)
> > +{
> > +	int ret;
> > +
> > +	ret = misc_register(&test_dev->misc_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	test_dev->dev = test_dev->misc_dev.this_device;
> 
> Why are you messing with the internals of a misc device's struct device?
> That's not for anyone to play with.

That's so we can easily get to the struct sysfs_test_device from the
device with dev_to_test_dev().

> > +
> > +	return 0;
> > +}
> > +
> > +static int register_test_dev_sysfs_block(struct sysfs_test_device *test_dev)
> > +{
> > +	int ret;
> > +
> > +	ret = device_add_disk(NULL, test_dev->disk, test_dev_groups);
> > +	if (ret) {
> > +		blk_cleanup_disk(test_dev->disk);
> > +		return ret;
> > +	}
> > +
> > +	test_dev->dev = disk_to_dev(test_dev->disk);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct sysfs_test_device *register_test_dev_sysfs(void)
> > +{
> > +	struct sysfs_test_device *test_dev = NULL;
> > +	int ret;
> > +
> > +	test_dev = alloc_test_dev_sysfs(0);
> > +	if (!test_dev)
> > +		goto out;
> > +
> > +	if (test_dev->devtype == TESTDEV_TYPE_MISC) {
> > +		ret = register_test_dev_sysfs_misc(test_dev);
> > +		if (ret) {
> > +			pr_err("could not register misc device: %d\n", ret);
> > +			goto out_free_dev;
> > +		}
> > +	} else if (test_dev->devtype == TESTDEV_TYPE_BLOCK) {
> > +		ret = register_test_dev_sysfs_block(test_dev);
> > +		if (ret) {
> > +			pr_err("could not register block device: %d\n", ret);
> > +			goto out_free_dev;
> > +		}
> > +	}
> > +
> > +	dev_info(test_dev->dev, "interface ready\n");
> > +
> > +out:
> > +	return test_dev;
> > +out_free_dev:
> > +	free_test_dev_sysfs(test_dev);
> > +	return NULL;
> > +}
> > +
> > +static struct sysfs_test_device *register_test_dev_set_config(void)
> > +{
> > +	struct sysfs_test_device *test_dev;
> > +	struct test_config *config;
> > +
> > +	test_dev = register_test_dev_sysfs();
> > +	if (!test_dev)
> > +		return NULL;
> > +
> > +	config = &test_dev->config;
> > +
> > +	if (enable_lock)
> > +		config->enable_lock = true;
> > +	if (enable_lock_on_rmmod)
> > +		config->enable_lock_on_rmmod = true;
> > +	if (use_rtnl_lock)
> > +		config->use_rtnl_lock = true;
> > +	if (enable_busy_alloc)
> > +		config->enable_busy_alloc = true;
> > +
> > +	config->write_delay_msec_y = write_delay_msec_y;
> > +	test_sysfs_reset_vals(test_dev);
> > +
> > +	return test_dev;
> > +}
> > +
> > +static void unregister_test_dev_sysfs_misc(struct sysfs_test_device *test_dev)
> > +{
> > +	misc_deregister(&test_dev->misc_dev);
> > +}
> > +
> > +static void unregister_test_dev_sysfs_block(struct sysfs_test_device *test_dev)
> > +{
> > +	del_gendisk(test_dev->disk);
> > +	blk_cleanup_disk(test_dev->disk);
> > +}
> > +
> > +static void unregister_test_dev_sysfs(struct sysfs_test_device *test_dev)
> > +{
> > +	test_dev_config_lock_rmmod(test_dev);
> > +
> > +	dev_info(test_dev->dev, "removing interface\n");
> > +
> > +	if (test_dev->devtype == TESTDEV_TYPE_MISC)
> > +		unregister_test_dev_sysfs_misc(test_dev);
> > +	else if (test_dev->devtype == TESTDEV_TYPE_BLOCK)
> > +		unregister_test_dev_sysfs_block(test_dev);
> > +
> > +	test_dev_config_unlock_rmmod(test_dev);
> > +
> > +	free_test_dev_sysfs(test_dev);
> > +}
> > +
> > +static struct dentry *debugfs_dir;
> 
> Why get debugfs involved?

I did that so to allow caling unregister_test_dev_sysfs_misc() and
unregister_test_dev_sysfs_block() *outside* of being tied to the
same sysfs attributes from the block/char device being registered.
In other words we would not be able to do that from sysfs files
for obvious reasons.

> > +/* When read represents how many times we have reset the first_test_dev */
> > +static u8 reset_first_test_dev;
> > +
> > +static ssize_t read_reset_first_test_dev(struct file *file,
> > +					 char __user *user_buf,
> > +					 size_t count, loff_t *ppos)
> > +{
> > +	ssize_t len;
> > +	char buf[32];
> > +
> > +	reset_first_test_dev++;
> > +	len = sprintf(buf, "%d\n", reset_first_test_dev);
> > +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> > +}
> > +
> > +static ssize_t write_reset_first_test_dev(struct file *file,
> > +					  const char __user *user_buf,
> > +					  size_t count, loff_t *ppos)
> > +{
> > +	if (!try_module_get(THIS_MODULE))
> 
> Totally racy, broken, and not allowed, sorry.

It is needed, given what this is trying to do.

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 3/6] selftests: add tests_sysfs module
  2021-12-09  1:48     ` Luis Chamberlain
@ 2021-12-10 21:39       ` Luis Chamberlain
  2021-12-14 19:31       ` [copyleft-next] " Richard Fontana
  1 sibling, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2021-12-10 21:39 UTC (permalink / raw)
  To: Greg KH, Petr Mladek
  Cc: tj, akpm, jeyu, shuah, bvanassche, dan.j.williams, joe, tglx,
	keescook, rostedt, minchan, linux-spdx, linux-doc, linux-block,
	linux-fsdevel, linux-kselftest, linux-kernel, copyleft-next

On Wed, Dec 08, 2021 at 05:48:25PM -0800, Luis Chamberlain wrote:
> On Fri, Dec 03, 2021 at 04:29:02PM +0100, Greg KH wrote:
> > On Fri, Oct 29, 2021 at 11:44:57AM -0700, Luis Chamberlain wrote:
> > > This adds a new selftest module which can be used to test sysfs, which
> > > would otherwise require using an existing driver. This lets us muck
> > > with a template driver to test breaking things without affecting
> > > system behaviour or requiring the dependencies of a real device
> > > driver.
> > 
> > Test sysfs "how"?  What exactly are you wanting to test?
> 
> You can look at the 32 tests added after all patches applied.
> 
> > I see lots of things in this code as examples of how to NOT use sysfs,
> > so are you testing my review cycles?  :)
> 
> You are exagerating, there are 32 tests there and only 2 tests deal
> with a deadlock which we are not yet sure how widespread it could be.

Also very important is how one test uses failure injection support to proove
how getting the kernfs active reference suffices to avoid crashes with
module removal and uses of sysfs ops in a driver, something which *you*
did not believe to be true but the code speaks for itself. This is also
why uses of try_module_get() is *safe* if used on sysfs ops.

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [copyleft-next] Re: [PATCH v9 3/6] selftests: add tests_sysfs module
  2021-12-09  1:48     ` Luis Chamberlain
  2021-12-10 21:39       ` Luis Chamberlain
@ 2021-12-14 19:31       ` Richard Fontana
  1 sibling, 0 replies; 36+ messages in thread
From: Richard Fontana @ 2021-12-14 19:31 UTC (permalink / raw)
  To: Discussion and development of copyleft-next
  Cc: Greg KH, Petr Mladek, akpm, jeyu, tglx, linux-kernel, rostedt,
	shuah, keescook, linux-fsdevel, linux-spdx, tj, dan.j.williams,
	bvanassche, linux-kselftest, joe, linux-doc, linux-block,
	minchan

 On Wed, Dec 8, 2021 at 8:52 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Fri, Dec 03, 2021 at 04:29:02PM +0100, Greg KH wrote:
> > On Fri, Oct 29, 2021 at 11:44:57AM -0700, Luis Chamberlain wrote:

> > > diff --git a/lib/test_sysfs.c b/lib/test_sysfs.c
> > > new file mode 100644
> > > index 000000000000..2a6ec072da60
> > > --- /dev/null
> > > +++ b/lib/test_sysfs.c
> > > @@ -0,0 +1,894 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
> >
> > Again, sorry, but no, I am going to object to this license as you are
> > only accessing a GPL-v2-only api.  Any other license on a file that
> > interacts with that, especially for core stuff like testing the
> > functionality of this code, needs to have that same license.  Sorry.
>
> Huh? The license is GPL-v2 compatible, and when used in the kernel the
> GPLv2 applies.
>
> Likewise, are you taking the position that permissively licensed code,
> say BSD or ISC licensed code, cannot use EXPORT_SYMBOL_GPL() symbols?

Just chiming in here, not really because of any association with the
copyleft-next license (or GPLv2 for that matter) but because of
general personal immersion in open source licensing. I would think
that code interacting with a GPLv2-only api could be under any
GPLv2-only-compatible license, such as ISC, GPLv2-or-later, or
copyleft-next. That said, of course kernel maintainers can establish
stricter policies around acceptable forms of licensing.

Richard

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 3/6] selftests: add tests_sysfs module
  2021-12-03 15:29   ` Greg KH
  2021-12-09  1:48     ` Luis Chamberlain
@ 2022-05-22 14:37     ` Thomas Gleixner
  2022-05-22 14:47       ` Greg KH
  1 sibling, 1 reply; 36+ messages in thread
From: Thomas Gleixner @ 2022-05-22 14:37 UTC (permalink / raw)
  To: Greg KH, Luis Chamberlain
  Cc: tj, akpm, jeyu, shuah, bvanassche, dan.j.williams, joe, keescook,
	rostedt, minchan, linux-spdx, linux-doc, linux-block,
	linux-fsdevel, linux-kselftest, linux-kernel

Greg,

On Fri, Dec 03 2021 at 16:29, Greg KH wrote:
> On Fri, Oct 29, 2021 at 11:44:57AM -0700, Luis Chamberlain wrote:

sorry for missing this thread. I came accross it now as I'm looking into
the licensing mess again.

>> +// SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
>
> Again, sorry, but no, I am going to object to this license as you are
> only accessing a GPL-v2-only api.  Any other license on a file that
> interacts with that, especially for core stuff like testing the
> functionality of this code, needs to have that same license.  Sorry.

That's a bogus argument. First of all the code is dual licensed and
second we have enough code in the kernel which is licensed MIT/BSD and
happily can access the GPL-v2-only APIs.

Aside of that we have already code in the kernel which is dual licensed

     GPL-2.0-or-later OR copyleft-next-0.3.1

We just can't make it SPDX clean because copyleft-next-0.3.1 is not in
LICENSING.

While I agree that we want to keep the number of licenses as small as
possible, we cannot really dictate which dual licensing options a
submitter selects unless the license is GPL-2.0-only incompatible, which
copyleft-next is not.

Can we just get over this, add the license with the SPDX identifier and
move on?

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 3/6] selftests: add tests_sysfs module
  2022-05-22 14:37     ` Thomas Gleixner
@ 2022-05-22 14:47       ` Greg KH
  2022-05-22 15:06         ` Thomas Gleixner
  0 siblings, 1 reply; 36+ messages in thread
From: Greg KH @ 2022-05-22 14:47 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Luis Chamberlain, tj, akpm, jeyu, shuah, bvanassche,
	dan.j.williams, joe, keescook, rostedt, minchan, linux-spdx,
	linux-doc, linux-block, linux-fsdevel, linux-kselftest,
	linux-kernel

On Sun, May 22, 2022 at 04:37:19PM +0200, Thomas Gleixner wrote:
> Greg,
> 
> On Fri, Dec 03 2021 at 16:29, Greg KH wrote:
> > On Fri, Oct 29, 2021 at 11:44:57AM -0700, Luis Chamberlain wrote:
> 
> sorry for missing this thread. I came accross it now as I'm looking into
> the licensing mess again.
> 
> >> +// SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
> >
> > Again, sorry, but no, I am going to object to this license as you are
> > only accessing a GPL-v2-only api.  Any other license on a file that
> > interacts with that, especially for core stuff like testing the
> > functionality of this code, needs to have that same license.  Sorry.
> 
> That's a bogus argument. First of all the code is dual licensed and
> second we have enough code in the kernel which is licensed MIT/BSD and
> happily can access the GPL-v2-only APIs.
> 
> Aside of that we have already code in the kernel which is dual licensed
> 
>      GPL-2.0-or-later OR copyleft-next-0.3.1
> 
> We just can't make it SPDX clean because copyleft-next-0.3.1 is not in
> LICENSING.
> 
> While I agree that we want to keep the number of licenses as small as
> possible, we cannot really dictate which dual licensing options a
> submitter selects unless the license is GPL-2.0-only incompatible, which
> copyleft-next is not.
> 
> Can we just get over this, add the license with the SPDX identifier and
> move on?

From what I recall, I had technical reasons I didn't take this series,
but that was a long time ago and I would be glad to review it again if
it were rebased and resubmitted after the next merge window is closed.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 3/6] selftests: add tests_sysfs module
  2022-05-22 14:47       ` Greg KH
@ 2022-05-22 15:06         ` Thomas Gleixner
  2022-05-23 19:37           ` Luis Chamberlain
  0 siblings, 1 reply; 36+ messages in thread
From: Thomas Gleixner @ 2022-05-22 15:06 UTC (permalink / raw)
  To: Greg KH
  Cc: Luis Chamberlain, tj, akpm, jeyu, shuah, bvanassche,
	dan.j.williams, joe, keescook, rostedt, minchan, linux-spdx,
	linux-doc, linux-block, linux-fsdevel, linux-kselftest,
	linux-kernel

On Sun, May 22 2022 at 16:47, Greg KH wrote:
> On Sun, May 22, 2022 at 04:37:19PM +0200, Thomas Gleixner wrote:
>> On Fri, Dec 03 2021 at 16:29, Greg KH wrote:
>> 
>> While I agree that we want to keep the number of licenses as small as
>> possible, we cannot really dictate which dual licensing options a
>> submitter selects unless the license is GPL-2.0-only incompatible, which
>> copyleft-next is not.
>> 
>> Can we just get over this, add the license with the SPDX identifier and
>> move on?
>
> From what I recall, I had technical reasons I didn't take this series,
> but that was a long time ago and I would be glad to review it again if
> it were rebased and resubmitted after the next merge window is closed.

The license addition and the SPDX identifier cleanup should be seperated
from the new test code which was part of the series.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 3/6] selftests: add tests_sysfs module
  2022-05-22 15:06         ` Thomas Gleixner
@ 2022-05-23 19:37           ` Luis Chamberlain
  0 siblings, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2022-05-23 19:37 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Greg KH, tj, akpm, jeyu, shuah, bvanassche, dan.j.williams, joe,
	keescook, rostedt, minchan, linux-spdx, linux-doc, linux-block,
	linux-fsdevel, linux-kselftest, linux-kernel

On Sun, May 22, 2022 at 05:06:44PM +0200, Thomas Gleixner wrote:
> On Sun, May 22 2022 at 16:47, Greg KH wrote:
> > On Sun, May 22, 2022 at 04:37:19PM +0200, Thomas Gleixner wrote:
> >> On Fri, Dec 03 2021 at 16:29, Greg KH wrote:
> >> 
> >> While I agree that we want to keep the number of licenses as small as
> >> possible, we cannot really dictate which dual licensing options a
> >> submitter selects unless the license is GPL-2.0-only incompatible, which
> >> copyleft-next is not.
> >> 
> >> Can we just get over this, add the license with the SPDX identifier and
> >> move on?
> >
> > From what I recall, I had technical reasons I didn't take this series,
> > but that was a long time ago and I would be glad to review it again if
> > it were rebased and resubmitted after the next merge window is closed.
> 
> The license addition and the SPDX identifier cleanup should be seperated
> from the new test code which was part of the series.

I'll send a re-spin after the merge window and split this up.

And FWIW, AFAICT I addressed all the comments, so I can resend after
the spdx stuff gets merged so to make the series easier to read /
review.

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2021-10-29 18:44 ` [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license Luis Chamberlain
@ 2022-05-23 21:10   ` Thomas Gleixner
  2022-05-24 13:59     ` Richard Fontana
                       ` (2 more replies)
  2022-05-23 21:22   ` Thomas Gleixner
  2022-05-23 21:27   ` Thomas Gleixner
  2 siblings, 3 replies; 36+ messages in thread
From: Thomas Gleixner @ 2022-05-23 21:10 UTC (permalink / raw)
  To: Luis Chamberlain, tj, gregkh, akpm, jeyu, shuah
  Cc: bvanassche, dan.j.williams, joe, mcgrof, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	Richard Fontana, copyleft-next, Ciaran Farrell,
	Christopher De Nicolo, Christoph Hellwig, Jonathan Corbet,
	Thorsten Leemhuis

On Fri, Oct 29 2021 at 11:44, Luis Chamberlain wrote:
> preferred. A summary of benefits why projects outside of Linux might
> prefer to use copyleft-next >= 0.3.1 over GPLv2:
>
<snip>
>
> o copyleft-next has a 'built-in or-later' provision

Not convinced that this is a benefit under all circumstances, but that's
a philosopical problem. The real problem is this:

> +Valid-License-Identifier: copyleft-next-0.3.1

and

> +11. Later License Versions
> +
> +    The Copyleft-Next Project may release new versions of copyleft-next,
> +    designated by a distinguishing version number ("Later Versions").
> +    Unless I explicitly remove the option of Distributing Covered Works
> +    under Later Versions, You may Distribute Covered Works under any Later
> +    Version.

If I want to remove this option, then how do I express this with a SPDX
license identifier?

Sigh!

        tglx

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2021-10-29 18:44 ` [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license Luis Chamberlain
  2022-05-23 21:10   ` Thomas Gleixner
@ 2022-05-23 21:22   ` Thomas Gleixner
  2022-05-25 16:57     ` Luis Chamberlain
  2022-05-23 21:27   ` Thomas Gleixner
  2 siblings, 1 reply; 36+ messages in thread
From: Thomas Gleixner @ 2022-05-23 21:22 UTC (permalink / raw)
  To: Luis Chamberlain, tj, gregkh, akpm, jeyu, shuah
  Cc: bvanassche, dan.j.williams, joe, mcgrof, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	Richard Fontana, copyleft-next, Ciaran Farrell,
	Christopher De Nicolo, Christoph Hellwig, Jonathan Corbet,
	Thorsten Leemhuis

On Fri, Oct 29 2021 at 11:44, Luis Chamberlain wrote:
> --- /dev/null
> +++ b/LICENSES/dual/copyleft-next-0.3.1
> @@ -0,0 +1,237 @@
> +Valid-License-Identifier: copyleft-next-0.3.1
> +SPDX-URL: https://spdx.org/licenses/copyleft-next-0.3.1
> +Usage-Guide:
> +  This license can be used in code, it has been found to be GPLv2 compatible
> +  by attorneys at Redhat and SUSE, however to air on the side of caution,

air ?

> +  it's best to only use it together with a GPL2 compatible license using "OR".

This paragraph is not really understandable for Joe Developer.

  copyleft-next-0.3.1 is explicitly compatible with GPLv2 (or later) and
  can therefore be used for kernel code. Though the best and recommended
  practice is to express this in the SPDX license identifier by
  licensing the code under both licenses expressed by the OR operator.

Hmm?

> +  To use the copyleft-next-0.3.1 license put the following SPDX tag/value
> +  pair into a comment according to the placement guidelines in the
> +  licensing rules documentation:
> +    SPDX-License-Identifier: GPL-2.0 OR copyleft-next-0.3.1
> +    SPDX-License-Identifier: GPL-2.0-only OR copyleft-next 0.3.1
> +    SPDX-License-Identifier: GPL-2.0+ OR copyleft-next-0.3.1
> +    SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1

Please don't propagate the GPL-2.0 and GPL-2.0+ tags. They are
outdated (still valid) in the SPDX spec, which reminds me that I should
update the relevant documentation...

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2021-10-29 18:44 ` [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license Luis Chamberlain
  2022-05-23 21:10   ` Thomas Gleixner
  2022-05-23 21:22   ` Thomas Gleixner
@ 2022-05-23 21:27   ` Thomas Gleixner
  2022-05-25 16:43     ` Luis Chamberlain
  2 siblings, 1 reply; 36+ messages in thread
From: Thomas Gleixner @ 2022-05-23 21:27 UTC (permalink / raw)
  To: Luis Chamberlain, tj, gregkh, akpm, jeyu, shuah
  Cc: bvanassche, dan.j.williams, joe, mcgrof, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	Richard Fontana, copyleft-next, Ciaran Farrell,
	Christopher De Nicolo, Christoph Hellwig, Jonathan Corbet,
	Thorsten Leemhuis

On Fri, Oct 29 2021 at 11:44, Luis Chamberlain wrote:
> +    "FSF-Free" means classified as 'free' by the Free Software Foundation.
> +
> +    "OSI-Approved" means approved as 'Open Source' by the Open Source
> +    Initiative.

copyleft-next is neither nor. Confused...

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-23 21:10   ` Thomas Gleixner
@ 2022-05-24 13:59     ` Richard Fontana
  2022-05-25 17:10     ` Luis Chamberlain
  2022-05-25 19:13     ` Bradley M. Kuhn
  2 siblings, 0 replies; 36+ messages in thread
From: Richard Fontana @ 2022-05-24 13:59 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Luis Chamberlain, tj, Greg KH, akpm, jeyu, Shuah Khan,
	bvanassche, dan.j.williams, joe, keescook, rostedt, minchan,
	linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	Richard Fontana, copyleft-next, Ciaran Farrell,
	Christopher De Nicolo, Christoph Hellwig, Jonathan Corbet,
	Thorsten Leemhuis

On Mon, May 23, 2022 at 5:10 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Fri, Oct 29 2021 at 11:44, Luis Chamberlain wrote:
> > preferred. A summary of benefits why projects outside of Linux might
> > prefer to use copyleft-next >= 0.3.1 over GPLv2:
> >
> <snip>
> >
> > o copyleft-next has a 'built-in or-later' provision
>
> Not convinced that this is a benefit under all circumstances, but that's
> a philosopical problem. The real problem is this:
>
> > +Valid-License-Identifier: copyleft-next-0.3.1
>
> and
>
> > +11. Later License Versions
> > +
> > +    The Copyleft-Next Project may release new versions of copyleft-next,
> > +    designated by a distinguishing version number ("Later Versions").
> > +    Unless I explicitly remove the option of Distributing Covered Works
> > +    under Later Versions, You may Distribute Covered Works under any Later
> > +    Version.
>
> If I want to remove this option, then how do I express this with a SPDX
> license identifier?

Probably off-topic but: I think as things currently stand in SPDX you
would have to use an ad hoc LicenseRef- identifier to express the
entirety of copyleft-next-0.3.1 coupled with an amendment that sort of
strikes the later versions provision. This issue is also somewhat
relevant: https://github.com/spdx/spdx-spec/issues/153

FWIW, built-in 'or-later' clauses are actually common in copyleft open
source licenses; the GPL family is the oddity here. (Then again, the
whole idea of a downstream license upgradability option is sort of
unusual in the bigger scheme of things, but that's another topic.)

Richard


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-23 21:27   ` Thomas Gleixner
@ 2022-05-25 16:43     ` Luis Chamberlain
  2022-05-25 17:05       ` Bird, Tim
  0 siblings, 1 reply; 36+ messages in thread
From: Luis Chamberlain @ 2022-05-25 16:43 UTC (permalink / raw)
  To: Thomas Gleixner, Richard Fontana
  Cc: tj, gregkh, akpm, jeyu, shuah, bvanassche, dan.j.williams, joe,
	keescook, rostedt, minchan, linux-spdx, linux-doc, linux-block,
	linux-fsdevel, linux-kselftest, linux-kernel, Goldwyn Rodrigues,
	Kuno Woudt, copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis

On Mon, May 23, 2022 at 11:27:34PM +0200, Thomas Gleixner wrote:
> On Fri, Oct 29 2021 at 11:44, Luis Chamberlain wrote:
> > +    "FSF-Free" means classified as 'free' by the Free Software Foundation.
> > +
> > +    "OSI-Approved" means approved as 'Open Source' by the Open Source
> > +    Initiative.
> 
> copyleft-next is neither nor. Confused...

The terms are used in two clauses:

4. Condition Against Further Restrictions; Inbound License Compatibility
7. Nullification of Copyleft/Proprietary Dual Licensing

IANAL but at least as per my reading, in both cases it is used to refer to
"other licenses", not itself, so I see no issue with that use. If there
is an issue it would be nice to hear more details about it, so that
perhaps new versions of the license can make this clearer somehow, if
not already.

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-23 21:22   ` Thomas Gleixner
@ 2022-05-25 16:57     ` Luis Chamberlain
  2022-05-25 20:51       ` Thomas Gleixner
  0 siblings, 1 reply; 36+ messages in thread
From: Luis Chamberlain @ 2022-05-25 16:57 UTC (permalink / raw)
  To: Thomas Gleixner, Richard Fontana
  Cc: tj, gregkh, akpm, jeyu, shuah, bvanassche, dan.j.williams, joe,
	keescook, rostedt, minchan, linux-spdx, linux-doc, linux-block,
	linux-fsdevel, linux-kselftest, linux-kernel, Goldwyn Rodrigues,
	Kuno Woudt, copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis

On Mon, May 23, 2022 at 11:22:36PM +0200, Thomas Gleixner wrote:
> On Fri, Oct 29 2021 at 11:44, Luis Chamberlain wrote:
> > --- /dev/null
> > +++ b/LICENSES/dual/copyleft-next-0.3.1
> > @@ -0,0 +1,237 @@
> > +Valid-License-Identifier: copyleft-next-0.3.1
> > +SPDX-URL: https://spdx.org/licenses/copyleft-next-0.3.1
> > +Usage-Guide:
> > +  This license can be used in code, it has been found to be GPLv2 compatible
> > +  by attorneys at Redhat and SUSE, however to air on the side of caution,
> 
> air ?

Hehe, thanks I'll fix in the next spin.

> > +  it's best to only use it together with a GPL2 compatible license using "OR".
> 
> This paragraph is not really understandable for Joe Developer.
> 
>   copyleft-next-0.3.1 is explicitly compatible with GPLv2 (or later) and
>   can therefore be used for kernel code. Though the best and recommended
>   practice is to express this in the SPDX license identifier by
>   licensing the code under both licenses expressed by the OR operator.
> 
> Hmm?

Let me try clarifying this further, how about:

   copyleft-next-0.3.1 is explicitly compatible with GPLv2 (or later) and
   can therefore be used for kernel code. Despite this, if you use
   copyleft-next-0.3.1 on Linux, the recommended practice is to express
   dual licensing with GPL using in the SPDX license identifiers by
   using by the OR operator.

> > +  To use the copyleft-next-0.3.1 license put the following SPDX tag/value
> > +  pair into a comment according to the placement guidelines in the
> > +  licensing rules documentation:
> > +    SPDX-License-Identifier: GPL-2.0 OR copyleft-next-0.3.1
> > +    SPDX-License-Identifier: GPL-2.0-only OR copyleft-next 0.3.1
> > +    SPDX-License-Identifier: GPL-2.0+ OR copyleft-next-0.3.1
> > +    SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
> 
> Please don't propagate the GPL-2.0 and GPL-2.0+ tags. They are
> outdated (still valid) in the SPDX spec, which reminds me that I should
> update the relevant documentation...

OK thanks for the recommendation, I'll leave it at:

+    SPDX-License-Identifier: GPL-2.0 OR copyleft-next-0.3.1
+    SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-25 16:43     ` Luis Chamberlain
@ 2022-05-25 17:05       ` Bird, Tim
  2022-05-25 18:11         ` Luis Chamberlain
  0 siblings, 1 reply; 36+ messages in thread
From: Bird, Tim @ 2022-05-25 17:05 UTC (permalink / raw)
  To: Luis Chamberlain, Thomas Gleixner, Richard Fontana
  Cc: tj, gregkh, akpm, jeyu, shuah, bvanassche, dan.j.williams, joe,
	keescook, rostedt, minchan, linux-spdx, linux-doc, linux-block,
	linux-fsdevel, linux-kselftest, linux-kernel, Goldwyn Rodrigues,
	Kuno Woudt, copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis



> -----Original Message-----
> From: Luis Chamberlain <mcgrof@infradead.org> On Behalf Of Luis Chamberlain
> 
> On Mon, May 23, 2022 at 11:27:34PM +0200, Thomas Gleixner wrote:
> > On Fri, Oct 29 2021 at 11:44, Luis Chamberlain wrote:
> > > +    "FSF-Free" means classified as 'free' by the Free Software Foundation.
> > > +
> > > +    "OSI-Approved" means approved as 'Open Source' by the Open Source
> > > +    Initiative.
> >
> > copyleft-next is neither nor. Confused...
> 
> The terms are used in two clauses:
> 
> 4. Condition Against Further Restrictions; Inbound License Compatibility
> 7. Nullification of Copyleft/Proprietary Dual Licensing
> 
> IANAL but at least as per my reading, in both cases it is used to refer to
> "other licenses", not itself, so I see no issue with that use. If there
> is an issue it would be nice to hear more details about it, so that
> perhaps new versions of the license can make this clearer somehow, if
> not already.

I don't begrudge Luis his licensing choice, but one
of the main reasons to stick with a well-known and reviewed license
is to avoid kernel developers having to do license-vetting
themselves.  I know it's being submitted as an OR, but I question
the value of introducing another license into the kernel's licensing mix.
    -- Tim


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-23 21:10   ` Thomas Gleixner
  2022-05-24 13:59     ` Richard Fontana
@ 2022-05-25 17:10     ` Luis Chamberlain
  2022-05-25 19:13     ` Bradley M. Kuhn
  2 siblings, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2022-05-25 17:10 UTC (permalink / raw)
  To: Thomas Gleixner, Richard Fontana
  Cc: tj, gregkh, akpm, jeyu, shuah, bvanassche, dan.j.williams, joe,
	keescook, rostedt, minchan, linux-spdx, linux-doc, linux-block,
	linux-fsdevel, linux-kselftest, linux-kernel, Goldwyn Rodrigues,
	Kuno Woudt, copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis

On Mon, May 23, 2022 at 11:10:37PM +0200, Thomas Gleixner wrote:
> On Fri, Oct 29 2021 at 11:44, Luis Chamberlain wrote:
> > preferred. A summary of benefits why projects outside of Linux might
> > prefer to use copyleft-next >= 0.3.1 over GPLv2:
> >
> <snip>
> >
> > o copyleft-next has a 'built-in or-later' provision
> 
> Not convinced that this is a benefit under all circumstances,

I'll just drop it.

> but that's
> a philosopical problem. The real problem is this:


> > +Valid-License-Identifier: copyleft-next-0.3.1
> 
> and

Why is this an issue?

> > +11. Later License Versions
> > +
> > +    The Copyleft-Next Project may release new versions of copyleft-next,
> > +    designated by a distinguishing version number ("Later Versions").
> > +    Unless I explicitly remove the option of Distributing Covered Works
> > +    under Later Versions, You may Distribute Covered Works under any Later
> > +    Version.
> 
> If I want to remove this option, then how do I express this with a SPDX
> license identifier?

Good question, souinds like generic semantics for SPDX evolution?

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-25 17:05       ` Bird, Tim
@ 2022-05-25 18:11         ` Luis Chamberlain
  2022-05-25 19:05           ` Bird, Tim
  0 siblings, 1 reply; 36+ messages in thread
From: Luis Chamberlain @ 2022-05-25 18:11 UTC (permalink / raw)
  To: Bird, Tim
  Cc: Thomas Gleixner, Richard Fontana, tj, gregkh, akpm, jeyu, shuah,
	bvanassche, dan.j.williams, joe, keescook, rostedt, minchan,
	linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis

On Wed, May 25, 2022 at 05:05:54PM +0000, Bird, Tim wrote:
> I know it's being submitted as an OR, but I question
> the value of introducing another license into the kernel's licensing mix.

As a free software hacker *I* value the evolution of copyleft and copyleft-next
does just that. Some may have thought that it may not have been possible to
evolve copyleft and work with an evolved license on Linux, but copyleft-next
shows it is possible. Here in lies the value I see in it.

I agree that we want to keep the number of licenses as small as
possible but we cannot really dictate which dual licensing options a
submitter selects unless the license is GPL-2.0-only incompatible,
which copyleft-next is not.

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-25 18:11         ` Luis Chamberlain
@ 2022-05-25 19:05           ` Bird, Tim
  2022-05-25 19:44             ` Luis Chamberlain
  2022-05-25 22:16             ` Thomas Gleixner
  0 siblings, 2 replies; 36+ messages in thread
From: Bird, Tim @ 2022-05-25 19:05 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: Thomas Gleixner, Richard Fontana, tj, gregkh, akpm, jeyu, shuah,
	bvanassche, dan.j.williams, joe, keescook, rostedt, minchan,
	linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis

> -----Original Message-----
> From: Luis Chamberlain <mcgrof@infradead.org> On Behalf Of Luis Chamberlain
> 
> On Wed, May 25, 2022 at 05:05:54PM +0000, Bird, Tim wrote:
> > I know it's being submitted as an OR, but I question
> > the value of introducing another license into the kernel's licensing mix.
> 
> As a free software hacker *I* value the evolution of copyleft and copyleft-next
> does just that. Some may have thought that it may not have been possible to
> evolve copyleft and work with an evolved license on Linux, but copyleft-next
> shows it is possible. Here in lies the value I see in it.
> 
> I agree that we want to keep the number of licenses as small as
> possible but we cannot really dictate which dual licensing options a
> submitter selects unless the license is GPL-2.0-only incompatible,
> which copyleft-next is not.

Um, yes we can dictate that.  There were good reasons that the original
BSD dual-licenses were allowed.  Those same reasons don't apply here.
Each license added to the kernel (even when added as an OR), requires
additional legal analysis.  Corporate lawyers don't usually rely on the
interpretation of novel licenses from external sources.  They do it themselves.
This means that hundreds of lawyers may find themselves reading and
trying to interpret the copyleft-next license.

And here's the thing.
The copyleft-next license has a number of legal issues that make it problematic.
Not the least of which are that some of its terms are dependent on external
situations that can change over time, in a matter that is uncontrolled by either
the licensor or the licensee.  In order to determine what terms are effective, you
have to know when the license was granted, and what the FSF and OSI approved
licenses were at various points in time.  You literally have to use the Internet
Archive wayback machine, and do a bunch of research, to interpret the license terms.
It is not, as currently constructed, a good license, due to this lack of legal clarity.

Regards,
 -- Tim




^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-23 21:10   ` Thomas Gleixner
  2022-05-24 13:59     ` Richard Fontana
  2022-05-25 17:10     ` Luis Chamberlain
@ 2022-05-25 19:13     ` Bradley M. Kuhn
  2022-05-25 21:50       ` J Lovejoy
  2 siblings, 1 reply; 36+ messages in thread
From: Bradley M. Kuhn @ 2022-05-25 19:13 UTC (permalink / raw)
  To: Thomas Gleixner, copyleft-next
  Cc: Luis Chamberlain, tj, gregkh, akpm, jeyu, shuah, bvanassche,
	dan.j.williams, joe, keescook, rostedt, minchan, linux-spdx,
	linux-doc, linux-block, linux-fsdevel, linux-kselftest,
	linux-kernel, Goldwyn Rodrigues, Kuno Woudt, Richard Fontana,
	Ciaran Farrell, Christopher De Nicolo, Christoph Hellwig,
	Jonathan Corbet, Thorsten Leemhuis

In answering Thomas' question …

Thomas Gleixner wrote at 14:10 (PDT) on Monday:
> If I want to remove this option, then how do I express this with a SPDX
> license identifier?

… some licensing/SPDX background is in order.  (I apologize in advance for a
few paragraphs of license-splaining, as I know that many on this thread know
these points already, but I suspect most only have only vague familiarity
with this issue.)

copyleft-next 0.3.1 reads:
>> +11. Later License Versions
>> +    The Copyleft-Next Project may release new versions of copyleft-next,
>> +    designated by a distinguishing version number ("Later Versions").

Many don't realize that GPL is (or was, pre-copyleft-next) unique in
structure among copyleft licenses in that the -or-later clause of all
licenses in the GPL family is configurable.  That yields the complex forms
of: GPLv1-only, GPLv1-or-later, GPLv2-only, GPLv2-or-later, etc.  GPLv3 even
added the proxy upgrade clause (— a formulation SPDX can't handle at all).

Other non-trivial FOSS licenses — such as Mozilla Public License (MPL),
Common Development and Distribution License (CDDL), and Eclipse Public
License (EPL) (as just three examples) — all have “automatic -or-later”.
Thus, “MPLv2.0” *always* means “MPLv2.0-or-later”, so if you use the SPDX
moniker for that (“MPL-2.0”), it really is akin to using “GPLv2-or-later”.
Meanwhile, there is no *actual* way to license code under “MPLv2-only” — the
license text itself prohibits it.

All that's to say: the GPL has (historically) always been a huge FOSS
licensing special-case because of the complex configurability of its
“-or-later” clause.

One of the last activities I did with SPDX (in late 2017) was to help
negotiate a solution on reworking the GPL identifiers to deal with this
special case.  The solution was a classic political compromise — where
*everyone* left unhappy — but that's what led to the deprecation of SPDX's
“GPL-2.0” identifier in favor of “GPL-2.0-or-later” and “GPL-2.0-only”.

I wasn't involved with SPDX anymore when they (much later) created the
identifier “copyleft-next-0.3.1” — but it appears it was a case of “those
who forget the past is condemned to repeat it” — because copyleft-next's
SPDX identifier indeed has a similarly confusing ambiguity to “GPL-2.0”:

copyleft-next 0.3.1 text reads further:
>> +    Unless I explicitly remove the option of Distributing Covered Works
>> +    under Later Versions, You may Distribute Covered Works under any Later
>> +    Version.
Thomas Gleixner noted about it at 14:10 (PDT) on Monday:
> If I want to remove this option, then how do I express this with a SPDX
> license identifier?  Sigh!

So, this problem that Thomas notes above is definitely an error by the SPDX
project, *just like* the one that exists for the deprecated “GPL-2.0”
identifier.  But, that isn't copyleft-next's fault [0], nor Luis's fault.
IMO, Luis shouldn't be punished (i.e., by being prohibited by the Linux
project from licensing under the GPLv2-compatible terms of his choosing)
simply because the SPDX project erred.

Fortunately, the problem *is* hypothetical here because Luis has *not*
indicated that he's licensing under “copyleft-next-0.3.1 REVOKING
new-version-upgrade”, so it's not a problem for Luis' patch that SPDX offers
no way to represent that licensing sub-option in copyleft-next.

[0] Nevertheless, I am wondering, given that (a) opting-out-of-auto-upgrade is
    *so* GPL-specific, and (b) the auto-upgrade opt out has caused decades
    of pain and woe throughout the GPL-using community (and for SPDX!),
    maybe copyleft-next should, in fact, drop that clause entirely in future
    versions. Discussion of that is likely not of interest to most folks on
    this wide thread, so I'll pick up that conversation more narrowly just
    on the copyleft-next list from here …

 -- bkuhn

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-25 19:05           ` Bird, Tim
@ 2022-05-25 19:44             ` Luis Chamberlain
  2022-05-25 22:16             ` Thomas Gleixner
  1 sibling, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2022-05-25 19:44 UTC (permalink / raw)
  To: Bird, Tim
  Cc: Thomas Gleixner, Richard Fontana, tj, gregkh, akpm, jeyu, shuah,
	bvanassche, dan.j.williams, joe, keescook, rostedt, minchan,
	linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis

On Wed, May 25, 2022 at 07:05:31PM +0000, Bird, Tim wrote:
> > -----Original Message-----
> > From: Luis Chamberlain <mcgrof@infradead.org> On Behalf Of Luis Chamberlain
> > 
> > On Wed, May 25, 2022 at 05:05:54PM +0000, Bird, Tim wrote:
> > > I know it's being submitted as an OR, but I question
> > > the value of introducing another license into the kernel's licensing mix.
> > 
> > I agree that we want to keep the number of licenses as small as
> > possible but we cannot really dictate which dual licensing options a
> > submitter selects unless the license is GPL-2.0-only incompatible,
> > which copyleft-next is not.
> 
> Um, yes we can dictate that. 

The statement about us not being able to dictate which dual licensing
options a submitter selects does not actually come from me, Thomas noted
this [0].

[0] https://lkml.kernel.org/r/87fsl1iqg0.ffs@tglx

> There were good reasons that the original
> BSD dual-licenses were allowed.

I helped spearhead some of that effort.

> Those same reasons don't apply here.

Correct, and I noted my own reasoning for now dual licensing with
copyleft-next, which you seem to be disregarding?

> Each license added to the kernel (even when added as an OR), requires
> additional legal analysis.

And I noted in my cover letter that copyleft-next-0.3.1 has been found to be
to be GPLv2 compatible by three attorneys at SUSE and Redhat [1], but
to err on the side of caution we simply recommend to always use the "OR"
language for this license [2].

[1] https://lore.kernel.org/lkml/20170516232702.GL17314@wotan.suse.de/
[2] https://lkml.kernel.org/r/1495234558.7848.122.camel@linux.intel.com

> And here's the thing.
> The copyleft-next license has a number of legal issues that make it problematic.

You say number of legal issues.

> Not the least of which are that some of its terms are dependent on external
> situations that can change over time, in a matter that is uncontrolled by either
> the licensor or the licensee.  In order to determine what terms are effective, you
> have to know when the license was granted, and what the FSF and OSI approved
> licenses were at various points in time.  You literally have to use the Internet
> Archive wayback machine, and do a bunch of research, to interpret the license terms.
> It is not, as currently constructed, a good license, due to this lack of legal clarity.

But the above seems to indicate one technical pain point in so far as
two sections:

4. Condition Against Further Restrictions; Inbound License Compatibility
7. Nullification of Copyleft/Proprietary Dual Licensing

If you are going to offer to pay for an alternative proprietary
licensing, I'm sure you can do the work.

And if in so far as clause 4 is concerned, yeah I think wayback machine
is a sensible solution. Good idea, seems like we have that covered since
1999 [3].

[3] https://web.archive.org/web/*/https://opensource.org/licenses

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-25 16:57     ` Luis Chamberlain
@ 2022-05-25 20:51       ` Thomas Gleixner
  2022-05-25 23:53         ` Luis Chamberlain
  0 siblings, 1 reply; 36+ messages in thread
From: Thomas Gleixner @ 2022-05-25 20:51 UTC (permalink / raw)
  To: Luis Chamberlain, Richard Fontana
  Cc: tj, gregkh, akpm, jeyu, shuah, bvanassche, dan.j.williams, joe,
	keescook, rostedt, minchan, linux-spdx, linux-doc, linux-block,
	linux-fsdevel, linux-kselftest, linux-kernel, Goldwyn Rodrigues,
	Kuno Woudt, copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis

On Wed, May 25 2022 at 09:57, Luis Chamberlain wrote:
> On Mon, May 23, 2022 at 11:22:36PM +0200, Thomas Gleixner wrote:
>> This paragraph is not really understandable for Joe Developer.
>> 
>>   copyleft-next-0.3.1 is explicitly compatible with GPLv2 (or later) and
>>   can therefore be used for kernel code. Though the best and recommended
>>   practice is to express this in the SPDX license identifier by
>>   licensing the code under both licenses expressed by the OR operator.
>> 
>> Hmm?
>
> Let me try clarifying this further, how about:
>
>    copyleft-next-0.3.1 is explicitly compatible with GPLv2 (or later) and
>    can therefore be used for kernel code. Despite this, if you use
>    copyleft-next-0.3.1 on Linux, the recommended practice is to express
>    dual licensing with GPL using in the SPDX license identifiers by
>    using by the OR operator.

  'using in the ..' ?

and

  'by using by' is off by one 'by' :)

I'm not seeing how that clarifies stuff further. I might be biased, but
the version I suggested is crystal clear.

>> > +  To use the copyleft-next-0.3.1 license put the following SPDX tag/value
>> > +  pair into a comment according to the placement guidelines in the
>> > +  licensing rules documentation:
>> > +    SPDX-License-Identifier: GPL-2.0 OR copyleft-next-0.3.1
>> > +    SPDX-License-Identifier: GPL-2.0-only OR copyleft-next 0.3.1
>> > +    SPDX-License-Identifier: GPL-2.0+ OR copyleft-next-0.3.1
>> > +    SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
>> 
>> Please don't propagate the GPL-2.0 and GPL-2.0+ tags. They are
>> outdated (still valid) in the SPDX spec, which reminds me that I should
>> update the relevant documentation...
>
> OK thanks for the recommendation, I'll leave it at:
>
> +    SPDX-License-Identifier: GPL-2.0 OR copyleft-next-0.3.1

	SPDX-License-Identifier: GPL-2.0-only OR copyleft-next-0.3.1

please. See my previous reply quoted above.

> +    SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-25 19:13     ` Bradley M. Kuhn
@ 2022-05-25 21:50       ` J Lovejoy
  2022-05-25 22:29         ` Bradley M. Kuhn
  0 siblings, 1 reply; 36+ messages in thread
From: J Lovejoy @ 2022-05-25 21:50 UTC (permalink / raw)
  To: Bradley M. Kuhn, Thomas Gleixner, copyleft-next
  Cc: Luis Chamberlain, tj, gregkh, akpm, jeyu, shuah, bvanassche,
	dan.j.williams, joe, keescook, rostedt, minchan, linux-spdx,
	linux-doc, linux-block, linux-fsdevel, linux-kselftest,
	linux-kernel, Goldwyn Rodrigues, Kuno Woudt, Richard Fontana,
	Ciaran Farrell, Christopher De Nicolo, Christoph Hellwig,
	Jonathan Corbet, Thorsten Leemhuis



On 5/25/22 1:13 PM, Bradley M. Kuhn wrote:
> In answering Thomas' question …
>
> Thomas Gleixner wrote at 14:10 (PDT) on Monday:
>> If I want to remove this option, then how do I express this with a SPDX
>> license identifier?
> … some licensing/SPDX background is in order.  (I apologize in advance for a
> few paragraphs of license-splaining, as I know that many on this thread know
> these points already, but I suspect most only have only vague familiarity
> with this issue.)
>
> copyleft-next 0.3.1 reads:
>>> +11. Later License Versions
>>> +    The Copyleft-Next Project may release new versions of copyleft-next,
>>> +    designated by a distinguishing version number ("Later Versions").
> Many don't realize that GPL is (or was, pre-copyleft-next) unique in
> structure among copyleft licenses in that the -or-later clause of all
> licenses in the GPL family is configurable.  That yields the complex forms
> of: GPLv1-only, GPLv1-or-later, GPLv2-only, GPLv2-or-later, etc.  GPLv3 even
> added the proxy upgrade clause (— a formulation SPDX can't handle at all).
>
> Other non-trivial FOSS licenses — such as Mozilla Public License (MPL),
> Common Development and Distribution License (CDDL), and Eclipse Public
> License (EPL) (as just three examples) — all have “automatic -or-later”.
> Thus, “MPLv2.0” *always* means “MPLv2.0-or-later”, so if you use the SPDX
> moniker for that (“MPL-2.0”), it really is akin to using “GPLv2-or-later”.
> Meanwhile, there is no *actual* way to license code under “MPLv2-only” — the
> license text itself prohibits it.
A few folks on the SPDX legal team did a summary chart of all the 
nuances and while I'm not going to go down that road again, suffice to 
say, the "or later" clauses have more variation than most people would 
think (which is probably b/c most people don't need to pay attention to 
it). The "+" operator is always available if someone so chooses to apply 
it as needed.
>
> All that's to say: the GPL has (historically) always been a huge FOSS
> licensing special-case because of the complex configurability of its
> “-or-later” clause.
Agreed.
>
> One of the last activities I did with SPDX (in late 2017) was to help
> negotiate a solution on reworking the GPL identifiers to deal with this
> special case.  The solution was a classic political compromise — where
> *everyone* left unhappy — but that's what led to the deprecation of SPDX's
> “GPL-2.0” identifier in favor of “GPL-2.0-or-later” and “GPL-2.0-only”.
I would agree with this characterization, except this was the outcome 
the FSF wanted, so ostensibly they were happy (and you forgot that 
GPL-2.0+ ).
(And to give credit where credit is due, Bradley's input during that 
challenging "negotiation" was very helpful. :)
>
> So, this problem that Thomas notes above is definitely an error by the SPDX
> project, *just like* the one that exists for the deprecated “GPL-2.0”
To be clear, the GPL-2.0 identifier was never an error by the SPDX team 
- we were always very clear as to what it meant/means. It was that the 
FSF didn't like it. That is clearly explained in the blog post on the 
SPDX website, as well as the post on the FSF site on the subject.

Jilayne

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-25 19:05           ` Bird, Tim
  2022-05-25 19:44             ` Luis Chamberlain
@ 2022-05-25 22:16             ` Thomas Gleixner
  2022-06-02  4:11               ` Bird, Tim
  1 sibling, 1 reply; 36+ messages in thread
From: Thomas Gleixner @ 2022-05-25 22:16 UTC (permalink / raw)
  To: Bird, Tim, Luis Chamberlain
  Cc: Richard Fontana, tj, gregkh, akpm, jeyu, shuah, bvanassche,
	dan.j.williams, joe, keescook, rostedt, minchan, linux-spdx,
	linux-doc, linux-block, linux-fsdevel, linux-kselftest,
	linux-kernel, Goldwyn Rodrigues, Kuno Woudt, copyleft-next,
	Ciaran Farrell, Christopher De Nicolo, Christoph Hellwig,
	Jonathan Corbet, Thorsten Leemhuis

Tim!

On Wed, May 25 2022 at 19:05, Bird, Tim wrote:
>> From: Luis Chamberlain <mcgrof@infradead.org> On Behalf Of Luis Chamberlain
>> I agree that we want to keep the number of licenses as small as
>> possible but we cannot really dictate which dual licensing options a
>> submitter selects unless the license is GPL-2.0-only incompatible,
>> which copyleft-next is not.
>
> Um, yes we can dictate that.

No!

> There were good reasons that the original BSD dual-licenses were
> allowed.  Those same reasons don't apply here.

That's just wrong. The reason why dual licensing is allowed is to share
code across licesce preferences. The very same reason applies here.

> Each license added to the kernel (even when added as an OR), requires
> additional legal analysis.  Corporate lawyers don't usually rely on
> the interpretation of novel licenses from external sources.  They do
> it themselves.  This means that hundreds of lawyers may find
> themselves reading and trying to interpret the copyleft-next license.

It's none of our problems that corporate lawyers are obviously unable to
act, cooperate and to agree on something.

     The license in question is around since 2016 and the kernel carries
     code under that license since 2017.

  So what the hell are you complaining 5 years after the fact? The whole
  point here is to convert this to SPDX identifiers.

Aside of that I'm impressed by your chutzpah to make up an argument on
corporate lawyer costs.

  Do you realize how much costs the very same crowd of corporate lawyers
  created by ill advising their employees to put corporate defined
  boiler plate into every other kernel source code file or by not
  advising them at all and let them add random crap?

  Do you realize that the costs to cleanup this mess have been mostly
  covered by community members with the help from a _very_ small subset
  of corporate lawyers?

  Do you realize that it's hilarious that your so costly corporate
  lawyers only need to react now that we are trying to convert licensing
  information to SPDX 5 years after the fact?

  Do you realize that the SPDX cleanup effort is reducing the costs for
  everyone including _all_ of the corporate lawyers you are so concerned
  of?

Sure, complaining about your individual corporate costs is way simpler
than:

   - contributing to community efforts to reduce those costs

   - acknowledging that the community efforts even without contribution
     or your particular organization are reducing those costs

Try again.

> And here's the thing.
> The copyleft-next license has a number of legal issues that make it problematic.
> Not the least of which are that some of its terms are dependent on external
> situations that can change over time, in a matter that is uncontrolled by either
> the licensor or the licensee.  In order to determine what terms are effective, you
> have to know when the license was granted, and what the FSF and OSI approved
> licenses were at various points in time.  You literally have to use the Internet
> Archive wayback machine, and do a bunch of research, to interpret the license terms.
> It is not, as currently constructed, a good license, due to this lack of legal clarity.

And here is the thing:

    Whether you consider it to be a good license or not, does not matter
    in this context.

    The license _IS_ GPLv2 compatible which is even understandable for
    mere mortals, i.e. non lawyers. That's the only relevant point for
    including code under this license into the kernel.

    Your way-back machine argument is beyond hilarious. Guess what the
    folks who try to cleanup the corporate lawyers induced mess in the
    kernel rely on? But that's absolutely not applicable to this
    problem. Why?

        The kernel has very strict rules for licensing today. Any valid
        SPDX tag in a source file has to be accompanied with a
        corresponding machine readable license file.

        This license file is version controlled and if there is a
        dependency on any other license in the context then the
        dependency is also available in the git history.

        So no, you do not need Internet archive for this at all simply
        because if the kernel git history vanishes from the planet
        then this particular licensing problem is the least of your
        worries.

Maybe it's about time to move your corporate lawyers, who are
caught in their own past sins, to the reality of today.

Thanks,

        Thomas

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-25 21:50       ` J Lovejoy
@ 2022-05-25 22:29         ` Bradley M. Kuhn
  0 siblings, 0 replies; 36+ messages in thread
From: Bradley M. Kuhn @ 2022-05-25 22:29 UTC (permalink / raw)
  To: linux-spdx, J Lovejoy, copyleft-next
  Cc: Thomas Gleixner, Luis Chamberlain, tj, gregkh, akpm, jeyu, shuah,
	bvanassche, dan.j.williams, joe, keescook, rostedt, minchan,
	linux-doc, linux-block, linux-fsdevel, linux-kselftest,
	linux-kernel, Goldwyn Rodrigues, Kuno Woudt, Richard Fontana,
	Ciaran Farrell, Christopher De Nicolo, Christoph Hellwig,
	Jonathan Corbet, Thorsten Leemhuis

J Lovejoy wrote:
> (And to give credit where credit is due, Bradley's input during that
> challenging "negotiation" was very helpful. :)

😊 … thank you!

I'd written today:
>> So, this problem that Thomas notes above is definitely an error by the
>> SPDX project, *just like* the one that exists for the deprecated “GPL-2.0”

J Lovejoy replied:
> To be clear, the GPL-2.0 identifier was never an error by the SPDX team - we
> were always very clear as to what it meant/means.

… but notwithstanding a clear definition of a moniker (which I agree indeed
you've made for most SPDX identifiers), if that definition fails to
adequately match historically understanding (and/or fails to take into
account nuances in the document it represents), confusion ensues for users.
Users *were* confused about “GPL-2.0” (remember, we did a small (admittedly
non-scientific) survey at a session at a conference — FOSDEM I think it was?)

Most SPDX *users* won't speak its defined terms fluently; I suspect most of
Linux's licensors (and even most licensees) don't speak SPDX fluently, so
presumably you want SPDX identifiers to have some intuitiveness —
particularly for the use case of linux-spdx, which requires the identifiers
to be *both* human-readable and machine-readable.

This is relevant to the copyleft-next-0.3.1 identifier.  SPDX could define
“copyleft-next-0.3.1” to mean for SPDX purposes: “the text of copyleft-next
without any options in its terms exercised/removed” (— although I note
https://spdx.org/licenses/copyleft-next-0.3.1.html seems to be wholly silent
regarding options exercising/removing).  However, there is currently
confusion — shown in the fact that Thomas still asked:
>>>> If I want to remove this option, then how do I express this with a SPDX
>>>> license identifier?  Sigh!
… upon noticing this part of copyleft-next:
>>> +    Unless I explicitly remove the option of Distributing Covered Works
>>> +    under Later Versions, You may Distribute Covered Works under any Later
>>> +    Version.

Anyway, I'm pointing out SPDX's shortcomings on this point *not* to
captiously admonish SPDX, but rather to point out that any issues with SPDX
identifiers and their formal definitions shouldn't influence a decision about
what licenses are acceptable for inclusion as dual-license options in Linux.

Plus, I remain hopeful that over the long-term, the SPDX project will take
feedback from efforts like linux-spdx to solve the kinds of problems that
have come up in this thread and others.

Finally, I've already started a sub-thread on the copyleft-next list to start
discussing maybe the license (in future versions) shouldn't have this option
anyway (for unrelated policy reasons).  That might yield a side-benefit of
making the problem evaporate entirely for SPDX.  (Anyway, after 25 years of
living with GPL's “-or-later vs. -only” mess — I, for one, am convinced new
licenses like copyleft-next should try very hard to not repeat that mistake.)

 -- bkuhn

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-25 20:51       ` Thomas Gleixner
@ 2022-05-25 23:53         ` Luis Chamberlain
  0 siblings, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2022-05-25 23:53 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Richard Fontana, tj, gregkh, akpm, jeyu, shuah, bvanassche,
	dan.j.williams, joe, keescook, rostedt, minchan, linux-spdx,
	linux-doc, linux-block, linux-fsdevel, linux-kselftest,
	linux-kernel, Goldwyn Rodrigues, Kuno Woudt, copyleft-next,
	Ciaran Farrell, Christopher De Nicolo, Christoph Hellwig,
	Jonathan Corbet, Thorsten Leemhuis

On Wed, May 25, 2022 at 10:51:43PM +0200, Thomas Gleixner wrote:
> On Wed, May 25 2022 at 09:57, Luis Chamberlain wrote:
> > On Mon, May 23, 2022 at 11:22:36PM +0200, Thomas Gleixner wrote:
> >> This paragraph is not really understandable for Joe Developer.
> >> 
> >>   copyleft-next-0.3.1 is explicitly compatible with GPLv2 (or later) and
> >>   can therefore be used for kernel code. Though the best and recommended
> >>   practice is to express this in the SPDX license identifier by
> >>   licensing the code under both licenses expressed by the OR operator.
> >> 
> >> Hmm?
> >
> > Let me try clarifying this further, how about:
> >
> >    copyleft-next-0.3.1 is explicitly compatible with GPLv2 (or later) and
> >    can therefore be used for kernel code. Despite this, if you use
> >    copyleft-next-0.3.1 on Linux, the recommended practice is to express
> >    dual licensing with GPL using in the SPDX license identifiers by
> >    using by the OR operator.
> 
>   'using in the ..' ?
> 
> and
> 
>   'by using by' is off by one 'by' :)
> 
> I'm not seeing how that clarifies stuff further. I might be biased, but
> the version I suggested is crystal clear.

Oh sorry, I didn't realize the paragraph you posted was a suggestion, I
thought it was the one you were indicating needed further enhancement!

I'll just take yours then.

> >> > +  To use the copyleft-next-0.3.1 license put the following SPDX tag/value
> >> > +  pair into a comment according to the placement guidelines in the
> >> > +  licensing rules documentation:
> >> > +    SPDX-License-Identifier: GPL-2.0 OR copyleft-next-0.3.1
> >> > +    SPDX-License-Identifier: GPL-2.0-only OR copyleft-next 0.3.1
> >> > +    SPDX-License-Identifier: GPL-2.0+ OR copyleft-next-0.3.1
> >> > +    SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1
> >> 
> >> Please don't propagate the GPL-2.0 and GPL-2.0+ tags. They are
> >> outdated (still valid) in the SPDX spec, which reminds me that I should
> >> update the relevant documentation...
> >
> > OK thanks for the recommendation, I'll leave it at:
> >
> > +    SPDX-License-Identifier: GPL-2.0 OR copyleft-next-0.3.1
> 
> 	SPDX-License-Identifier: GPL-2.0-only OR copyleft-next-0.3.1
> 
> please. See my previous reply quoted above.
> 
> > +    SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1

Sorry I hadn't had my coffee yet so I should only list:

SPDX-License-Identifier: GPL-2.0-only OR copyleft-next 0.3.1
SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1

Will do this on the next spin.

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-05-25 22:16             ` Thomas Gleixner
@ 2022-06-02  4:11               ` Bird, Tim
  2022-06-02  6:20                 ` gregkh
  2022-06-02 19:30                 ` Luis Chamberlain
  0 siblings, 2 replies; 36+ messages in thread
From: Bird, Tim @ 2022-06-02  4:11 UTC (permalink / raw)
  To: Thomas Gleixner, Luis Chamberlain
  Cc: Richard Fontana, tj, gregkh, akpm, jeyu, shuah, bvanassche,
	dan.j.williams, joe, keescook, rostedt, minchan, linux-spdx,
	linux-doc, linux-block, linux-fsdevel, linux-kselftest,
	linux-kernel, Goldwyn Rodrigues, Kuno Woudt, copyleft-next,
	Ciaran Farrell, Christopher De Nicolo, Christoph Hellwig,
	Jonathan Corbet, Thorsten Leemhuis

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Tim!
> 
> On Wed, May 25 2022 at 19:05, Bird, Tim wrote:
> >> From: Luis Chamberlain <mcgrof@infradead.org> On Behalf Of Luis Chamberlain
> >> I agree that we want to keep the number of licenses as small as
> >> possible but we cannot really dictate which dual licensing options a
> >> submitter selects unless the license is GPL-2.0-only incompatible,
> >> which copyleft-next is not.
> >
> > Um, yes we can dictate that.
> 
> No!
Sorry for the delayed response.  I was on vacation over memorial day weekend
(holiday in the US.)

I think that the option to reject a contribution based on its license should be
available to the community, using criteria beyond those that Luis has mentioned
(and that you mention below).

I could create a license that was GPL-2.0-only compatible, and use it to cover a new
contribution to the Linux kernel (in dual-license format), in order to get exposure
for the license or to promote it.  We could use the SPDX identifier "Tims-license-0.1".
I think it would be fair for the community to reject a contribution based
on those license circumstances, even though it met all the criteria you mention.

I don't think that the Linux kernel should be used for license promotion, but if it is,
then it should be used to promote GPL-v2-only.

> 
> > There were good reasons that the original BSD dual-licenses were
> > allowed.  Those same reasons don't apply here.
> 
> That's just wrong. The reason why dual licensing is allowed is to share
> code across licesce preferences. The very same reason applies here.

I was talking about why dual licensing was originally introduced, which was
a situation different from what went on in 2016, when the copyleft-next
dual license was discussed.

Dual-licensing in the Linux kernel was originally introduced because code was being
taken from BSD and placed into Linux (under GPL v2), often by someone other than the
original author.  This created a bit of hard feelings between the BSD community
and the Linux community.  So dual-licensing was introduced so that derivative works
(created by Linux developers) of BSD code could flow back into the BSD project.

This was code that existed before being introduced into Linux, and there was
no notion of using the kernel to promote the BSD license.

I have a problem with the statement:

"The reason why dual licensing is allowed is to share
 code across license preferences. The very same reason applies here."

I'm concerned about license proliferation, and that statement has no limiting principle.
Although you do raise some limiting principles below, I don't think they are adequate.
I don't think it should be the mission of the kernel to allow shared code across [any and all]
license preferences.

Just by way of example, I would hesitate to support the adoption into the kernel of any
license that had an "or later" provision.  That's true, even on the other side of
an "OR" clause. Also, my preference would be to limit new licenses to OSI-approved
licenses, which have already had extensive vetting.

> 
> > Each license added to the kernel (even when added as an OR), requires
> > additional legal analysis.  Corporate lawyers don't usually rely on
> > the interpretation of novel licenses from external sources.  They do
> > it themselves.  This means that hundreds of lawyers may find
> > themselves reading and trying to interpret the copyleft-next license.
> 
> It's none of our problems that corporate lawyers are obviously unable to
> act, cooperate and to agree on something.
That's an over-generalization.

> 
>      The license in question is around since 2016 and the kernel carries
>      code under that license since 2017.
> 
>   So what the hell are you complaining 5 years after the fact? The whole
>   point here is to convert this to SPDX identifiers.

I missed the original discussion, and the license was limited in application.
I agree that my complaint is late, but this conversion to SPDX adds the license
text to LICENSES, which IMHO makes this license stand out more than it
has previously, and possibly provides an implied endorsement.

I think it's a good thing to convert from the current language to an SPDX identifier.
And that I'm late to the party with complaints about this particular license.
But I'd rather not encourage developers to use this license going forward.

> 
> Aside of that I'm impressed by your chutzpah to make up an argument on
> corporate lawyer costs.
Burden on corporate lawyers (and, dare I say, OSPO people like myself
who deal in license training at our respective companies) has been a
consideration in past discussions of contributions.

The 2016  LKML thread about the original submission included
discussion about the amount of "lawyer ink" required for
different licensing constructs and development scenarios. For example, see
https://lore.kernel.org/lkml/20170518221205.gcfs2t4ihlpx5kj6@thunk.org/#t

> 
>   Do you realize how much costs the very same crowd of corporate lawyers

"very same crowd"?

>   created by ill advising their employees to put corporate defined
>   boiler plate into every other kernel source code file or by not
>   advising them at all and let them add random crap?

I think we're talking about different sets of lawyers, so the rest of the complaint
about some lawyers and the costs they've imposed on the kernel community
I think is unrelated.

> 
>   Do you realize that the costs to cleanup this mess have been mostly
>   covered by community members with the help from a _very_ small subset
>   of corporate lawyers?
> 
>   Do you realize that it's hilarious that your so costly corporate
>   lawyers only need to react now that we are trying to convert licensing
>   information to SPDX 5 years after the fact?

The fact that the conversion being discussed *now* may not yield
an identical licensing situation seems to warrant some legal discussion.
I assume Luis is on-board with the change in licensing situation due
to the conversion, but I have yet to see the final patches.

> 
>   Do you realize that the SPDX cleanup effort is reducing the costs for
>   everyone including _all_ of the corporate lawyers you are so concerned
>   of?

Yes.
I'm supportive of using SPDX in the kernel, as are all the lawyers I work with.

> 
> Sure, complaining about your individual corporate costs is way simpler
> than:
> 
>    - contributing to community efforts to reduce those costs
> 
>    - acknowledging that the community efforts even without contribution
>      or your particular organization are reducing those costs
> 
> Try again.

Try what again?  Communicating a preference on license proliferation?
That's what this message is.

> 
> > And here's the thing.
> > The copyleft-next license has a number of legal issues that make it problematic.
> > Not the least of which are that some of its terms are dependent on external
> > situations that can change over time, in a matter that is uncontrolled by either
> > the licensor or the licensee.  In order to determine what terms are effective, you
> > have to know when the license was granted, and what the FSF and OSI approved
> > licenses were at various points in time.  You literally have to use the Internet
> > Archive wayback machine, and do a bunch of research, to interpret the license terms.
> > It is not, as currently constructed, a good license, due to this lack of legal clarity.
> 
> And here is the thing:
> 
>     Whether you consider it to be a good license or not, does not matter
>     in this context.
> 
>     The license _IS_ GPLv2 compatible which is even understandable for
>     mere mortals, i.e. non lawyers. That's the only relevant point for
>     including code under this license into the kernel.

I disagree.  There are other relevant points if the kernel is being used
to promote a license.  I have made all my contributions to the Linux kernel
(meager though they are) under GPL-v2-only.  I think straying from this
should be rare and well-justified.

> 
>     Your way-back machine argument is beyond hilarious. Guess what the
>     folks who try to cleanup the corporate lawyers induced mess in the
>     kernel rely on? But that's absolutely not applicable to this
>     problem. Why?
> 
>         The kernel has very strict rules for licensing today. Any valid
>         SPDX tag in a source file has to be accompanied with a
>         corresponding machine readable license file.
> 
>         This license file is version controlled and if there is a
>         dependency on any other license in the context then the
>         dependency is also available in the git history.
> 
>         So no, you do not need Internet archive for this at all simply
>         because if the kernel git history vanishes from the planet
>         then this particular licensing problem is the least of your
>         worries.


I said that the textual terms of copyleft-next require the Internet archive
to interpret.  That's completely independent of what version control
system you use to track the license and its application to the kernel.
Linux kernel git logs are not going to tell you what the OSI and
FSF-approved licenses were in 2016.

> 
> Maybe it's about time to move your corporate lawyers, who are
> caught in their own past sins, to the reality of today.
*My* corporate lawyers and their "past sins"?  I think you're taking
actions by some lawyers and overgeneralizing them.  It is not unheard-of
to consider the cost in legal overhead in licensing considerations in the kernel.

I believe Luis is working on an independent patch that isolates the SPDX
conversion from the changes to the testing module code.  As such, I'll give
feedback on new patches going forward.

Regards,
 -- Tim

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-06-02  4:11               ` Bird, Tim
@ 2022-06-02  6:20                 ` gregkh
  2022-06-02 19:41                   ` Luis Chamberlain
  2022-06-02 19:30                 ` Luis Chamberlain
  1 sibling, 1 reply; 36+ messages in thread
From: gregkh @ 2022-06-02  6:20 UTC (permalink / raw)
  To: Bird, Tim
  Cc: Thomas Gleixner, Luis Chamberlain, Richard Fontana, tj, akpm,
	jeyu, shuah, bvanassche, dan.j.williams, joe, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis

On Thu, Jun 02, 2022 at 04:11:16AM +0000, Bird, Tim wrote:
> > -----Original Message-----
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > Tim!
> > 
> > On Wed, May 25 2022 at 19:05, Bird, Tim wrote:
> > >> From: Luis Chamberlain <mcgrof@infradead.org> On Behalf Of Luis Chamberlain
> > >> I agree that we want to keep the number of licenses as small as
> > >> possible but we cannot really dictate which dual licensing options a
> > >> submitter selects unless the license is GPL-2.0-only incompatible,
> > >> which copyleft-next is not.
> > >
> > > Um, yes we can dictate that.
> > 
> > No!
> Sorry for the delayed response.  I was on vacation over memorial day weekend
> (holiday in the US.)
> 
> I think that the option to reject a contribution based on its license should be
> available to the community, using criteria beyond those that Luis has mentioned
> (and that you mention below).
> 
> I could create a license that was GPL-2.0-only compatible, and use it to cover a new
> contribution to the Linux kernel (in dual-license format), in order to get exposure
> for the license or to promote it.  We could use the SPDX identifier "Tims-license-0.1".
> I think it would be fair for the community to reject a contribution based
> on those license circumstances, even though it met all the criteria you mention.
> 
> I don't think that the Linux kernel should be used for license promotion, but if it is,
> then it should be used to promote GPL-v2-only.

I agree, and in a way, I feel like that is what is happening here for
this original submission.  See below for more...

> > > There were good reasons that the original BSD dual-licenses were
> > > allowed.  Those same reasons don't apply here.
> > 
> > That's just wrong. The reason why dual licensing is allowed is to share
> > code across licesce preferences. The very same reason applies here.
> 
> I was talking about why dual licensing was originally introduced, which was
> a situation different from what went on in 2016, when the copyleft-next
> dual license was discussed.
> 
> Dual-licensing in the Linux kernel was originally introduced because code was being
> taken from BSD and placed into Linux (under GPL v2), often by someone other than the
> original author.  This created a bit of hard feelings between the BSD community
> and the Linux community.  So dual-licensing was introduced so that derivative works
> (created by Linux developers) of BSD code could flow back into the BSD project.
> 
> This was code that existed before being introduced into Linux, and there was
> no notion of using the kernel to promote the BSD license.

I agree, dual licensed code that is added to the kernel is either done:
	- because the original code had a non-GPL license and it was
	  added so that it could be compatible so that it could be added
	  to Linux.
	- because the code being accepted into Linux can also be used in
	  another non-Linux codebase now or in the future.

The submission here was neither of these.

It was to test core Linux kernel functionality that is ONLY GPL-v2.
That functionality and interactions within the Linux core could never be
used in any non-Linux project as it does not make any sense.  Or if it
could be used in a non-Linux project, that would only be if that project
was also GPLv2 licensed as the kernel core code would have been copied
out of Linux into that other project.

I feel that the dual-license of this code is purely done to support an
additional license and give it attention as it could never be invoked on
this codebase due to the contents of it.  Which makes it not necessary
and has only distracted us from the real technical issues of why I
rejected this code in the first place :(

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-06-02  4:11               ` Bird, Tim
  2022-06-02  6:20                 ` gregkh
@ 2022-06-02 19:30                 ` Luis Chamberlain
  1 sibling, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2022-06-02 19:30 UTC (permalink / raw)
  To: Bird, Tim
  Cc: Thomas Gleixner, Richard Fontana, tj, gregkh, akpm, jeyu, shuah,
	bvanassche, dan.j.williams, joe, keescook, rostedt, minchan,
	linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis

On Thu, Jun 02, 2022 at 04:11:16AM +0000, Bird, Tim wrote:
> I don't think that the Linux kernel should be used for license promotion, but if it is,
> then it should be used to promote GPL-v2-only.

I already *used* copyleft-next on Linux on a dual licensed manner, the
patches in question are about using SPDX to simpllify things and adaopt
the SPDX annotations.

And, to re-iterate once more, I'm using copyeft-next as that is *my*
license of choice for *any* new software projects I use and I want to enable
cross prolination between them. I have been doing this since I wrote
CRDA many years ago. I have many reasons for using copyleft-next and I've
listed many of the reasons why a free software developer would care to enable
this cross polination but yet again you seem to be disregarding all that.

This is similar to how 2-cluase BSD is compatible with the GPL
and is used for cross polination to BSDs even though in practice
a lot of that cross polination may not happen.

There are practical uses here and I've been using this license for years now
and I have no intention on stopping.

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license
  2022-06-02  6:20                 ` gregkh
@ 2022-06-02 19:41                   ` Luis Chamberlain
  0 siblings, 0 replies; 36+ messages in thread
From: Luis Chamberlain @ 2022-06-02 19:41 UTC (permalink / raw)
  To: gregkh
  Cc: Bird, Tim, Thomas Gleixner, Richard Fontana, tj, akpm, jeyu,
	shuah, bvanassche, dan.j.williams, joe, keescook, rostedt,
	minchan, linux-spdx, linux-doc, linux-block, linux-fsdevel,
	linux-kselftest, linux-kernel, Goldwyn Rodrigues, Kuno Woudt,
	copyleft-next, Ciaran Farrell, Christopher De Nicolo,
	Christoph Hellwig, Jonathan Corbet, Thorsten Leemhuis

On Thu, Jun 02, 2022 at 08:20:18AM +0200, gregkh@linuxfoundation.org wrote:
> On Thu, Jun 02, 2022 at 04:11:16AM +0000, Bird, Tim wrote:
> > I don't think that the Linux kernel should be used for license promotion, but if it is,
> > then it should be used to promote GPL-v2-only.
> 
> I agree, and in a way, I feel like that is what is happening here for
> this original submission.  See below for more...

I have been using copyleft-next for years and cross polination for me is real.

> I agree, dual licensed code that is added to the kernel is either done:
> 	- because the original code had a non-GPL license and it was
> 	  added so that it could be compatible so that it could be added
> 	  to Linux.
> 	- because the code being accepted into Linux can also be used in
> 	  another non-Linux codebase now or in the future.

Sometimes such cross polination is purely speculative, but we don't
stop and ask people for evidence of this.

You forgot that another reason is because some attorneys like some
permissive license more. That's it. The Clear BSD license is an example.
In that case it was a new license to Linux. So let us be clear that one
could argue that was a bit of license promotion.

> The submission here was neither of these.

You are incorrect.

I have a vision for Linux kernel testing and I have put *years* of
effort in this regard and thinking about architecture behind all this.
So yes cross polination is *extremely* important to me and hence some
of this effort.

  Luis

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2022-06-02 19:41 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-29 18:44 [PATCH v9 0/6] test_sysfs: add new selftest for sysfs Luis Chamberlain
2021-10-29 18:44 ` [PATCH v9 1/6] LICENSES: Add the copyleft-next-0.3.1 license Luis Chamberlain
2022-05-23 21:10   ` Thomas Gleixner
2022-05-24 13:59     ` Richard Fontana
2022-05-25 17:10     ` Luis Chamberlain
2022-05-25 19:13     ` Bradley M. Kuhn
2022-05-25 21:50       ` J Lovejoy
2022-05-25 22:29         ` Bradley M. Kuhn
2022-05-23 21:22   ` Thomas Gleixner
2022-05-25 16:57     ` Luis Chamberlain
2022-05-25 20:51       ` Thomas Gleixner
2022-05-25 23:53         ` Luis Chamberlain
2022-05-23 21:27   ` Thomas Gleixner
2022-05-25 16:43     ` Luis Chamberlain
2022-05-25 17:05       ` Bird, Tim
2022-05-25 18:11         ` Luis Chamberlain
2022-05-25 19:05           ` Bird, Tim
2022-05-25 19:44             ` Luis Chamberlain
2022-05-25 22:16             ` Thomas Gleixner
2022-06-02  4:11               ` Bird, Tim
2022-06-02  6:20                 ` gregkh
2022-06-02 19:41                   ` Luis Chamberlain
2022-06-02 19:30                 ` Luis Chamberlain
2021-10-29 18:44 ` [PATCH v9 2/6] testing: use the copyleft-next-0.3.1 SPDX tag Luis Chamberlain
2021-10-29 18:44 ` [PATCH v9 3/6] selftests: add tests_sysfs module Luis Chamberlain
2021-12-03 15:29   ` Greg KH
2021-12-09  1:48     ` Luis Chamberlain
2021-12-10 21:39       ` Luis Chamberlain
2021-12-14 19:31       ` [copyleft-next] " Richard Fontana
2022-05-22 14:37     ` Thomas Gleixner
2022-05-22 14:47       ` Greg KH
2022-05-22 15:06         ` Thomas Gleixner
2022-05-23 19:37           ` Luis Chamberlain
2021-10-29 18:44 ` [PATCH v9 4/6] kernfs: add initial failure injection support Luis Chamberlain
2021-10-29 18:44 ` [PATCH v9 5/6] test_sysfs: add support to use kernfs failure injection Luis Chamberlain
2021-10-29 18:45 ` [PATCH v9 6/6] kernel/module: add documentation for try_module_get() Luis Chamberlain

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.