Skip to contentSkip to conversation

i2c: dev: fix blocked adapter deregistration

PATCH In review Johan Hovold 3 participants 16 messages Aug 27 – Nov 10

Subsystems (3)
Stable branch I2C subsystem host drivers I2C
lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

v2 cover i2c: dev: fix blocked adapter deregistration

As part of addressing I2C lifetime issues, this series fixes an issue which may actually matter to some people, at least if they are playing around with USB attached I2C controllers, and whose fix does not depend on any larger rework.

This could possibly go into 7.3, but can also wait for 7.4 as the impact of the issue (both in terms of number of affected users and symptoms) should be limited.

Johan

Changes in v2:
- take rwsem also on open()
- clear adap pointer also if chardev registration fails

Johan Hovold (3):
i2c: dev: fix blocked adapter deregistration
i2c: dev: drop unnecessary sysfs device lookup
i2c: dev: clean up registration

 drivers/i2c/i2c-dev.c | 162 +++++++++++++++++++++++++++---------------
 1 file changed, 103 insertions(+), 59 deletions(-)
Signature · 1 line
2.54.0
author
#1 lore raw

PATCH v2 1/3 i2c: dev: fix blocked adapter deregistration

The I2C subsystem allows controllers to be used by non-child devices that may remain registered after an adapter goes away.

To handle this, adapter deregistration blocks until the last reference to the adapter is released. Albeit unorthodox, this is mostly fine for the vast majority of controllers but can cause some trouble when controllers reside on hotpluggable buses.

Specifically, userspace can prevent an adapter from being deregistered indefinitely by holding an i2c-dev character device file open. And with USB attached controllers this prevents further hub events from being processed by the parent hub until the file is closed.

Fix the i2c-dev implementation by dropping the additional reference taken at open() and using an rwsem to make sure the adapter is only accessed while registered.

Note that before commit 611e12ea0f12 ("i2c: core: manage i2c bus device refcount in i2c_[get|put]_adapter") an adapter going away would instead have resulted in a use-after-free.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-dev.c | 95 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 19 deletions(-)
drivers/i2c/i2c-dev.c +76 -19
index ccaac5e29f90..0940070c0317 100644
@@ -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>
 

@@ -41,11 +42,17 @@
  */
 struct i2c_dev {
 	struct list_head list;
+	struct rw_semaphore rwsem;
 	struct i2c_adapter *adap;
 	struct device dev;
 	struct cdev cdev;
 };
 
+struct i2c_dev_data {
+	struct i2c_dev *i2c_dev;
+	struct i2c_client client;
+};
+
 #define I2C_MINORS	(MINORMASK + 1)
 static LIST_HEAD(i2c_dev_list);
 static DEFINE_SPINLOCK(i2c_dev_list_lock);

@@ -92,6 +99,11 @@ static void put_i2c_dev(struct i2c_dev *i2c_dev, bool del_cdev)
 	spin_unlock(&i2c_dev_list_lock);
 	if (del_cdev)
 		cdev_device_del(&i2c_dev->cdev, &i2c_dev->dev);
+
+	scoped_guard(rwsem_write, &i2c_dev->rwsem) {
+		i2c_dev->adap = NULL;
+	}
+
 	put_device(&i2c_dev->dev);
 }
 

@@ -134,10 +146,16 @@ ATTRIBUTE_GROUPS(i2c);
 static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
 		loff_t *offset)
 {
+	struct i2c_dev_data *data = file->private_data;
+	struct i2c_client *client = &data->client;
+	struct i2c_dev *i2c_dev = data->i2c_dev;
 	char *tmp;
 	int ret;
 
-	struct i2c_client *client = file->private_data;
+	guard(rwsem_read)(&i2c_dev->rwsem);
+
+	if (!i2c_dev->adap)
+		return -ENODEV;
 
 	/* Adapter must support I2C transfers */
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))

