Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 8 Mar 2019 23:32:53 +0100
From:   Tomasz Duszynski <tduszyns@gmail.com>
To:     Sven Van Asbroeck <thesven73@gmail.com>
Cc:     Tomasz Duszynski <tduszyns@gmail.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        linux-iio@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Matt Ranostay <matt.ranostay@konsulko.com>
Subject: Re: [PATCH v2] iio: proximity: as3935: fix use-after-free on device
 remove
Message-ID: <20190308223249.GC32641@arch>
References: <20190308175935.21904-1-TheSven73@gmail.com>
 <20190308202936.GA32641@arch>
 <CAGngYiVgEDTQxynjPDMO_ZApAX6uyvUL+iqyK-QHnhDz=E4eCw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAGngYiVgEDTQxynjPDMO_ZApAX6uyvUL+iqyK-QHnhDz=E4eCw@mail.gmail.com>
User-Agent: Mutt/1.11.3 (2019-02-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 08, 2019 at 03:42:04PM -0500, Sven Van Asbroeck wrote:
> On Fri, Mar 8, 2019 at 3:30 PM Tomasz Duszynski <tduszyns@gmail.com> wrote:
> >
> > > @@ -368,7 +376,6 @@ static int as3935_probe(struct spi_device *spi)
> > >
> > >       spi_set_drvdata(spi, indio_dev);
> > >       mutex_init(&st->lock);
> > > -     INIT_DELAYED_WORK(&st->work, as3935_event_work);
> >
> > Any specific reason for moving this elsewhere?
>
> Yes. On the remove path, cancel_delayed_work_sync() should execute after
> free_irq(), but before triggered_buffer_cleanup(). So the devm_add_action()
> must run right before devm_request_irq(). I figured it would make sense to
> group the devm_add_action() and INIT_WORK() together, as they are
> related. This also makes it easier to understand the probe/remove order
> when reading the code.
>

So perhaps that change deserves a separate patch because it smells like
a code cleanup.

> > >
> > >       ret = of_property_read_u32(np,
> > >                       "ams,tuning-capacitor-pf", &st->tune_cap);
> > > @@ -414,22 +421,27 @@ static int as3935_probe(struct spi_device *spi)
> > >       iio_trigger_set_drvdata(trig, indio_dev);
> > >       trig->ops = &iio_interrupt_trigger_ops;
> > >
> > > -     ret = iio_trigger_register(trig);
> > > +     ret = devm_iio_trigger_register(&spi->dev, trig);
> > >       if (ret) {
> > >               dev_err(&spi->dev, "failed to register trigger\n");
> > >               return ret;
> > >       }
> > >
> > > -     ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
> > > -             &as3935_trigger_handler, NULL);
> > > +     ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> > > +             iio_pollfunc_store_time, as3935_trigger_handler, NULL);
> >
> > You can fix arguments alignment while you are at it.
> >
>
> What type of alignment would you prefer? This?
>
>         ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
>                                               iio_pollfunc_store_time,
>                                               as3935_trigger_handler, NULL);

Yes, this is what I've been thinking about. Thanks.