[BUG] intersil: p54: possible deadlock in p54_remove_interface() and p54_stop()

[BUG] intersil: p54: possible deadlock in p54_remove_interface() and p54_stop()

[Jia-Ju Bai]

Hello,

My static analysis tool reports a possible deadlock in the p54 driver in 
Linux 5.16:

p54_remove_interface()
   mutex_lock(&priv->conf_mutex); --> Line 262 (Lock A)
wait_for_completion_interruptible_timeout(&priv->beacon_comp, HZ); --> 
Line 271 (Wait X)

p54_stop()
   mutex_lock(&priv->conf_mutex); --> Line 208 (Lock A)
   p54p_stop() (call via priv->stop)
     p54_free_skb()
       p54_tx_qos_accounting_free()
         complete(&priv->beacon_comp); --> Line 230 (Wake X)

When p54_remove_interface() is executed, "Wait X" is performed by 
holding "Lock A". If p54_stop() is executed at this time, "Wake X" 
cannot be performed to wake up "Wait X" in p54_remove_interface(), 
because "Lock A" has been already hold by p54_remove_interface(), 
causing a possible deadlock.
I find that "Wait X" is performed with a timeout, to relieve the 
possible deadlock; but I think this timeout can cause inefficient execution.

I am not quite sure whether this possible problem is real and how to fix 
it if it is real.
Any feedback would be appreciated, thanks :)


Best wishes,
Jia-Ju Bai

Re: [BUG] intersil: p54: possible deadlock in p54_remove_interface() and p54_stop()

[Christian Lamparter]

Hi,

On 07/02/2022 16:31, Jia-Ju Bai wrote:
> Hello,
> 
> My static analysis tool reports a possible deadlock in the p54 driver in Linux 5.16:
> 
> p54_remove_interface()
>    mutex_lock(&priv->conf_mutex); --> Line 262 (Lock A)
> wait_for_completion_interruptible_timeout(&priv->beacon_comp, HZ); --> Line 271 (Wait X)
> 
> p54_stop()
>    mutex_lock(&priv->conf_mutex); --> Line 208 (Lock A)
>    p54p_stop() (call via priv->stop)
>      p54_free_skb()
>        p54_tx_qos_accounting_free()
>          complete(&priv->beacon_comp); --> Line 230 (Wake X)
> 
> When p54_remove_interface() is executed, "Wait X" is performed by holding "Lock A". 
> If p54_stop() is executed at this time, "Wake X" cannot be performed to wake 
> up "Wait X" in p54_remove_interface(), because "Lock A" has been already hold by
> p54_remove_interface(), causing a possible deadlock.
>
> I find that "Wait X" is performed with a timeout, to relieve the possible deadlock;
> but I think this timeout can cause inefficient execution.
> 
> I am not quite sure whether this possible problem is real and how to fix it if it is real.
> Any feedback would be appreciated, thanks :)

This has been such a long long time ago. But I think I found
the right documentation entry for you:

<https://www.kernel.org/doc/html/v5.16/driver-api/80211/mac80211.html> (scroll down a bit)
| remove_interface
| 	Notifies a driver that an interface is going down.
| 	>>The stop callback is called after this if it is the last interface
|	and no monitor interfaces are present.<<
|	(it goes on a bit. But I don't think there's anything important left)

The documentation tells you not to worry. p54_stop() and p54_remove_interface()
are being serialized by mac80211.

Cheers,
Christian