Hello,
On Wed, Jan 18, 2023 at 02:49:40PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
@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
>
>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
