Received-SPF: pass (google.com: domain of linux-wireless+bounces-5991-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <0496fb7e-53cc-476f-8052-985d82fd8d01@quicinc.com>
Date: Tue, 9 Apr 2024 11:32:10 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: mac80211: handle link ID during management Tx
Content-Language: en-US
To: Johannes Berg <johannes@sipsolutions.net>
CC: <linux-wireless@vger.kernel.org>, Sriram R <quic_srirrama@quicinc.com>
References: <20240326045242.3190611-1-quic_adisi@quicinc.com>
 <930cc3514cd73b837b9e818061215773444f0ecd.camel@sipsolutions.net>
From: Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <930cc3514cd73b837b9e818061215773444f0ecd.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 4/9/24 00:01, Johannes Berg wrote:
> On Tue, 2024-03-26 at 10:22 +0530, Aditya Kumar Singh wrote:
>> From: Sriram R <quic_srirrama@quicinc.com>
>>
>> During management Tx, when source address is same as the link conf's
>> address and even when userspace requested Tx on a specific link, link ID
>> is not set which leads to dropping of the frame later.
> 
> Sorry, don't understand. Please be more precise about the scenario and
> why it's dropped.

So here, as per existing code, link_id would be -1 still even when user 
space requested it and there is a channel to Tx on.

This -1 would be sent forward like this -

* ieee80211_tx_skb_tid()
    * __ieee80211_tx_skb_tid_band()       // band = 0
	Now, if your link address happen to be same as your MLD
	address, then mac80211 would put IEEE80211_LINK_UNSPECIFIED.
    	Looks like this results in mac80211 unable to set certain
	sta related fields in tx_control.

As a result when the packet arrives in driver, with various condition 
checks it fails and goes to either wrong path or dropped completely.

For example in mac80211_hw_sim driver,

Since link is unspecified, it goes in mac80211_hwsim_select_tx_link() 
but there this condition is hit -

if (WARN_ON_ONCE(!sta || !sta->valid_links))
	return &vif->bss_conf;

Following is the trace -
------------[ cut here ]------------
WARNING: CPU: 0 PID: 518 at 
drivers/net/wireless/virtual/mac80211_hwsim.c:1889 
mac80211_hwsim_tx+0x23e/0x7df
CPU: 0 PID: 518 Comm: hostapd Tainted: G        W 
6.8.0-rc7-g43e63eea1b51-dirty #140
Stack:
  6060f8e1 604af0ce 6003ba75 00000001
  6060f8e1 604c74ef 00000000 604cf3b0
  00000000 00000000 00000009 60044d6a
Call Trace:
  [<6003bcc9>] ? os_is_signal_stack+0x1b/0x27
  [<604c74ef>] ? _printk+0x0/0x9f
  [<60025b3b>] ? show_stack+0x13a/0x147
  [<604af0ce>] ? dump_stack_print_info+0xe4/0xf0
  [<6003ba75>] ? um_set_signals+0x0/0x3c
  [<604c74ef>] ? _printk+0x0/0x9f
  [<604cf3b0>] ? dump_stack_lvl+0x47/0x52
  [<60044d6a>] ? __warn+0xd5/0x107
  [<604c64ac>] ? warn_slowpath_fmt+0xd6/0xe2
  [<604c7586>] ? _printk+0x97/0x9f
  [<604c63d6>] ? warn_slowpath_fmt+0x0/0xe2
  [<60078561>] ? find_held_lock+0x3b/0x82
  [<6046b953>] ? ieee80211_tx_frags+0x1d8/0x27f
  [<6007aa1d>] ? lock_release+0x239/0x248
  [<60241801>] ? mac80211_hwsim_tx+0x23e/0x7df
  [<6046b9a0>] ? ieee80211_tx_frags+0x225/0x27f
  [<6046cd82>] ? invoke_tx_handlers_late+0x2da/0x7a3
  [<604c74ef>] ? _printk+0x0/0x9f
  [<604bf08b>] ? memcmp+0x0/0x21
  [<6046baf9>] ? __ieee80211_tx+0xff/0x147
  [<6046f658>] ? ieee80211_tx+0x11a/0x128
  [<60471742>] ? __ieee80211_tx_skb_tid_band+0x20d/0x227
  [<60471888>] ? ieee80211_tx_skb_tid+0x12c/0x148
  [<6044439c>] ? ieee80211_mgmt_tx+0x528/0x5c7


Later in mac80211_hwsim_tx(), channel is selected as NULL ultimately due 
to this and hence this condition is hit -

if (WARN(!channel, "TX w/o channel - queue = %d\n", txi->hw_queue)) {
	ieee80211_free_txskb(hw, skb);
	return;
}

Trace-
------------[ cut here ]------------
WARNING: CPU: 0 PID: 518 at 
drivers/net/wireless/virtual/mac80211_hwsim.c:2005 
mac80211_hwsim_tx+0x54e/0x7df
TX w/o channel - queue = 0
CPU: 0 PID: 518 Comm: hostapd Tainted: G        W 
6.8.0-rc7-g43e63eea1b51-dirty #140
Stack:
  6060f8e1 604af0ce 6003ba75 00000001
  6060f8e1 604c74ef 00000000 604cf3b0
  00000000 826d7560 00000009 60044d6a
Call Trace:
  [<6003bcc9>] ? os_is_signal_stack+0x1b/0x27
  [<604c74ef>] ? _printk+0x0/0x9f
  [<60025b3b>] ? show_stack+0x13a/0x147
  [<604af0ce>] ? dump_stack_print_info+0xe4/0xf0
  [<6003ba75>] ? um_set_signals+0x0/0x3c
  [<604c74ef>] ? _printk+0x0/0x9f
  [<604cf3b0>] ? dump_stack_lvl+0x47/0x52
  [<60044d6a>] ? __warn+0xd5/0x107
  [<604c64ac>] ? warn_slowpath_fmt+0xd6/0xe2
  [<604c63d6>] ? warn_slowpath_fmt+0x0/0xe2
  [<60078561>] ? find_held_lock+0x3b/0x82
  [<6046b953>] ? ieee80211_tx_frags+0x1d8/0x27f
  [<6007aa1d>] ? lock_release+0x239/0x248
  [<60241b11>] ? mac80211_hwsim_tx+0x54e/0x7df
  [<6046b9a0>] ? ieee80211_tx_frags+0x225/0x27f
  [<6046cd82>] ? invoke_tx_handlers_late+0x2da/0x7a3
  [<604c74ef>] ? _printk+0x0/0x9f
  [<604bf08b>] ? memcmp+0x0/0x21
  [<6046baf9>] ? __ieee80211_tx+0xff/0x147
  [<6046f658>] ? ieee80211_tx+0x11a/0x128
  [<60471742>] ? __ieee80211_tx_skb_tid_band+0x20d/0x227
  [<60471888>] ? ieee80211_tx_skb_tid+0x12c/0x148
  [<6044439c>] ? ieee80211_mgmt_tx+0x528/0x5c7
  [<6041bf86>] ? nl80211_tx_mgmt+0x298/0x319


> 
> Also, client mode, so wpa_s is actually sending a link-specific action
> frame?

As per current supplicant code I don't see it sending. This change is 
agnostic to supplicant or hostapd sending link id or not in action frame.

If user space has requested in a particular link and if channel is set 
in that link then use that link. This is what this change is trying to 
do. And all this for management frames only.