Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a424a23753eb922c980d54414a0ad0f015f84e94.camel@gmail.com>
Subject: Re: [PATCH v2 2/2] mac80211: Support POWERED_ADDR_CHANGE feature
From:   James Prestwood <prestwoj@gmail.com>
To:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org
Date:   Thu, 25 Aug 2022 15:26:29 -0700
In-Reply-To: <6fa6b1b62f6a1bc945708cca9e27136f1737386f.camel@sipsolutions.net>
References: <20220811231338.563794-1-prestwoj@gmail.com>
         <20220811231338.563794-3-prestwoj@gmail.com>
         <e898282f0766bdf0bd9cbb92d2a3551ff28cdf6b.camel@sipsolutions.net>
         <1cdf35f95aca2a65d0d738544fb04079125b9581.camel@gmail.com>
         <6fa6b1b62f6a1bc945708cca9e27136f1737386f.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.4 (3.40.4-5.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 2022-08-25 at 20:42 +0200, Johannes Berg wrote:
> Hi,
>  
> > > > +       if (live)
> > > > +               drv_remove_interface(local, sdata);
> > > >         ret = eth_mac_addr(dev, sa);
> > > > +       if (live)
> > > > +               ret = drv_add_interface(local, sdata);
> > > >  
> > > >         if (ret == 0)
> > > >                 memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);
> > > > 
> > > 
> > > I still don't like the (lack of) error checking here. As far as I
> > > know,
> > > eth_mac_addr() can very happily fail if the passed address is
> > > invalid,
> > > so we really shouldn't overwrite the ret value by
> > > drv_add_interface().
> > 
> > Ah yes, that was an oversight. I assume we do want to add_interface
> > even if eth_mac_addr fails though.
> 
> Right.
> 
> > So my only question is about the
> > return from drv_add_interface(). Is this unlikely to fail? If so
> > would
> > just a WARN_ON be sufficient and return the value from
> > eth_mac_addr()?
> 
> Hm, yeah, I guess it really ought to not fail here.
> 
> > So something like:
> > 
> >         if (live)
> >                 drv_remove_interface(local, sdata);
> >         ret = eth_mac_addr(dev, sa);
> >         if (ret == 0)
> >                 memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);
> > 
> >         if (live)
> >                 WARN_ON(drv_add_interface(local, sdata));
> > 
> >         return ret;
> 
> Seems reasonable. We could do something like
> 
>   err = drv_add_interface(...);
>   if (err) {
>      dev_close(...);
>      ret = ret ?: err;
>   }
> 
> or something, but not sure that's worth it, it really shouldn't fail
> at
> this point.
> 
> But I guess we could leave setting
> NL80211_EXT_FEATURE_AUTH_TX_RANDOM_TA
> to the driver if we think it'd be less risky that way?

(I assume you meant this new POWERED_ADDR_CHANGE feature)

Anyways, is this something all mac80211 drivers _should_ be able to do
(I thought I remember you indicating this was the case)? I'd hate to
introduce (expose) a bug for some driver I couldn't test... but also
the WARN_ON would be very easy to pinpoint what happened.

IMO if mac80211-based drivers should be able to handle this, I think we
should keep it in mac80211. Any issues that arise would be a driver bug
that was exposed, and needs to be fixed anyways.

Ultimately its up to you. I would like to avoid doing it per-driver but
at the same time address randomization is completely broken on 6GHz so
if we can only do it for iwlwifi so be it.

Thanks,
James

> 
> johannes