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: <6fa6b1b62f6a1bc945708cca9e27136f1737386f.camel@sipsolutions.net>
Subject: Re: [PATCH v2 2/2] mac80211: Support POWERED_ADDR_CHANGE feature
From:   Johannes Berg <johannes@sipsolutions.net>
To:     James Prestwood <prestwoj@gmail.com>,
        linux-wireless@vger.kernel.org
Date:   Thu, 25 Aug 2022 20:42:32 +0200
In-Reply-To: <1cdf35f95aca2a65d0d738544fb04079125b9581.camel@gmail.com>
References: <20220811231338.563794-1-prestwoj@gmail.com>
         <20220811231338.563794-3-prestwoj@gmail.com>
         <e898282f0766bdf0bd9cbb92d2a3551ff28cdf6b.camel@sipsolutions.net>
         <1cdf35f95aca2a65d0d738544fb04079125b9581.camel@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

Hi,
=C2=A0
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (live)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0drv_remove_interface(local, sdata);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret =3D eth_mac_addr(=
dev, sa);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (live)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0ret =3D drv_add_interface(local, sdata);
> > > =C2=A0
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (ret =3D=3D 0)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);
> > >=20
> >=20
> > 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().
>=20
> 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:
>=20
> 	if (live)
> 		drv_remove_interface(local, sdata);
> 	ret =3D eth_mac_addr(dev, sa);
> 	if (ret =3D=3D 0)
> 		memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);
>=20
> 	if (live)
> 		WARN_ON(drv_add_interface(local, sdata));
>=20
> 	return ret;

Seems reasonable. We could do something like

  err =3D drv_add_interface(...);
  if (err) {
     dev_close(...);
     ret =3D 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?

johannes