linux-kernel - Re: [PATCH v1 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220518122515.aby5lbb4xusr6pdt@CAB-WSD-L081021.sigma.sbrf.ru>
Date:   Wed, 18 May 2022 12:25:59 +0000
From:   Dmitry Rokosov <DDRokosov@...rdevices.ru>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "stano.jakubek@...il.com" <stano.jakubek@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "stephan@...hold.net" <stephan@...hold.net>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        kernel <kernel@...rdevices.ru>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/3] iio: add MEMSensing MSA311 3-axis accelerometer
 driver

Hi Jonathan,

I have two items to be discussed about iio_trigger_get().
Please see my questions below and correct me if I'm wrong.

On Tue, Apr 26, 2022 at 08:24:10PM +0300, Dmitry Rokosov wrote:
> > > +							       "%s-new-data",
> > > +							       indio_dev->name);
> > > +		if (!msa311->new_data_trig) {
> > > +			dev_err(&i2c->dev, "cannot allocate new data trig\n");
> > > +			err = -ENOMEM;
> > > +			goto err_lock_destroy;
> > > +		}
> > > +
> > > +		msa311->new_data_trig->dev.parent = &i2c->dev;
> > > +		msa311->new_data_trig->ops = &msa311_new_data_trig_ops;
> > > +		iio_trigger_set_drvdata(msa311->new_data_trig, indio_dev);
> > > +		indio_dev->trig = msa311->new_data_trig;
> > 
> > This will create a double free if you were to change the trigger.
> > 		indio_dev->trig = iio_trigger_get(trig);
> > 
> 
> I didn't take into account other trigger usage.
> I'll rework this place for the v2.
> 

The first one problem is module_get() calling for trigger get()
semantic.
I've applied iio_trigger_get() function to acquire module refcnt,
but I've faced with rmmod busy problem. IIO driver module doesn't want to
stop and unload due to not having zero module refcnt.
Syscall delete_module() tries to stop module first and after calls
driver exit() function (which executes devm_* handlers inside, including IIO
trigger unregister). It means we have the chicken or the egg dilemma here.
Module can't be unloaded until module refcnt is not zero and we can't
execute IIO trigger unregister (decrease module refcnt) only when module
refcnt is zero.
I suppose the possible solution to such a problem is a different semantic
for internal triggers (inside driver itself) and external drivers (like
hwtimer trigger). What do you think?

The second one issue is located in the different IIO drivers. Some modules
call iio_trigger_get() before iio_trigger_register(), trig->owner is not
initialized to the right value (THIS_MODULE) and we don't acquire refcnt
for proper driver object.
I'm going to send patchset to problem driver set, but I can test only
buildable status for such modules, are you okay with that?

-- 
Thank you,
Dmitry