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:

quoted

On Fri, Jul 02, 2021 at 05:19:57PM -0700, Luis Chamberlain wrote:

quoted

+#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.

quoted

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