| Message ID | 20221229160045.535778-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | i2c: fortify the subsystem against user-space induced deadlocks | expand |
On Thu, Dec 29, 2022 at 5:00 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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 > Hi Wolfram, It's been two weeks without any comments on this series and over a month since v1 so let me send a gentle ping on this. Bart
Hi Bart, > It's been two weeks without any comments on this series and over a > month since v1 so let me send a gentle ping on this. This series has a priority for the next merge window. I love it! However, patch 2 is difficult to review because I need to dive in deeply into the topic. Hopes are that I can do this next week. Thanks and happy hacking, Wolfram
On Thu, Dec 29, 2022 at 05:00:44PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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 <bartosz.golaszewski@linaro.org> Applied to for-next, thanks! I start reviewing patch 2 now...
Hi Bartosz, On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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 <bartosz.golaszewski@linaro.org> 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
On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Bartosz, > > On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > 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 <bartosz.golaszewski@linaro.org> > > 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
Hi Bartosz, On Wed, Mar 8, 2023 at 8:33 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > 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 <bartosz.golaszewski@linaro.org> > > > > 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
Hi Bartosz, On Wed, Mar 8, 2023 at 8:51 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Mar 8, 2023 at 8:33 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > 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 <bartosz.golaszewski@linaro.org> > > > > > > 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
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> 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(-)