@@ -163,9 +181,16 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
 static ssize_t i2cdev_write(struct file *file, const char __user *buf,
 		size_t count, loff_t *offset)
 {
+	struct i2c_dev_data *data = file->private_data;
+	struct i2c_client *client = &data->client;
+	struct i2c_dev *i2c_dev = data->i2c_dev;
 	int ret;
 	char *tmp;
-	struct i2c_client *client = file->private_data;
+
+	guard(rwsem_read)(&i2c_dev->rwsem);
+
+	if (!i2c_dev->adap)
+		return -ENODEV;
 
 	/* Adapter must support I2C transfers */
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))

@@ -397,11 +422,15 @@ 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_locked(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	struct i2c_client *client = file->private_data;
+	struct i2c_dev_data *data = file->private_data;
+	struct i2c_client *client = &data->client;
+	struct i2c_dev *i2c_dev = data->i2c_dev;
 	unsigned long funcs;
 
+	lockdep_assert_held(&i2c_dev->rwsem);
+
 	dev_dbg(&client->adapter->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n",
 		cmd, arg);
 

@@ -507,6 +536,19 @@ 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_data *data = file->private_data;
+	struct i2c_dev *i2c_dev = data->i2c_dev;
+
+	guard(rwsem_read)(&i2c_dev->rwsem);
+
+	if (!i2c_dev->adap)
+		return -ENODEV;
+
+	return i2cdev_ioctl_locked(file, cmd, arg);
+}
+
 #ifdef CONFIG_COMPAT
 
 struct i2c_smbus_ioctl_data32 {

@@ -530,8 +572,16 @@ struct i2c_rdwr_ioctl_data32 {
 
 static long compat_i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	struct i2c_client *client = file->private_data;
+	struct i2c_dev_data *data = file->private_data;
+	struct i2c_client *client = &data->client;
+	struct i2c_dev *i2c_dev = data->i2c_dev;
 	unsigned long funcs;
+
+	guard(rwsem_read)(&i2c_dev->rwsem);
+
+	if (!i2c_dev->adap)
+		return -ENODEV;
+
 	switch (cmd) {
 	case I2C_FUNCS:
 		funcs = i2c_get_functionality(client->adapter);

@@ -588,7 +638,7 @@ static long compat_i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned lo
 					  compat_ptr(data32.data));
 	}
 	default:
-		return i2cdev_ioctl(file, cmd, arg);
+		return i2cdev_ioctl_locked(file, cmd, arg);
 	}
 }
 #else

@@ -597,14 +647,18 @@ static long compat_i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned lo
 
 static int i2cdev_open(struct inode *inode, struct file *file)
 {
-	unsigned int minor = iminor(inode);
+	struct i2c_dev *i2c_dev = container_of(inode->i_cdev, struct i2c_dev, cdev);
+	struct i2c_dev_data *data;
 	struct i2c_client *client;
 	struct i2c_adapter *adap;
 
-	adap = i2c_get_adapter(minor);
-	if (!adap)
+	guard(rwsem_read)(&i2c_dev->rwsem);
+
+	if (!i2c_dev->adap)
 		return -ENODEV;
 
+	adap = i2c_dev->adap;
+
 	/* This creates an anonymous i2c_client, which may later be
 	 * pointed to some address using I2C_SLAVE or I2C_SLAVE_FORCE.
 	 *

@@ -612,26 +666,27 @@ static int i2cdev_open(struct inode *inode, struct file *file)
 	 * or I2C core code!!  It just holds private copies of addressing
 	 * information and maybe a PEC flag.
 	 */
-	client = kzalloc_obj(*client);
-	if (!client) {
-		i2c_put_adapter(adap);
+	data = kzalloc_obj(*data);
+	if (!data)
 		return -ENOMEM;
-	}
+
+	data->i2c_dev = i2c_dev;
+
+	client = &data->client;
+
 	snprintf(client->name, I2C_NAME_SIZE, "i2c-dev %d", adap->nr);
 
 	client->adapter = adap;
-	file->private_data = client;
+	file->private_data = data;
 
 	return 0;
 }
 
 static int i2cdev_release(struct inode *inode, struct file *file)
 {
-	struct i2c_client *client = file->private_data;
+	struct i2c_dev_data *data = file->private_data;
 
-	i2c_put_adapter(client->adapter);
-	kfree(client);
-	file->private_data = NULL;
+	kfree(data);
 
 	return 0;
 }

