LinuxLists.cc - [PATCH v2 0/2] i2c: fortify the subsystem against user-space induced deadlocks

2022-12-29 16:12:39

Subject: [PATCH v2 0/2] i2c: fortify the subsystem against user-space induced deadlocks

From: Bartosz Golaszewski <[email protected]>

Several subsystems in the kernel that export device files to user-space
suffer from a bug where keeping an open file descriptor associated with
this device file, unbinding the device from its driver and then calling
any of the supported system calls on that file descriptor will result in
either a crash or - as is the case with i2c - a deadlock.

This behavior has been blamed on extensive usage of device resource
management interfaces but it seems that devres has nothing to do with it,
the problem would be the same whether using devres or freeing resources
in .remove() that should survive the driver detach.

Many subsystems already deal with this by implementing some kind of flags
in the character device data together with locking preventing the
user-space from dropping the subsystem data from under the open device.

In i2c the deadlock comes from the fact that the function unregistering
the adapter waits for a completion which will not be passed until all
references to the character device are dropped.

The first patch in this series is just a tweak of return values of the
notifier callback. The second addresses the deadlock problem in a way
similar to how we fixed this issue in the GPIO subystem. Details are in
the commit message.

v1 -> v2:
- keep the device release callback and use it to free the IDR number
- rebase on top of v6.2-rc1

Bartosz Golaszewski (2):
i2c: dev: fix notifier return values
i2c: dev: don't allow user-space to deadlock the kernel

drivers/i2c/i2c-core-base.c | 26 ++-------
drivers/i2c/i2c-dev.c | 112 +++++++++++++++++++++++++++++-------
include/linux/i2c.h | 2 -
3 files changed, 96 insertions(+), 44 deletions(-)

--
2.37.2

2022-12-29 16:12:42

by Bartosz Golaszewski

[permalink] [raw]

Subject: [PATCH v2 1/2] i2c: dev: fix notifier return values

From: Bartosz Golaszewski <[email protected]>

We have a set of return values that notifier callbacks can return. They
should not return 0, error codes or anything other than those predefined
values. Make the i2c character device's callback return NOTIFY_DONE or
NOTIFY_OK depending on the situation.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/i2c/i2c-dev.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index ab0adaa130da..107623c4cc14 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -653,12 +653,12 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
int res;

if (dev->type != &i2c_adapter_type)
- return 0;
+ return NOTIFY_DONE;
adap = to_i2c_adapter(dev);

i2c_dev = get_free_i2c_dev(adap);
if (IS_ERR(i2c_dev))
- return PTR_ERR(i2c_dev);
+ return NOTIFY_DONE;

cdev_init(&i2c_dev->cdev, &i2cdev_fops);
i2c_dev->cdev.owner = THIS_MODULE;
@@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
goto err_put_i2c_dev;

pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
- return 0;
+ return NOTIFY_OK;

err_put_i2c_dev:
put_i2c_dev(i2c_dev, false);
- return res;
+ return NOTIFY_DONE;
}

static int i2cdev_detach_adapter(struct device *dev, void *dummy)
@@ -691,17 +691,17 @@ static int i2cdev_detach_adapter(struct device *dev, void *dummy)
struct i2c_dev *i2c_dev;

if (dev->type != &i2c_adapter_type)
- return 0;
+ return NOTIFY_DONE;
adap = to_i2c_adapter(dev);

i2c_dev = i2c_dev_get_by_minor(adap->nr);
if (!i2c_dev) /* attach_adapter must have failed */
- return 0;
+ return NOTIFY_DONE;

put_i2c_dev(i2c_dev, true);

pr_debug("adapter [%s] unregistered\n", adap->name);
- return 0;
+ return NOTIFY_OK;
}

static int i2cdev_notifier_call(struct notifier_block *nb, unsigned long action,
@@ -716,7 +716,7 @@ static int i2cdev_notifier_call(struct notifier_block *nb, unsigned long action,
return i2cdev_detach_adapter(dev, NULL);
}

- return 0;
+ return NOTIFY_DONE;
}

