* re: spi: Add call to spi_slave_abort() function when spidev driver is released
@ 2019-09-26 10:00 Colin Ian King
2019-09-26 10:14 ` Lukasz Majewski
0 siblings, 1 reply; 10+ messages in thread
From: Colin Ian King @ 2019-09-26 10:00 UTC (permalink / raw)
To: Lukasz Majewski, Mark Brown, linux-spi; +Cc: linux-kernel
Hi,
Static analysis with Coverity has detected an potential dereference of a
free'd object with commit:
commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
Author: Lukasz Majewski <lukma@denx.de>
Date: Wed Sep 25 11:11:42 2019 +0200
spi: Add call to spi_slave_abort() function when spidev driver is
released
In spidev_release() in drivers/spi/spidev.c the analysis is as follows:
600static int spidev_release(struct inode *inode, struct file *filp)
601{
602 struct spidev_data *spidev;
603
604 mutex_lock(&device_list_lock);
1. alias: Assigning: spidev = filp->private_data. Now both point to
the same storage.
605 spidev = filp->private_data;
606 filp->private_data = NULL;
607
608 /* last close? */
609 spidev->users--;
2. Condition !spidev->users, taking true branch.
610 if (!spidev->users) {
611 int dofree;
612
613 kfree(spidev->tx_buffer);
614 spidev->tx_buffer = NULL;
615
616 kfree(spidev->rx_buffer);
617 spidev->rx_buffer = NULL;
618
619 spin_lock_irq(&spidev->spi_lock);
3. Condition spidev->spi, taking false branch.
620 if (spidev->spi)
621 spidev->speed_hz = spidev->spi->max_speed_hz;
622
623 /* ... after we unbound from the underlying device? */
4. Condition spidev->spi == NULL, taking true branch.
624 dofree = (spidev->spi == NULL);
625 spin_unlock_irq(&spidev->spi_lock);
626
5. Condition dofree, taking true branch.
627 if (dofree)
6. freed_arg: kfree frees spidev.
628 kfree(spidev);
629 }
630#ifdef CONFIG_SPI_SLAVE
CID 89726 (#1 of 1): Read from pointer after free (USE_AFTER_FREE)
7. deref_after_free: Dereferencing freed pointer spidev.
631 spi_slave_abort(spidev->spi);
632#endif
633 mutex_unlock(&device_list_lock);
634
635 return 0;
636}
The call to spi_slave_abort() on spidev is reading an earlier kfree'd
spidev.
Colin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: spi: Add call to spi_slave_abort() function when spidev driver is released
2019-09-26 10:00 spi: Add call to spi_slave_abort() function when spidev driver is released Colin Ian King
@ 2019-09-26 10:14 ` Lukasz Majewski
2019-09-26 10:15 ` Colin Ian King
2019-09-26 10:32 ` Geert Uytterhoeven
0 siblings, 2 replies; 10+ messages in thread
From: Lukasz Majewski @ 2019-09-26 10:14 UTC (permalink / raw)
To: Colin Ian King; +Cc: Mark Brown, linux-spi, linux-kernel, Geert Uytterhoeven
[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]
Hi Colin,
> Hi,
>
> Static analysis with Coverity has detected an potential dereference
> of a free'd object with commit:
>
> commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> Author: Lukasz Majewski <lukma@denx.de>
> Date: Wed Sep 25 11:11:42 2019 +0200
>
> spi: Add call to spi_slave_abort() function when spidev driver is
> released
>
> In spidev_release() in drivers/spi/spidev.c the analysis is as
> follows:
>
> 600static int spidev_release(struct inode *inode, struct file *filp)
> 601{
> 602 struct spidev_data *spidev;
> 603
> 604 mutex_lock(&device_list_lock);
>
> 1. alias: Assigning: spidev = filp->private_data. Now both point to
> the same storage.
>
> 605 spidev = filp->private_data;
> 606 filp->private_data = NULL;
> 607
> 608 /* last close? */
> 609 spidev->users--;
>
> 2. Condition !spidev->users, taking true branch.
>
> 610 if (!spidev->users) {
> 611 int dofree;
> 612
> 613 kfree(spidev->tx_buffer);
> 614 spidev->tx_buffer = NULL;
> 615
> 616 kfree(spidev->rx_buffer);
> 617 spidev->rx_buffer = NULL;
> 618
> 619 spin_lock_irq(&spidev->spi_lock);
>
> 3. Condition spidev->spi, taking false branch.
>
> 620 if (spidev->spi)
> 621 spidev->speed_hz =
> spidev->spi->max_speed_hz; 622
> 623 /* ... after we unbound from the underlying
> device? */
>
> 4. Condition spidev->spi == NULL, taking true branch.
>
> 624 dofree = (spidev->spi == NULL);
> 625 spin_unlock_irq(&spidev->spi_lock);
> 626
>
> 5. Condition dofree, taking true branch.
>
> 627 if (dofree)
>
> 6. freed_arg: kfree frees spidev.
>
> 628 kfree(spidev);
> 629 }
> 630#ifdef CONFIG_SPI_SLAVE
>
> CID 89726 (#1 of 1): Read from pointer after free (USE_AFTER_FREE)
> 7. deref_after_free: Dereferencing freed pointer spidev.
>
> 631 spi_slave_abort(spidev->spi);
> 632#endif
> 633 mutex_unlock(&device_list_lock);
> 634
> 635 return 0;
> 636}
>
> 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.
However, Geert (CC'ed) had some questions about placement of this
function call, so I will wait with providing fix until he replies.
>
> Colin
>
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@denx.de
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: spi: Add call to spi_slave_abort() function when spidev driver is released
2019-09-26 10:14 ` Lukasz Majewski
@ 2019-09-26 10:15 ` Colin Ian King
2019-09-26 10:32 ` Geert Uytterhoeven
1 sibling, 0 replies; 10+ messages in thread
From: Colin Ian King @ 2019-09-26 10:15 UTC (permalink / raw)
To: Lukasz Majewski; +Cc: Mark Brown, linux-spi, linux-kernel, Geert Uytterhoeven
[-- Attachment #1.1: Type: text/plain, Size: 3060 bytes --]
On 26/09/2019 11:14, Lukasz Majewski wrote:
> Hi Colin,
>
>> Hi,
>>
>> Static analysis with Coverity has detected an potential dereference
>> of a free'd object with commit:
>>
>> commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
>> Author: Lukasz Majewski <lukma@denx.de>
>> Date: Wed Sep 25 11:11:42 2019 +0200
>>
>> spi: Add call to spi_slave_abort() function when spidev driver is
>> released
>>
>> In spidev_release() in drivers/spi/spidev.c the analysis is as
>> follows:
>>
>> 600static int spidev_release(struct inode *inode, struct file *filp)
>> 601{
>> 602 struct spidev_data *spidev;
>> 603
>> 604 mutex_lock(&device_list_lock);
>>
>> 1. alias: Assigning: spidev = filp->private_data. Now both point to
>> the same storage.
>>
>> 605 spidev = filp->private_data;
>> 606 filp->private_data = NULL;
>> 607
>> 608 /* last close? */
>> 609 spidev->users--;
>>
>> 2. Condition !spidev->users, taking true branch.
>>
>> 610 if (!spidev->users) {
>> 611 int dofree;
>> 612
>> 613 kfree(spidev->tx_buffer);
>> 614 spidev->tx_buffer = NULL;
>> 615
>> 616 kfree(spidev->rx_buffer);
>> 617 spidev->rx_buffer = NULL;
>> 618
>> 619 spin_lock_irq(&spidev->spi_lock);
>>
>> 3. Condition spidev->spi, taking false branch.
>>
>> 620 if (spidev->spi)
>> 621 spidev->speed_hz =
>> spidev->spi->max_speed_hz; 622
>> 623 /* ... after we unbound from the underlying
>> device? */
>>
>> 4. Condition spidev->spi == NULL, taking true branch.
>>
>> 624 dofree = (spidev->spi == NULL);
>> 625 spin_unlock_irq(&spidev->spi_lock);
>> 626
>>
>> 5. Condition dofree, taking true branch.
>>
>> 627 if (dofree)
>>
>> 6. freed_arg: kfree frees spidev.
>>
>> 628 kfree(spidev);
>> 629 }
>> 630#ifdef CONFIG_SPI_SLAVE
>>
>> CID 89726 (#1 of 1): Read from pointer after free (USE_AFTER_FREE)
>> 7. deref_after_free: Dereferencing freed pointer spidev.
>>
>> 631 spi_slave_abort(spidev->spi);
>> 632#endif
>> 633 mutex_unlock(&device_list_lock);
>> 634
>> 635 return 0;
>> 636}
>>
>> 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.
>
> However, Geert (CC'ed) had some questions about placement of this
> function call, so I will wait with providing fix until he replies.
Cool, thanks for the update.
Colin
>
>>
>> Colin
>>
>
>
> 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@denx.de
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: spi: Add call to spi_slave_abort() function when spidev driver is released
2019-09-26 10:14 ` Lukasz Majewski
2019-09-26 10:15 ` Colin Ian King
@ 2019-09-26 10:32 ` Geert Uytterhoeven
2019-09-26 12:49 ` Lukasz Majewski
1 sibling, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2019-09-26 10:32 UTC (permalink / raw)
To: Lukasz Majewski; +Cc: Colin Ian King, Mark Brown, linux-spi, linux-kernel
Hi Lukasz,
On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <lukma@denx.de> wrote:
> > Static analysis with Coverity has detected an potential dereference
> > of a free'd object with commit:
> >
> > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > Author: Lukasz Majewski <lukma@denx.de>
> > Date: Wed Sep 25 11:11:42 2019 +0200
> >
> > spi: Add call to spi_slave_abort() function when spidev driver is
> > released
> >
> > In spidev_release() in drivers/spi/spidev.c the analysis is as
> > follows:
> >
> > 600static int spidev_release(struct inode *inode, struct file *filp)
> > 601{
> > 602 struct spidev_data *spidev;
> > 603
> > 604 mutex_lock(&device_list_lock);
> >
> > 1. alias: Assigning: spidev = filp->private_data. Now both point to
> > the same storage.
> >
> > 605 spidev = filp->private_data;
> > 606 filp->private_data = NULL;
> > 607
> > 608 /* last close? */
> > 609 spidev->users--;
> >
> > 2. Condition !spidev->users, taking true branch.
> >
> > 610 if (!spidev->users) {
> > 611 int dofree;
> > 612
> > 613 kfree(spidev->tx_buffer);
> > 614 spidev->tx_buffer = NULL;
> > 615
> > 616 kfree(spidev->rx_buffer);
> > 617 spidev->rx_buffer = NULL;
> > 618
> > 619 spin_lock_irq(&spidev->spi_lock);
> >
> > 3. Condition spidev->spi, taking false branch.
> >
> > 620 if (spidev->spi)
> > 621 spidev->speed_hz =
> > spidev->spi->max_speed_hz; 622
> > 623 /* ... after we unbound from the underlying
> > device? */
> >
> > 4. Condition spidev->spi == NULL, taking true branch.
> >
> > 624 dofree = (spidev->spi == NULL);
> > 625 spin_unlock_irq(&spidev->spi_lock);
> > 626
> >
> > 5. Condition dofree, taking true branch.
> >
> > 627 if (dofree)
> >
> > 6. freed_arg: kfree frees spidev.
> >
> > 628 kfree(spidev);
> > 629 }
> > 630#ifdef CONFIG_SPI_SLAVE
> >
> > CID 89726 (#1 of 1): Read from pointer after free (USE_AFTER_FREE)
> > 7. deref_after_free: Dereferencing freed pointer spidev.
> >
> > 631 spi_slave_abort(spidev->spi);
> > 632#endif
> > 633 mutex_unlock(&device_list_lock);
> > 634
> > 635 return 0;
> > 636}
> >
> > 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.
> 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...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: spi: Add call to spi_slave_abort() function when spidev driver is released
2019-09-26 10:32 ` Geert Uytterhoeven
@ 2019-09-26 12:49 ` Lukasz Majewski
2019-09-26 13:51 ` Geert Uytterhoeven
0 siblings, 1 reply; 10+ messages in thread
From: Lukasz Majewski @ 2019-09-26 12:49 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Colin Ian King, Mark Brown, linux-spi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4628 bytes --]
Hi Geert,
> Hi Lukasz,
>
> On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <lukma@denx.de>
> wrote:
> > > Static analysis with Coverity has detected an potential
> > > dereference of a free'd object with commit:
> > >
> > > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > > Author: Lukasz Majewski <lukma@denx.de>
> > > Date: Wed Sep 25 11:11:42 2019 +0200
> > >
> > > spi: Add call to spi_slave_abort() function when spidev
> > > driver is released
> > >
> > > In spidev_release() in drivers/spi/spidev.c the analysis is as
> > > follows:
> > >
> > > 600static int spidev_release(struct inode *inode, struct file
> > > *filp) 601{
> > > 602 struct spidev_data *spidev;
> > > 603
> > > 604 mutex_lock(&device_list_lock);
> > >
> > > 1. alias: Assigning: spidev = filp->private_data. Now both
> > > point to the same storage.
> > >
> > > 605 spidev = filp->private_data;
> > > 606 filp->private_data = NULL;
> > > 607
> > > 608 /* last close? */
> > > 609 spidev->users--;
> > >
> > > 2. Condition !spidev->users, taking true branch.
> > >
> > > 610 if (!spidev->users) {
> > > 611 int dofree;
> > > 612
> > > 613 kfree(spidev->tx_buffer);
> > > 614 spidev->tx_buffer = NULL;
> > > 615
> > > 616 kfree(spidev->rx_buffer);
> > > 617 spidev->rx_buffer = NULL;
> > > 618
> > > 619 spin_lock_irq(&spidev->spi_lock);
> > >
> > > 3. Condition spidev->spi, taking false branch.
> > >
> > > 620 if (spidev->spi)
> > > 621 spidev->speed_hz =
> > > spidev->spi->max_speed_hz; 622
> > > 623 /* ... after we unbound from the underlying
> > > device? */
> > >
> > > 4. Condition spidev->spi == NULL, taking true branch.
> > >
> > > 624 dofree = (spidev->spi == NULL);
> > > 625 spin_unlock_irq(&spidev->spi_lock);
> > > 626
> > >
> > > 5. Condition dofree, taking true branch.
> > >
> > > 627 if (dofree)
> > >
> > > 6. freed_arg: kfree frees spidev.
> > >
> > > 628 kfree(spidev);
> > > 629 }
> > > 630#ifdef CONFIG_SPI_SLAVE
> > >
> > > CID 89726 (#1 of 1): Read from pointer after free
> > > (USE_AFTER_FREE) 7. deref_after_free: Dereferencing freed pointer
> > > spidev.
> > >
> > > 631 spi_slave_abort(spidev->spi);
> > > 632#endif
> > > 633 mutex_unlock(&device_list_lock);
> > > 634
> > > 635 return 0;
> > > 636}
> > >
> > > 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 ?
> > 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 ? ([*]).
>
> 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@denx.de
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: spi: Add call to spi_slave_abort() function when spidev driver is released
2019-09-26 12:49 ` Lukasz Majewski
@ 2019-09-26 13:51 ` Geert Uytterhoeven
2019-09-26 14:06 ` Lukasz Majewski
0 siblings, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2019-09-26 13:51 UTC (permalink / raw)
To: Lukasz Majewski; +Cc: Colin Ian King, Mark Brown, linux-spi, linux-kernel
Hi Lukasz,
On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <lukma@denx.de> wrote:
> > On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <lukma@denx.de>
> > wrote:
> > > > Static analysis with Coverity has detected an potential
> > > > dereference of a free'd object with commit:
> > > >
> > > > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > > > Author: Lukasz Majewski <lukma@denx.de>
> > > > 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?
> > > 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?), 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?
The more I think about it, the more things I see that can go wrong...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: spi: Add call to spi_slave_abort() function when spidev driver is released
2019-09-26 13:51 ` Geert Uytterhoeven
@ 2019-09-26 14:06 ` Lukasz Majewski
2019-09-26 15:17 ` Mark Brown
0 siblings, 1 reply; 10+ messages in thread
From: Lukasz Majewski @ 2019-09-26 14:06 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Colin Ian King, Mark Brown, linux-spi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4400 bytes --]
Hi Geert,
> Hi Lukasz,
>
> On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <lukma@denx.de> wrote:
> > > On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <lukma@denx.de>
> > > wrote:
> > > > > Static analysis with Coverity has detected an potential
> > > > > dereference of a free'd object with commit:
> > > > >
> > > > > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > > > > Author: Lukasz Majewski <lukma@denx.de>
> > > > > 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@denx.de
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: spi: Add call to spi_slave_abort() function when spidev driver is released
2019-09-26 14:06 ` Lukasz Majewski
@ 2019-09-26 15:17 ` Mark Brown
2019-09-26 22:38 ` Lukasz Majewski
0 siblings, 1 reply; 10+ messages in thread
From: Mark Brown @ 2019-09-26 15:17 UTC (permalink / raw)
To: Lukasz Majewski
Cc: Geert Uytterhoeven, Colin Ian King, linux-spi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]
On Thu, Sep 26, 2019 at 04:06:45PM +0200, Lukasz Majewski wrote:
> > On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <lukma@denx.de> wrote:
> > > 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...
It doesn't seem entirely realistic, but I can imagine cases like
fork()/exec() where we end up with two copies of the file open
but end up immediately closing one.
> > 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).
Should be on file close IIRC.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: spi: Add call to spi_slave_abort() function when spidev driver is released
2019-09-26 15:17 ` Mark Brown
@ 2019-09-26 22:38 ` Lukasz Majewski
2019-10-07 17:02 ` Mark Brown
0 siblings, 1 reply; 10+ messages in thread
From: Lukasz Majewski @ 2019-09-26 22:38 UTC (permalink / raw)
To: Mark Brown; +Cc: Geert Uytterhoeven, Colin Ian King, linux-spi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]
Hi Mark, Geert,
> On Thu, Sep 26, 2019 at 04:06:45PM +0200, Lukasz Majewski wrote:
> > > On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <lukma@denx.de>
> > > wrote:
>
> > > > 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...
>
> It doesn't seem entirely realistic, but I can imagine cases like
> fork()/exec() where we end up with two copies of the file open
> but end up immediately closing one.
>
> > > 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).
>
> Should be on file close IIRC.
Any ideas on how to solve this issue?
Maybe, it would be sufficient for now to move the spi_slave_abort() in
spi_release() before we decrease (spidev->users--) the use count?
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@denx.de
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: spi: Add call to spi_slave_abort() function when spidev driver is released
2019-09-26 22:38 ` Lukasz Majewski
@ 2019-10-07 17:02 ` Mark Brown
0 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2019-10-07 17:02 UTC (permalink / raw)
To: Lukasz Majewski
Cc: Geert Uytterhoeven, Colin Ian King, linux-spi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 529 bytes --]
On Fri, Sep 27, 2019 at 12:38:49AM +0200, Lukasz Majewski wrote:
> Maybe, it would be sufficient for now to move the spi_slave_abort() in
> spi_release() before we decrease (spidev->users--) the use count?
I think that should be OK, or possibly safer to do it at the start of
the if (!spidev->users) section to avoid problems with fork()/exec()
combinations. It'll need an if (spidev->spi) check as well, if we
manage to hit that we've pretty much lost anyway though and I'm not sure
I see a route to sensible error handling.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-07 17:02 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-26 10:00 spi: Add call to spi_slave_abort() function when spidev driver is released Colin Ian King
2019-09-26 10:14 ` Lukasz Majewski
2019-09-26 10:15 ` Colin Ian King
2019-09-26 10:32 ` Geert Uytterhoeven
2019-09-26 12:49 ` Lukasz Majewski
2019-09-26 13:51 ` Geert Uytterhoeven
2019-09-26 14:06 ` Lukasz Majewski
2019-09-26 15:17 ` Mark Brown
2019-09-26 22:38 ` Lukasz Majewski
2019-10-07 17:02 ` Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).