@@ -675,8 +730,10 @@ static int i2cdev_attach_adapter(struct device *dev)
 	if (IS_ERR(i2c_dev))
 		return NOTIFY_DONE;
 
+	init_rwsem(&i2c_dev->rwsem);
+
 	cdev_init(&i2c_dev->cdev, &i2cdev_fops);
-	i2c_dev->cdev.owner = THIS_MODULE;
+	i2c_dev->cdev.owner = adap->owner;
 
 	device_initialize(&i2c_dev->dev);
 	i2c_dev->dev.devt = MKDEV(I2C_MAJOR, adap->nr);
Signature · 1 line
2.54.0
#2 lore raw
Quoted 72 lines · Johan Hovold

The I2C subsystem allows controllers to be used by non-child devices that may remain registered after an adapter goes away.

To handle this, adapter deregistration blocks until the last reference to the adapter is released. Albeit unorthodox, this is mostly fine for the vast majority of controllers but can cause some trouble when controllers reside on hotpluggable buses.

Specifically, userspace can prevent an adapter from being deregistered indefinitely by holding an i2c-dev character device file open. And with USB attached controllers this prevents further hub events from being processed by the parent hub until the file is closed.

Fix the i2c-dev implementation by dropping the additional reference taken at open() and using an rwsem to make sure the adapter is only accessed while registered.

Note that before commit 611e12ea0f12 ("i2c: core: manage i2c bus device refcount in i2c_[get|put]_adapter") an adapter going away would instead have resulted in a use-after-free.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-dev.c | 95 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 19 deletions(-)
drivers/i2c/i2c-dev.c +12 -0
index ccaac5e29f90..0940070c0317 100644
@@ -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>
 

@@ -41,11 +42,17 @@
  */
 struct i2c_dev {
 	struct list_head list;
+	struct rw_semaphore rwsem;
 	struct i2c_adapter *adap;
 	struct device dev;
 	struct cdev cdev;
 };
 
+struct i2c_dev_data {
+	struct i2c_dev *i2c_dev;
+	struct i2c_client client;
+};
+
 #define I2C_MINORS	(MINORMASK + 1)
 static LIST_HEAD(i2c_dev_list);
 static DEFINE_SPINLOCK(i2c_dev_list_lock);

@@ -92,6 +99,11 @@ static void put_i2c_dev(struct i2c_dev *i2c_dev, bool del_cdev)
 	spin_unlock(&i2c_dev_list_lock);
 	if (del_cdev)
 		cdev_device_del(&i2c_dev->cdev, &i2c_dev->dev);
+
+	scoped_guard(rwsem_write, &i2c_dev->rwsem) {
+		i2c_dev->adap = NULL;
+	}
+
 	put_device(&i2c_dev->dev);
 }
 

@@ -134,10 +146,16 @@ ATTRIBUTE_GROUPS(i2c);
 static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,

drivers/i2c/i2c-dev.c ATTRIBUTE_GROUPS(i2c); · line 155

