[BUG] skge: a possible sleep-in-atomic bug in skge_remove

[BUG] skge: a possible sleep-in-atomic bug in skge_remove

[Jia-Ju Bai]

According to drivers/net/ethernet/marvell/skge.c, the driver may sleep 
under a spinlock.
The function call path is:
skge_remove (acquire the spinlock)
   free_irq --> may sleep

I do not find a good way to fix it, so I only report.
This possible bug is found by my static analysis tool (DSAC) and checked 
by my code review.


Thanks,
Jia-Ju Bai

Re: [BUG] skge: a possible sleep-in-atomic bug in skge_remove

[David Miller]

From: Jia-Ju Bai <baijiaju1990@gmail.com>
Date: Tue, 12 Dec 2017 16:38:12 +0800

> According to drivers/net/ethernet/marvell/skge.c, the driver may sleep
> under a spinlock.
> The function call path is:
> skge_remove (acquire the spinlock)
>   free_irq --> may sleep
> 
> I do not find a good way to fix it, so I only report.
> This possible bug is found by my static analysis tool (DSAC) and
> checked by my code review.

This was added by:

commit a9e9fd7182332d0cf5f3e601df3e71dd431b70d7
Author: Stephen Hemminger <shemminger@vyatta.com>
Date:   Tue Sep 27 13:41:37 2011 -0400

    skge: handle irq better on single port card

I think the free_irq() can be moved below the unlock.

Stephen, please take a look.

Thanks!

Re: [BUG] skge: a possible sleep-in-atomic bug in skge_remove

[Stephen Hemminger]

On Tue, 12 Dec 2017 08:34:45 -0500 (EST)
David Miller <davem@davemloft.net> wrote:

> From: Jia-Ju Bai <baijiaju1990@gmail.com>
> Date: Tue, 12 Dec 2017 16:38:12 +0800
> 
> > According to drivers/net/ethernet/marvell/skge.c, the driver may sleep
> > under a spinlock.
> > The function call path is:
> > skge_remove (acquire the spinlock)
> >   free_irq --> may sleep
> > 
> > I do not find a good way to fix it, so I only report.
> > This possible bug is found by my static analysis tool (DSAC) and
> > checked by my code review.  
> 
> This was added by:
> 
> commit a9e9fd7182332d0cf5f3e601df3e71dd431b70d7
> Author: Stephen Hemminger <shemminger@vyatta.com>
> Date:   Tue Sep 27 13:41:37 2011 -0400
> 
>     skge: handle irq better on single port card
> 
> I think the free_irq() can be moved below the unlock.
> 
> Stephen, please take a look.

The IRQ was being free twice.
How did you see it, I really doubt any multi-port SKGE cards
still exist.

Re: [BUG] skge: a possible sleep-in-atomic bug in skge_remove

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Tue, 12 Dec 2017 10:22:40 -0800

> On Tue, 12 Dec 2017 08:34:45 -0500 (EST)
> David Miller <davem@davemloft.net> wrote:
> 
>> From: Jia-Ju Bai <baijiaju1990@gmail.com>
>> Date: Tue, 12 Dec 2017 16:38:12 +0800
>> 
>> > According to drivers/net/ethernet/marvell/skge.c, the driver may sleep
>> > under a spinlock.
>> > The function call path is:
>> > skge_remove (acquire the spinlock)
>> >   free_irq --> may sleep
>> > 
>> > I do not find a good way to fix it, so I only report.
>> > This possible bug is found by my static analysis tool (DSAC) and
>> > checked by my code review.  
>> 
>> This was added by:
>> 
>> commit a9e9fd7182332d0cf5f3e601df3e71dd431b70d7
>> Author: Stephen Hemminger <shemminger@vyatta.com>
>> Date:   Tue Sep 27 13:41:37 2011 -0400
>> 
>>     skge: handle irq better on single port card
>> 
>> I think the free_irq() can be moved below the unlock.
>> 
>> Stephen, please take a look.
> 
> The IRQ was being free twice.
> How did you see it, I really doubt any multi-port SKGE cards
> still exist.

He sees it by reading the code, please take a look at this
and move the free_irq() out of the spin locked section since
it can sleep.

Re: [BUG] skge: a possible sleep-in-atomic bug in skge_remove

[Stephen Hemminger]

