lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 15 Nov 2022 13:47:56 +0100 From: Nuno Sá <noname.nuno@...il.com> To: Jonathan Cameron <jic23@...nel.org>, "Sa, Nuno" <Nuno.Sa@...log.com> Cc: "Bolboaca, Ramona" <Ramona.Bolboaca@...log.com>, "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v2 0/8] Remove adis_initial_startup usage On Sat, 2022-11-05 at 15:06 +0000, Jonathan Cameron wrote: > On Thu, 3 Nov 2022 12:35:31 +0000 > "Sa, Nuno" <Nuno.Sa@...log.com> wrote: > > > > From: Ramona Bolboaca <ramona.bolboaca@...log.com> > > > Sent: Thursday, November 3, 2022 9:09 AM > > > To: jic23@...nel.org; linux-iio@...r.kernel.org; linux- > > > kernel@...r.kernel.org > > > Cc: Bolboaca, Ramona <Ramona.Bolboaca@...log.com> > > > Subject: [PATCH v2 0/8] Remove adis_initial_startup usage > > > > > > > > > Remove 'adis_initial_startup()' usage due to the fact that it > > > leads to a > > > deadlock. > > > The same mutex is acquired twice, without releasing it, once > > > inside > > > 'adis_initial_startup()' and once inside 'adis_enable_irq()'. > > > Instead of 'adis_initial_startup()', use > > > '__adis_initial_startup()'. > > > > > > Ramona Bolboaca (8): > > > iio: accel: adis16201: Fix deadlock in probe > > > iio: accel: adis16209: Fix deadlock in probe > > > iio: gyro: adis16136: Fix deadlock in probe > > > iio: gyro: adis16260: Fix deadlock in probe > > > iio: imu: adis16400: Fix deadlock in probe > > > staging: iio: accel: adis16203: Fix deadlock in probe > > > staging: iio: accel: adis16240: Fix deadlock in probe > > > iio: imu: adis: Remove adis_initial_startup function > > > > > > drivers/iio/accel/adis16201.c | 2 +- > > > drivers/iio/accel/adis16209.c | 2 +- > > > drivers/iio/gyro/adis16136.c | 2 +- > > > drivers/iio/gyro/adis16260.c | 2 +- > > > drivers/iio/imu/adis16400.c | 2 +- > > > drivers/staging/iio/accel/adis16203.c | 2 +- > > > drivers/staging/iio/accel/adis16240.c | 2 +- > > > include/linux/iio/imu/adis.h | 12 ------------ > > > 8 files changed, 7 insertions(+), 19 deletions(-) > > > > > > > You could have placed your v2 changelog in the cover letter. > > Moreover it's the same for all patches... Anyways: > > > > Reviewed-by: Nuno Sá <nuno.sa@...log.com> > > This feels a little backwards. Normally we'd expect the > outer function to take the lock and the inner call to not > do so. Now it's fine to not take the lock here at all because > the outer function call is in probe anyway, before we reach > the point where there should be an concurrency. > > I wonder if we should instead do this by having > an unlocked __adis_enable_irq() that is always called > by __adis_initial_startup(). That would be the fix that > then needs backporting. > I did mentioned the same thing in the first version of the series but did not really pushed for it. Now that you mention, I agree it feels weird (and wrong from a design perspective) to have the lock, "silently", taken inside a function starting with double underscore (which should mean unlocked call). > Switching the calls from adis_initial_startup() to > __adis_initial_startup() would then just be a trivial > optimization to not take locks before they should ever matter. > > This all hinges on my assumption that the lock isn't useful. > Am I right on that? > I think so as all the calls happen during probe before registering the userspace interface. - Nuno Sá
Powered by blists - more mailing lists