Quoted 10 lines
drivers/i2c/i2c-dev.c +4 -1
 		loff_t *offset)
 {
+	struct i2c_dev_data *data = file->private_data;
+	struct i2c_client *client = &data->client;
+	struct i2c_dev *i2c_dev = data->i2c_dev;
 	char *tmp;
 	int ret;
 
-	struct i2c_client *client = file->private_data;
+	guard(rwsem_read)(&i2c_dev->rwsem);

I think sashiko is correct in its comment and you should limit the critical section to until right after i2c_master_recv(). Same elsewhere.

Looks good otherwise.

Thanks,
Bartosz

[1] https://sashiko.dev/#/patchset/20260827102043.673273-1-johan%40kernel.org?part=1

author
#3 lore raw
Quoted 36 lines · Bartosz Golaszewski
Quoted 35 lines · Johan Hovold

The I2C subsystem allows controllers to be used by non-child devices that may remain registered after an adapter goes away.

To handle this, adapter deregistration blocks until the last reference to the adapter is released. Albeit unorthodox, this is mostly fine for the vast majority of controllers but can cause some trouble when controllers reside on hotpluggable buses.

Specifically, userspace can prevent an adapter from being deregistered indefinitely by holding an i2c-dev character device file open. And with USB attached controllers this prevents further hub events from being processed by the parent hub until the file is closed.

Fix the i2c-dev implementation by dropping the additional reference taken at open() and using an rwsem to make sure the adapter is only accessed while registered.

Note that before commit 611e12ea0f12 ("i2c: core: manage i2c bus device refcount in i2c_[get|put]_adapter") an adapter going away would instead have resulted in a use-after-free.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>

---

static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,

drivers/i2c/i2c-dev.c +3 -0
 		loff_t *offset)
 {
+	struct i2c_dev_data *data = file->private_data;
+	struct i2c_client *client = &data->client;
+	struct i2c_dev *i2c_dev = data->i2c_dev;
 	char *tmp;
 	int ret;
Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +1 -1
-	struct i2c_client *client = file->private_data;
+	guard(rwsem_read)(&i2c_dev->rwsem);

I think sashiko is correct in its comment and you should limit the critical section to until right after i2c_master_recv(). Same elsewhere.

I don't think that's anything we need to care about. If a malicious actor has physical access to the machine, we have bigger problems than worrying about them going out of their way to cause a USB hub worker thread to hang. :)

Johan

Quoted 1 line

[1] https://sashiko.dev/#/patchset/20260827102043.673273-1-johan%40kernel.org?part=1

author
#4 lore raw

PATCH v2 2/3 i2c: dev: drop unnecessary sysfs device lookup

In reply to Johan Hovold

The sysfs lifetime rules guarantees that the i2c-dev class device is still valid while its attribute callbacks are executing so drop the unnecessary reverse lookup.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-dev.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/i2c/i2c-dev.c +3 -5
index 0940070c0317..49c05f2eb223 100644
@@ -47,6 +47,7 @@ struct i2c_dev {
 	struct device dev;
 	struct cdev cdev;
 };
+#define to_i2c_dev(d) container_of((d), struct i2c_dev, dev)
 
 struct i2c_dev_data {
 	struct i2c_dev *i2c_dev;

@@ -110,10 +111,8 @@ static void put_i2c_dev(struct i2c_dev *i2c_dev, bool del_cdev)
 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));
+	struct i2c_dev *i2c_dev = to_i2c_dev(dev);
 
-	if (!i2c_dev)
-		return -ENODEV;
 	return sysfs_emit(buf, "%s\n", i2c_dev->adap->name);
 }
 static DEVICE_ATTR_RO(name);

@@ -710,9 +709,8 @@ static const struct class i2c_dev_class = {
 
 static void i2cdev_dev_release(struct device *dev)
 {
-	struct i2c_dev *i2c_dev;
+	struct i2c_dev *i2c_dev = to_i2c_dev(dev);
 
-	i2c_dev = container_of(dev, struct i2c_dev, dev);
 	kfree(i2c_dev);
 }
Signature · 1 line
2.54.0
author
#7 lore raw

PATCH v2 3/3 i2c: dev: clean up registration

In reply to Johan Hovold

Drop the get_free_i2c_dev() and put_i2c_dev() helpers and do all setup and teardown directly in i2cdev_attach_adapter() and i2cdev_detach_adapter() for consistency and to make the logic clearer.

Note that the device list is only used at detach so the i2c-dev can be added after registering the class device.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-dev.c | 69 ++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 40 deletions(-)
drivers/i2c/i2c-dev.c +29 -40
index 49c05f2eb223..9ad145f67fc1 100644
@@ -73,41 +73,6 @@ static struct i2c_dev *i2c_dev_get_by_minor(unsigned index)
 	return i2c_dev;
 }
 
