Message-ID: <5fe1e6eea00f33be59ae0aaf0429be08595dfc03.camel@sipsolutions.net>
Subject: Re: [PATCH v2] wifi: mac80211: validate link status before deciding
 on mgmt tx
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Sriram R <quic_srirrama@quicinc.com>
Cc:     linux-wireless@vger.kernel.org
Date:   Tue, 14 Feb 2023 13:45:53 +0100
In-Reply-To: <20230119221306.24526-1-quic_srirrama@quicinc.com>
References: <20230119221306.24526-1-quic_srirrama@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-01-20 at 03:43 +0530, Sriram R wrote:
> Currently we check the status of bss active flag to see if the
> AP is active.
>=20

Following so far :)

> But in case of a MLD AP, when some of the links
> are getting teardown=C2=A0

"getting torn down"?

> and some are active, mgmt Tx(like deauth)
> can be sent on some links before they are brought down as well.

Makes sense.

> In such cases, the bss active flag might not provide the exact
> status of the MLD links, which becomes false on first link deleted.

Wait, isn't that already a bug?

> Hence check if any of the links can be used for mgmt tx
> before returning error status.
>=20
> Also, use the link id passed from userspace when the link bss
> address matches the mgmt SA and the chan params match the request.
> This will avoid scenario where the link id from userspace
> gets reset.

"gets reset"??

> =20
> +static bool ieee80211_is_link_bss_active(struct ieee80211_sub_if_data *s=
data,
> +					 int link_id)
> +{
[...]
> +	sdata_lock(sdata);
> +	link =3D sdata_dereference(sdata->link[link_id], sdata);
> +	if (!link) {
> +		sdata_unlock(sdata);
> +		return false;
> +	}
> +
> +	if (sdata_dereference(link->u.ap.beacon, sdata)) {
> +		sdata_unlock(sdata);
> +		return true;
> +	}
> +
> +	sdata_unlock(sdata);

The locking here is ... decidedly odd. It feels like with all the
wdev_lock()ing going on in cfg80211_mlme_mgmt_tx() we should probably
just lock around the *entire* thing in cfg80211, including the
driver/mac80211 call?

> @@ -883,8 +920,17 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wi=
reless_dev *wdev,
>  				break;
>  			}
> =20
> -			if (ether_addr_equal(conf->addr, mgmt->sa))
> +			if (ether_addr_equal(conf->addr, mgmt->sa)) {
> +				/* If userspace requested Tx on a specific link
> +				 * use the same link id if the link bss is matching
> +				 * the requested chan.
> +				 */
> +				if (sdata->vif.valid_links &&
> +				    params->link_id >=3D 0 && params->link_id =3D=3D i &&
> +				    params->chan =3D=3D chanctx_conf->def.chan)
> +					link_id =3D i;
>  				break;
> +			}


Not sure I get it, if it's bad (link ID doesn't match BSS) then
shouldn't we just reject it?

johannes