static struct notifier_block i2cdev_notifier = {
--
2.37.2

2022-12-29 16:12:49

by Bartosz Golaszewski

[permalink] [raw]

Subject: [PATCH v2 2/2] i2c: dev: don't allow user-space to deadlock the kernel

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>
---
drivers/i2c/i2c-core-base.c | 26 ++--------
drivers/i2c/i2c-dev.c | 96 ++++++++++++++++++++++++++++++++-----
include/linux/i2c.h | 2 -
3 files changed, 88 insertions(+), 36 deletions(-)

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;
--
2.37.2

2023-01-12 09:21:32

On Tue, Jan 17, 2023 at 7:57 PM Wolfram Sang <[email protected]> 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 <[email protected]>
>
> 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

2023-01-18 15:24:37

by Bartosz Golaszewski

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] i2c: dev: don't allow user-space to deadlock the kernel

On Wed, Jan 18, 2023 at 3:59 PM Hillf Danton <[email protected]> wrote:
>
> On Thu, 29 Dec 2022 17:00:45 +0100 Bartosz Golaszewski <[email protected]>
> >
> > @@ -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

2023-03-08 17:01:42

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] i2c: dev: fix notifier return values

Hi Bartosz,

On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <[email protected]> wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> We have a set of return values that notifier callbacks can return. They
> should not return 0, error codes or anything other than those predefined
> values. Make the i2c character device's callback return NOTIFY_DONE or
> NOTIFY_OK depending on the situation.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>

