| Message ID | 20221229160045.535778-3-brgl@bgdev.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: fortify the subsystem against user-space induced deadlocks | expand |
> In order to fix that, we introduce a couple changes. They need to be > part of a single commit in order to preserve bisectability. First, drop Do you have maybe a branch at hand with more finegrained commits just for reviewing?
On Tue, 17 Jan 2023 at 11:08, Wolfram Sang <wsa@kernel.org> wrote: > > > > In order to fix that, we introduce a couple changes. They need to be > > part of a single commit in order to preserve bisectability. First, drop > > Do you have maybe a branch at hand with more finegrained commits just > for reviewing? > No, not really. It can be logically split into two elements though: removing the existing completion and then adding the locking bits which unfortunately go into many places. I tried to make the commit description as elaborate as possible but let me know if you have any specific questions. Bart
> No, not really. It can be logically split into two elements though: Ok, no worries. > removing the existing completion and then adding the locking bits > which unfortunately go into many places. I tried to make the commit > description as elaborate as possible but let me know if you have any > specific questions. Will do. The commit message _is_ nicely elaborate, still I need to dive into the topic thoroughly. I'll let you know how it goes.
Hi Bartosz, > If we open an i2c character device and then unbind the underlying i2c > adapter (either by unbinding it manually via sysfs or - for a real-life > example - when unplugging a USB device with an i2c adaper), the kernel > thread calling i2c_del_adapter() will become blocked waiting for the > completion that only completes once all references to the character > device get dropped. > > In order to fix that, we introduce a couple changes. They need to be > part of a single commit in order to preserve bisectability. First, drop > the dev_release completion. That removes the risk of a deadlock but > we now need to protect the character device structures against NULL > pointer dereferences. To that end introduce an rw semaphore. It will > protect the dummy i2c_client structure against dropping the adapter from > under it. It will be taken for reading by all file_operations callbacks > and for writing by the notifier's unbind handler. This way we don't > prohibit the syscalls that don't get in each other's way from running > concurrently but the adapter will not be unbound before all syscalls > return. > > Finally: upon being notified about an unbind event for the i2c adapter, > we take the lock for writing and set the adapter pointer in the character > device's structure to NULL. This "numbs down" the device - it still exists > but is no longer functional. Meanwhile every syscall callback checks that > pointer after taking the lock but before executing any code that requires > it. If it's NULL, we return an error to user-space. > > This way we can safely open an i2c device from user-space, unbind the > device without triggering a deadlock and any subsequent system-call for > the file descriptor associated with the removed adapter will gracefully > fail. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> First of all, thank you for tackling this problem. It was long overdue and I really appreciate that you took the initiative to get it solved. Here are some review comments already. I'd like to do some more testing. So far, everything works nicely. Also, I'd like to invite more people to have a look at this code. We really don't want to have a regression here, so more eyes are very welcome. > @@ -1713,25 +1715,7 @@ void i2c_del_adapter(struct i2c_adapter *adap) > > i2c_host_notify_irq_teardown(adap); > > - /* wait until all references to the device are gone > - * > - * FIXME: This is old code and should ideally be replaced by an > - * alternative which results in decoupling the lifetime of the struct > - * device from the i2c_adapter, like spi or netdev do. Any solution > - * should be thoroughly tested with DEBUG_KOBJECT_RELEASE enabled! Have you done this? Debugging with DEBUG_KOBJECT_RELEASE enabled? > - */ > - init_completion(&adap->dev_released); > device_unregister(&adap->dev); > - wait_for_completion(&adap->dev_released); So, this is basically the key change. I think you handled the userspace part via i2c-dev well. I don't have proof yet, but my gut feeling wonders if this is complete. Aren't there maybe sysfs-references as well. I need to check. > - > - /* free bus id */ > - mutex_lock(&core_lock); > - idr_remove(&i2c_adapter_idr, adap->nr); > - mutex_unlock(&core_lock); > - > - /* Clear the device structure in case this adapter is ever going to be > - added again */ > - memset(&adap->dev, 0, sizeof(adap->dev)); Any reason you didn't add this to release function above? Reading the introducing commit, the old drivers needings this still exist IMO. (Yes, they should be converted to use the i2c-mux subsystem, but I don't think someone will do this) > @@ -44,8 +45,14 @@ struct i2c_dev { > struct i2c_adapter *adap; > struct device dev; > struct cdev cdev; > + struct rw_semaphore sem; I'd like to name it 'rwsem' so it is always super-clear what it is. > }; > > +static inline struct i2c_dev *to_i2c_dev(struct inode *ino) I'd also prefer a more specific 'inode_to_i2c_dev' function name. Personally, I'd also name the argument 'inode' but I'll leave that to you. > +{ > + return container_of(ino->i_cdev, struct i2c_dev, cdev); > +} ... Doesn't the function 'name_show()' also need protection? It dereferences i2c_dev->adap. So much for now, Wolfram
On Tue, Jan 17, 2023 at 7:57 PM Wolfram Sang <wsa@kernel.org> wrote: > > Hi Bartosz, > > > If we open an i2c character device and then unbind the underlying i2c > > adapter (either by unbinding it manually via sysfs or - for a real-life > > example - when unplugging a USB device with an i2c adaper), the kernel > > thread calling i2c_del_adapter() will become blocked waiting for the > > completion that only completes once all references to the character > > device get dropped. > > > > In order to fix that, we introduce a couple changes. They need to be > > part of a single commit in order to preserve bisectability. First, drop > > the dev_release completion. That removes the risk of a deadlock but > > we now need to protect the character device structures against NULL > > pointer dereferences. To that end introduce an rw semaphore. It will > > protect the dummy i2c_client structure against dropping the adapter from > > under it. It will be taken for reading by all file_operations callbacks > > and for writing by the notifier's unbind handler. This way we don't > > prohibit the syscalls that don't get in each other's way from running > > concurrently but the adapter will not be unbound before all syscalls > > return. > > > > Finally: upon being notified about an unbind event for the i2c adapter, > > we take the lock for writing and set the adapter pointer in the character > > device's structure to NULL. This "numbs down" the device - it still exists > > but is no longer functional. Meanwhile every syscall callback checks that > > pointer after taking the lock but before executing any code that requires > > it. If it's NULL, we return an error to user-space. > > > > This way we can safely open an i2c device from user-space, unbind the > > device without triggering a deadlock and any subsequent system-call for > > the file descriptor associated with the removed adapter will gracefully > > fail. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > First of all, thank you for tackling this problem. It was long overdue > and I really appreciate that you took the initiative to get it solved. > > Here are some review comments already. I'd like to do some more testing. > So far, everything works nicely. Also, I'd like to invite more people to > have a look at this code. We really don't want to have a regression > here, so more eyes are very welcome. > > > > @@ -1713,25 +1715,7 @@ void i2c_del_adapter(struct i2c_adapter *adap) > > > > i2c_host_notify_irq_teardown(adap); > > > > - /* wait until all references to the device are gone > > - * > > - * FIXME: This is old code and should ideally be replaced by an > > - * alternative which results in decoupling the lifetime of the struct > > - * device from the i2c_adapter, like spi or netdev do. Any solution > > - * should be thoroughly tested with DEBUG_KOBJECT_RELEASE enabled! > > Have you done this? Debugging with DEBUG_KOBJECT_RELEASE enabled? > Yes, I ran a simple test script with this option enabled - nothing suspicious reported. > > - */ > > - init_completion(&adap->dev_released); > > device_unregister(&adap->dev); > > - wait_for_completion(&adap->dev_released); > > So, this is basically the key change. I think you handled the userspace > part via i2c-dev well. I don't have proof yet, but my gut feeling > wonders if this is complete. Aren't there maybe sysfs-references as > well. I need to check. > > > - > > - /* free bus id */ > > - mutex_lock(&core_lock); > > - idr_remove(&i2c_adapter_idr, adap->nr); > > - mutex_unlock(&core_lock); > > - > > - /* Clear the device structure in case this adapter is ever going to be > > - added again */ > > - memset(&adap->dev, 0, sizeof(adap->dev)); > > Any reason you didn't add this to release function above? Reading the > introducing commit, the old drivers needings this still exist IMO. > (Yes, they should be converted to use the i2c-mux subsystem, but I don't > think someone will do this) > Good catch, added it back. > > @@ -44,8 +45,14 @@ struct i2c_dev { > > struct i2c_adapter *adap; > > struct device dev; > > struct cdev cdev; > > + struct rw_semaphore sem; > > I'd like to name it 'rwsem' so it is always super-clear what it is. > > > }; > > > > +static inline struct i2c_dev *to_i2c_dev(struct inode *ino) > > I'd also prefer a more specific 'inode_to_i2c_dev' function name. > Personally, I'd also name the argument 'inode' but I'll leave that to > you. > > > +{ > > + return container_of(ino->i_cdev, struct i2c_dev, cdev); > > +} > > ... > > Doesn't the function 'name_show()' also need protection? It dereferences > i2c_dev->adap. > Another good catch. > So much for now, > > Wolfram > Thanks, will send a v2 shortly. Bart
On Wed, Jan 18, 2023 at 3:59 PM Hillf Danton <hdanton@sina.com> wrote: > > On Thu, 29 Dec 2022 17:00:45 +0100 Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > @@ -44,8 +45,14 @@ struct i2c_dev { > > struct i2c_adapter *adap; > > struct device dev; > > struct cdev cdev; > > + struct rw_semaphore sem; > > int state; > #define STATE_RELEASED 1 > #define STATE_DETACHED 2 > > }; > > Because syscalls like read, write and ioctl have completed upon release, > serializing the release and detach pathes is a cure to the issue. > Please elaborate because I have no idea what you mean by that. Bart
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 087e480b624c..9ce63d513f72 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1139,7 +1139,11 @@ EXPORT_SYMBOL_GPL(i2c_new_ancillary_device); static void i2c_adapter_dev_release(struct device *dev) { struct i2c_adapter *adap = to_i2c_adapter(dev); - complete(&adap->dev_released); + + /* free bus id */ + mutex_lock(&core_lock); + idr_remove(&i2c_adapter_idr, adap->nr); + mutex_unlock(&core_lock); } unsigned int i2c_adapter_depth(struct i2c_adapter *adapter) @@ -1512,9 +1516,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap) return 0; out_reg: - init_completion(&adap->dev_released); device_unregister(&adap->dev); - wait_for_completion(&adap->dev_released); out_list: mutex_lock(&core_lock); idr_remove(&i2c_adapter_idr, adap->nr); @@ -1713,25 +1715,7 @@ void i2c_del_adapter(struct i2c_adapter *adap) i2c_host_notify_irq_teardown(adap); - /* wait until all references to the device are gone - * - * FIXME: This is old code and should ideally be replaced by an - * alternative which results in decoupling the lifetime of the struct - * device from the i2c_adapter, like spi or netdev do. Any solution - * should be thoroughly tested with DEBUG_KOBJECT_RELEASE enabled! - */ - init_completion(&adap->dev_released); device_unregister(&adap->dev); - wait_for_completion(&adap->dev_released); - - /* free bus id */ - mutex_lock(&core_lock); - idr_remove(&i2c_adapter_idr, adap->nr); - mutex_unlock(&core_lock); - - /* Clear the device structure in case this adapter is ever going to be - added again */ - memset(&adap->dev, 0, sizeof(adap->dev)); } EXPORT_SYMBOL(i2c_del_adapter); diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c index 107623c4cc14..305b64a5d4d4 100644 --- a/drivers/i2c/i2c-dev.c +++ b/drivers/i2c/i2c-dev.c @@ -28,6 +28,7 @@ #include <linux/list.h> #include <linux/module.h> #include <linux/notifier.h> +#include <linux/rwsem.h> #include <linux/slab.h> #include <linux/uaccess.h> @@ -44,8 +45,14 @@ struct i2c_dev { struct i2c_adapter *adap; struct device dev; struct cdev cdev; + struct rw_semaphore sem; }; +static inline struct i2c_dev *to_i2c_dev(struct inode *ino) +{ + return container_of(ino->i_cdev, struct i2c_dev, cdev); +} + #define I2C_MINORS (MINORMASK + 1) static LIST_HEAD(i2c_dev_list); static DEFINE_SPINLOCK(i2c_dev_list_lock); @@ -136,15 +143,23 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count, { char *tmp; int ret; - + struct i2c_dev *i2c_dev = to_i2c_dev(file_inode(file)); struct i2c_client *client = file->private_data; if (count > 8192) count = 8192; + down_read(&i2c_dev->sem); + if (!i2c_dev->adap) { + up_read(&i2c_dev->sem); + return -ENODEV; + } + tmp = kzalloc(count, GFP_KERNEL); - if (tmp == NULL) + if (tmp == NULL) { + up_read(&i2c_dev->sem); return -ENOMEM; + } pr_debug("i2c-%d reading %zu bytes.\n", iminor(file_inode(file)), count); @@ -152,6 +167,7 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count, if (ret >= 0) if (copy_to_user(buf, tmp, ret)) ret = -EFAULT; + up_read(&i2c_dev->sem); kfree(tmp); return ret; } @@ -161,18 +177,28 @@ static ssize_t i2cdev_write(struct file *file, const char __user *buf, { int ret; char *tmp; + struct i2c_dev *i2c_dev = to_i2c_dev(file_inode(file)); struct i2c_client *client = file->private_data; if (count > 8192) count = 8192; + down_read(&i2c_dev->sem); + if (!i2c_dev->adap) { + up_read(&i2c_dev->sem); + return -ENODEV; + } + tmp = memdup_user(buf, count); - if (IS_ERR(tmp)) + if (IS_ERR(tmp)) { + up_read(&i2c_dev->sem); return PTR_ERR(tmp); + } pr_debug("i2c-%d writing %zu bytes.\n", iminor(file_inode(file)), count); ret = i2c_master_send(client, tmp, count); + up_read(&i2c_dev->sem); kfree(tmp); return ret; } @@ -389,7 +415,8 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client, return res; } -static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +static long i2cdev_ioctl_unlocked(struct file *file, unsigned int cmd, + unsigned long arg) { struct i2c_client *client = file->private_data; unsigned long funcs; @@ -495,6 +522,20 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg) return 0; } +static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +{ + struct i2c_dev *i2c_dev = to_i2c_dev(file_inode(file)); + long ret; + + down_read(&i2c_dev->sem); + if (!i2c_dev->adap) + ret = -ENODEV; + else + ret = i2cdev_ioctl_unlocked(file, cmd, arg); + up_read(&i2c_dev->sem); + + return ret; +} #ifdef CONFIG_COMPAT struct i2c_smbus_ioctl_data32 { @@ -516,10 +557,12 @@ struct i2c_rdwr_ioctl_data32 { u32 nmsgs; }; -static long compat_i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +static long compat_i2cdev_ioctl_unlocked(struct file *file, unsigned int cmd, + unsigned long arg) { struct i2c_client *client = file->private_data; unsigned long funcs; + switch (cmd) { case I2C_FUNCS: funcs = i2c_get_functionality(client->adapter); @@ -578,19 +621,39 @@ static long compat_i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned lo return i2cdev_ioctl(file, cmd, arg); } } + +static long compat_i2cdev_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct i2c_dev *i2c_dev = to_i2c_dev(file_inode(file)); + long ret; + + down_read(&i2c_dev->sem); + if (!i2c_dev->adap) + ret = -ENODEV; + else + ret = compat_i2cdev_ioctl_unlocked(file, cmd, arg); + up_read(&i2c_dev->sem); + + return ret; +} #else #define compat_i2cdev_ioctl NULL #endif static int i2cdev_open(struct inode *inode, struct file *file) { - unsigned int minor = iminor(inode); + struct i2c_dev *i2c_dev = to_i2c_dev(inode); struct i2c_client *client; struct i2c_adapter *adap; + int ret = 0; - adap = i2c_get_adapter(minor); - if (!adap) - return -ENODEV; + down_read(&i2c_dev->sem); + adap = i2c_dev->adap; + if (!adap) { + ret = -ENODEV; + goto out_unlock; + } /* This creates an anonymous i2c_client, which may later be * pointed to some address using I2C_SLAVE or I2C_SLAVE_FORCE. @@ -601,22 +664,23 @@ static int i2cdev_open(struct inode *inode, struct file *file) */ client = kzalloc(sizeof(*client), GFP_KERNEL); if (!client) { - i2c_put_adapter(adap); - return -ENOMEM; + ret = -ENOMEM; + goto out_unlock; } snprintf(client->name, I2C_NAME_SIZE, "i2c-dev %d", adap->nr); client->adapter = adap; file->private_data = client; - return 0; +out_unlock: + up_read(&i2c_dev->sem); + return ret; } static int i2cdev_release(struct inode *inode, struct file *file) { struct i2c_client *client = file->private_data; - i2c_put_adapter(client->adapter); kfree(client); file->private_data = NULL; @@ -669,6 +733,8 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy) i2c_dev->dev.parent = &adap->dev; i2c_dev->dev.release = i2cdev_dev_release; + init_rwsem(&i2c_dev->sem); + res = dev_set_name(&i2c_dev->dev, "i2c-%d", adap->nr); if (res) goto err_put_i2c_dev; @@ -698,6 +764,10 @@ static int i2cdev_detach_adapter(struct device *dev, void *dummy) if (!i2c_dev) /* attach_adapter must have failed */ return NOTIFY_DONE; + down_write(&i2c_dev->sem); + i2c_dev->adap = NULL; + up_write(&i2c_dev->sem); + put_i2c_dev(i2c_dev, true); pr_debug("adapter [%s] unregistered\n", adap->name); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index d84e0e99f084..3f31e4032044 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -14,7 +14,6 @@ #include <linux/bits.h> #include <linux/mod_devicetable.h> #include <linux/device.h> /* for struct device */ -#include <linux/sched.h> /* for completion */ #include <linux/mutex.h> #include <linux/regulator/consumer.h> #include <linux/rtmutex.h> @@ -739,7 +738,6 @@ struct i2c_adapter { int nr; char name[48]; - struct completion dev_released; struct mutex userspace_clients_lock; struct list_head userspace_clients;