-static struct i2c_dev *get_free_i2c_dev(struct i2c_adapter *adap)
-{
-	struct i2c_dev *i2c_dev;
-
-	if (adap->nr >= I2C_MINORS) {
-		pr_err("Out of device minors (%d)\n", adap->nr);
-		return ERR_PTR(-ENODEV);
-	}
-
-	i2c_dev = kzalloc_obj(*i2c_dev);
-	if (!i2c_dev)
-		return ERR_PTR(-ENOMEM);
-	i2c_dev->adap = adap;
-
-	spin_lock(&i2c_dev_list_lock);
-	list_add_tail(&i2c_dev->list, &i2c_dev_list);
-	spin_unlock(&i2c_dev_list_lock);
-	return i2c_dev;
-}
-
-static void put_i2c_dev(struct i2c_dev *i2c_dev, bool del_cdev)
-{
-	spin_lock(&i2c_dev_list_lock);
-	list_del(&i2c_dev->list);
-	spin_unlock(&i2c_dev_list_lock);
-	if (del_cdev)
-		cdev_device_del(&i2c_dev->cdev, &i2c_dev->dev);
-
-	scoped_guard(rwsem_write, &i2c_dev->rwsem) {
-		i2c_dev->adap = NULL;
-	}
-
-	put_device(&i2c_dev->dev);
-}
-
 static ssize_t name_show(struct device *dev,
 			 struct device_attribute *attr, char *buf)
 {

@@ -724,11 +689,17 @@ static int i2cdev_attach_adapter(struct device *dev)
 		return NOTIFY_DONE;
 	adap = to_i2c_adapter(dev);
 
-	i2c_dev = get_free_i2c_dev(adap);
-	if (IS_ERR(i2c_dev))
+	if (adap->nr >= I2C_MINORS) {
+		pr_err("Out of device minors (%d)\n", adap->nr);
+		return NOTIFY_DONE;
+	}
+
+	i2c_dev = kzalloc_obj(*i2c_dev);
+	if (!i2c_dev)
 		return NOTIFY_DONE;
 
 	init_rwsem(&i2c_dev->rwsem);
+	i2c_dev->adap = adap;
 
 	cdev_init(&i2c_dev->cdev, &i2cdev_fops);
 	i2c_dev->cdev.owner = adap->owner;

@@ -745,13 +716,21 @@ static int i2cdev_attach_adapter(struct device *dev)
 
 	res = cdev_device_add(&i2c_dev->cdev, &i2c_dev->dev);
 	if (res)
-		goto err_put_i2c_dev;
+		goto err_clear_adap;
+
+	spin_lock(&i2c_dev_list_lock);
+	list_add_tail(&i2c_dev->list, &i2c_dev_list);
+	spin_unlock(&i2c_dev_list_lock);
 
 	pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
 	return NOTIFY_OK;
 
+err_clear_adap:
+	scoped_guard(rwsem_write, &i2c_dev->rwsem) {
+		i2c_dev->adap = NULL;
+	}
 err_put_i2c_dev:
-	put_i2c_dev(i2c_dev, false);
+	put_device(&i2c_dev->dev);
 	return NOTIFY_DONE;
 }
 

@@ -768,7 +747,17 @@ static int i2cdev_detach_adapter(struct device *dev)
 	if (!i2c_dev) /* attach_adapter must have failed */
 		return NOTIFY_DONE;
 
