Re: [PATCH] mac80211: fix race in ieee80211_register_hw()

Sumit Garg [off-list ref] writes:

A race condition leading to a kernel crash is observed during invocation
of ieee80211_register_hw() on a dragonboard410c device having wcn36xx
driver built as a loadable module along with a wifi manager in user-space
waiting for a wifi device (wlanX) to be active.

Sequence diagram for a particular kernel crash scenario:

    user-space  ieee80211_register_hw()  RX IRQ
    +++++++++++++++++++++++++++++++++++++++++++++
       |                    |             |
       |<---wlan0---wiphy_register()      |
       |----start wlan0---->|             |
       |                    |<---IRQ---(RX packet)
       |              Kernel crash        |
       |              due to unallocated  |
       |              workqueue.          |
       |                    |             |
       |       alloc_ordered_workqueue()  |
       |                    |             |
       |              Misc wiphy init.    |
       |                    |             |
       |            ieee80211_if_add()    |
       |                    |             |

As evident from above sequence diagram, this race condition isn't specific
to a particular wifi driver but rather the initialization sequence in
ieee80211_register_hw() needs to be fixed. So re-order the initialization
sequence and the updated sequence diagram would look like:

    user-space  ieee80211_register_hw()  RX IRQ
    +++++++++++++++++++++++++++++++++++++++++++++
       |                    |             |
       |       alloc_ordered_workqueue()  |
       |                    |             |
       |              Misc wiphy init.    |
       |                    |             |
       |<---wlan0---wiphy_register()      |
       |----start wlan0---->|             |
       |                    |<---IRQ---(RX packet)
       |                    |             |

5 more quoted lines

I have understood that no frames should be received until mac80211 calls
struct ieee80211_ops::start:

 * @start: Called before the first netdevice attached to the hardware
 *         is enabled. This should turn on the hardware and must turn on
 *         frame reception (for possibly enabled monitor interfaces.)
   
So I would claim that this is a bug in wcn36xx.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

On Mon, 2020-04-06 at 15:44 +0300, Kalle Valo wrote:

quoted

    user-space  ieee80211_register_hw()  RX IRQ
    +++++++++++++++++++++++++++++++++++++++++++++
       |                    |             |
       |<---wlan0---wiphy_register()      |
       |----start wlan0---->|             |
       |                    |<---IRQ---(RX packet)
       |              Kernel crash        |
       |              due to unallocated  |
       |              workqueue.          |

[snip]

I have understood that no frames should be received until mac80211 calls
struct ieee80211_ops::start:

 * @start: Called before the first netdevice attached to the hardware
 *         is enabled. This should turn on the hardware and must turn on
 *         frame reception (for possibly enabled monitor interfaces.)

True, but I think he's saying that you can actually add and configure an
interface as soon as the wiphy is registered?

The "wlan0" is kinda wrong there, should be "phy0" I guess, and then
interface added from iw?

johannes

On Mon, 6 Apr 2020 at 18:17, Johannes Berg [off-list ref] wrote:

On Mon, 2020-04-06 at 15:44 +0300, Kalle Valo wrote:

quoted

quoted

    user-space  ieee80211_register_hw()  RX IRQ
    +++++++++++++++++++++++++++++++++++++++++++++
       |                    |             |
       |<---wlan0---wiphy_register()      |
       |----start wlan0---->|             |
       |                    |<---IRQ---(RX packet)
       |              Kernel crash        |
       |              due to unallocated  |
       |              workqueue.          |

[snip]

quoted

I have understood that no frames should be received until mac80211 calls
struct ieee80211_ops::start:

 * @start: Called before the first netdevice attached to the hardware
 *         is enabled. This should turn on the hardware and must turn on
 *         frame reception (for possibly enabled monitor interfaces.)

True, but I think he's saying that you can actually add and configure an
interface as soon as the wiphy is registered?

Indeed, it's a call to "struct ieee80211_ops::start" just after wiphy
is registered that causes the frame to be received leading to RX IRQ.

The "wlan0" is kinda wrong there, should be "phy0" I guess, and then
interface added from iw?

Okay, will update the sequence diagram.

-Sumit

johannes