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