Thanks for your patch, which is now commit cddf70d0bce71c2a ("i2c:
dev: fix notifier return values") in v6.3-rc1.

On SH/R-Mobile platforms, this leads to missing /dev/i2c-* entries.
On R-Car Gen4, they are still present, as all I2C adapters are
initialized after i2cdev.

> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -653,12 +653,12 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> int res;
>
> if (dev->type != &i2c_adapter_type)
> - return 0;
> + return NOTIFY_DONE;
> adap = to_i2c_adapter(dev);
>
> i2c_dev = get_free_i2c_dev(adap);
> if (IS_ERR(i2c_dev))
> - return PTR_ERR(i2c_dev);
> + return NOTIFY_DONE;
>
> cdev_init(&i2c_dev->cdev, &i2cdev_fops);
> i2c_dev->cdev.owner = THIS_MODULE;
> @@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> goto err_put_i2c_dev;
>
> pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
> - return 0;
> + return NOTIFY_OK;

Unfortunately i2cdev_{at,de}tach_adapter() are not only used as
notifiers (called from i2cdev_notifier_call()), but also called from
i2c_dev_init():

/* Bind to already existing adapters right away */
i2c_for_each_dev(NULL, i2cdev_attach_adapter);

and i2c_dev_exit():

i2c_for_each_dev(NULL, i2cdev_detach_adapter);

As soon i2c_dev_{at,de}tach_adapter() returns a non-zero
value (e.g. NOTIFY_OK), {i2c,bus}_for_each_dev() aborts
processing.

In i2c_dev_init(), this leads to a failure in registering any
already existing i2c adapters after the first one, causing missing
/dev/i2c-* entries.

In i2c_dev_exit(), this leads to a failure unregistering any but the
first i2c adapter.

As there is no one-to-one mapping from error codes to notify codes,
I think this cannot just be handled inside i2cdev_notifier_call() :-(

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-03-08 19:33:57

by Bartosz Golaszewski

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] i2c: dev: fix notifier return values

On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Bartosz,
>
> On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <[email protected]> wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > We have a set of return values that notifier callbacks can return. They
> > should not return 0, error codes or anything other than those predefined
> > values. Make the i2c character device's callback return NOTIFY_DONE or
> > NOTIFY_OK depending on the situation.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> Thanks for your patch, which is now commit cddf70d0bce71c2a ("i2c:
> dev: fix notifier return values") in v6.3-rc1.
>
> On SH/R-Mobile platforms, this leads to missing /dev/i2c-* entries.
> On R-Car Gen4, they are still present, as all I2C adapters are
> initialized after i2cdev.
>
> > --- a/drivers/i2c/i2c-dev.c
> > +++ b/drivers/i2c/i2c-dev.c
> > @@ -653,12 +653,12 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > int res;
> >
> > if (dev->type != &i2c_adapter_type)
> > - return 0;
> > + return NOTIFY_DONE;
> > adap = to_i2c_adapter(dev);
> >
> > i2c_dev = get_free_i2c_dev(adap);
> > if (IS_ERR(i2c_dev))
> > - return PTR_ERR(i2c_dev);
> > + return NOTIFY_DONE;
> >
> > cdev_init(&i2c_dev->cdev, &i2cdev_fops);
> > i2c_dev->cdev.owner = THIS_MODULE;
> > @@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > goto err_put_i2c_dev;
> >
> > pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
> > - return 0;
> > + return NOTIFY_OK;
>
> Unfortunately i2cdev_{at,de}tach_adapter() are not only used as
> notifiers (called from i2cdev_notifier_call()), but also called from
> i2c_dev_init():
>
> /* Bind to already existing adapters right away */
> i2c_for_each_dev(NULL, i2cdev_attach_adapter);
>
> and i2c_dev_exit():
>
> i2c_for_each_dev(NULL, i2cdev_detach_adapter);
>
> As soon i2c_dev_{at,de}tach_adapter() returns a non-zero
> value (e.g. NOTIFY_OK), {i2c,bus}_for_each_dev() aborts
> processing.
>
> In i2c_dev_init(), this leads to a failure in registering any
> already existing i2c adapters after the first one, causing missing
> /dev/i2c-* entries.
>
> In i2c_dev_exit(), this leads to a failure unregistering any but the
> first i2c adapter.
>
> As there is no one-to-one mapping from error codes to notify codes,
> I think this cannot just be handled inside i2cdev_notifier_call() :-(
>

Would wrapping i2c_a/detach_adapter() in a notifier callback work? So
that SH can call it directly while notifiers would call it indirectly
through the wrapper?

Bart

2023-03-08 19:52:16

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] i2c: dev: fix notifier return values

Hi Bartosz,

On Wed, Mar 8, 2023 at 8:33 PM Bartosz Golaszewski <[email protected]> wrote:
> On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <[email protected]> wrote:
> > > From: Bartosz Golaszewski <[email protected]>
> > >
> > > We have a set of return values that notifier callbacks can return. They
> > > should not return 0, error codes or anything other than those predefined
> > > values. Make the i2c character device's callback return NOTIFY_DONE or
> > > NOTIFY_OK depending on the situation.
> > >
> > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> >
> > Thanks for your patch, which is now commit cddf70d0bce71c2a ("i2c:
> > dev: fix notifier return values") in v6.3-rc1.
> >
> > On SH/R-Mobile platforms, this leads to missing /dev/i2c-* entries.
> > On R-Car Gen4, they are still present, as all I2C adapters are
> > initialized after i2cdev.
> >
> > > --- a/drivers/i2c/i2c-dev.c
> > > +++ b/drivers/i2c/i2c-dev.c
> > > @@ -653,12 +653,12 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > > int res;
> > >
> > > if (dev->type != &i2c_adapter_type)
> > > - return 0;
> > > + return NOTIFY_DONE;
> > > adap = to_i2c_adapter(dev);
> > >
> > > i2c_dev = get_free_i2c_dev(adap);
> > > if (IS_ERR(i2c_dev))
> > > - return PTR_ERR(i2c_dev);
> > > + return NOTIFY_DONE;
> > >
> > > cdev_init(&i2c_dev->cdev, &i2cdev_fops);
> > > i2c_dev->cdev.owner = THIS_MODULE;
> > > @@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > > goto err_put_i2c_dev;
> > >
> > > pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
> > > - return 0;
> > > + return NOTIFY_OK;
> >
> > Unfortunately i2cdev_{at,de}tach_adapter() are not only used as
> > notifiers (called from i2cdev_notifier_call()), but also called from
> > i2c_dev_init():
> >
> > /* Bind to already existing adapters right away */
> > i2c_for_each_dev(NULL, i2cdev_attach_adapter);
> >
> > and i2c_dev_exit():
> >
> > i2c_for_each_dev(NULL, i2cdev_detach_adapter);
> >
> > As soon i2c_dev_{at,de}tach_adapter() returns a non-zero
> > value (e.g. NOTIFY_OK), {i2c,bus}_for_each_dev() aborts
> > processing.
> >
> > In i2c_dev_init(), this leads to a failure in registering any
> > already existing i2c adapters after the first one, causing missing
> > /dev/i2c-* entries.
> >
> > In i2c_dev_exit(), this leads to a failure unregistering any but the
> > first i2c adapter.
> >
> > As there is no one-to-one mapping from error codes to notify codes,
> > I think this cannot just be handled inside i2cdev_notifier_call() :-(
>
> Would wrapping i2c_a/detach_adapter() in a notifier callback work? So
> that SH can call it directly while notifiers would call it indirectly
> through the wrapper?

That would be a wrapper that ignores the NOTIFY_* return
value, and always returns zero? I.e. we can no longer return an
error. I guess that's OK, as i2c_dev_init() doesn't take any
action based on the returned error code anyway.

The only error conditions that can happen in i2c_attach_adapter()
are:
- "Out of device minors" message in get_free_i2c_dev(),
- WARN_ON(dev == WHITEOUT_DEV) in cdev_add(),
- Generic -ENOMEM.
Looks like all of the above can be ignored, as they are all unlikely to
happen, and there is nothing to be done to recover...

Note that this is not "called directly from SH".
The SH/R-Mobile SoCs where I noticed the issue are ARM32.
I guess it can happen on other platforms, too, depending on initialization
order...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-03-09 07:51:04

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] i2c: dev: fix notifier return values

Hi Bartosz,

On Wed, Mar 8, 2023 at 8:51 PM Geert Uytterhoeven <[email protected]> wrote:
> On Wed, Mar 8, 2023 at 8:33 PM Bartosz Golaszewski <[email protected]> wrote:
> > On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <[email protected]> wrote:
> > > On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <[email protected]> wrote:
> > > > From: Bartosz Golaszewski <[email protected]>
> > > >
> > > > We have a set of return values that notifier callbacks can return. They
> > > > should not return 0, error codes or anything other than those predefined
> > > > values. Make the i2c character device's callback return NOTIFY_DONE or
> > > > NOTIFY_OK depending on the situation.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > >
> > > Thanks for your patch, which is now commit cddf70d0bce71c2a ("i2c:
> > > dev: fix notifier return values") in v6.3-rc1.
> > >
> > > On SH/R-Mobile platforms, this leads to missing /dev/i2c-* entries.
> > > On R-Car Gen4, they are still present, as all I2C adapters are
> > > initialized after i2cdev.
> > >
> > > > --- a/drivers/i2c/i2c-dev.c
> > > > +++ b/drivers/i2c/i2c-dev.c
> > > > @@ -653,12 +653,12 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > > > int res;
> > > >
> > > > if (dev->type != &i2c_adapter_type)
> > > > - return 0;
> > > > + return NOTIFY_DONE;
> > > > adap = to_i2c_adapter(dev);
> > > >
> > > > i2c_dev = get_free_i2c_dev(adap);
> > > > if (IS_ERR(i2c_dev))
> > > > - return PTR_ERR(i2c_dev);
> > > > + return NOTIFY_DONE;
> > > >
> > > > cdev_init(&i2c_dev->cdev, &i2cdev_fops);
> > > > i2c_dev->cdev.owner = THIS_MODULE;
> > > > @@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > > > goto err_put_i2c_dev;
> > > >
> > > > pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
> > > > - return 0;
> > > > + return NOTIFY_OK;
> > >
> > > Unfortunately i2cdev_{at,de}tach_adapter() are not only used as
> > > notifiers (called from i2cdev_notifier_call()), but also called from
> > > i2c_dev_init():
> > >
> > > /* Bind to already existing adapters right away */
> > > i2c_for_each_dev(NULL, i2cdev_attach_adapter);
> > >
> > > and i2c_dev_exit():
> > >
> > > i2c_for_each_dev(NULL, i2cdev_detach_adapter);
> > >
> > > As soon i2c_dev_{at,de}tach_adapter() returns a non-zero
> > > value (e.g. NOTIFY_OK), {i2c,bus}_for_each_dev() aborts
> > > processing.
> > >
> > > In i2c_dev_init(), this leads to a failure in registering any
> > > already existing i2c adapters after the first one, causing missing
> > > /dev/i2c-* entries.
> > >
> > > In i2c_dev_exit(), this leads to a failure unregistering any but the
> > > first i2c adapter.
> > >
> > > As there is no one-to-one mapping from error codes to notify codes,
> > > I think this cannot just be handled inside i2cdev_notifier_call() :-(
> >
> > Would wrapping i2c_a/detach_adapter() in a notifier callback work? So
> > that SH can call it directly while notifiers would call it indirectly
> > through the wrapper?
>
> That would be a wrapper that ignores the NOTIFY_* return
> value, and always returns zero? I.e. we can no longer return an
> error. I guess that's OK, as i2c_dev_init() doesn't take any
> action based on the returned error code anyway.

This works, so I've sent a fix
https://lore.kernel.org/r/03a8cd13af352c4d990bc70b72df4915b9fa2874.1678347776.git.geert+renesas@glider.be

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds