Hi all,
smatch reported a sleeping in atomic context.
rtw_set_802_11_disassociate() <- disables preempt
-> _rtw_pwr_wakeup()
-> ips_leave()
rtw_set_802_11_disassociate() takes a spinlock and ips_leave() uses a
mutex.
I'm fairly new to the locking stuff, but as far as I know this is not a
false positive since mutex can sleep, but that's not allowed under a
spinlock.
What is the best way to handle this?
I'm not sure if converting the mutex to a spinlock (including all the
other places where the mutex is used) is the right thing to do?
thanks,
Michael
On 4/3/22 12:49, Fabio M. De Francesco wrote:
> On domenica 3 aprile 2022 12:43:04 CEST Fabio M. De Francesco wrote:
>> On sabato 2 aprile 2022 22:47:27 CEST Michael Straube wrote:
>>> Hi all,
>>>
>>> smatch reported a sleeping in atomic context.
>>>
>>> rtw_set_802_11_disassociate() <- disables preempt
>>> -> _rtw_pwr_wakeup()
>>> -> ips_leave()
>>>
>>> rtw_set_802_11_disassociate() takes a spinlock and ips_leave() uses a
>>> mutex.
>>>
>>> I'm fairly new to the locking stuff, but as far as I know this is not a
>>> false positive since mutex can sleep, but that's not allowed under a
>>> spinlock.
>>>
>>> What is the best way to handle this?
>>> I'm not sure if converting the mutex to a spinlock (including all the
>>> other places where the mutex is used) is the right thing to do?
>>>
>>> thanks,
>>> Michael
>>>
>> Hi Michael,
>>
>> No, this is a false positive: ips_leave is never called under spinlocks.
>> Some time ago I blindly trusted Smatch and submitted a patch for what you
>> are reporting just now again. Soon after submission I realized it and
>> then I had to ask Greg to discard my patch.
>>
>> Please read the related thread:
>>
>> [PATCH] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context
>> https://lore.kernel.org/lkml/[email protected]/
>>
>> Thanks,
>>
>> Fabio
>
> I'm sorry, the correct link is the following:
> [PATCH v2 2/2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context
> https://lore.kernel.org/lkml/[email protected]/
>
> Fabio
>
Hi Fabio,
Ah I see now, thanks. Well, I think the code is not very clear and easy
to follow here. Perhaps we should refactor this area someday to avoid
future confusions.
regards,
Michael
From: Pavel Skripkin
> Sent: 04 April 2022 17:39
>
> Hi David,
>
> On 4/4/22 11:50, David Laight wrote:
> >>
> >> > while (pwrpriv->bInSuspend &&
> >>
> >> I've looked into what gcc11 produced from this function and looks like
> >> my compiler is smart enough to not cache that value, but I am afraid not
> >> all compilers are that smart.
> >
> > The compiler can't cache the value because of the function call.
> >
>
> Hm, I am a newbie in compilers, so can you, please, explain (or give a
> link to any resource where I can read about it) how function call here
> prevent caching.
>
> IIUC compiler generates code that works well in scope of single-threaded
> application, so why can't compiler cache that value instead of accessing
> memory on each iteration... Isn't register access a way faster than even
> cache hit?
Because calls to external functions are allowed to change
any data via 'other' references.
For instance the structure pointer the function has could
also be in global data somewhere.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 4/4/22 15:33, Dan Carpenter wrote:
> On Sun, Apr 03, 2022 at 02:18:04PM +0200, Michael Straube wrote:
>> On 4/3/22 14:10, Fabio M. De Francesco wrote:
>>> For a list of all the paths to a given function you may use Smatch:
>>>
>>> ./smatch/smatch_data/db/smdb.py ips_leave
>>>
>>> or
>>>
>>> ./smatch/smatch_data/db/smdb.py call_tree ips_leave
>>>
>>> But perhaps you already know how to do it.
>>
>> Yes, but thank you anyway. :)
>>
>
> My email (gmail account) has been so weird recently. I don't know why
> I'm not getting Fabio's emails... Presumably they will show up in a few
> days.
>
> The other command to use is:
>
> $ ./smatch/smatch_data/db/smdb.py preempt ips_leave
> rtw_set_802_11_disassociate() <- disables preempt
> -> _rtw_pwr_wakeup()
> -> LeaveAllPowerSaveMode()
> -> ips_leave()
>
> I save that to a file, open it with with vim and run a vim function
> `hall` (for highlight all) from my .vimrc file.
>
> " Use :hall to highlight all the words in a file (for debugging sleeping bugs)
> function HLall()
> let a=[]
> %s/\w\+/\=add(a, submatch(0))/gn
> let @/ = join(a, "\\|")
> endfunction
> cnoreabbrev hall call HLall() <CR>:set hls<CR>
>
> That highlights all the functions is the call tree, then I use cscope to
> jump to rtw_set_802_11_disassociate and follow the highlighted functions
> to ips_leave().
Thank you for this hint Dan. :)
thanks,
Michael
Hi David,
On 4/4/22 11:50, David Laight wrote:
>>
>> > while (pwrpriv->bInSuspend &&
>>
>> I've looked into what gcc11 produced from this function and looks like
>> my compiler is smart enough to not cache that value, but I am afraid not
>> all compilers are that smart.
>
> The compiler can't cache the value because of the function call.
>
Hm, I am a newbie in compilers, so can you, please, explain (or give a
link to any resource where I can read about it) how function call here
prevent caching.
IIUC compiler generates code that works well in scope of single-threaded
application, so why can't compiler cache that value instead of accessing
memory on each iteration... Isn't register access a way faster than even
cache hit?
Thanks!
With regards,
Pavel Skripkin
Hi Fabio,
On 4/3/22 15:37, Fabio M. De Francesco wrote:
>> >
>> > drivers/staging/r8188eu/core/rtw_pwrctrl.c:379
>> >
>> > if (pwrpriv->ps_processing) {
>> > while (pwrpriv->ps_processing && rtw_get_passing_time_ms(start) <= 3000)
>> > msleep(10);
>> > }
>> >
>>
>> Hm, just wondering, shouldn't we annotate load from
>> pwrpriv->ps_processing with READ_ONCE() inside while loop?
>> IIUC compiler might want to cache first load into register and we will
>> stuck here forever.
>
> You're right. This can be cached. In situations like these one should use
> barriers or other API that use barriers implicitly (completions, for example).
>
Not sure about completions, since they may sleep.
Also, don't think that barriers are needed here, since this code just
waiting for observing value 1. Might be barrier will slightly speed up
waiting thread, but will also slow down other thread
> In this list, a couple of weeks ago, I suggested someone to use them. He/she
> re-submitted his/her patches with wait_for_completion() / complete(), I placed
> my Reviewed-by tag and Greg was happy to accept it and merge.
>
> Not sure about READ_ONCE(), because, as far as I know, it is used only to
> prevent load tearing. But I might be wrong here.
>
> Fabio
>
> P.S.: Is Paul's book still un-read? Unfortunately, I am not yet done :(
>
If I could read it 24/7 I will be the happiest person ever :) As you
might expect I am not yet done as well
With regards,
Pavel Skripkin
On 4/3/22 13:17, Fabio M. De Francesco wrote:
> On domenica 3 aprile 2022 13:08:35 CEST Michael Straube wrote:
>> On 4/3/22 12:49, Fabio M. De Francesco wrote:
>>> On domenica 3 aprile 2022 12:43:04 CEST Fabio M. De Francesco wrote:
>>>> On sabato 2 aprile 2022 22:47:27 CEST Michael Straube wrote:
>>>>> Hi all,
>>>>>
>>>>> smatch reported a sleeping in atomic context.
>>>>>
>>>>> rtw_set_802_11_disassociate() <- disables preempt
>>>>> -> _rtw_pwr_wakeup()
>>>>> -> ips_leave()
>>>>>
>>>>> rtw_set_802_11_disassociate() takes a spinlock and ips_leave() uses a
>>>>> mutex.
>>>>>
>>>>> I'm fairly new to the locking stuff, but as far as I know this is not a
>>>>> false positive since mutex can sleep, but that's not allowed under a
>>>>> spinlock.
>>>>>
>>>>> What is the best way to handle this?
>>>>> I'm not sure if converting the mutex to a spinlock (including all the
>>>>> other places where the mutex is used) is the right thing to do?
>>>>>
>>>>> thanks,
>>>>> Michael
>>>>>
>>>> Hi Michael,
>>>>
>>>> No, this is a false positive: ips_leave is never called under spinlocks.
>>>> Some time ago I blindly trusted Smatch and submitted a patch for what you
>>>> are reporting just now again. Soon after submission I realized it and
>>>> then I had to ask Greg to discard my patch.
>>>>
>>>> Please read the related thread:
>>>>
>>>> [PATCH] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context
>>>> https://lore.kernel.org/lkml/[email protected]/
>>>>
>>>> Thanks,
>>>>
>>>> Fabio
>>>
>>> I'm sorry, the correct link is the following:
>>> [PATCH v2 2/2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context
>>> https://lore.kernel.org/lkml/[email protected]/
>>>
>>> Fabio
>>>
>>
>> Hi Fabio,
>>
>> Ah I see now, thanks. Well, I think the code is not very clear and easy
>> to follow here. Perhaps we should refactor this area someday to avoid
>> future confusions.
>>
>> regards,
>> Michael
>>
> Soon after I sent the email above, I read yours anew. The issue I were trying
> to address were different than what you noticed today. I didn't even see that
> we were in nested mutexes under spinlocks and bottom halves disabled. I just
> saw those kmalloc() with GFP_KERNEL.
>
> You are noticing something one layer up. And you are right, this is a real
> issue. Larry's suggestion is the only correct one for fixing this.
>
> I've analyzed and reviewed some code in the tty layer that implements the
> same solution that Larry is talking about. Let me find the link and I'll
> soon send it to you, so that you might be inspired from that implementation.
>
> Sorry for the confusion.
>
> Thanks,
>
> Fabio
>
>
>
Hi Fabio,
wait..
rtw_set_802_11_disassociate() calls rtw_pwr_wakeup() only if
check_fwstate(pmlmepriv, _FW_LINKED) is true.
if (check_fwstate(pmlmepriv, _FW_LINKED)) {
rtw_disassoc_cmd(padapter, 0, true);
rtw_indicate_disconnect(padapter);
rtw_free_assoc_resources(padapter, 1);
rtw_pwr_wakeup(padapter);
}
in rtw_pwr_wakeup() there is the same check and if it is true the
function returns before calling ips_leave().
if (check_fwstate(pmlmepriv, _FW_LINKED)) {
ret = _SUCCESS;
goto exit;
}
if (rf_off == pwrpriv->rf_pwrstate) {
if (_FAIL == ips_leave(padapter)) {
ret = _FAIL;
goto exit;
}
}
So ips_leave() is not called in atomic context in this case and smatch
reports indeed a false positive, or am I wrong?
I have not checked the other calls to rtw_pwr_wakeup().
regards,
Michael
Hi Fabio,
On 4/3/22 15:55, Fabio M. De Francesco wrote:
> On domenica 3 aprile 2022 14:45:49 CEST Pavel Skripkin wrote:
>> Hi Fabio,
>>
>> On 4/3/22 15:37, Fabio M. De Francesco wrote:
>> >> >
>> >> > drivers/staging/r8188eu/core/rtw_pwrctrl.c:379
>> >> >
>> >> > if (pwrpriv->ps_processing) {
>> >> > while (pwrpriv->ps_processing && rtw_get_passing_time_ms(start) <= 3000)
>> >> > msleep(10);
>> >> > }
>> >> >
>> >>
>> >> Hm, just wondering, shouldn't we annotate load from
>> >> pwrpriv->ps_processing with READ_ONCE() inside while loop?
>> >> IIUC compiler might want to cache first load into register and we will
>> >> stuck here forever.
>> >
>> > You're right. This can be cached. In situations like these one should use
>> > barriers or other API that use barriers implicitly (completions, for example).
>> >
>>
>> Not sure about completions, since they may sleep.
>
> No completions in this special context. They for _sure_ might sleep. I was
> talking about general cases when you are in a loop and wait for status change.
>
>>
>> Also, don't think that barriers are needed here, since this code just
>> waiting for observing value 1. Might be barrier will slightly speed up
>> waiting thread, but will also slow down other thread
>
> Here, I cannot help with a 100% good answer. Maybe Greg wants to say something
> about it?
>
IMO, the best answer is just remove this loop, since it does nothing. Or
redesign it to be more sane
It waits for ps_processing to become 0 for 3000 ms, but if 3000 ms
expires... execution goes forward like as ps_processing was 0 from the
beginning
Maybe it's something hw related, like wait for 3000 ms and all will be
ok. Can't say...
With regards,
Pavel Skripkin
On 4/3/22 14:10, Fabio M. De Francesco wrote:
> For a list of all the paths to a given function you may use Smatch:
>
> ./smatch/smatch_data/db/smdb.py ips_leave
>
> or
>
> ./smatch/smatch_data/db/smdb.py call_tree ips_leave
>
> But perhaps you already know how to do it.
Yes, but thank you anyway. :)
Michael
Hi David,
On 4/4/22 19:59, David Laight wrote:
> From: Pavel Skripkin
>> Sent: 04 April 2022 17:39
>>
>> Hi David,
>>
>> On 4/4/22 11:50, David Laight wrote:
>> >>
>> >> > while (pwrpriv->bInSuspend &&
>> >>
>> >> I've looked into what gcc11 produced from this function and looks like
>> >> my compiler is smart enough to not cache that value, but I am afraid not
>> >> all compilers are that smart.
>> >
>> > The compiler can't cache the value because of the function call.
>> >
>>
>> Hm, I am a newbie in compilers, so can you, please, explain (or give a
>> link to any resource where I can read about it) how function call here
>> prevent caching.
>>
>> IIUC compiler generates code that works well in scope of single-threaded
>> application, so why can't compiler cache that value instead of accessing
>> memory on each iteration... Isn't register access a way faster than even
>> cache hit?
>
> Because calls to external functions are allowed to change
> any data via 'other' references.
> For instance the structure pointer the function has could
> also be in global data somewhere.
>
Make sense, thank you for explanation!
With regards,
Pavel Skripkin
Hi Michael,
On 4/2/22 23:47, Michael Straube wrote:
> Hi all,
>
> smatch reported a sleeping in atomic context.
>
> rtw_set_802_11_disassociate() <- disables preempt
> -> _rtw_pwr_wakeup()
> -> ips_leave()
>
> rtw_set_802_11_disassociate() takes a spinlock and ips_leave() uses a
> mutex.
>
> I'm fairly new to the locking stuff, but as far as I know this is not a
> false positive since mutex can sleep, but that's not allowed under a
> spinlock.
>
> What is the best way to handle this?
> I'm not sure if converting the mutex to a spinlock (including all the
> other places where the mutex is used) is the right thing to do?
>
I've looked into this like a month ago.
IMO, this code just need to be redesigned, since locking scheme is very
complicated there and, as smatch says, not correct.
Simple s/mutex_lock/spin_lock/ may work in that case, but one day
locking scheme should be reworked... Or just some code parts should be
dropped :))
With regards,
Pavel Skripkin
On Sun, Apr 03, 2022 at 02:18:04PM +0200, Michael Straube wrote:
> On 4/3/22 14:10, Fabio M. De Francesco wrote:
> > For a list of all the paths to a given function you may use Smatch:
> >
> > ./smatch/smatch_data/db/smdb.py ips_leave
> >
> > or
> >
> > ./smatch/smatch_data/db/smdb.py call_tree ips_leave
> >
> > But perhaps you already know how to do it.
>
> Yes, but thank you anyway. :)
>
My email (gmail account) has been so weird recently. I don't know why
I'm not getting Fabio's emails... Presumably they will show up in a few
days.
The other command to use is:
$ ./smatch/smatch_data/db/smdb.py preempt ips_leave
rtw_set_802_11_disassociate() <- disables preempt
-> _rtw_pwr_wakeup()
-> LeaveAllPowerSaveMode()
-> ips_leave()
I save that to a file, open it with with vim and run a vim function
`hall` (for highlight all) from my .vimrc file.
" Use :hall to highlight all the words in a file (for debugging sleeping bugs)
function HLall()
let a=[]
%s/\w\+/\=add(a, submatch(0))/gn
let @/ = join(a, "\\|")
endfunction
cnoreabbrev hall call HLall() <CR>:set hls<CR>
That highlights all the functions is the call tree, then I use cscope to
jump to rtw_set_802_11_disassociate and follow the highlighted functions
to ips_leave().
regards,
dan carpenter