Re: staging: r8188eu: how to handle nested mutex under spinlock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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/20220206225943.7848-1-fmdefrancesco@xxxxxxxxx/

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/20220208180426.27455-3-fmdefrancesco@xxxxxxxxx/

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





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux