Re: [PATCH] cfg80211: Handle bss expiry during connection

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2019-04-26 at 16:05 +0530, Krishna Chaitanya wrote:

> > > +             /* Meanwhile if BSS is expired then add it back to the list as
> > > +              * we have just connected with it.
> > > +              */
> > > +             if (list_empty(&ibss->list))
> > > +                     cfg80211_bss_update(rdev, ibss, ibss->pub.signal);
> > 
> > But I think this adds *another* reference, which is wrong? We do need
> > one reference (the original one the driver gave us) but not a second
> > one?
> 
> This was assuming found will be false so refcnt will be reset and a fresh ref
> is taken.

Yes, it actually adds a new entry entirely, not refs the old entry. But
then you have a new entry returned from cfg80211_bss_update() with a
reference, and leak that reference in the code as written.

You probably need to unref the old one, and keep the new one to pass on
to the next function etc.

> > Maybe the copy we do in cfg80211_bss_update() isn't so bad, but then I'd
> > probably expose it without the signal_valid part and signal/timestamp
> > updates, and just copy the information raw there? However, the "if
> > (found)" part should never be possible here, right? Actually, maybe it
> > is, if we got a *new* entry in the meantime, but then we'd override the
> > newer information with the older, which is kinda bad?
> 
> If we get new information (scan during connection) the bss would not have
> expired right? so this code will not be triggered.

It could have expired and been re-added, no? Ok, I'll admit that's a
stretch since the driver is busy connecting and not scanning, but I
suppose it could happen with multiple VIFs etc?

> My initial stab was in similar lines, something like below: But as the
> bss is expired
> we need to udpate ts, signal and other fields anyway, so I went ahead
> with full update.

Why would we want to update ts, signal etc.? We just keep the old
information, and then the entry will be held for the duration of the
connection, presumably it'll get new updates while you're connected.

> +void cfg80211_add_expired_bss(struct cfg80211_registered_device *rdev,
> +                            struct cfg80211_internal_bss *new)
> +{
> +       if (!new)
> +               return;
> +
> +       spin_lock_bh(&rdev->bss_lock);
> +       new->ts = jiffies
> +       list_add_tail(&new->list, &rdev->bss_list);
> +       rdev->bss_entries++;
> +
> +       rb_insert_bss(rdev, new);
> +       rdev->bss_generation++;
> +
> +       bss_ref_get(rdev, new);
> +
> +       spin_unlock_bh(&rdev->bss_lock);
> +}

Yeah, I was thinking along those lines too, except there's a bit of an
issue with the hidden and non-transmitting lists?

johannes