Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190426095448.3169-1-chaitanya.mgit@gmail.com>
 <cb1c3fcafc7addf0ed8d8d946658801472ad8a1b.camel@sipsolutions.net>
 <CABPxzYK3MuJT=r3o6UgBqv-m1B2pqCACwq+i1DDRkC2ZHExvxQ@mail.gmail.com> <d569a6171e50a8bdb026a2ec86c78268c9d7e989.camel@sipsolutions.net>
In-Reply-To: <d569a6171e50a8bdb026a2ec86c78268c9d7e989.camel@sipsolutions.net>
From:   Krishna Chaitanya <chaitanya.mgit@gmail.com>
Date:   Fri, 26 Apr 2019 16:26:43 +0530
Message-ID: <CABPxzYKKMJ0PYwbA706qferBGPEitw26uG5Jp4qKzsaGPx+Dqg@mail.gmail.com>
Subject: Re: [PATCH] cfg80211: Handle bss expiry during connection
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>,
        Chaitanya Tata <chaitanya.tata@bluwireless.co.uk>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Fri, Apr 26, 2019 at 4:15 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> 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.
>

Right, we should unref after the update.

> > > 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?

Yes, it might be possible with multiple VIF's, when one VIF is connecting
and other VIF is scanning, it can find the connecting bssid in its scan results.

> > 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.

If we are holding it, then we will not be using ts anyway, so updating or
not should not matter. But as its old info better to leave it that way, and
update these fields only from scan results.

> > +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?
Ok, let me think for a bit and will send you V2. Thanks for quick review.
> johannes
>


-- 
Thanks,
Regards,
Chaitanya T K.