On Tue, 12 Dec 2017 20:57:01 -0500 (EST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Tue, 12 Dec 2017 10:22:40 -0800
> 
> > On Tue, 12 Dec 2017 08:34:45 -0500 (EST)
> > David Miller <davem@davemloft.net> wrote:
> >   
> >> From: Jia-Ju Bai <baijiaju1990@gmail.com>
> >> Date: Tue, 12 Dec 2017 16:38:12 +0800
> >>   
> >> > According to drivers/net/ethernet/marvell/skge.c, the driver may sleep
> >> > under a spinlock.
> >> > The function call path is:
> >> > skge_remove (acquire the spinlock)
> >> >   free_irq --> may sleep
> >> > 
> >> > I do not find a good way to fix it, so I only report.
> >> > This possible bug is found by my static analysis tool (DSAC) and
> >> > checked by my code review.    
> >> 
> >> This was added by:
> >> 
> >> commit a9e9fd7182332d0cf5f3e601df3e71dd431b70d7
> >> Author: Stephen Hemminger <shemminger@vyatta.com>
> >> Date:   Tue Sep 27 13:41:37 2011 -0400
> >> 
> >>     skge: handle irq better on single port card
> >> 
> >> I think the free_irq() can be moved below the unlock.
> >> 
> >> Stephen, please take a look.  
> > 
> > The IRQ was being free twice.
> > How did you see it, I really doubt any multi-port SKGE cards
> > still exist.  
> 
> He sees it by reading the code, please take a look at this
> and move the free_irq() out of the spin locked section since
> it can sleep.

Thanks, I was hoping for some automated static analysis tool.

Re: [BUG] skge: a possible sleep-in-atomic bug in skge_remove

[Jia-Ju Bai]

On 2017/12/13 13:18, Stephen Hemminger wrote:
> On Tue, 12 Dec 2017 20:57:01 -0500 (EST)
> David Miller <davem@davemloft.net> wrote:
>
>> From: Stephen Hemminger <stephen@networkplumber.org>
>> Date: Tue, 12 Dec 2017 10:22:40 -0800
>>
>>> On Tue, 12 Dec 2017 08:34:45 -0500 (EST)
>>> David Miller <davem@davemloft.net> wrote:
>>>    
>>>> From: Jia-Ju Bai <baijiaju1990@gmail.com>
>>>> Date: Tue, 12 Dec 2017 16:38:12 +0800
>>>>    
>>>>> According to drivers/net/ethernet/marvell/skge.c, the driver may sleep
>>>>> under a spinlock.
>>>>> The function call path is:
>>>>> skge_remove (acquire the spinlock)
>>>>>    free_irq --> may sleep
>>>>>
>>>>> I do not find a good way to fix it, so I only report.
>>>>> This possible bug is found by my static analysis tool (DSAC) and
>>>>> checked by my code review.
>>>> This was added by:
>>>>
>>>> commit a9e9fd7182332d0cf5f3e601df3e71dd431b70d7
>>>> Author: Stephen Hemminger <shemminger@vyatta.com>
>>>> Date:   Tue Sep 27 13:41:37 2011 -0400
>>>>
>>>>      skge: handle irq better on single port card
>>>>
>>>> I think the free_irq() can be moved below the unlock.
>>>>
>>>> Stephen, please take a look.
>>> The IRQ was being free twice.
>>> How did you see it, I really doubt any multi-port SKGE cards
>>> still exist.
>> He sees it by reading the code, please take a look at this
>> and move the free_irq() out of the spin locked section since
>> it can sleep.
> Thanks, I was hoping for some automated static analysis tool.

This bug was found by an automated static analysis tool named DSAC, 
which is written by myself.
Then I manually checked driver source code, and finally sent the bug report.


Thanks,
Jia-Ju Bai

Re: [BUG] skge: a possible sleep-in-atomic bug in skge_remove

[Stephen Hemminger]

On Wed, 13 Dec 2017 15:42:56 +0800
Jia-Ju Bai <baijiaju1990@gmail.com> wrote:

> On 2017/12/13 13:18, Stephen Hemminger wrote:
> > On Tue, 12 Dec 2017 20:57:01 -0500 (EST)
> > David Miller <davem@davemloft.net> wrote:
> >  
> >> From: Stephen Hemminger <stephen@networkplumber.org>
> >> Date: Tue, 12 Dec 2017 10:22:40 -0800
> >>  
> >>> On Tue, 12 Dec 2017 08:34:45 -0500 (EST)
> >>> David Miller <davem@davemloft.net> wrote:
> >>>      
> >>>> From: Jia-Ju Bai <baijiaju1990@gmail.com>
> >>>> Date: Tue, 12 Dec 2017 16:38:12 +0800
> >>>>      
> >>>>> According to drivers/net/ethernet/marvell/skge.c, the driver may sleep
> >>>>> under a spinlock.
> >>>>> The function call path is:
> >>>>> skge_remove (acquire the spinlock)
> >>>>>    free_irq --> may sleep
> >>>>>
> >>>>> I do not find a good way to fix it, so I only report.
> >>>>> This possible bug is found by my static analysis tool (DSAC) and
> >>>>> checked by my code review.  
> >>>> This was added by:
> >>>>
> >>>> commit a9e9fd7182332d0cf5f3e601df3e71dd431b70d7
> >>>> Author: Stephen Hemminger <shemminger@vyatta.com>
> >>>> Date:   Tue Sep 27 13:41:37 2011 -0400
> >>>>
> >>>>      skge: handle irq better on single port card
> >>>>
> >>>> I think the free_irq() can be moved below the unlock.
> >>>>
> >>>> Stephen, please take a look.  
> >>> The IRQ was being free twice.
> >>> How did you see it, I really doubt any multi-port SKGE cards
> >>> still exist.  
> >> He sees it by reading the code, please take a look at this
> >> and move the free_irq() out of the spin locked section since
> >> it can sleep.  
> > Thanks, I was hoping for some automated static analysis tool.  
> 
> This bug was found by an automated static analysis tool named DSAC, 
> which is written by myself.
> Then I manually checked driver source code, and finally sent the bug report.

Thanks.
Would it be possible to put tool in tools directory and then have
it automated by kbuild robot?

Re: [BUG] skge: a possible sleep-in-atomic bug in skge_remove

[Jia-Ju Bai]

On 2017/12/14 0:50, Stephen Hemminger wrote:
> On Wed, 13 Dec 2017 15:42:56 +0800
> Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
>
>> On 2017/12/13 13:18, Stephen Hemminger wrote:
>>> On Tue, 12 Dec 2017 20:57:01 -0500 (EST)
>>> David Miller <davem@davemloft.net> wrote:
>>>   
>>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>>> Date: Tue, 12 Dec 2017 10:22:40 -0800
>>>>   
>>>>> On Tue, 12 Dec 2017 08:34:45 -0500 (EST)
>>>>> David Miller <davem@davemloft.net> wrote:
>>>>>       
>>>>>> From: Jia-Ju Bai <baijiaju1990@gmail.com>
>>>>>> Date: Tue, 12 Dec 2017 16:38:12 +0800
>>>>>>       
>>>>>>> According to drivers/net/ethernet/marvell/skge.c, the driver may sleep
>>>>>>> under a spinlock.
>>>>>>> The function call path is:
>>>>>>> skge_remove (acquire the spinlock)
>>>>>>>     free_irq --> may sleep
>>>>>>>
>>>>>>> I do not find a good way to fix it, so I only report.
>>>>>>> This possible bug is found by my static analysis tool (DSAC) and
>>>>>>> checked by my code review.
>>>>>> This was added by:
>>>>>>
>>>>>> commit a9e9fd7182332d0cf5f3e601df3e71dd431b70d7
>>>>>> Author: Stephen Hemminger <shemminger@vyatta.com>
>>>>>> Date:   Tue Sep 27 13:41:37 2011 -0400
>>>>>>
>>>>>>       skge: handle irq better on single port card
>>>>>>
>>>>>> I think the free_irq() can be moved below the unlock.
>>>>>>
>>>>>> Stephen, please take a look.
>>>>> The IRQ was being free twice.
>>>>> How did you see it, I really doubt any multi-port SKGE cards
>>>>> still exist.
>>>> He sees it by reading the code, please take a look at this
>>>> and move the free_irq() out of the spin locked section since
>>>> it can sleep.
>>> Thanks, I was hoping for some automated static analysis tool.
>> This bug was found by an automated static analysis tool named DSAC,
>> which is written by myself.
>> Then I manually checked driver source code, and finally sent the bug report.
> Thanks.
> Would it be possible to put tool in tools directory and then have
> it automated by kbuild robot?
Hi,

Thanks for your appreciation of my tool :)

I think it is possible. But before releasing, I need to improve it to 
solve some problems:
1. It produces some false positives and negatives. Thus I need 
developer's response and confirmation of my bug reports, to help me to 
improve my tool's accuracy.
2. It is based on Clang-3.2 (a LLVM-based compiler, not GCC), and some 
driver code cannot compile normally. Maybe I should re-implement my tool 
based on a recent Clang version.
3. Some software is needed when running my tool, thus as MySQL (maybe I 
directly can write to files, instead of database in the future) and 
Clang (Clang is necessary).

I think it may be difficult to directly put my tool in "tools" 
directory. But with some configuration, it is possible to run it 
automatically in a testing environment.
I also plan to open this tool in the future, after finishing the 
improvements :)


Thanks,
Jia-Ju Bai