Hi Geert,
> Hi Lukasz,
>
> On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <lukma@xxxxxxx> wrote:
> > > On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <lukma@xxxxxxx>
> > > wrote:
> > > > > Static analysis with Coverity has detected an potential
> > > > > dereference of a free'd object with commit:
> > > > >
> > > > > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > > > > Author: Lukasz Majewski <lukma@xxxxxxx>
> > > > > Date: Wed Sep 25 11:11:42 2019 +0200
> > > > >
> > > > > spi: Add call to spi_slave_abort() function when spidev
> > > > > driver is released
>
> > > > > The call to spi_slave_abort() on spidev is reading an earlier
> > > > > kfree'd spidev.
> > > >
> > > > Thanks for spotting this issue - indeed there is a possibility
> > > > to use spidev after being kfree'd.
> > >
> > > Worse, this makes me realize spidev->spi may be a NULL pointer,
> > > which will be dereferenced by spi_slave_abort(), so caching it
> > > before the call to kfree() won't work.
> >
> > The patch as it is now can be fixed as follows:
> >
> > static int spidev_release(struct inode *inode, struct file *filp)
> > {
> > struct spidev_data *spidev;
> >
> > mutex_lock(&device_list_lock);
> > spidev = filp->private_data;
> > filp->private_data = NULL;
> >
> > #ifdef CONFIG_SPI_SLAVE
> > if (spidev->spi)
> > spi_slave_abort(spidev->spi);
> > #endif
> >
> > /* last close? */
> > spidev->users--;
> > if (!spidev->users) {
> > int dofree;
> >
> > /* free buffers */
> >
> > spin_lock_irq(&spidev->spi_lock);
> > if (spidev->spi)
> > spidev->speed_hz =
> > spidev->spi->max_speed_hz;
> >
> > /* ... after we unbound from the underlying device?
> > */ //
> > // [*]
> > //
> > dofree = (spidev->spi == NULL);
> > spin_unlock_irq(&spidev->spi_lock);
> >
> > if (dofree)
> > kfree(spidev);
> > }
> >
> > mutex_unlock(&device_list_lock);
> >
> > return 0;
> > }
> >
> > The question is if we shall call the spi_slave_abort() when
> > cleaning up spi after releasing last reference, or each time
> > release callback is called ?
>
> TBH, I don't know. Is it realistic that there are multiple opens?
I'm using on my setup only one test program to use /dev/spidevX.Y and
/dev/spidevA.B (loopback with wired connection).
However, you also shall be able to connect via ssh and run the same
setup in parallel...
>
> > > > However, Geert (CC'ed) had some questions about placement of
> > > > this function call, so I will wait with providing fix until he
> > > > replies.
> > >
> > > Seems like this needs more thought...
> >
> > Could you be more specific?
> >
> > Do you mean to move the spi_slave_abort() call just before dofree
> > evaluation ? ([*]).
>
> That means the abort is called only for the last user.
> And only if the underlying device still exists. Which means that if
> it has disappeared (how can that happen? spidev unbind?),
In my case, I just disconnect some SPI signals and the test program
just hangs. I do need to ctrl+c to stop it (or use timeout).
From my debugging the .release callback is called each time the program
is aborted (either with ctrl+c or timeout).
> the slave
> was never aborted. Non-spidev slaves can do the abort in their
> .remove() callbacks (at least my two sample slave drivers do).
> So probably we need some explicit slave abort in the unbind case too?
As I've described above - after "introducing" distortion to SPI I need
to explicitly exit the hung test program with ctrl+c.
>
> The more I think about it, the more things I see that can go wrong...
But for now we don't have any way to recover the slave after corruption
on SPI transmission.
>
> Gr{oetje,eeting}s,
>
> Geert
>
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@xxxxxxx
Attachment:
pgpqNn9Fct4vG.pgp
Description: OpenPGP digital signature
