On Wed, Jun 23, 2021 at 09:14:34AM -0700, Luis Chamberlain wrote:

kernfs / sysfs is in not struct device specific, it has no semantics for
modules or even devices. The aspect of kernfs which deals with locking
a struct kernfs_node is kernfs_get_active(). The refcount there of
importance is the call to atomic_inc_unless_negative(&kn->active).

struct kernfs_node *kernfs_get_active(struct kernfs_node *kn)                   
{                                                                               
	if (unlikely(!kn))
		return NULL;                                                    

	if (!atomic_inc_unless_negative(&kn->active))                           
		return NULL;                                                    

	if (kernfs_lockdep(kn))
		rwsem_acquire_read(&kn->dep_map, 0, 1, _RET_IP_);               
	return kn;                                                              
}

BTW this was also precisely where I had suggested to extend the
kernfs_node with an optional kn->owner and if set we try_module_get() to
prevent the deadlock case if the module exit routine also happens to use
a lock also used by a sysfs attribute store/read.

The flow would be (from a real live gdb crash backtrace from an original
bug report from a customer):

write system call -->
  ksys_write ()
    vfs_write() -->
      __vfs_write() -->
       kernfs_fop_write() (note now this is kernfs_fop_write_iter()) -->
         sysfs_kf_write() -->
           dev_attr_store() -->
	     null reference

The dereference is because the dev_attr_store() call which we are
modifying

LINE-001 static ssize_t dev_attr_store(struct kobject *kobj, struct attribute *attr,                                                                                         
LINE-002 const char *buf, size_t count)                                                                                                                 
LINE-003 {
LINE-004	struct device_attribute *dev_attr = to_dev_attr(attr);
LINE-005	struct device *dev = kobj_to_dev(kobj);
LINE-006	ssize_t ret = -EIO;
LINE-007
LINE-008	if (dev_attr->store)
LINE-009		ret = dev_attr->store(dev, dev_attr, buf, count);
	...
	}

The race happens because a sysfs store / read can be triggered, the CPU
could be preempted after LINE-008, and the ->store is gone by LINE-009.
This begs the question if kernfs_fop_write_iter() or sysfs protects this
somehow? Let's see.

For kernfs we have:

static ssize_t kernfs_fop_write_iter(struct kiocb *iocb, struct iov_iter *iter) 
{
	struct kernfs_open_file *of = kernfs_of(iocb->ki_filp);
	...
	mutex_lock(&of->mutex);
	if (!kernfs_get_active(of->kn)) {
		mutex_unlock(&of->mutex);
		len = -ENODEV;
		goto out_free;
	}

	ops = kernfs_ops(of->kn);
	if (ops->write)
		len = ops->write(of, buf, len, iocb->ki_pos);
	else
		len = -EINVAL;

	kernfs_put_active(of->kn);
	mutex_unlock(&of->mutex);
	...
}

And the write call here is a syfs calls:

static ssize_t sysfs_kf_write(struct kernfs_open_file *of, char *buf,           
			      size_t count, loff_t pos)                         
{                                                                               
	const struct sysfs_ops *ops = sysfs_file_ops(of->kn);                   
	struct kobject *kobj = of->kn->parent->priv;                            

	if (!count)                                                             
		return 0;                                                       

	return ops->store(kobj, of->kn->priv, buf, count);                      
}           

As we have observed already, the active reference obtained through
kernfs_get_active() was for the struct kernfs_node. Sure, the
ops->write() is valid, in this case it sysfs_kf_write().

sysfs isn't doing any active reference check for the kobject device
attribute as it doesn't care for them. So syfs calls
dev_attr_store(), but the dev_attr_store() is not preventing the device
attribute ops to go fishing, and we destroy them while we're destroying
the device on module removal.

Ah, but sysfs _should_ be doing this properly.

I think the issue is that when we store the kobject pointer in kernfs,
it is NOT incremented.  Look at sysfs_create_dir_ns(), if we call
kobject_get(kobj) right before we call kernfs_create_dir_ns(), and then
properly clean things up later on when we remove the sysfs directory
(i.e. the kobject), it _should_ fix this problem.

Then, we know, whenever show/store/whatever is called, when we cast out
of kernfs the private pointer to a kobject, that the kobject really is
still alive, so we can use it properly.

Can you try that, it should be a much "simpler" change here.

thanks,

greg k-h