wifi: Check if socket flags are valid

The check, if a particular SO_* flag_bit is set, may give a wrong result
since sk_flags are part of a union and the union is used otherwise.
This happens, if a socket is not a full socket, like a request socket 
for example.

Add a check to verify, if the union is used for sk_flags.

This solution is taken from commit
e8a64bbaaad1 ("net/sched: taprio: Check if socket flags are valid").

Fixes: 76a853f86c97 ("wifi: free SKBTX_WIFI_STATUS skb tx_flags flag")

Signed-off-by: Bert Karwatzki <spasswolf@web.de>
---
 drivers/net/wireless/ath/wil6210/txrx.h     | 2 +-
 drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
 net/mac80211/mesh.c                         | 2 +-
 net/mac80211/tx.c                           | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)


I'm not sure if all sk_fullsock() checks are necessary, or if it can
be guessed from context if the socket is valid, though.

This has been tested for ~1h so far.

Bert Karwatzki

On Mon, May 19, 2025 at 12:46 AM Bert Karwatzki <spasswolf@web.de> wrote:
>
> The check, if a particular SO_* flag_bit is set, may give a wrong result
> since sk_flags are part of a union and the union is used otherwise.
> This happens, if a socket is not a full socket, like a request socket
> for example.
>
> Add a check to verify, if the union is used for sk_flags.
>
> This solution is taken from commit
> e8a64bbaaad1 ("net/sched: taprio: Check if socket flags are valid").
>
> Fixes: 76a853f86c97 ("wifi: free SKBTX_WIFI_STATUS skb tx_flags flag")
>

nit: empty line

> Signed-off-by: Bert Karwatzki <spasswolf@web.de>

Great! Thanks!

Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>

