On Fri, 2023-12-22 at 10:10 +0100, Martin Kaistra wrote:
>
> Am 22.12.23 um 09:59 schrieb Ping-Ke Shih:
> > On Fri, 2023-12-22 at 09:25 +0100, Martin Kaistra wrote:
> > > Am 22.12.23 um 09:05 schrieb Martin Kaistra:
> > > > Am 22.12.23 um 02:45 schrieb Ping-Ke Shih:
> > > > > On Fri, Dec 22, 2023 at 12:45 AM Martin Kaistra
> > > > > <martin.kaistra@xxxxxxxxxxxxx> wrote:
> > > > > > Check first whether priv->vifs[0] exists and is of type STATION, then go
> > > > > > to priv->vifs[1]. Make sure to call refresh_rate_mask for both
> > > > > > interfaces.
> > > > > >
> > > > > > - if (vif && vif->type == NL80211_IFTYPE_STATION) {
> > > > > > int signal;
> > > > > > struct ieee80211_sta *sta;
> > > > > >
> > > > > > @@ -7215,22 +7219,21 @@ static void rtl8xxxu_watchdog_callback(struct
> > > > > > work_struct *work)
> > > > > >
> > > > > > dev_dbg(dev, "%s: no sta found\n", __func__);
> > > > > > rcu_read_unlock();
> > > > > > - goto out;
> > > > > > + continue;
> > > > > > }
> > > > > > rcu_read_unlock();
> > > > > >
> > > > > > signal = ieee80211_ave_rssi(vif);
> > > > > >
> > > > > > - priv->fops->report_rssi(priv, 0,
> > > > > > + priv->fops->report_rssi(priv, rtl8xxxu_get_macid(priv, sta),
> > > > > > rtl8xxxu_signal_to_snr(signal));
> > > > > >
> > > > > > - if (priv->fops->set_crystal_cap)
> > > > > > - rtl8xxxu_track_cfo(priv);
> > > > > > -
> > > > > > rtl8xxxu_refresh_rate_mask(priv, signal, sta, false);
> > > > >
> > > > > It seems like 'sta' isn't protected by RCU read lock.
> > > > > (an existing bug, not introduced by this patch)
> > > >
> > > > I will add a patch which moves the rcu_read_unlock() to fix this.
> > >
> > > Actually, looking at the code again, rtl8xxxu_refresh_rate_mask() seems to
> > > already contain calls to rcu_read_lock(). Just the call to
> > > rtl8xxxu_get_macid(priv, sta) in there might need additional protection.
> > >
> >
> > We must use RCU lock to protect 'sta' from dereference to whole access, but
> > it looks like
> >
> > rtl8xxxu_watchdog_callback()
> >
> > rcu_read_lock();
> > sta = ...
> > rcu_read_unlock() // after this point, use 'sta' is unsafe..
> >
> > rtl8xxxu_refresh_rate_mask(sta)
> > rcu_read_lock();
> > rate_bitmap = sta->...
> > rcu_read_unlock();
> >
> >
> If it is not an easy fix, does it make sense to you to do this also separately
> from this series?
Sure. As I said, this isn't introduced by this patchset. We can fix it later.