-	put_i2c_dev(i2c_dev, true);
+	spin_lock(&i2c_dev_list_lock);
+	list_del(&i2c_dev->list);
+	spin_unlock(&i2c_dev_list_lock);
+
+	cdev_device_del(&i2c_dev->cdev, &i2c_dev->dev);
+
+	scoped_guard(rwsem_write, &i2c_dev->rwsem) {
+		i2c_dev->adap = NULL;
+	}
+
+	put_device(&i2c_dev->dev);
 
 	pr_debug("adapter [%s] unregistered\n", adap->name);
 	return NOTIFY_OK;
Signature · 1 line
2.54.0
Acked-by on v2 3/3
#8 lore raw

Re: [PATCH v2 3/3] i2c: dev: clean up registration

Quoted 9 lines · Johan Hovold

Drop the get_free_i2c_dev() and put_i2c_dev() helpers and do all setup and teardown directly in i2cdev_attach_adapter() and i2cdev_detach_adapter() for consistency and to make the logic clearer.

Note that the device list is only used at detach so the i2c-dev can be added after registering the class device.

Signed-off-by Johan Hovold <[email protected]>

---

I'm not convinced this results in better or cleaner code but I'll let Wolfram be the judge here.

FWIW:

Acked-by Bartosz Golaszewski <[email protected]>

Bartosz

#9 lore raw

Re: [PATCH v2 3/3] i2c: dev: clean up registration

In reply to Johan Hovold
Quoted 6 lines · Johan Hovold

Drop the get_free_i2c_dev() and put_i2c_dev() helpers and do all setup and teardown directly in i2cdev_attach_adapter() and i2cdev_detach_adapter() for consistency and to make the logic clearer.

Note that the device list is only used at detach so the i2c-dev can be added after registering the class device.

Can we have the last sentence as a comment in the code, too, please?

Quoted 1 line
Signed-off-by Johan Hovold <[email protected]>

Other than that, I think this patch is an improvement. The logic is more explicit. I didn't like 'put_i2c_dev' having a bool parameter, for example.

So, only patch 1 needs a bigger update, it seems. Rest is looking good to me.

author
#10 lore raw

Re: [PATCH v2 3/3] i2c: dev: clean up registration

Quoted 9 lines · Wolfram Sang
Quoted 6 lines · Johan Hovold

Drop the get_free_i2c_dev() and put_i2c_dev() helpers and do all setup and teardown directly in i2cdev_attach_adapter() and i2cdev_detach_adapter() for consistency and to make the logic clearer.

Note that the device list is only used at detach so the i2c-dev can be added after registering the class device.

Can we have the last sentence as a comment in the code, too, please?

Sure.

Quoted 8 lines
Quoted 1 line
Signed-off-by Johan Hovold <[email protected]>

Other than that, I think this patch is an improvement. The logic is more explicit. I didn't like 'put_i2c_dev' having a bool parameter, for example.

So, only patch 1 needs a bigger update, it seems. Rest is looking good to me.

I had rejected that Sashiko finding as being too outlandish for us to have to worry about. As I just in a reply to Bartosz:

If a malicious actor has physical access to the machine, we have bigger problems than worrying about them going out of their way to cause a USB hub worker thread to hang. :)

Limiting the critical section can possibly be done as a follow-up, but it may not even be worth the effort doing that.

Johan

#11 lore raw

Re: [PATCH v2 3/3] i2c: dev: clean up registration

Quoted 25 lines · Johan Hovold
Quoted 9 lines · Wolfram Sang
Quoted 6 lines · Johan Hovold

Drop the get_free_i2c_dev() and put_i2c_dev() helpers and do all setup and teardown directly in i2cdev_attach_adapter() and i2cdev_detach_adapter() for consistency and to make the logic clearer.

Note that the device list is only used at detach so the i2c-dev can be added after registering the class device.

Can we have the last sentence as a comment in the code, too, please?

Sure.

Quoted 8 lines
Quoted 1 line
Signed-off-by Johan Hovold <[email protected]>

Other than that, I think this patch is an improvement. The logic is more explicit. I didn't like 'put_i2c_dev' having a bool parameter, for example.

So, only patch 1 needs a bigger update, it seems. Rest is looking good to me.

