[bug report] wifi: wilc1000: convert list management to RCU

[bug report] wifi: wilc1000: convert list management to RCU

[Dan Carpenter]

Hello Alexis Lothoré,

Commit f236464f1db7 ("wifi: wilc1000: convert list management to
RCU") from Apr 10, 2024 (linux-next), leads to the following Smatch
static checker warning:

	drivers/net/wireless/microchip/wilc1000/mon.c:236 wilc_wfi_init_mon_interface()
	warn: sleeping in atomic context

The problem is in the caller:

drivers/net/wireless/microchip/wilc1000/cfg80211.c
  1527  static struct wireless_dev *add_virtual_intf(struct wiphy *wiphy,
  1528                                               const char *name,
  1529                                               unsigned char name_assign_type,
  1530                                               enum nl80211_iftype type,
  1531                                               struct vif_params *params)
  1532  {
  1533          struct wilc *wl = wiphy_priv(wiphy);
  1534          struct wilc_vif *vif;
  1535          struct wireless_dev *wdev;
  1536          int iftype;
  1537  
  1538          if (type == NL80211_IFTYPE_MONITOR) {
  1539                  struct net_device *ndev;
  1540  
  1541                  rcu_read_lock();
                        ^^^^^^^^^^^^^^^
The patch changes this to disable preemption.

  1542                  vif = wilc_get_vif_from_type(wl, WILC_AP_MODE);
  1543                  if (!vif) {
  1544                          vif = wilc_get_vif_from_type(wl, WILC_GO_MODE);
  1545                          if (!vif) {
  1546                                  rcu_read_unlock();
  1547                                  goto validate_interface;
  1548                          }
  1549                  }
  1550  
  1551                  if (vif->monitor_flag) {
  1552                          rcu_read_unlock();
  1553                          goto validate_interface;
  1554                  }
  1555  
  1556                  ndev = wilc_wfi_init_mon_interface(wl, name, vif->ndev);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Does a few sleeping allocations.

  1557                  if (ndev) {
  1558                          vif->monitor_flag = 1;
  1559                  } else {
  1560                          rcu_read_unlock();
  1561                          return ERR_PTR(-EINVAL);
  1562                  }
  1563  
  1564                  wdev = &vif->priv.wdev;
  1565                  rcu_read_unlock();
  1566                  return wdev;
  1567          }

regards,
dan carpenter

Re: [bug report] wifi: wilc1000: convert list management to RCU

[Dan Carpenter]

A related warning is:

drivers/net/wireless/microchip/wilc1000/wlan.c:237
wilc_wlan_txq_filter_dup_tcp_ack() warn: sleeping in atomic context

drivers/net/wireless/microchip/wilc1000/wlan.c
   726          rcu_read_lock();
                ^^^^^^^^^^^^^^^
Disables preemption.

   727          wilc_for_each_vif(wilc, vif)
   728                  wilc_wlan_txq_filter_dup_tcp_ack(vif->ndev);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Sleeps.

   729          rcu_read_unlock();

regards,
dan carpenter

Re: [bug report] wifi: wilc1000: convert list management to RCU

[Alexis Lothoré]

Hello Dan,
sorry for the late reply

On 5/9/24 15:24, Dan Carpenter wrote:
> Hello Alexis Lothoré,
> 
> Commit f236464f1db7 ("wifi: wilc1000: convert list management to
> RCU") from Apr 10, 2024 (linux-next), leads to the following Smatch
> static checker warning:
> 
> 	drivers/net/wireless/microchip/wilc1000/mon.c:236 wilc_wfi_init_mon_interface()
> 	warn: sleeping in atomic context

I am struggling to reproduce the warning in smatch. I executed those basic steps:
- clone and build latest smatch
- checkout linux-next master branch
- export ARCH and CROSS_COMPILE
- load a defconfig enabling all features in wilc driver
- run ~/src/smatch/smatch_scripts/build_kernel_data.sh
- then run ~/src/smatch/smatch_scripts/test_kernel.sh
Yet I do no see any mention to wilc in the resulting smatch_warns.txt file. The
outcome is the same if I run smatch only on the wilc driver:
- ~/src/smatch/smatch_scripts/kchecker drivers/net/wireless/microchip/

Am I missing something obvious ?

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [bug report] wifi: wilc1000: convert list management to RCU

[Dan Carpenter]

On Mon, May 13, 2024 at 04:16:05PM +0200, Alexis Lothoré wrote:
> Hello Dan,
> sorry for the late reply
> 
> On 5/9/24 15:24, Dan Carpenter wrote:
> > Hello Alexis Lothoré,
> > 
> > Commit f236464f1db7 ("wifi: wilc1000: convert list management to
> > RCU") from Apr 10, 2024 (linux-next), leads to the following Smatch
> > static checker warning:
> > 
> > 	drivers/net/wireless/microchip/wilc1000/mon.c:236 wilc_wfi_init_mon_interface()
> > 	warn: sleeping in atomic context
> 
> I am struggling to reproduce the warning in smatch. I executed those basic steps:
> - clone and build latest smatch
> - checkout linux-next master branch
> - export ARCH and CROSS_COMPILE
> - load a defconfig enabling all features in wilc driver
> - run ~/src/smatch/smatch_scripts/build_kernel_data.sh
> - then run ~/src/smatch/smatch_scripts/test_kernel.sh
> Yet I do no see any mention to wilc in the resulting smatch_warns.txt file. The
> outcome is the same if I run smatch only on the wilc driver:
> - ~/src/smatch/smatch_scripts/kchecker drivers/net/wireless/microchip/
> 
> Am I missing something obvious ?

Yeah.  I'm sorry, I need to write a blog entry about this.  These checks
rely on the cross function database, and you need to rebuild it a bunch
of times.  It's a simple process but very time consuming.

Instead of that, it's better to do run time testing using
CONFIG_DEBUG_ATOMIC_SLEEP=y

regards,
dan carpenter

Re: [bug report] wifi: wilc1000: convert list management to RCU

[Alexis Lothoré]

On 5/14/24 10:54, Dan Carpenter wrote:
> On Mon, May 13, 2024 at 04:16:05PM +0200, Alexis Lothoré wrote:
>> Hello Dan,
>> sorry for the late reply
>>
>> On 5/9/24 15:24, Dan Carpenter wrote:
>>> Hello Alexis Lothoré,
>>>
>>> Commit f236464f1db7 ("wifi: wilc1000: convert list management to
>>> RCU") from Apr 10, 2024 (linux-next), leads to the following Smatch
>>> static checker warning:
>>>
>>> 	drivers/net/wireless/microchip/wilc1000/mon.c:236 wilc_wfi_init_mon_interface()
>>> 	warn: sleeping in atomic context
>>
>> I am struggling to reproduce the warning in smatch. I executed those basic steps:
>> - clone and build latest smatch
>> - checkout linux-next master branch
>> - export ARCH and CROSS_COMPILE
>> - load a defconfig enabling all features in wilc driver
>> - run ~/src/smatch/smatch_scripts/build_kernel_data.sh
>> - then run ~/src/smatch/smatch_scripts/test_kernel.sh
>> Yet I do no see any mention to wilc in the resulting smatch_warns.txt file. The
>> outcome is the same if I run smatch only on the wilc driver:
>> - ~/src/smatch/smatch_scripts/kchecker drivers/net/wireless/microchip/
>>
>> Am I missing something obvious ?
> 
> Yeah.  I'm sorry, I need to write a blog entry about this.  These checks
> rely on the cross function database, and you need to rebuild it a bunch
> of times.  It's a simple process but very time consuming.
> 
> Instead of that, it's better to do run time testing using
> CONFIG_DEBUG_ATOMIC_SLEEP=y

Yes, that's what I have been doing in the mean time, and it allowed me to at
least reproduce the second bug you raised, but I just wanted to make sure to
have suppressed those issues once I will have written the proper fixes. I'll
continue with the runtime checkers like CONFIG_DEBUG_ATOMIC_SLEEP :)

Thanks,

Alexis

> 
> regards,
> dan carpenter
> 

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [bug report] wifi: wilc1000: convert list management to RCU

[Alexis Lothoré]

Hello,

On 5/9/24 15:24, Dan Carpenter wrote:
> Hello Alexis Lothoré,
> 
> Commit f236464f1db7 ("wifi: wilc1000: convert list management to
> RCU") from Apr 10, 2024 (linux-next), leads to the following Smatch
> static checker warning:
> 
> 	drivers/net/wireless/microchip/wilc1000/mon.c:236 wilc_wfi_init_mon_interface()
> 	warn: sleeping in atomic context

My bad for the extensive delay to fix this. I have been in fact scratching my
head quite a lot around it. Adding Kalle and Ajay to keep them updated, and
possibly to get opinions.

This issue is indeed due to my recent series converting back SRCU to RCU in wilc
driver (submitted because at this time, there was no obvious reason nor
documentation about why SRCU was needed). By checking further the consequence, I
find in fact three issues, and I suspect those to be the original reason for
SRCU usage in the driver instead of classical RCU. All of them are reproducible
with runtime checkers enabled regarding RCU and sleeps in atomic sections,
either by triggering some heavy traffic for the first one, or raising an access
points for the two others:

  - one issue is in wilc_wfi_init_mon_interface (the initial warning raised by
Dan). This one:
    - parses the vif list (under RCU) to perform some checks, possibly canceling
    the interface registration
    - then registers the monitoring interface (has sleeping calls, especially a
register_netdevice call)
    - then if registration is successful, updates appropriate vif to flag it as
monitoring interface (then leave RCU section)
  I have a hotfix for this, but not a very satisfying one, which consists in
splitting the RCU section into two (first and third points), but additionally
using the vif list lock to make sure vif list has not been altered in-between.
IMHO this kind of fix just make things worse in the driver, just to tame RCU.

  - one issue is in wilc_wlan_txq_filter_dup_tcp_ack (the second warning raised
by Dan), which calls wait_for_completion_timeout while being in RCU critical
read. The issue can be properly fixed by just counting the number of packets to
be dropped inside the critical section, then effectively applying the multiple
wait_for_completion_timeout _after_ the RCU section.

  - finally, there is an issue in set_channel ops (cfg80211.c), which fetches
the first vif (under RCU), and then uses this vif to send appropriate channel
configuration command (which sleeps at some point). Since used vif here comes
from the vif list, I don't think it is safe to leave earlier RCU section (vif
pointer needs to remain valid until command is sent to chip).

Because of the second point which would bring a not-so-clean fix, and the third
one for which I still don't have a proper fix, I am considering to submit a
revert for my RCU conversion series, to come back to SRCU. I don´t know if those
issues do or do not make SRCU usage more legitimate, but at least I feel like
really fixing it need slightly larger rework. I will still search for better
options, but if I do not find any, I will submit the revert.

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [bug report] wifi: wilc1000: convert list management to RCU

[Kalle Valo]

Alexis Lothoré <alexis.lothore@bootlin.com> writes:

> Hello,
>
> On 5/9/24 15:24, Dan Carpenter wrote:
>> Hello Alexis Lothoré,
>> 
>> Commit f236464f1db7 ("wifi: wilc1000: convert list management to
>> RCU") from Apr 10, 2024 (linux-next), leads to the following Smatch
>> static checker warning:
>> 
>> 	drivers/net/wireless/microchip/wilc1000/mon.c:236
>> wilc_wfi_init_mon_interface()
>> 	warn: sleeping in atomic context
>
> My bad for the extensive delay to fix this. I have been in fact scratching my
> head quite a lot around it. Adding Kalle and Ajay to keep them updated, and
> possibly to get opinions.
>
> This issue is indeed due to my recent series converting back SRCU to RCU in wilc
> driver (submitted because at this time, there was no obvious reason nor
> documentation about why SRCU was needed). By checking further the consequence, I
> find in fact three issues, and I suspect those to be the original reason for
> SRCU usage in the driver instead of classical RCU. All of them are reproducible
> with runtime checkers enabled regarding RCU and sleeps in atomic sections,
> either by triggering some heavy traffic for the first one, or raising an access
> points for the two others:
>
>   - one issue is in wilc_wfi_init_mon_interface (the initial warning raised by
> Dan). This one:
>     - parses the vif list (under RCU) to perform some checks, possibly canceling
>     the interface registration
>     - then registers the monitoring interface (has sleeping calls, especially a
> register_netdevice call)
>     - then if registration is successful, updates appropriate vif to flag it as
> monitoring interface (then leave RCU section)
>   I have a hotfix for this, but not a very satisfying one, which consists in
> splitting the RCU section into two (first and third points), but additionally
> using the vif list lock to make sure vif list has not been altered in-between.
> IMHO this kind of fix just make things worse in the driver, just to tame RCU.
>
>   - one issue is in wilc_wlan_txq_filter_dup_tcp_ack (the second warning raised
> by Dan), which calls wait_for_completion_timeout while being in RCU critical
> read. The issue can be properly fixed by just counting the number of packets to
> be dropped inside the critical section, then effectively applying the multiple
> wait_for_completion_timeout _after_ the RCU section.
>
>   - finally, there is an issue in set_channel ops (cfg80211.c), which fetches
> the first vif (under RCU), and then uses this vif to send appropriate channel
> configuration command (which sleeps at some point). Since used vif here comes
> from the vif list, I don't think it is safe to leave earlier RCU section (vif
> pointer needs to remain valid until command is sent to chip).
>
> Because of the second point which would bring a not-so-clean fix, and the third
> one for which I still don't have a proper fix, I am considering to submit a
> revert for my RCU conversion series, to come back to SRCU. I don´t know if those
> issues do or do not make SRCU usage more legitimate, but at least I feel like
> really fixing it need slightly larger rework. I will still search for better
> options, but if I do not find any, I will submit the revert.

Thanks for the good summary.Maybe it's easier just to revert the commit
immediately so that you don't have to waste more time on this?
Especially if Ajay is missing.

But if would be nice if you could also include a separate patch which
documents in the code why SRCU is needed. Just to avoid duplicate work
later :)

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [bug report] wifi: wilc1000: convert list management to RCU

[Alexis Lothoré]

Hello Kalle,

On 5/28/24 11:44, Kalle Valo wrote:
> Alexis Lothoré <alexis.lothore@bootlin.com> writes:
>> Because of the second point which would bring a not-so-clean fix, and the third
>> one for which I still don't have a proper fix, I am considering to submit a
>> revert for my RCU conversion series, to come back to SRCU. I don´t know if those
>> issues do or do not make SRCU usage more legitimate, but at least I feel like
>> really fixing it need slightly larger rework. I will still search for better
>> options, but if I do not find any, I will submit the revert.
> 
> Thanks for the good summary.Maybe it's easier just to revert the commit
> immediately so that you don't have to waste more time on this?

Yes, I have used already quite some time for this, let's do as you suggest and
opt for the revert directly.

> Especially if Ajay is missing.
> 
> But if would be nice if you could also include a separate patch which
> documents in the code why SRCU is needed. Just to avoid duplicate work
> later :)

Sure, I'll make sure to include this.

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com