> ---
>  drivers/net/wireless/ath/wil6210/txrx.h     | 2 +-
>  drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
>  net/mac80211/mesh.c                         | 2 +-
>  net/mac80211/tx.c                           | 6 +++---
>  4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/txrx.h b/drivers/net/wireless/ath/wil6210/txrx.h
> index 33ccd0b248d4..91432b318ec2 100644
> --- a/drivers/net/wireless/ath/wil6210/txrx.h
> +++ b/drivers/net/wireless/ath/wil6210/txrx.h
> @@ -618,7 +618,7 @@ static inline bool wil_need_txstat(struct sk_buff *skb)
>         const u8 *da = wil_skb_get_da(skb);
>
>         return is_unicast_ether_addr(da) && skb->sk &&
> -              sock_flag(skb->sk, SOCK_WIFI_STATUS);
> +              sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS);
>  }
>
>  static inline void wil_consume_skb(struct sk_buff *skb, bool acked)
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 1485f949ad4e..973df2656238 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -913,7 +913,7 @@ mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
>
>         multicast = is_multicast_ether_addr(skb->data);
>
> -       if (unlikely(!multicast && skb->sk &&
> +       if (unlikely(!multicast && skb->sk && sk_fullsock(skb->sk) &&
>                      sock_flag(skb->sk, SOCK_WIFI_STATUS) &&
>                      priv->adapter->fw_api_ver == MWIFIEX_FW_V15))
>                 skb = mwifiex_clone_skb_for_tx_status(priv,
> diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
> index a381b4b756ea..11b6cb639ae7 100644
> --- a/net/mac80211/mesh.c
> +++ b/net/mac80211/mesh.c
> @@ -777,7 +777,7 @@ bool ieee80211_mesh_xmit_fast(struct ieee80211_sub_if_data *sdata,
>         if (ethertype < ETH_P_802_3_MIN)
>                 return false;
>
> -       if (skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS))
> +       if (skb->sk && sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS))
>                 return false;
>
>         if (skb->ip_summed == CHECKSUM_PARTIAL) {
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 3b9392a6ddb2..8b5bcddd5cc9 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -2859,7 +2859,7 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
>         }
>
>         if (unlikely(!multicast &&
> -                    ((skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS)) ||
> +                    ((skb->sk && sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS)) ||
>                       ctrl_flags & IEEE80211_TX_CTL_REQ_TX_STATUS)))
>                 info_id = ieee80211_store_ack_skb(local, skb, &info_flags,
>                                                   cookie);
> @@ -3756,7 +3756,7 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata,
>                 return false;
>
>         /* don't handle TX status request here either */
> -       if (skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS))
> +       if (skb->sk && sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS))
>                 return false;
>
>         if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) {
> @@ -4648,7 +4648,7 @@ static void ieee80211_8023_xmit(struct ieee80211_sub_if_data *sdata,
>                         memcpy(IEEE80211_SKB_CB(seg), info, sizeof(*info));
>         }
>
> -       if (unlikely(skb->sk && sock_flag(skb->sk, SOCK_WIFI_STATUS))) {
> +       if (unlikely(skb->sk && sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS))) {
>                 info->status_data = ieee80211_store_ack_skb(local, skb,
>                                                             &info->flags, NULL);
>                 if (info->status_data)
> --
> 2.49.0
>
> I've dug a little into the history of sk_flags (which have been introduced in
> v4.4) and found commit e8a64bbaaad1 ("net/sched: taprio: Check if socket flags
> are valid"), which seems to address the same problem we're currently facing:

Indeed, sk_flags can only be used by a full socket.

Thanks,
Jason

>
> commit e8a64bbaaad1f6548cec5508297bc6d45e8ab69e
> Author: Benedikt Spranger <b.spranger@linutronix.de>
> Date:   Fri Apr 8 11:47:45 2022 +0200
>
>     net/sched: taprio: Check if socket flags are valid
>
>     A user may set the SO_TXTIME socket option to ensure a packet is send
>     at a given time. The taprio scheduler has to confirm, that it is allowed
>     to send a packet at that given time, by a check against the packet time
>     schedule. The scheduler drop the packet, if the gates are closed at the
>     given send time.
>
>     The check, if SO_TXTIME is set, may fail since sk_flags are part of an
>     union and the union is used otherwise. This happen, if a socket is not
>     a full socket, like a request socket for example.
>
>     Add a check to verify, if the union is used for sk_flags.
>
>     Fixes: 4cfd5779bd6e ("taprio: Add support for txtime-assist mode")
>     Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
>     Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
>     Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 377f896bdedc..b9c71a304d39 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -417,7 +417,8 @@ static int taprio_enqueue_one(struct sk_buff *skb, struct Qdisc *sch,
>  {
>         struct taprio_sched *q = qdisc_priv(sch);
>
> -       if (skb->sk && sock_flag(skb->sk, SOCK_TXTIME)) {
> +       /* sk_flags are only safe to use on full sockets. */
> +       if (skb->sk && sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_TXTIME)) {
>                 if (!is_valid_interval(skb, sch))
>                         return qdisc_drop(skb, sch, to_free);
>         } else if (TXTIME_ASSIST_IS_ENABLED(q->flags)) {
>
> I'm not sure if all sk_fullsock() checks are necessary, or if it can
> be guessed from context if the socket is valid, though.
>
> This has been tested for ~1h so far.
>
> Bert Karwatzki

Hi Bert,

Thank you _very_ much for doing all this debug. I was somewhat following
along on my phone, but was out over the weekend.

> +++ b/drivers/net/wireless/ath/wil6210/txrx.h
> @@ -618,7 +618,7 @@ static inline bool wil_need_txstat(struct sk_buff *skb)
>  	const u8 *da = wil_skb_get_da(skb);
>  
>  	return is_unicast_ether_addr(da) && skb->sk &&
> -	       sock_flag(skb->sk, SOCK_WIFI_STATUS);
> +	       sk_fullsock(skb->sk) && sock_flag(skb->sk, SOCK_WIFI_STATUS);

I feel like in line with sk_is_refcounted(), maybe we could add

static inline bool
sk_requests_wifi_status(struct sock *sk)
{
	return sk && sk_fullsock(sk) && sock_flag(sk,
SOCK_WIFI_STATUS);
}

perhaps?

That's basically all the checks in all the callers.

I can also just do that though if you don't want to resend.

johannes