I had rejected that Sashiko finding as being too outlandish for us to have to worry about. As I just in a reply to Bartosz:

Quoted 7 lines

If a malicious actor has physical access to the machine, we have bigger problems than worrying about them going out of their way to cause a USB hub worker thread to hang. :)

Limiting the critical section can possibly be done as a follow-up, but it may not even be worth the effort doing that.

Why is limiting the scope of the semaphore such a big deal though? It's not a big change, literally just use scoped_guard() instead. Why would we commit incorrect code if we can correct it in a v3 with not much effort? You're fixing a use-case where user-space can hang a kernel thread, why not do it for corner-cases too?

Bart

author
#12 lore raw

Re: [PATCH v2 3/3] i2c: dev: clean up registration

Quoted 13 lines · Bartosz Golaszewski
Quoted 10 lines · Johan Hovold

I had rejected that Sashiko finding as being too outlandish for us to have to worry about. As I just in a reply to Bartosz:

If a malicious actor has physical access to the machine, we have bigger problems than worrying about them going out of their way to cause a USB hub worker thread to hang. :)

Limiting the critical section can possibly be done as a follow-up, but it may not even be worth the effort doing that.

Quoted 5 lines

Why is limiting the scope of the semaphore such a big deal though? It's not a big change, literally just use scoped_guard() instead. Why would we commit incorrect code if we can correct it in a v3 with not much effort? You're fixing a use-case where user-space can hang a kernel thread, why not do it for corner-cases too?

A *malicious* actor with physical access (and CAP_SYS_PTRACE) can cause a thread to hang while janking out an USB attached I2C adapter out a of machine if they try really hard. Don't you see how silly that is?

I posted this patch months ago, but IIRC it will complicate things somewhat without any relevant benefit. Probably just a larger diff which is unnecessary for a patch marked for stable.

If Wolfram insists, I can revisit this, but I don't think it's needed.

Johan

#13 lore raw

Re: [PATCH v2 3/3] i2c: dev: clean up registration

Quoted 2 lines

If Wolfram insists, I can revisit this, but I don't think it's needed.

Well, I can see both sides. One the one hand, it is a bit of an academic excercise, on the other hand it is more "correct".

I favor the incremental approach. I won't force Johan to do something which he thinks is superfluous. But if someone interested sends such an incremental patch, I'd vote for inclusion. And with all the code analysis out there, I think such a patch will come eventually.

That being said, it means I could then apply this series to my for-andi branch and add the comment I requested here myself. But I will wait until tomorrow to wait for further comments, if any.

#14 lore raw
In reply to Johan Hovold
Quoted 8 lines · Johan Hovold

As part of addressing I2C lifetime issues, this series fixes an issue which may actually matter to some people, at least if they are playing around with USB attached I2C controllers, and whose fix does not depend on any larger rework.

This could possibly go into 7.3, but can also wait for 7.4 as the impact of the issue (both in terms of number of affected users and symptoms) should be limited.

Oh wow, I completely missed this for some reason :( Just discovered it while digging around in patchwork.

I will try to find some time soon, so we can have it at least in 7.5.

Adding Bart to CC, maybe he is interested.

#15 lore raw
Quoted 10 lines · Wolfram Sang
Quoted 8 lines · Johan Hovold

As part of addressing I2C lifetime issues, this series fixes an issue which may actually matter to some people, at least if they are playing around with USB attached I2C controllers, and whose fix does not depend on any larger rework.

This could possibly go into 7.3, but can also wait for 7.4 as the impact of the issue (both in terms of number of affected users and symptoms) should be limited.

Quoted 7 lines

Oh wow, I completely missed this for some reason :( Just discovered it while digging around in patchwork.

I will try to find some time soon, so we can have it at least in 7.5.

Adding Bart to CC, maybe he is interested.

Yeah, I didn't see it either. And yes, I'm obviously interested, this has been my pet-peeve for years. :)

I'm adding this to my TODO list for today.

Bart