When sysfs attributes use a lock also used on module removal we can
potentially deadlock. This happens when for instance a sysfs file on
a driver is used, then at the same time we have module removal call
trigger. The module removal call code holds a lock, and then the sysfs
file entry waits for the same lock. While holding the lock the module
removal tries to remove the sysfs entries, but these cannot be removed
yet as one is waiting for a lock. This won't complete as the lock is
already held. Likewise module removal cannot complete, and so we deadlock.
To fix this we just *try* to get a refcount to the module when a shared
lock is used, prior to mucking with a sysfs attribute. If this fails we
just give up right away.
We use a try method as a full lock means we'd then make our sysfs
attributes busy us out from possible module removal, and so userspace
could force denying module removal, a silly form of "DOS" against module
removal. A try lock on the module removal ensures we give priority to
module removal and interacting with sysfs attributes only comes second.
Using a full lock could mean for instance that if you don't stop poking
at sysfs files you cannot remove a module.
This deadlock was first reported with the zram driver, a sketch of how
this can happen follows:
CPU A CPU B
whatever_store()
module_unload
mutex_lock(foo)
mutex_lock(foo)
del_gendisk(zram->disk);
device_del()
device_remove_groups()
In this situation whatever_store() is waiting for the mutex foo to
become unlocked, but that won't happen until module removal is complete.
But module removal won't complete until the sysfs file being poked
completes which is waiting for a lock already held.
This is a generic kernel issue with sysfs files which use any lock also
used on module removal. Different generic solutions have been proposed.
One approach proposed is by directly by augmenting attributes with module
information [0]. This patch implements a solution by adding macros with
the prefix MODULE_DEVICE_ATTR_*() which accomplish the same. Until we
don't have a generic agreed upon solution for this shared between drivers,
we must implement a fix for this on each driver.
We make zram use the new MODULE_DEVICE_ATTR_*() helpers, and completely
open code the solution for class attributes as there are only a few of
those.
This issue can be reproduced easily on the zram driver as follows:
Loop 1 on one terminal:
while true;
do modprobe zram;
modprobe -r zram;
done
Loop 2 on a second terminal:
while true; do
echo 1024 > /sys/block/zram0/disksize;
echo 1 > /sys/block/zram0/reset;
done
Without this patch we end up in a deadlock, and the following
stack trace is produced which hints to us what the issue was:
INFO: task bash:888 blocked for more than 120 seconds.
Tainted: G E 5.12.0-rc1-next-20210304+ #4
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:bash state:D stack: 0 pid: 888 ppid: 887 flags:<etc>
Call Trace:
__schedule+0x2e4/0x900
schedule+0x46/0xb0
schedule_preempt_disabled+0xa/0x10
__mutex_lock.constprop.0+0x2c3/0x490
? _kstrtoull+0x35/0xd0
reset_store+0x6c/0x160 [zram]
kernfs_fop_write_iter+0x124/0x1b0
new_sync_write+0x11c/0x1b0
vfs_write+0x1c2/0x260
ksys_write+0x5f/0xe0
do_syscall_64+0x33/0x80
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f34f2c3df33
RSP: 002b:00007ffe751df6e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f34f2c3df33
RDX: 0000000000000002 RSI: 0000561ccb06ec10 RDI: 0000000000000001
RBP: 0000561ccb06ec10 R08: 000000000000000a R09: 0000000000000001
R10: 0000561ccb157590 R11: 0000000000000246 R12: 0000000000000002
R13: 00007f34f2d0e6a0 R14: 0000000000000002 R15: 00007f34f2d0e8a0
INFO: task modprobe:1104 can't die for more than 120 seconds.
task:modprobe state:D stack: 0 pid: 1104 ppid: 916 flags:<etc>
Call Trace:
__schedule+0x2e4/0x900
schedule+0x46/0xb0
__kernfs_remove.part.0+0x228/0x2b0
? finish_wait+0x80/0x80
kernfs_remove_by_name_ns+0x50/0x90
remove_files+0x2b/0x60
sysfs_remove_group+0x38/0x80
sysfs_remove_groups+0x29/0x40
device_remove_attrs+0x4a/0x80
device_del+0x183/0x3e0
? mutex_lock+0xe/0x30
del_gendisk+0x27a/0x2d0
zram_remove+0x8a/0xb0 [zram]
? hot_remove_store+0xf0/0xf0 [zram]
zram_remove_cb+0xd/0x10 [zram]
idr_for_each+0x5e/0xd0
destroy_devices+0x39/0x6f [zram]
__do_sys_delete_module+0x190/0x2a0
do_syscall_64+0x33/0x80
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f32adf727d7
RSP: 002b:00007ffc08bb38a8 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
RAX: ffffffffffffffda RBX: 000055eea23cbb10 RCX: 00007f32adf727d7
RDX: 0000000000000000 RSI: 0000000000000800 RDI: 000055eea23cbb78
RBP: 000055eea23cbb10 R08: 0000000000000000 R09: 0000000000000000
R10: 00007f32adfe5ac0 R11: 0000000000000206 R12: 000055eea23cbb78
R13: 0000000000000000 R14: 0000000000000000 R15: 000055eea23cbc20
[0] https://lkml.kernel.org/r/[email protected]
Signed-off-by: Luis Chamberlain <[email protected]>
---
drivers/block/zram/zram_drv.c | 47 ++++++++++++++++++------------
drivers/block/zram/zram_drv.h | 54 +++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 18 deletions(-)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 43d4c9971330..205cf9287d0c 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1134,12 +1134,12 @@ static ssize_t debug_stat_show(struct device *dev,
return ret;
}
-static DEVICE_ATTR_RO(io_stat);
-static DEVICE_ATTR_RO(mm_stat);
+MODULE_DEVICE_ATTR_RO(io_stat);
+MODULE_DEVICE_ATTR_RO(mm_stat);
#ifdef CONFIG_ZRAM_WRITEBACK
-static DEVICE_ATTR_RO(bd_stat);
+MODULE_DEVICE_ATTR_RO(bd_stat);
#endif
-static DEVICE_ATTR_RO(debug_stat);
+MODULE_DEVICE_ATTR_RO(debug_stat);
static void zram_meta_free(struct zram *zram, u64 disksize)
{
@@ -1861,20 +1861,20 @@ static const struct block_device_operations zram_wb_devops = {
.owner = THIS_MODULE
};
-static DEVICE_ATTR_WO(compact);
-static DEVICE_ATTR_RW(disksize);
-static DEVICE_ATTR_RO(initstate);
-static DEVICE_ATTR_WO(reset);
-static DEVICE_ATTR_WO(mem_limit);
-static DEVICE_ATTR_WO(mem_used_max);
-static DEVICE_ATTR_WO(idle);
-static DEVICE_ATTR_RW(max_comp_streams);
-static DEVICE_ATTR_RW(comp_algorithm);
+MODULE_DEVICE_ATTR_WO(compact);
+MODULE_DEVICE_ATTR_RW(disksize);
+MODULE_DEVICE_ATTR_RO(initstate);
+MODULE_DEVICE_ATTR_WO(reset);
+MODULE_DEVICE_ATTR_WO(mem_limit);
+MODULE_DEVICE_ATTR_WO(mem_used_max);
+MODULE_DEVICE_ATTR_WO(idle);
+MODULE_DEVICE_ATTR_RW(max_comp_streams);
+MODULE_DEVICE_ATTR_RW(comp_algorithm);
#ifdef CONFIG_ZRAM_WRITEBACK
-static DEVICE_ATTR_RW(backing_dev);
-static DEVICE_ATTR_WO(writeback);
-static DEVICE_ATTR_RW(writeback_limit);
-static DEVICE_ATTR_RW(writeback_limit_enable);
+MODULE_DEVICE_ATTR_RW(backing_dev);
+MODULE_DEVICE_ATTR_WO(writeback);
+MODULE_DEVICE_ATTR_RW(writeback_limit);
+MODULE_DEVICE_ATTR_RW(writeback_limit_enable);
#endif
static struct attribute *zram_disk_attrs[] = {
@@ -2037,13 +2037,19 @@ static ssize_t hot_add_show(struct class *class,
{
int ret;
+ if (!try_module_get(THIS_MODULE))
+ return -ENODEV;
+
mutex_lock(&zram_index_mutex);
if (!zram_up) {
mutex_unlock(&zram_index_mutex);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out;
}
ret = zram_add();
+out:
mutex_unlock(&zram_index_mutex);
+ module_put(THIS_MODULE);
if (ret < 0)
return ret;
@@ -2052,6 +2058,7 @@ static ssize_t hot_add_show(struct class *class,
static struct class_attribute class_attr_hot_add =
__ATTR(hot_add, 0400, hot_add_show, NULL);
+#define module_hot_remove_store hot_remove_store
static ssize_t hot_remove_store(struct class *class,
struct class_attribute *attr,
const char *buf,
@@ -2067,6 +2074,9 @@ static ssize_t hot_remove_store(struct class *class,
if (dev_id < 0)
return -EINVAL;
+ if (!try_module_get(THIS_MODULE))
+ return -ENODEV;
+
mutex_lock(&zram_index_mutex);
if (!zram_up) {
@@ -2085,6 +2095,7 @@ static ssize_t hot_remove_store(struct class *class,
out:
mutex_unlock(&zram_index_mutex);
+ module_put(THIS_MODULE);
return ret ? ret : count;
}
static CLASS_ATTR_WO(hot_remove);
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 80c3b43b4828..90f6777d7d0a 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -126,4 +126,58 @@ struct zram {
struct dentry *debugfs_dir;
#endif
};
+
+#undef __ATTR_RO
+#undef __ATTR_RW
+#undef __ATTR_WO
+
+#define __ATTR_RO(_name) { \
+ .attr = { .name = __stringify(_name), .mode = 0444 }, \
+ .show = module_##_name##_show, \
+}
+#define __ATTR_RW(_name) __ATTR(_name, 0644, module_##_name##_show, module_##_name##_store)
+#define __ATTR_WO(_name) { \
+ .attr = { .name = __stringify(_name), .mode = 0200 }, \
+ .store = module_##_name##_store, \
+}
+
+#define MODULE_DEVICE_ATTR_FUNC_STORE(_name) \
+static ssize_t module_ ## _name ## _store(struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t len) \
+{ \
+ ssize_t __ret; \
+ if (!try_module_get(THIS_MODULE)) \
+ return -ENODEV; \
+ __ret = _name ## _store(dev, attr, buf, len); \
+ module_put(THIS_MODULE); \
+ return __ret; \
+}
+
+#define MODULE_DEVICE_ATTR_FUNC_SHOW(_name) \
+static ssize_t module_ ## _name ## _show(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+{ \
+ ssize_t __ret; \
+ if (!try_module_get(THIS_MODULE)) \
+ return -ENODEV; \
+ __ret = _name ## _show(dev, attr, buf); \
+ module_put(THIS_MODULE); \
+ return __ret; \
+}
+
+#define MODULE_DEVICE_ATTR_WO(_name) \
+MODULE_DEVICE_ATTR_FUNC_STORE(_name); \
+static DEVICE_ATTR_WO(_name)
+
+#define MODULE_DEVICE_ATTR_RW(_name) \
+MODULE_DEVICE_ATTR_FUNC_STORE(_name); \
+MODULE_DEVICE_ATTR_FUNC_SHOW(_name); \
+static DEVICE_ATTR_RW(_name)
+
+#define MODULE_DEVICE_ATTR_RO(_name) \
+MODULE_DEVICE_ATTR_FUNC_SHOW(_name); \
+static DEVICE_ATTR_RO(_name)
+
#endif
--
2.27.0
On Fri, 2 Jul 2021 17:19:57 -0700 Luis Chamberlain <[email protected]> wrote:
> +#define MODULE_DEVICE_ATTR_FUNC_STORE(_name) \
> +static ssize_t module_ ## _name ## _store(struct device *dev, \
> + struct device_attribute *attr, \
> + const char *buf, size_t len) \
> +{ \
> + ssize_t __ret; \
> + if (!try_module_get(THIS_MODULE)) \
> + return -ENODEV; \
> + __ret = _name ## _store(dev, attr, buf, len); \
> + module_put(THIS_MODULE); \
> + return __ret; \
> +}
I assume that Greg's comments on try_module_get() are applicable here
also.
On Sat, Jul 10, 2021 at 12:28:51PM -0700, Andrew Morton wrote:
> On Fri, 2 Jul 2021 17:19:57 -0700 Luis Chamberlain <[email protected]> wrote:
>
> > +#define MODULE_DEVICE_ATTR_FUNC_STORE(_name) \
> > +static ssize_t module_ ## _name ## _store(struct device *dev, \
> > + struct device_attribute *attr, \
> > + const char *buf, size_t len) \
> > +{ \
> > + ssize_t __ret; \
> > + if (!try_module_get(THIS_MODULE)) \
> > + return -ENODEV; \
> > + __ret = _name ## _store(dev, attr, buf, len); \
> > + module_put(THIS_MODULE); \
> > + return __ret; \
> > +}
>
> I assume that Greg's comments on try_module_get() are applicable here
> also.
Yes, this is still broken code and does not do what it says it does,
please do not merge it.
Again, almost anything that does try_module_get(THIS_MODULE) is broken,
this code included. I'll write more in a week or so when I get a chance
to get to this series in my reviews...
thanks,
greg k-h
On Sat, Jul 10, 2021 at 12:28:51PM -0700, Andrew Morton wrote:
> On Fri, 2 Jul 2021 17:19:57 -0700 Luis Chamberlain <[email protected]> wrote:
>
> > +#define MODULE_DEVICE_ATTR_FUNC_STORE(_name) \
> > +static ssize_t module_ ## _name ## _store(struct device *dev, \
> > + struct device_attribute *attr, \
> > + const char *buf, size_t len) \
> > +{ \
> > + ssize_t __ret; \
> > + if (!try_module_get(THIS_MODULE)) \
> > + return -ENODEV; \
> > + __ret = _name ## _store(dev, attr, buf, len); \
> > + module_put(THIS_MODULE); \
> > + return __ret; \
> > +}
>
> I assume that Greg's comments on try_module_get() are applicable here
> also.
While we wait for Greg for an alternative, patch #1 is still fine.
Luis
On Fri, Jul 02, 2021 at 05:19:57PM -0700, Luis Chamberlain wrote:
> +#define MODULE_DEVICE_ATTR_FUNC_STORE(_name) \
> +static ssize_t module_ ## _name ## _store(struct device *dev, \
> + struct device_attribute *attr, \
> + const char *buf, size_t len) \
> +{ \
> + ssize_t __ret; \
> + if (!try_module_get(THIS_MODULE)) \
> + return -ENODEV; \
I feel like this needs to be written down somewhere as I see it come up
all the time.
Again, this is racy and broken code. You can NEVER try to increment
your own module reference count unless it has already been incremented
by someone external first.
As "proof", what happens if this module is unloaded right _before_ this
call happens? The module will be unloaded, memory zeroed out (or
overridden), and then the processor will resume here and try to call (or
return into) this code path.
Boom.
Just say no to "try_module_get(THIS_MODULE)" as it is totally wrong.
thanks,
greg k-h
On Wed, Jul 21, 2021 at 01:29:29PM +0200, Greg KH wrote:
> On Fri, Jul 02, 2021 at 05:19:57PM -0700, Luis Chamberlain wrote:
> > +#define MODULE_DEVICE_ATTR_FUNC_STORE(_name) \
> > +static ssize_t module_ ## _name ## _store(struct device *dev, \
> > + struct device_attribute *attr, \
> > + const char *buf, size_t len) \
> > +{ \
> > + ssize_t __ret; \
> > + if (!try_module_get(THIS_MODULE)) \
> > + return -ENODEV; \
>
> I feel like this needs to be written down somewhere as I see it come up
> all the time.
I'll go ahead and cook up a patch to do just this after I send this
email out.
> Again, this is racy and broken code. You can NEVER try to increment
> your own module reference count unless it has already been incremented
> by someone external first.
In the zram driver's case the sysfs files are still pegged on, because
as we noted before the kernfs active reference will ensure the store
operation still exists. If the driver removes the operation prior to
getting the active reference, the write will just fail. kernfs ensures
once a file is opened the op is not removed until the operation completes.
If a file is opened then, the module cannot possibly be removed. The
piece of information we realy care about is the use of module_is_live()
inside try_module_get() which does:
static inline bool module_is_live(struct module *mod)
{
return mod->state != MODULE_STATE_GOING;
}
The try allows module removal to trump use of the sysfs file. If
userspace wants the module removed, it gives up in favor for that
operation.
> As "proof", what happens if this module is unloaded right _before_ this
> call happens? The module will be unloaded, memory zeroed out (or
> overridden), and then the processor will resume here and try to call (or
> return into) this code path.
The use of try_module_get() is protected to be correct by the kernfs active
reference, which in turn ensures the module is not gone. That is, when
a sysfs file read / write op is issued, if the file was opened we *know*
the module is not gone yet. It cannot possibly be removed. But once
inside the operation, try_module_get() can check to see if userspace did
want to remove the module, and if so it would immediately bail out and
yield to that operation.
Userspace cannot open a sysfs file with the module being gone.
The kernfs active prevents that.
> Boom.
I think it would be good we add a self test for this particular case.
I'll go ahead and extend my sysfs tests with one test for this case.
I could do this by adding a new sysfs file to the test driver and where
all it does is this try_module_get() thing. This can then be raced with
module removals attempts.
But it does not mean all you say is wrong.
I think the value of what you are saying requires documenting as it
was not clear to me either. I'll send a patch now.
> Just say no to "try_module_get(THIS_MODULE)" as it is totally wrong.
Context is required and documented. I'll end a patch.
Luis
On Thu, Jul 22, 2021 at 03:17:05PM -0700, Luis Chamberlain wrote:
> On Wed, Jul 21, 2021 at 01:29:29PM +0200, Greg KH wrote:
> > On Fri, Jul 02, 2021 at 05:19:57PM -0700, Luis Chamberlain wrote:
> > > +#define MODULE_DEVICE_ATTR_FUNC_STORE(_name) \
> > > +static ssize_t module_ ## _name ## _store(struct device *dev, \
> > > + struct device_attribute *attr, \
> > > + const char *buf, size_t len) \
> > > +{ \
> > > + ssize_t __ret; \
> > > + if (!try_module_get(THIS_MODULE)) \
> > > + return -ENODEV; \
> >
> > I feel like this needs to be written down somewhere as I see it come up
> > all the time.
>
> I'll go ahead and cook up a patch to do just this after I send this
> email out.
>
> > Again, this is racy and broken code. You can NEVER try to increment
> > your own module reference count unless it has already been incremented
> > by someone external first.
>
> In the zram driver's case the sysfs files are still pegged on, because
> as we noted before the kernfs active reference will ensure the store
> operation still exists.
How does that happen without a module lock?
> If the driver removes the operation prior to
> getting the active reference, the write will just fail. kernfs ensures
> once a file is opened the op is not removed until the operation completes.
How does it do that?
> If a file is opened then, the module cannot possibly be removed. The
> piece of information we realy care about is the use of module_is_live()
> inside try_module_get() which does:
>
> static inline bool module_is_live(struct module *mod)
> {
> return mod->state != MODULE_STATE_GOING;
> }
>
> The try allows module removal to trump use of the sysfs file. If
> userspace wants the module removed, it gives up in favor for that
> operation.
I do not see the tie in kernfs to module reference counts, what am I
missing?
thanks,
greg k-h
On Fri, Jul 23, 2021 at 01:15:49PM +0200, Greg KH wrote:
> On Thu, Jul 22, 2021 at 03:17:05PM -0700, Luis Chamberlain wrote:
> > On Wed, Jul 21, 2021 at 01:29:29PM +0200, Greg KH wrote:
> > > On Fri, Jul 02, 2021 at 05:19:57PM -0700, Luis Chamberlain wrote:
> > > > +#define MODULE_DEVICE_ATTR_FUNC_STORE(_name) \
> > > > +static ssize_t module_ ## _name ## _store(struct device *dev, \
> > > > + struct device_attribute *attr, \
> > > > + const char *buf, size_t len) \
> > > > +{ \
> > > > + ssize_t __ret; \
> > > > + if (!try_module_get(THIS_MODULE)) \
> > > > + return -ENODEV; \
> > >
> > > I feel like this needs to be written down somewhere as I see it come up
> > > all the time.
> >
> > I'll go ahead and cook up a patch to do just this after I send this
> > email out.
> >
> > > Again, this is racy and broken code. You can NEVER try to increment
> > > your own module reference count unless it has already been incremented
> > > by someone external first.
> >
> > In the zram driver's case the sysfs files are still pegged on, because
> > as we noted before the kernfs active reference will ensure the store
> > operation still exists.
>
> How does that happen without a module lock?
If a read / write operations is happening on a sysfs file created by a
module, the module cannot be removed because it is the module's own
responsibility to remove the sysfs file on module exit. There is no
module lock. It is inferred.
> > If the driver removes the operation prior to
> > getting the active reference, the write will just fail. kernfs ensures
> > once a file is opened the op is not removed until the operation completes.
>
> How does it do that?
Using an active reference.
> > If a file is opened then, the module cannot possibly be removed. The
> > piece of information we realy care about is the use of module_is_live()
> > inside try_module_get() which does:
> >
> > static inline bool module_is_live(struct module *mod)
> > {
> > return mod->state != MODULE_STATE_GOING;
> > }
> >
> > The try allows module removal to trump use of the sysfs file. If
> > userspace wants the module removed, it gives up in favor for that
> > operation.
>
> I do not see the tie in kernfs to module reference counts, what am I
> missing?
Let me try to describe this again. Let's take it step by step, premise
by premise on the inference assumption. Let me know at which point you
disagree.
We are talking about sysfs files and you're argument is that
try_module_get() should lock the module, and so cannot be used
in sysfs files. My point is that such module lock is inferred:
1) Sysfs files are created by a module, that same module is responsible
for removing the same sysfs files.
2) The module can only be removed and gone, once *all* sysfs files are
removed first.
3) If any of the module's sysfs files are present the module must
still be present
4) kernfs ensures that if a file is opened the file will not be
removed until any pending operation completes
5) If a sysfs file is used to write something, that means the
sysfs file has not yet been removed, and we know it will
remain in existance throughout its entire operation
6) When a sysfs file operation is being run, the module must
always exist
Luis
On Fri, Jul 23, 2021 at 10:49:19AM -0700, Luis Chamberlain wrote:
> We are talking about sysfs files and you're argument is that
> try_module_get() should lock the module, and so cannot be used
> in sysfs files. My point is that such module lock is inferred:
>
> 1) Sysfs files are created by a module, that same module is responsible
> for removing the same sysfs files.
> 2) The module can only be removed and gone, once *all* sysfs files are
> removed first.
> 3) If any of the module's sysfs files are present the module must
> still be present
> 4) kernfs ensures that if a file is opened the file will not be
> removed until any pending operation completes
> 5) If a sysfs file is used to write something, that means the
> sysfs file has not yet been removed, and we know it will
> remain in existance throughout its entire operation
> 6) When a sysfs file operation is being run, the module must
> always exist
Greg,
I'm inclined to believe my original generic solution would be better
again [0]. Specially since we can drop the dev_type_get() / dev_type_put()
stuff.
Thoughts?
[0] https://lore.kernel.org/linux-block/[email protected]/
Luis