| 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
| ||
|
Message-ID: <DB3PR0402MB39162CD8C7BF1431C16B378FF52C0@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date: Mon, 8 Apr 2019 02:07:24 +0000
From: Anson Huang <anson.huang@....com>
To: Jonathan Cameron <jic23@...nel.org>
CC: "knaack.h@....de" <knaack.h@....de>,
"lars@...afoo.de" <lars@...afoo.de>,
"pmeerw@...erw.net" <pmeerw@...erw.net>,
Leonard Crestez <leonard.crestez@....com>,
"gustavo@...eddedor.com" <gustavo@...eddedor.com>,
"martink@...teo.de" <martink@...teo.de>,
"rtresidd@...ctromag.com.au" <rtresidd@...ctromag.com.au>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
dl-linux-imx <linux-imx@....com>
Subject: RE: [EXT] Re: [PATCH] iio: accell: mma8452: free iio trigger pointer
when cleanup
Hi, Jonathan
Best Regards!
Anson Huang
> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@...nel.org]
> Sent: 2019年4月7日 18:40
> To: Anson Huang <anson.huang@....com>
> Cc: knaack.h@....de; lars@...afoo.de; pmeerw@...erw.net; Leonard
> Crestez <leonard.crestez@....com>; gustavo@...eddedor.com;
> martink@...teo.de; rtresidd@...ctromag.com.au; linux-iio@...r.kernel.org;
> linux-kernel@...r.kernel.org; dl-linux-imx <linux-imx@....com>
> Subject: [EXT] Re: [PATCH] iio: accell: mma8452: free iio trigger pointer when
> cleanup
>
> WARNING: This email was created outside of NXP. DO NOT CLICK links or
> attachments unless you recognize the sender and know the content is safe.
>
>
>
> On Thu, 4 Apr 2019 08:02:05 +0000
> Anson Huang <anson.huang@....com> wrote:
>
> > When mma8452 is built as module, once it is insmod and rmmod, below
> > kernel dump will show out, the root cause is module being put twice if
> > iio trigger pointer is NOT NULL, this patch frees iio trigger pointer
> > after iio trigger is unregistered to avoid below kernel
> > dump:
> >
> > ------------[ cut here ]------------
> > WARNING: CPU: 3 PID: 270 at kernel/module.c:1145
> module_put+0xd0/0x188
> > Modules linked in: mma8452(-)
> > CPU: 3 PID: 270 Comm: rmmod Not tainted
> > 5.1.0-rc3-next-20190403-00022-g5ede0c9-dirty #1596 Hardware name:
> > Freescale i.MX6 Quad/DualLite (Device Tree) [<c011272c>]
> > (unwind_backtrace) from [<c010d094>] (show_stack+0x10/0x14)
> > [<c010d094>] (show_stack) from [<c0bdc160>] (dump_stack+0xd8/0x10c)
> > [<c0bdc160>] (dump_stack) from [<c01275d8>] (__warn+0xf8/0x124)
> > [<c01275d8>] (__warn) from [<c0127714>]
> (warn_slowpath_null+0x3c/0x48)
> > [<c0127714>] (warn_slowpath_null) from [<c01ca4d0>]
> > (module_put+0xd0/0x188) [<c01ca4d0>] (module_put) from [<c08a5bb4>]
> > (iio_device_unregister_trigger_consumer+0x18/0x24)
> > [<c08a5bb4>] (iio_device_unregister_trigger_consumer) from
> > [<c08a0ab4>] (iio_dev_release+0x20/0) [<c08a0ab4>] (iio_dev_release)
> > from [<c065c0e8>] (device_release+0x24/0x8c) [<c065c0e8>]
> > (device_release) from [<c0be1810>] (kobject_put+0x74/0xd4)
> > [<c0be1810>] (kobject_put) from [<c0664dc0>]
> > (release_nodes+0x16c/0x1f0) [<c0664dc0>] (release_nodes) from
> > [<c0661b80>] (device_release_driver_internal+0xec/0x1a0)
> > [<c0661b80>] (device_release_driver_internal) from [<c0661c90>]
> > (driver_detach+0x44/0x80) [<c0661c90>] (driver_detach) from
> > [<c066096c>] (bus_remove_driver+0x4c/0xa0) [<c066096c>]
> > (bus_remove_driver) from [<c01cc1a4>] (sys_delete_module+0x130/0x1dc)
> > [<c01cc1a4>] (sys_delete_module) from [<c0101000>]
> (ret_fast_syscall+0x0/0x28) Exception stack(0xe8d87fa8 to 0xe8d87ff0)
> > 7fa0: 0001ffc0 38616d6d be87bbf0 00000880 00000000 be87be98
> > 7fc0: 0001ffc0 38616d6d 00323534 00000081 000a9744 be87bf81 000a9790
> > 00000000
> > 7fe0: be87bbe8 be87bbd8 0001fe18 b6e381a0 irq event stamp: 4017
> > hardirqs last enabled at (4035): [<c0188a0c>]
> > console_unlock+0x400/0x630 hardirqs last disabled at (4052):
> > [<c018868c>] console_unlock+0x80/0x630 softirqs last enabled at
> > (4050): [<c0102698>] __do_softirq+0x458/0x554 softirqs last disabled
> > at (4069): [<c012ed6c>] irq_exit+0x130/0x180 ---[ end trace
> > a7ba8f1823b1e073 ]---
> >
> > Signed-off-by: Anson Huang <Anson.Huang@....com>
> Good fine, but the fix is not in the best place. The key thing is that any
> assignment inside a driver to iio_dev->trig should be done with
> iio_trigger_get.
>
> indio_dev->trig = iio_trigger_get(trig). The intent is to avoid this exact double
> free, but also handle it correctly if we are using devm_ for all the handling.
>
> I just did a grep and there are quite a few drivers not doing this though so it's
> one we should be more careful about.
>
> Hmm. Anyone fancy doing an audit of existing drivers and checking which
> ones have this problem? Some seem to do exactly what you have here, but
> that isn't a the best pattern to encourage.
>
> For this one would you mind trying with the iio_trigger_get approach and if
> I'm not missing something, send a v2 fixing it that way?
With below patch to use iio_trigger_get, looks like try_release_module_ref() will return 1
and cause try_stop_module() return fail and we will see " rmmod: can't unload 'mma8452': Resource temporarily unavailable ",
As the module ref count check is before iio_device_unregister_trigger_consumer() is called which
will do iio_trigger_put(), so looks like there is still something wrong with the module ref count?
+++ b/drivers/iio/accel/mma8452.c
@@ -1468,17 +1468,15 @@ static int mma8452_trigger_setup(struct iio_dev *indio_dev)
if (ret)
return ret;
- indio_dev->trig = trig;
+ indio_dev->trig = iio_trigger_get(trig);
Thanks,
Anson.
>
> Thanks
>
> Jonathan
>
>
> > ---
> > drivers/iio/accel/mma8452.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> > index 3027811..6b18177 100644
> > --- a/drivers/iio/accel/mma8452.c
> > +++ b/drivers/iio/accel/mma8452.c
> > @@ -1475,8 +1475,10 @@ static int mma8452_trigger_setup(struct iio_dev
> > *indio_dev)
> >
> > static void mma8452_trigger_cleanup(struct iio_dev *indio_dev) {
> > - if (indio_dev->trig)
> > + if (indio_dev->trig) {
> > iio_trigger_unregister(indio_dev->trig);
> > + indio_dev->trig = NULL;
> > + }
> > }
> >
> > static int mma8452_reset(struct i2c_client *client)
Powered by blists - more mailing lists