| Message ID | 20230118134940.240102-1-brgl@bgdev.pl |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v3] i2c: dev: don't allow user-space to deadlock the kernel | expand |
On Wed, Jan 18, 2023 at 02:49:40PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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> > --- > v1 -> v2: > - keep the device release callback and use it to free the IDR number > - rebase on top of v6.2-rc1 > > v2 -> v3: > - make symbol names more descriptive > - protect the name_show() sysfs callback too > - zero the adapter's struct device on device release > - make sure the code works nicely with CONFIG_DEBUG_KOBJECT_RELEASE enabled So, this code handled all my stress-testing well so far. I'll try to think of some more ideas until this evening, but likely I will apply it later. Nonetheless, more review eyes are still welcome!
> So, this code handled all my stress-testing well so far. I'll try to > think of some more ideas until this evening, but likely I will apply it > later. Nonetheless, more review eyes are still welcome! Ah yes, I now recalled why I had the gut feeling that this solution is not complete. See this mail thread from 2015: https://lkml.iu.edu/hypermail/linux/kernel/1501.2/01700.html There are still drivers using i2c_del_adapter()+kfree(), so removing the completion could cause use-after-free there, or?
On Wed, Jan 25, 2023 at 11:12 PM Wolfram Sang <wsa@kernel.org> wrote: > > > > So, this code handled all my stress-testing well so far. I'll try to > > think of some more ideas until this evening, but likely I will apply it > > later. Nonetheless, more review eyes are still welcome! > > Ah yes, I now recalled why I had the gut feeling that this solution is > not complete. See this mail thread from 2015: > > https://lkml.iu.edu/hypermail/linux/kernel/1501.2/01700.html > > There are still drivers using i2c_del_adapter()+kfree(), so removing the > completion could cause use-after-free there, or? > Ugh, what a mess... I was mostly focused on the character device side of it but now I realized the true extent of the problem. It's not like there are just *some* odd drivers that delete the adapter struct at .remove() - it's literally all of them one way or another. It's all because the adapter struct really should be allocated by i2c_add_adapter() and bus drivers should only really provide some structure containing the adapter description for the subsystem the lifetime of which would not affect the adapter itself. This way the adapter (embedding struct device) would be freed by device type's .release() like we do over in the GPIO subsystem. Instead the adapter struct is allocated by drivers at .probe() meaning it will get dropped at .remove(). I wonder how many more subsystems do that. No wonder people blame devres for these user-space device node crashes... I don't have a good solution. I've been thinking about it for an hour and every solution requires sweeping changes across the entire subsystem. Or else we'd introduce a parallel solution that would do the right thing and wait in perpetuity until all drivers convert - like with i2e probe_new() which is after all much simpler. Anyway, that's all I've got. We probably need to drop this change and live with what we have now. Bart
Hi Bartosz, > > https://lkml.iu.edu/hypermail/linux/kernel/1501.2/01700.html > > > > There are still drivers using i2c_del_adapter()+kfree(), so removing the > > completion could cause use-after-free there, or? > > > > Ugh, what a mess... I was mostly focused on the character device side > of it but now I realized the true extent of the problem. Still, thanks for trying. Really! > It's all because the adapter struct really should be allocated by > i2c_add_adapter() and bus drivers should only really provide some > structure containing the adapter description for the subsystem the > lifetime of which would not affect the adapter itself. This way the > adapter (embedding struct device) would be freed by device type's > .release() like we do over in the GPIO subsystem. Instead the adapter > struct is allocated by drivers at .probe() meaning it will get dropped > at .remove(). Or, like SPI does, use controller_alloc() which initializes the parts needed by the core, returns to the driver which needs to setup the private data, and finally calls controller_register(). > I don't have a good solution. I've been thinking about it for an hour > and every solution requires sweeping changes across the entire > subsystem. Or else we'd introduce a parallel solution that would do > the right thing and wait in perpetuity until all drivers convert - > like with i2e probe_new() which is after all much simpler. Thank you for spending time on another solution. "Perpetuity" is a good word to put it :/ > Anyway, that's all I've got. We probably need to drop this change and > live with what we have now. I am curious to see if this finding will make it into your FOSDEM talk. Looking already forward to it! Happy hacking, Wolfram
On Sat, Jan 28, 2023 at 8:02 PM Wolfram Sang <wsa@kernel.org> wrote: > > Hi Bartosz, > > > > https://lkml.iu.edu/hypermail/linux/kernel/1501.2/01700.html > > > > > > There are still drivers using i2c_del_adapter()+kfree(), so removing the > > > completion could cause use-after-free there, or? > > > > > > > Ugh, what a mess... I was mostly focused on the character device side > > of it but now I realized the true extent of the problem. > > Still, thanks for trying. Really! > > > It's all because the adapter struct really should be allocated by > > i2c_add_adapter() and bus drivers should only really provide some > > structure containing the adapter description for the subsystem the > > lifetime of which would not affect the adapter itself. This way the > > adapter (embedding struct device) would be freed by device type's > > .release() like we do over in the GPIO subsystem. Instead the adapter > > struct is allocated by drivers at .probe() meaning it will get dropped > > at .remove(). > > Or, like SPI does, use controller_alloc() which initializes the parts > needed by the core, returns to the driver which needs to setup the > private data, and finally calls controller_register(). > If we could add a helper like struct *i2c_adapter_get_device(struct i2c_adapter *adap) and convert all users who access adap.dev directly to using it instead in a sweeping change across the subsystem - that would already be a huge improvement as this would allow us to later move struct device memory into the subsystem and free it in .release() and not allow provider drivers to free it at .remove(). Something to consider. Let me know if that's an interesting option. > > I don't have a good solution. I've been thinking about it for an hour > > and every solution requires sweeping changes across the entire > > subsystem. Or else we'd introduce a parallel solution that would do > > the right thing and wait in perpetuity until all drivers convert - > > like with i2e probe_new() which is after all much simpler. > > Thank you for spending time on another solution. "Perpetuity" is a good > word to put it :/ > > > Anyway, that's all I've got. We probably need to drop this change and > > live with what we have now. > > I am curious to see if this finding will make it into your FOSDEM talk. > Looking already forward to it! > Ha! Yeah, definitely. I went down that rabbit hole a while ago and am bothered by the amount of latent bugs I'm finding. I sent some patches for GPIO and SPI (and this one for I2C). See you there, I guess? > Happy hacking, > > Wolfram > Bart
Hello, On Wed, Jan 18, 2023 at 02:49:40PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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. Is this bad? > 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> @Bartosz, is that the patch you talked about on FOSDEM? I thought Wolfram had some concerns but I thought they were unaddressed still. What am I missing? > --- > v1 -> v2: > - keep the device release callback and use it to free the IDR number > - rebase on top of v6.2-rc1 > > v2 -> v3: > - make symbol names more descriptive > - protect the name_show() sysfs callback too > - zero the adapter's struct device on device release > - make sure the code works nicely with CONFIG_DEBUG_KOBJECT_RELEASE enabled > > drivers/i2c/i2c-core-base.c | 32 ++++------- > drivers/i2c/i2c-dev.c | 109 +++++++++++++++++++++++++++++++----- > include/linux/i2c.h | 2 - > 3 files changed, 106 insertions(+), 37 deletions(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 087e480b624c..ec8140d907c2 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -1139,7 +1139,17 @@ 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); > + > + /* > + * Clear the device structure in case this adapter is ever going to be > + * added again. > + */ > + memset(&adap->dev, 0, sizeof(adap->dev)); > } > > unsigned int i2c_adapter_depth(struct i2c_adapter *adapter) > @@ -1512,9 +1522,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 +1721,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..38c83ee408be 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 rwsem; I'd add a comment here that the semaphone protects access to adap. > }; > > +static inline struct i2c_dev *inode_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); > @@ -99,10 +106,21 @@ static ssize_t name_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct i2c_dev *i2c_dev = i2c_dev_get_by_minor(MINOR(dev->devt)); > + const char *name; > > if (!i2c_dev) > return -ENODEV; > - return sysfs_emit(buf, "%s\n", i2c_dev->adap->name); > + > + down_read(&i2c_dev->rwsem); > + if (!i2c_dev->adap) { > + up_read(&i2c_dev->rwsem); > + return -ENODEV; > + } > + > + name = i2c_dev->adap->name; > + up_read(&i2c_dev->rwsem); > + It might happen that if the adapter disappears here, name might point to an invalid location, too, doesn't it? I think you have to do the sysfs_emit under the lock. Would it defeat the patch's sense to keep a reference on the adapter? I would consider it natural to hold such a reference, but I admit I didn't claim to fully understand the device core and this patch's effects. Does it defeat the patch's purpose to hold a reference? I think it would simplify some things, e.g. name wouldn't disappear here. > + return sysfs_emit(buf, "%s\n", name); > } > static DEVICE_ATTR_RO(name); > > @@ -136,15 +154,23 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count, > { > char *tmp; > int ret; > - > + struct i2c_dev *i2c_dev = inode_to_i2c_dev(file_inode(file)); > struct i2c_client *client = file->private_data; > > if (count > 8192) > count = 8192; > > + down_read(&i2c_dev->rwsem); > + if (!i2c_dev->adap) { > + up_read(&i2c_dev->rwsem); > + return -ENODEV; > + } > + > tmp = kzalloc(count, GFP_KERNEL); > - if (tmp == NULL) > + if (tmp == NULL) { > + up_read(&i2c_dev->rwsem); > return -ENOMEM; > + } > > pr_debug("i2c-%d reading %zu bytes.\n", iminor(file_inode(file)), count); > > @@ -152,6 +178,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->rwsem); > kfree(tmp); > return ret; > } > @@ -161,18 +188,28 @@ static ssize_t i2cdev_write(struct file *file, const char __user *buf, > { > int ret; > char *tmp; > + struct i2c_dev *i2c_dev = inode_to_i2c_dev(file_inode(file)); > struct i2c_client *client = file->private_data; > > if (count > 8192) > count = 8192; > > + down_read(&i2c_dev->rwsem); > + if (!i2c_dev->adap) { > + up_read(&i2c_dev->rwsem); > + return -ENODEV; > + } > + > tmp = memdup_user(buf, count); > - if (IS_ERR(tmp)) > + if (IS_ERR(tmp)) { > + up_read(&i2c_dev->rwsem); > 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->rwsem); > kfree(tmp); > return ret; > } > @@ -389,7 +426,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) IMHO this name is a bit unfortunate because of file_operations::unlocked_ioctl. The unlocked in the callback name is something different to the meaning of unlocked here. > { > struct i2c_client *client = file->private_data; > unsigned long funcs; > @@ -495,6 +533,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 = inode_to_i2c_dev(file_inode(file)); > + long ret; > + > + down_read(&i2c_dev->rwsem); > + if (!i2c_dev->adap) > + ret = -ENODEV; > + else > + ret = i2cdev_ioctl_unlocked(file, cmd, arg); > + up_read(&i2c_dev->rwsem); > + > + return ret; > +} > #ifdef CONFIG_COMPAT > > struct i2c_smbus_ioctl_data32 { > @@ -516,10 +568,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 +632,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 = inode_to_i2c_dev(file_inode(file)); > + long ret; > + > + down_read(&i2c_dev->rwsem); > + if (!i2c_dev->adap) > + ret = -ENODEV; > + else > + ret = compat_i2cdev_ioctl_unlocked(file, cmd, arg); > + up_read(&i2c_dev->rwsem); > + > + 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 = inode_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->rwsem); > + 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 +675,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->rwsem); > + 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 +744,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->rwsem); > + > res = dev_set_name(&i2c_dev->dev, "i2c-%d", adap->nr); > if (res) > goto err_put_i2c_dev; > @@ -698,6 +775,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->rwsem); > + i2c_dev->adap = NULL; > + up_write(&i2c_dev->rwsem); > + > 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; > -- > 2.37.2 > >
On Mon, Feb 6, 2023 at 4:28 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > On Wed, Jan 18, 2023 at 02:49:40PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > 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. > > Is this bad? > > > 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> > > @Bartosz, is that the patch you talked about on FOSDEM? I thought > Wolfram had some concerns but I thought they were unaddressed still. > What am I missing? Hi Uwe, yes, this patch was dropped, I can see the rest of the discussion on the list, have you got the rest of the email too? Bart
Hello, ah, this is the mail I missed before. On Wed, Jan 25, 2023 at 11:11:59PM +0100, Wolfram Sang wrote: > > > So, this code handled all my stress-testing well so far. I'll try to > > think of some more ideas until this evening, but likely I will apply it > > later. Nonetheless, more review eyes are still welcome! > > Ah yes, I now recalled why I had the gut feeling that this solution is > not complete. See this mail thread from 2015: > > https://lkml.iu.edu/hypermail/linux/kernel/1501.2/01700.html > > There are still drivers using i2c_del_adapter()+kfree(), so removing the > completion could cause use-after-free there, or? There is also a strange construct in spi that I understand at one point in time, but I failed to swap it in quickly. It's about commit 794aaf01444d4e765e2b067cba01cc69c1c68ed9. I think there should be a nicer solution than to track if the controller was allocated using devm, but I don't remember the details. But before addressing the i2c problem it might be worth to invest some time into that spi issue to not make the same mistake for i2c. Best regards Uwe
On Mon, Feb 6, 2023 at 4:51 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > ah, this is the mail I missed before. > > On Wed, Jan 25, 2023 at 11:11:59PM +0100, Wolfram Sang wrote: > > > > > So, this code handled all my stress-testing well so far. I'll try to > > > think of some more ideas until this evening, but likely I will apply it > > > later. Nonetheless, more review eyes are still welcome! > > > > Ah yes, I now recalled why I had the gut feeling that this solution is > > not complete. See this mail thread from 2015: > > > > https://lkml.iu.edu/hypermail/linux/kernel/1501.2/01700.html > > > > There are still drivers using i2c_del_adapter()+kfree(), so removing the > > completion could cause use-after-free there, or? > > There is also a strange construct in spi that I understand at one point > in time, but I failed to swap it in quickly. It's about commit > 794aaf01444d4e765e2b067cba01cc69c1c68ed9. I think there should be a > nicer solution than to track if the controller was allocated using devm, > but I don't remember the details. But before addressing the i2c problem > it might be worth to invest some time into that spi issue to not make > the same mistake for i2c. > Yeah, I've seen these constructs before elsewhere... Sadly, we have workarounds upon workarounds within workarounds chased by other workarounds due to this issue. Bart
Hello Bartosz, On Mon, Feb 06, 2023 at 07:01:11PM +0100, Bartosz Golaszewski wrote: > On Mon, Feb 6, 2023 at 4:51 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > Hello, > > > > ah, this is the mail I missed before. > > > > On Wed, Jan 25, 2023 at 11:11:59PM +0100, Wolfram Sang wrote: > > > > > > > So, this code handled all my stress-testing well so far. I'll try to > > > > think of some more ideas until this evening, but likely I will apply it > > > > later. Nonetheless, more review eyes are still welcome! > > > > > > Ah yes, I now recalled why I had the gut feeling that this solution is > > > not complete. See this mail thread from 2015: > > > > > > https://lkml.iu.edu/hypermail/linux/kernel/1501.2/01700.html > > > > > > There are still drivers using i2c_del_adapter()+kfree(), so removing the > > > completion could cause use-after-free there, or? > > > > There is also a strange construct in spi that I understand at one point > > in time, but I failed to swap it in quickly. It's about commit > > 794aaf01444d4e765e2b067cba01cc69c1c68ed9. I think there should be a > > nicer solution than to track if the controller was allocated using devm, > > but I don't remember the details. But before addressing the i2c problem > > it might be worth to invest some time into that spi issue to not make > > the same mistake for i2c. > > Yeah, I've seen these constructs before elsewhere... Sadly, we have > workarounds upon workarounds within workarounds chased by other > workarounds due to this issue. Another pointer: Some time ago I did to the counter framework what would be needed here for i2c. See https://lore.kernel.org/all/20211230150300.72196-1-u.kleine-koenig@pengutronix.de/. This was applied in commit f2ee4759fb700b32a1bd830960fe86bf6bdfd0ab and its parents. Maybe it can serve as a template for i2c. Best regards Uwe
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 087e480b624c..ec8140d907c2 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1139,7 +1139,17 @@ 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); + + /* + * Clear the device structure in case this adapter is ever going to be + * added again. + */ + memset(&adap->dev, 0, sizeof(adap->dev)); } unsigned int i2c_adapter_depth(struct i2c_adapter *adapter) @@ -1512,9 +1522,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 +1721,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..38c83ee408be 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 rwsem; }; +static inline struct i2c_dev *inode_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); @@ -99,10 +106,21 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr, char *buf) { struct i2c_dev *i2c_dev = i2c_dev_get_by_minor(MINOR(dev->devt)); + const char *name; if (!i2c_dev) return -ENODEV; - return sysfs_emit(buf, "%s\n", i2c_dev->adap->name); + + down_read(&i2c_dev->rwsem); + if (!i2c_dev->adap) { + up_read(&i2c_dev->rwsem); + return -ENODEV; + } + + name = i2c_dev->adap->name; + up_read(&i2c_dev->rwsem); + + return sysfs_emit(buf, "%s\n", name); } static DEVICE_ATTR_RO(name); @@ -136,15 +154,23 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count, { char *tmp; int ret; - + struct i2c_dev *i2c_dev = inode_to_i2c_dev(file_inode(file)); struct i2c_client *client = file->private_data; if (count > 8192) count = 8192; + down_read(&i2c_dev->rwsem); + if (!i2c_dev->adap) { + up_read(&i2c_dev->rwsem); + return -ENODEV; + } + tmp = kzalloc(count, GFP_KERNEL); - if (tmp == NULL) + if (tmp == NULL) { + up_read(&i2c_dev->rwsem); return -ENOMEM; + } pr_debug("i2c-%d reading %zu bytes.\n", iminor(file_inode(file)), count); @@ -152,6 +178,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->rwsem); kfree(tmp); return ret; } @@ -161,18 +188,28 @@ static ssize_t i2cdev_write(struct file *file, const char __user *buf, { int ret; char *tmp; + struct i2c_dev *i2c_dev = inode_to_i2c_dev(file_inode(file)); struct i2c_client *client = file->private_data; if (count > 8192) count = 8192; + down_read(&i2c_dev->rwsem); + if (!i2c_dev->adap) { + up_read(&i2c_dev->rwsem); + return -ENODEV; + } + tmp = memdup_user(buf, count); - if (IS_ERR(tmp)) + if (IS_ERR(tmp)) { + up_read(&i2c_dev->rwsem); 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->rwsem); kfree(tmp); return ret; } @@ -389,7 +426,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 +533,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 = inode_to_i2c_dev(file_inode(file)); + long ret; + + down_read(&i2c_dev->rwsem); + if (!i2c_dev->adap) + ret = -ENODEV; + else + ret = i2cdev_ioctl_unlocked(file, cmd, arg); + up_read(&i2c_dev->rwsem); + + return ret; +} #ifdef CONFIG_COMPAT struct i2c_smbus_ioctl_data32 { @@ -516,10 +568,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 +632,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 = inode_to_i2c_dev(file_inode(file)); + long ret; + + down_read(&i2c_dev->rwsem); + if (!i2c_dev->adap) + ret = -ENODEV; + else + ret = compat_i2cdev_ioctl_unlocked(file, cmd, arg); + up_read(&i2c_dev->rwsem); + + 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 = inode_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->rwsem); + 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 +675,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->rwsem); + 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 +744,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->rwsem); + res = dev_set_name(&i2c_dev->dev, "i2c-%d", adap->nr); if (res) goto err_put_i2c_dev; @@ -698,6 +775,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->rwsem); + i2c_dev->adap = NULL; + up_write(&i2c_dev->rwsem); + 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;