[RFC] staging: r8188eu: Sleeping in atomic context (SAC) bugs

From: Fabio M. De Francesco
Date: Sun Feb 06 2022 - 19:02:31 EST


Hello everyone,

I've noticed a couple of SAC bugs in r8188eu. Recently two of them have been
addressed by me and one by Michael Straube (sorry if I recall it wrongly).

Prof. Julia Lawall wrote with Others in a paper that "[] Code executing in
atomic context monopolizes a CPU core, and the progress of other threads that
need to concurrently access the same resources is delayed. Thus the code
execution in atomic context should complete as quickly as possible. Sleeping
in atomic context is forbidden, as it can block a CPU core for a long period
and may lead to a system hang. [] SAC bugs do not always cause problems in real
execution, and they are often hard to reproduce at runtime. Recent studies []
have shown that SAC bugs have caused serious system hangs at runtime. Thus,
it is necessary to detect and fix SAC bugs in kernel modules.".

Probably the phrase "SAC bugs do not always cause problems in real execution"
is the key to understand why very few people seems to care and fix them.

While working on my last patch for this driver, I noticed that a Mutex is
acquired while holding a Spinlock and while bottom halves are disabled.

In staging/r8188eu/core/rtw_ioctl_set.c, the function rtw_set_802_11_disassociate()
calls spin_lock_bh(). While holding that Spinlock, it calls _rtw_pwr_wakeup()
that, in turn, does a call to msleep().

My first question is whether or not msleep() can be called in atomic context. If
I understand its semantics and implementation it seems that it should be forbidden.
What about changing it to mdelay()? Again, it seems that mdelay() spins without
sleeping so it should be safe. Isn't it?

Furthermore, I noticed that _rtw_pwr_wakeup() calls ips_leave(). The latter
acquires the "pwrpriv->lock" Mutex. Aren't we forbidden to call Mutexes while
holding Spinlocks? That lock is used to protect struct "pwrctrl_priv". I didn't
yet provide a patch because I'm pretty sure that I'm missing the good and
correct reasons that led the author(s) to use a Mutex in that context...

My second question is: should we substitute that Mutex with a Spinlock and use
it everywhere else the struct "pwrctrl_priv" must be protected in the driver?

Looking forward to reading what you all think about these two questions of mine.

Regards,

Fabio M. De Francesco