sysfs: fix kobject refcount to address races with kobject removal

It's possible today to have a device attribute read or store
race against device removal. This is known to happen as follows:

write system call -->
  ksys_write () -->
    vfs_write() -->
      __vfs_write() -->
        kernfs_fop_write_iter() -->
          sysfs_kf_write() -->
            dev_attr_store() -->
              null reference

This happens because the dev_attr->store() callback can be
removed prior to its call, after dev_attr_store() was initiated.
The null dereference is possible because the sysfs ops can be
removed on module removal, for instance, when device_del() is
called, and a sysfs read / store is not doing any kobject reference
bumps either. This allows a read/store call to initiate, a
device_del() to kick off, and then the read/store call can be
gone by the time to execute it.

The sysfs filesystem is not doing any kobject reference bumps during a
read / store ops to prevent this.

To fix this in a simplified way, just bump the kobject reference when
we create a directory and remove it on directory removal.

The big unfortunate eye-sore is addressing the manual kobject reference
assumption on the networking code, which leads me to believe we should
end up replacing that eventually with another sort of check.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---

This v4 moves to fixing the race condition on dev_attr_store() and
dev_attr_read() to sysfs by bumping the kobject reference count
on directory creation / deletion as suggested by Greg.

Unfortunately at least the networking core has a manual refcount

10 more lines

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 59dffd5ca517..6c47aa4af6f5 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c

@@ -56,12 +56,14 @@ int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
 
 	kobject_get_ownership(kobj, &uid, &gid);
 
+	kobject_get(kobj);
 	kn = kernfs_create_dir_ns(parent, kobject_name(kobj),
 				  S_IRWXU | S_IRUGO | S_IXUGO, uid, gid,
 				  kobj, ns);
 	if (IS_ERR(kn)) {
 		if (PTR_ERR(kn) == -EEXIST)
 			sysfs_warn_dup(parent, kobject_name(kobj));
+		kobject_put(kobj);
 		return PTR_ERR(kn);
 	}
 

@@ -100,6 +102,7 @@ void sysfs_remove_dir(struct kobject *kobj)
 	if (kn) {
 		WARN_ON_ONCE(kernfs_type(kn) != KERNFS_DIR);
 		kernfs_remove(kn);
+		kobject_put(kobj);
 	}
 }
 

diff --git a/net/core/dev.c b/net/core/dev.c
index 222b1d322c96..3a0ffa603d14 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -10429,7 +10429,7 @@ static void netdev_wait_allrefs(struct net_device *dev)
 	rebroadcast_time = warning_time = jiffies;
 	refcnt = netdev_refcnt_read(dev);
 
-	while (refcnt != 1) {
+	while (refcnt != 3) {
 		if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
 			rtnl_lock();
 

@@ -10544,7 +10544,7 @@ void netdev_run_todo(void)
 		netdev_wait_allrefs(dev);
 
 		/* paranoia */
-		BUG_ON(netdev_refcnt_read(dev) != 1);
+		BUG_ON(netdev_refcnt_read(dev) != 3);
 		BUG_ON(!list_empty(&dev->ptype_all));
 		BUG_ON(!list_empty(&dev->ptype_specific));
 		WARN_ON(rcu_access_pointer(dev->ip_ptr));

-- 
2.27.0

On Wed, Jun 23, 2021 at 02:50:07PM -0700, Luis Chamberlain wrote:

It's possible today to have a device attribute read or store
race against device removal. This is known to happen as follows:

write system call -->
  ksys_write () -->
    vfs_write() -->
      __vfs_write() -->
        kernfs_fop_write_iter() -->
          sysfs_kf_write() -->
            dev_attr_store() -->
              null reference

This happens because the dev_attr->store() callback can be
removed prior to its call, after dev_attr_store() was initiated.
The null dereference is possible because the sysfs ops can be
removed on module removal, for instance, when device_del() is
called, and a sysfs read / store is not doing any kobject reference
bumps either. This allows a read/store call to initiate, a
device_del() to kick off, and then the read/store call can be
gone by the time to execute it.

The sysfs filesystem is not doing any kobject reference bumps during a
read / store ops to prevent this.

To fix this in a simplified way, just bump the kobject reference when
we create a directory and remove it on directory removal.

The big unfortunate eye-sore is addressing the manual kobject reference
assumption on the networking code, which leads me to believe we should
end up replacing that eventually with another sort of check.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---

This v4 moves to fixing the race condition on dev_attr_store() and
dev_attr_read() to sysfs by bumping the kobject reference count
on directory creation / deletion as suggested by Greg.

Unfortunately at least the networking core has a manual refcount

36 more quoted lines

Shouldn't this be taken on open() not sysfs creation, or is the problem
that the kobject is held the module memory rather than duplicated by
sysfs?

And surely there are things besides netdevs that would suffer from this
change?

 		BUG_ON(!list_empty(&dev->ptype_all));
 		BUG_ON(!list_empty(&dev->ptype_specific));
 		WARN_ON(rcu_access_pointer(dev->ip_ptr));
-- 
2.27.0

-- 
Kees Cook

On Wed, Jun 23, 2021 at 03:59:37PM -0700, Kees Cook wrote:

On Wed, Jun 23, 2021 at 02:50:07PM -0700, Luis Chamberlain wrote:

quoted

It's possible today to have a device attribute read or store
race against device removal. This is known to happen as follows:

write system call -->
  ksys_write () -->
    vfs_write() -->
      __vfs_write() -->
        kernfs_fop_write_iter() -->
          sysfs_kf_write() -->
            dev_attr_store() -->
              null reference

This happens because the dev_attr->store() callback can be
removed prior to its call, after dev_attr_store() was initiated.
The null dereference is possible because the sysfs ops can be
removed on module removal, for instance, when device_del() is
called, and a sysfs read / store is not doing any kobject reference
bumps either. This allows a read/store call to initiate, a
device_del() to kick off, and then the read/store call can be
gone by the time to execute it.

The sysfs filesystem is not doing any kobject reference bumps during a
read / store ops to prevent this.

To fix this in a simplified way, just bump the kobject reference when
we create a directory and remove it on directory removal.

The big unfortunate eye-sore is addressing the manual kobject reference
assumption on the networking code, which leads me to believe we should
end up replacing that eventually with another sort of check.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---

This v4 moves to fixing the race condition on dev_attr_store() and
dev_attr_read() to sysfs by bumping the kobject reference count
on directory creation / deletion as suggested by Greg.

41 more quoted lines

If you kobject_get() you also protect from module removal to complete
until the calls ends. The issue is the dereference for the attribute
op will be gone if we don't protect the kobject during the op.

We only have an open() call for the bin sysfs files. So the alternative
is to sprinkly the get on all calls. The goal with adding it to the
directory creation was to simplify this.

quoted

diff --git a/net/core/dev.c b/net/core/dev.c
index 222b1d322c96..3a0ffa603d14 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -10429,7 +10429,7 @@ static void netdev_wait_allrefs(struct net_device *dev)
 	rebroadcast_time = warning_time = jiffies;
 	refcnt = netdev_refcnt_read(dev);
 
-	while (refcnt != 1) {
+	while (refcnt != 3) {
 		if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
 			rtnl_lock();
 

@@ -10544,7 +10544,7 @@ void netdev_run_todo(void)
 		netdev_wait_allrefs(dev);
 
 		/* paranoia */
-		BUG_ON(netdev_refcnt_read(dev) != 1);
+		BUG_ON(netdev_refcnt_read(dev) != 3);

And surely there are things besides netdevs that would suffer from this
change?

Yes, I noted this possibility below the commit log, below the "---".

  Luis

On Wed, Jun 23, 2021 at 03:59:37PM -0700, Kees Cook wrote:

On Wed, Jun 23, 2021 at 02:50:07PM -0700, Luis Chamberlain wrote:

quoted

It's possible today to have a device attribute read or store
race against device removal. This is known to happen as follows:

write system call -->
  ksys_write () -->
    vfs_write() -->
      __vfs_write() -->
        kernfs_fop_write_iter() -->
          sysfs_kf_write() -->
            dev_attr_store() -->
              null reference

This happens because the dev_attr->store() callback can be
removed prior to its call, after dev_attr_store() was initiated.
The null dereference is possible because the sysfs ops can be
removed on module removal, for instance, when device_del() is
called, and a sysfs read / store is not doing any kobject reference
bumps either. This allows a read/store call to initiate, a
device_del() to kick off, and then the read/store call can be
gone by the time to execute it.

The sysfs filesystem is not doing any kobject reference bumps during a
read / store ops to prevent this.

To fix this in a simplified way, just bump the kobject reference when
we create a directory and remove it on directory removal.

The big unfortunate eye-sore is addressing the manual kobject reference
assumption on the networking code, which leads me to believe we should
end up replacing that eventually with another sort of check.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---

This v4 moves to fixing the race condition on dev_attr_store() and
dev_attr_read() to sysfs by bumping the kobject reference count
on directory creation / deletion as suggested by Greg.

41 more quoted lines

No, this is when we are passing a pointer off to kernfs, it needs to
have the reference count incremented then otherwise it could "disappear"
without kernfs knowing about it.

quoted

diff --git a/net/core/dev.c b/net/core/dev.c
index 222b1d322c96..3a0ffa603d14 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -10429,7 +10429,7 @@ static void netdev_wait_allrefs(struct net_device *dev)
 	rebroadcast_time = warning_time = jiffies;
 	refcnt = netdev_refcnt_read(dev);
 
-	while (refcnt != 1) {
+	while (refcnt != 3) {
 		if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
 			rtnl_lock();
 

@@ -10544,7 +10544,7 @@ void netdev_run_todo(void)
 		netdev_wait_allrefs(dev);
 
 		/* paranoia */
-		BUG_ON(netdev_refcnt_read(dev) != 1);
+		BUG_ON(netdev_refcnt_read(dev) != 3);

And surely there are things besides netdevs that would suffer from this
change?

I _REALLY_ hope not.  No one should ever be looking at the count of a
kobject for anything as there are way too many ways that means nothing.

This is a fun hack for networking to be paranoid that they got their
initializion logic correct, so I'll leave it be right now.  No one else
should care, and if they do, we should fix it up...

thanks,

greg k-h

On Wed, Jun 23, 2021 at 02:50:07PM -0700, Luis Chamberlain wrote:

It's possible today to have a device attribute read or store
race against device removal. This is known to happen as follows:

write system call -->
  ksys_write () -->
    vfs_write() -->
      __vfs_write() -->
        kernfs_fop_write_iter() -->
          sysfs_kf_write() -->
            dev_attr_store() -->
              null reference

This happens because the dev_attr->store() callback can be
removed prior to its call, after dev_attr_store() was initiated.
The null dereference is possible because the sysfs ops can be
removed on module removal, for instance, when device_del() is
called, and a sysfs read / store is not doing any kobject reference
bumps either. This allows a read/store call to initiate, a
device_del() to kick off, and then the read/store call can be
gone by the time to execute it.

The sysfs filesystem is not doing any kobject reference bumps during a
read / store ops to prevent this.

To fix this in a simplified way, just bump the kobject reference when
we create a directory and remove it on directory removal.

The big unfortunate eye-sore is addressing the manual kobject reference
assumption on the networking code, which leads me to believe we should
end up replacing that eventually with another sort of check.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---

This v4 moves to fixing the race condition on dev_attr_store() and
dev_attr_read() to sysfs by bumping the kobject reference count
on directory creation / deletion as suggested by Greg.

This looks good.

It's late in the development cycle, I'll hold off on adding this to my
tree until 5.14-rc1 is out because of:

Unfortunately at least the networking core has a manual refcount
assumption, which needs to be adjusted to account for this change.
This should also mean there is runtime for other kobjects which may
not be explored yet which may need fixing as well. We may want to
change the check to something else on the networking front, but its
not clear to me yet what to use.

That's crazy what networking is doing here, hopefully no one else is.
If they are, let's shake it out in linux-next to find the problems which
is why a good "soak" there is a good idea.

thanks for making this change and sticking with it!

Oh, and with this change, does your modprobe/rmmod crazy test now work?

greg k-h

On Thu, Jun 24, 2021 at 01:09:03PM +0200, Greg KH wrote:

thanks for making this change and sticking with it!

Oh, and with this change, does your modprobe/rmmod crazy test now work?

It does but I wrote a test_syfs driver and I believe I see an issue with
this. I'll debug a bit more and see what it was, and I'll then also use
the driver to demo the issue more clearly, and then verification can be
an easy selftest test.

  Luis

Greeting,

FYI, we noticed the following commit (built with gcc-9):

commit: 1c04296f8fb2ab3c350c5889f8ac58f1690cd19c ("[PATCH v4] sysfs: fix kobject refcount to address races with kobject removal")
url: https://github.com/0day-ci/linux/commits/Luis-Chamberlain/sysfs-fix-kobject-refcount-to-address-races-with-kobject-removal/20210624-055134
base: https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git 09705dcb63d269000595284b5dd7f5c938d647b9

in testcase: suspend-stress
version: 
with following parameters:

	mode: mem
	iterations: 10



on test machine: 4 threads Ivy Bridge with 4G memory

caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):




If you fix the issue, kindly add following tag
Reported-by: kernel test robot <redacted>

SUSPEND RESUME TEST STARTED
Suspend to mem 1/10:
/usr/bin/wget -q --timeout=1800 --tries=1 --local-encoding=UTF-8 http://internal-lkp-server:80/~lkp/cgi-bin/lkp-jobfile-append-var?job_file=/lkp/jobs/scheduled/lkp-ivb-toshiba1/suspend-stress-10-mem-debian-x86_64-20180403.cgz-1c04296f8fb2ab3c350c5889f8ac58f1690cd19c-20210628-95711-737wjl-19.yaml&job_state=suspending-1/10 -O /dev/null
Failed



To reproduce:

        git clone https://github.com/intel/lkp-tests.git
        cd lkp-tests

13 more lines

Greg,

we can drop the patch "sysfs: fix kobject refcount to address races with
kobject removal", it took quite some work but I now have a selftest
which does prove this is not neeed. I'll post patches for this new
sysfs selftest soon.

  Luis

On Thu, Jul 01, 2021 at 10:27:37AM +0800, kernel test robot wrote:

Greeting,

FYI, we noticed the following commit (built with gcc-9):

commit: 1c04296f8fb2ab3c350c5889f8ac58f1690cd19c ("[PATCH v4] sysfs: fix kobject refcount to address races with kobject removal")
url: https://github.com/0day-ci/linux/commits/Luis-Chamberlain/sysfs-fix-kobject-refcount-to-address-races-with-kobject-removal/20210624-055134
base: https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git 09705dcb63d269000595284b5dd7f5c938d647b9

in testcase: suspend-stress
version: 
with following parameters:

	mode: mem
	iterations: 10



on test machine: 4 threads Ivy Bridge with 4G memory

caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):




If you fix the issue, kindly add following tag
Reported-by: kernel test robot <redacted>

SUSPEND RESUME TEST STARTED
Suspend to mem 1/10:
/usr/bin/wget -q --timeout=1800 --tries=1 --local-encoding=UTF-8 http://internal-lkp-server:80/~lkp/cgi-bin/lkp-jobfile-append-var?job_file=/lkp/jobs/scheduled/lkp-ivb-toshiba1/suspend-stress-10-mem-debian-x86_64-20180403.cgz-1c04296f8fb2ab3c350c5889f8ac58f1690cd19c-20210628-95711-737wjl-19.yaml&job_state=suspending-1/10 -O /dev/null
Failed



To reproduce:

        git clone https://github.com/intel/lkp-tests.git
        cd lkp-tests

13 more quoted lines

#
# Automatically generated file; DO NOT EDIT.
# Linux/x86_64 5.13.0-rc6 Kernel Configuration
#
CONFIG_CC_VERSION_TEXT="gcc-9 (Debian 9.3.0-22) 9.3.0"
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=90300
CONFIG_CLANG_VERSION=0
CONFIG_AS_IS_GNU=y
CONFIG_AS_VERSION=23502
CONFIG_LD_IS_BFD=y
CONFIG_LD_VERSION=23502
CONFIG_LLD_VERSION=0
CONFIG_CC_CAN_LINK=y
CONFIG_CC_CAN_LINK_STATIC=y
CONFIG_CC_HAS_ASM_GOTO=y
CONFIG_CC_HAS_ASM_INLINE=y
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_TABLE_SORT=y
CONFIG_THREAD_INFO_IN_TASK=y

#
# General setup
#
CONFIG_INIT_ENV_ARG_LIMIT=32
# CONFIG_COMPILE_TEST is not set
CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_BUILD_SALT=""
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_BZIP2=y
CONFIG_HAVE_KERNEL_LZMA=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_HAVE_KERNEL_LZO=y
CONFIG_HAVE_KERNEL_LZ4=y
CONFIG_HAVE_KERNEL_ZSTD=y
CONFIG_KERNEL_GZIP=y
# CONFIG_KERNEL_BZIP2 is not set
# CONFIG_KERNEL_LZMA is not set
# CONFIG_KERNEL_XZ is not set

6871 more quoted lines

#!/bin/sh

export_top_env()
{
	export suite='suspend-stress'
	export testcase='suspend-stress'
	export category='functional'
	export kernel_cmdline='initcall_debug log_buf_len=4M no_console_suspend ignore_loglevel'
	export iterations=10
	export runtime=
	export rtcwake=
	export idle_time=
	export job_origin='suspend-stress.yaml'
	export queue_cmdline_keys='branch
commit
queue_at_least_once'
	export queue='validate'
	export testbox='lkp-ivb-toshiba1'
	export tbox_group='lkp-ivb-toshiba1'
	export kconfig='x86_64-rhel-8.3'
	export submit_id='60d98c5543b45475dfbb4d58'
	export job_file='/lkp/jobs/scheduled/lkp-ivb-toshiba1/suspend-stress-10-mem-debian-x86_64-20180403.cgz-1c04296f8fb2ab3c350c5889f8ac58f1690cd19c-20210628-95711-737wjl-19.yaml'
	export id='08978452e87a05b957b48c91cdd3ad79ba922752'
	export queuer_version='/lkp-src'
	export model='Ivy Bridge'
	export nr_cpu=4
	export memory='4G'
	export belongs_to='power'
	export avoid_nfs=1
	export need_kconfig='CONFIG_SUSPEND=y
CONFIG_HIBERNATION=y
CONFIG_PM_DEBUG=y'
	export commit='1c04296f8fb2ab3c350c5889f8ac58f1690cd19c'
	export rootfs='debian-x86_64-20180403.cgz'
	export need_kconfig_hw='CONFIG_E1000E=y
CONFIG_SATA_AHCI
CONFIG_USB_NET_AX8817X=y
CONFIG_USB_NET_AX8817X=y
CONFIG_USB_NET_AX88179_178A=y
CONFIG_USB_RTL8152=y'

101 more quoted lines

SUSPEND RESUME TEST STARTED
Suspend to mem 1/10:
/usr/bin/wget -q --timeout=1800 --tries=1 --local-encoding=UTF-8 http://internal-lkp-server:80/~lkp/cgi-bin/lkp-jobfile-append-var?job_file=/lkp/jobs/scheduled/lkp-ivb-toshiba1/suspend-stress-10-mem-debian-x86_64-20180403.cgz-1c04296f8fb2ab3c350c5889f8ac58f1690cd19c-20210628-95711-737wjl-19.yaml&job_state=suspending-1/10 -O /dev/null
Failed

---
:#! jobs/suspend-stress.yaml:
suite: suspend-stress
testcase: suspend-stress
category: functional
kernel_cmdline: initcall_debug log_buf_len=4M no_console_suspend ignore_loglevel
suspend-stress:
  mode: mem
iterations: 10
runtime:
rtcwake:
idle_time:
job_origin: suspend-stress.yaml
:#! queue options:
queue_cmdline_keys:
- branch
- commit
queue: bisect
testbox: lkp-ivb-toshiba1
tbox_group: lkp-ivb-toshiba1
kconfig: x86_64-rhel-8.3
submit_id: 60d6052164e7c151e51e4406
job_file: "/lkp/jobs/scheduled/lkp-ivb-toshiba1/suspend-stress-10-mem-debian-x86_64-20180403.cgz-1c04296f8fb2ab3c350c5889f8ac58f1690cd19c-20210626-86501-1hs7lua-0.yaml"
id: dc895f6084372b9716923d670713e2653cbb09d0
queuer_version: "/lkp-src"
:#! hosts/lkp-ivb-toshiba1:
model: Ivy Bridge
nr_cpu: 4
memory: 4G
belongs_to: power
:#! include/category/functional:
kmsg:
heartbeat:
meminfo:
:#! include/suspend-stress:
avoid_nfs: 1
need_kconfig:
- CONFIG_SUSPEND=y
- CONFIG_HIBERNATION=y
- CONFIG_PM_DEBUG=y

80 more quoted lines

On Fri, Jun 25, 2021 at 02:56:03PM -0700, Luis Chamberlain wrote:

On Thu, Jun 24, 2021 at 01:09:03PM +0200, Greg KH wrote:

quoted

thanks for making this change and sticking with it!

Oh, and with this change, does your modprobe/rmmod crazy test now work?

It does but I wrote a test_syfs driver and I believe I see an issue with
this. I'll debug a bit more and see what it was, and I'll then also use
the driver to demo the issue more clearly, and then verification can be
an easy selftest test.

OK my conclusion based on a new selftest driver I wrote is we can drop
this patch safely. The selftest will cover this corner case well now.

In short: the kernfs active reference will ensure the store operation
still exists. The kernfs mutex is not enough, but if the driver removes
the operation prior to getting the active reference, the write will just
fail. The deferencing inside of the sysfs operation is abstract to
kernfs, and while kernfs can't do anything to prevent a driver from
doing something stupid, it at least can ensure an open file ensure the
op is not removed until the operation completes.

  Luis

On Thu, Jul 01, 2021 at 03:48:22PM -0700, Luis Chamberlain wrote:

On Fri, Jun 25, 2021 at 02:56:03PM -0700, Luis Chamberlain wrote:

quoted

On Thu, Jun 24, 2021 at 01:09:03PM +0200, Greg KH wrote:

quoted

thanks for making this change and sticking with it!

Oh, and with this change, does your modprobe/rmmod crazy test now work?

It does but I wrote a test_syfs driver and I believe I see an issue with
this. I'll debug a bit more and see what it was, and I'll then also use
the driver to demo the issue more clearly, and then verification can be
an easy selftest test.

OK my conclusion based on a new selftest driver I wrote is we can drop
this patch safely. The selftest will cover this corner case well now.

In short: the kernfs active reference will ensure the store operation
still exists. The kernfs mutex is not enough, but if the driver removes
the operation prior to getting the active reference, the write will just
fail. The deferencing inside of the sysfs operation is abstract to
kernfs, and while kernfs can't do anything to prevent a driver from
doing something stupid, it at least can ensure an open file ensure the
op is not removed until the operation completes.

OK and now its not so clear, as it would seem the refcount can indeed
get reduced after we validated it. In any case we'll have enough tools
to reproduce any possible failure soon.

  Luis

On Thu, Jul 01, 2021 at 03:48:16PM -0700, Luis Chamberlain wrote:

On Fri, Jun 25, 2021 at 02:56:03PM -0700, Luis Chamberlain wrote:

quoted

On Thu, Jun 24, 2021 at 01:09:03PM +0200, Greg KH wrote:

quoted

thanks for making this change and sticking with it!

Oh, and with this change, does your modprobe/rmmod crazy test now work?

It does but I wrote a test_syfs driver and I believe I see an issue with
this. I'll debug a bit more and see what it was, and I'll then also use
the driver to demo the issue more clearly, and then verification can be
an easy selftest test.

OK my conclusion based on a new selftest driver I wrote is we can drop
this patch safely. The selftest will cover this corner case well now.

In short: the kernfs active reference will ensure the store operation
still exists. The kernfs mutex is not enough, but if the driver removes
the operation prior to getting the active reference, the write will just
fail. The deferencing inside of the sysfs operation is abstract to
kernfs, and while kernfs can't do anything to prevent a driver from
doing something stupid, it at least can ensure an open file ensure the
op is not removed until the operation completes.

Ok, so all is good?  Then why is your zram test code blowing up so
badly?  Where is the reference counting going wrong?

thanks,

greg k-h

On Wed, Jul 21, 2021 at 01:30:29PM +0200, Greg KH wrote:

On Thu, Jul 01, 2021 at 03:48:16PM -0700, Luis Chamberlain wrote:

quoted

On Fri, Jun 25, 2021 at 02:56:03PM -0700, Luis Chamberlain wrote:

quoted

On Thu, Jun 24, 2021 at 01:09:03PM +0200, Greg KH wrote:

quoted

thanks for making this change and sticking with it!

Oh, and with this change, does your modprobe/rmmod crazy test now work?

It does but I wrote a test_syfs driver and I believe I see an issue with
this. I'll debug a bit more and see what it was, and I'll then also use
the driver to demo the issue more clearly, and then verification can be
an easy selftest test.

OK my conclusion based on a new selftest driver I wrote is we can drop
this patch safely. The selftest will cover this corner case well now.

In short: the kernfs active reference will ensure the store operation
still exists. The kernfs mutex is not enough, but if the driver removes
the operation prior to getting the active reference, the write will just
fail. The deferencing inside of the sysfs operation is abstract to
kernfs, and while kernfs can't do anything to prevent a driver from
doing something stupid, it at least can ensure an open file ensure the
op is not removed until the operation completes.

Ok, so all is good?

It would seem to be the case.

Then why is your zram test code blowing up so badly?

I checked the logs for the backtrace where the crash did happen
and we did see clear evidence of the race we feared here. The *first*
bug that happened was the CPU hotplug race:

[132004.787099] Error: Removing state 61 which has instances left.
[132004.787124] WARNING: CPU: 17 PID: 9307 at ../kernel/cpu.c:1879 __cpuhp_remove_state_cpuslocked+0x1c4/0x1d0

After this the crash happen:

[132005.254022] BUG: Unable to handle kernel instruction fetch
[132005.254049] Faulting instruction address: 0xc0080000004a0c24
[132005.254059] Oops: Kernel access of bad area, sig: 11 [#1]

And that's when the backtrace does come up with race. Given the first
race though, I think we can be skeptical of the rest, specially since
I cannot reproduce with a self bombing selftest.

Where is the reference counting going wrong?

It's not clear, as the misuse with the CPU multistate could lead
to to us leaking per cpu stuct zcomp instances, leaving these
behind as there is no one to remove them. I can't think of the
relationship of this leak and the crash other then memory pressure.

Because of this and the deadlock which is easily triggerable,
I decided to write a selftest to allow is to more cleanly be
able to reproduce any races we can dream up of.

  Luis

On Thu, Jul 22, 2021 at 02:31:37PM -0700, Luis Chamberlain wrote:

On Wed, Jul 21, 2021 at 01:30:29PM +0200, Greg KH wrote:

quoted

On Thu, Jul 01, 2021 at 03:48:16PM -0700, Luis Chamberlain wrote:

quoted

On Fri, Jun 25, 2021 at 02:56:03PM -0700, Luis Chamberlain wrote:

quoted

On Thu, Jun 24, 2021 at 01:09:03PM +0200, Greg KH wrote:

quoted

thanks for making this change and sticking with it!

Oh, and with this change, does your modprobe/rmmod crazy test now work?

It does but I wrote a test_syfs driver and I believe I see an issue with
this. I'll debug a bit more and see what it was, and I'll then also use
the driver to demo the issue more clearly, and then verification can be
an easy selftest test.

OK my conclusion based on a new selftest driver I wrote is we can drop
this patch safely. The selftest will cover this corner case well now.

In short: the kernfs active reference will ensure the store operation
still exists. The kernfs mutex is not enough, but if the driver removes
the operation prior to getting the active reference, the write will just
fail. The deferencing inside of the sysfs operation is abstract to
kernfs, and while kernfs can't do anything to prevent a driver from
doing something stupid, it at least can ensure an open file ensure the
op is not removed until the operation completes.

Ok, so all is good?

It would seem to be the case.

quoted

Then why is your zram test code blowing up so badly?

I checked the logs for the backtrace where the crash did happen
and we did see clear evidence of the race we feared here. The *first*
bug that happened was the CPU hotplug race:

[132004.787099] Error: Removing state 61 which has instances left.
[132004.787124] WARNING: CPU: 17 PID: 9307 at ../kernel/cpu.c:1879 __cpuhp_remove_state_cpuslocked+0x1c4/0x1d0

I do not understand what this issue is, is it fixed?  Why is a cpu being
hot unplugged at the same time a zram?

thanks,

greg k-h

On Fri, Jul 23, 2021 at 01:14:10PM +0200, Greg KH wrote:

On Thu, Jul 22, 2021 at 02:31:37PM -0700, Luis Chamberlain wrote:

quoted

On Wed, Jul 21, 2021 at 01:30:29PM +0200, Greg KH wrote:

quoted

On Thu, Jul 01, 2021 at 03:48:16PM -0700, Luis Chamberlain wrote:

quoted

On Fri, Jun 25, 2021 at 02:56:03PM -0700, Luis Chamberlain wrote:

quoted

On Thu, Jun 24, 2021 at 01:09:03PM +0200, Greg KH wrote:

quoted

thanks for making this change and sticking with it!

Oh, and with this change, does your modprobe/rmmod crazy test now work?

It does but I wrote a test_syfs driver and I believe I see an issue with
this. I'll debug a bit more and see what it was, and I'll then also use
the driver to demo the issue more clearly, and then verification can be
an easy selftest test.

OK my conclusion based on a new selftest driver I wrote is we can drop
this patch safely. The selftest will cover this corner case well now.

In short: the kernfs active reference will ensure the store operation
still exists. The kernfs mutex is not enough, but if the driver removes
the operation prior to getting the active reference, the write will just
fail. The deferencing inside of the sysfs operation is abstract to
kernfs, and while kernfs can't do anything to prevent a driver from
doing something stupid, it at least can ensure an open file ensure the
op is not removed until the operation completes.

Ok, so all is good?

It would seem to be the case.

quoted

Then why is your zram test code blowing up so badly?

I checked the logs for the backtrace where the crash did happen
and we did see clear evidence of the race we feared here. The *first*
bug that happened was the CPU hotplug race:

[132004.787099] Error: Removing state 61 which has instances left.
[132004.787124] WARNING: CPU: 17 PID: 9307 at ../kernel/cpu.c:1879 __cpuhp_remove_state_cpuslocked+0x1c4/0x1d0

I do not understand what this issue is, is it fixed?

My first patch for zram fixes the CPU multistate mis-use. And after that
patch is applied triggering the other race does not happen.  It is why I
decided to write a selftest driver, so that we can have a way to do all
sorts of crazy races in a self contained driver example.

Why is a cpu being hot unplugged at the same time a zram?

That's not what is happening. The description of the issue with zram's
misuse of CPU multistate is described clearly in my commit log for the
fix for that driver. You can refer to that commit log description.

  Luis