| Message ID | 20240815-ieee80211_convert_to_unicast-v1-1-648f0c195474@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: Fix ieee80211_convert_to_unicast() logic | expand |
Jeff Johnson <quic_jjohnson@quicinc.com> writes: > The current logic in ieee80211_convert_to_unicast() uses skb_clone() > to obtain an skb for each individual destination of a multicast > frame, and then updates the destination address in the cloned skb's > data buffer before placing that skb on the provided queue. > > This logic is flawed since skb_clone() shares the same data buffer > with the original and the cloned skb, and hence each time the > destination address is updated, it overwrites the previous destination > address in this shared buffer. As a result, due to the special handing > of the first valid destination, all of the skbs will eventually be > sent to that first destination. Did you actually observe this happen in practice? ieee80211_change_da() does an skb_ensure_writable() check on the Ethernet header before writing it, so AFAICT it does not, in fact, overwrite the data of the original frame. > Fix this issue by using skb_copy() instead of skb_clone(). This will > result in a duplicate data buffer being allocated for each > destination, and hence each skb will be transmitted to the proper > destination. Cf the above, it seems this change will just lead to more needless copying. -Toke
On 8/16/2024 4:31 AM, Toke Høiland-Jørgensen wrote: > Jeff Johnson <quic_jjohnson@quicinc.com> writes: > >> The current logic in ieee80211_convert_to_unicast() uses skb_clone() >> to obtain an skb for each individual destination of a multicast >> frame, and then updates the destination address in the cloned skb's >> data buffer before placing that skb on the provided queue. >> >> This logic is flawed since skb_clone() shares the same data buffer >> with the original and the cloned skb, and hence each time the >> destination address is updated, it overwrites the previous destination >> address in this shared buffer. As a result, due to the special handing >> of the first valid destination, all of the skbs will eventually be >> sent to that first destination. > > Did you actually observe this happen in practice? ieee80211_change_da() > does an skb_ensure_writable() check on the Ethernet header before > writing it, so AFAICT it does not, in fact, overwrite the data of the > original frame. I'm proxying this change for our internal team, and they have observed that unicast frames are not being sent to the separate STAs. In response to your reply I went through the code again and it seems the manner in which this functionality fails isn't as it was described to me. Instead this functionality fails because we'd fail on the first ieee80211_change_da() call and hence goto multicast and where only the original skb would be queued and transmitted as a multicast frame So the original logic is still faulty, only the actual faulty behavior is not being described correctly: instead of sending multiple unicast frames to the same STA we'd send a single multicast frame. >> Fix this issue by using skb_copy() instead of skb_clone(). This will >> result in a duplicate data buffer being allocated for each >> destination, and hence each skb will be transmitted to the proper >> destination. > > Cf the above, it seems this change will just lead to more needless > copying. What other way is there to implement this multicast to unicast functionality? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ebceec860fc370b2f4c23e95c51daa932e047913 All of this logic exists to support that feature. Also note MLO multicast uses the skb_copy() methodology as well. /jeff
On 16.08.24 16:30, Jeff Johnson wrote: > On 8/16/2024 4:31 AM, Toke Høiland-Jørgensen wrote: >> Jeff Johnson <quic_jjohnson@quicinc.com> writes: >> >>> The current logic in ieee80211_convert_to_unicast() uses skb_clone() >>> to obtain an skb for each individual destination of a multicast >>> frame, and then updates the destination address in the cloned skb's >>> data buffer before placing that skb on the provided queue. >>> >>> This logic is flawed since skb_clone() shares the same data buffer >>> with the original and the cloned skb, and hence each time the >>> destination address is updated, it overwrites the previous destination >>> address in this shared buffer. As a result, due to the special handing >>> of the first valid destination, all of the skbs will eventually be >>> sent to that first destination. >> >> Did you actually observe this happen in practice? ieee80211_change_da() >> does an skb_ensure_writable() check on the Ethernet header before >> writing it, so AFAICT it does not, in fact, overwrite the data of the >> original frame. > > I'm proxying this change for our internal team, and they have observed that > unicast frames are not being sent to the separate STAs. > > In response to your reply I went through the code again and it seems the > manner in which this functionality fails isn't as it was described to me. > > Instead this functionality fails because we'd fail on the first > ieee80211_change_da() call and hence goto multicast and where only the > original skb would be queued and transmitted as a multicast frame > > So the original logic is still faulty, only the actual faulty behavior is not > being described correctly: instead of sending multiple unicast frames to the > same STA we'd send a single multicast frame. While I agree with switching over to skb_copy (or maybe pskb_copy to reduce realloc on fragmented skbs), it's still not clear to me why ieee80211_change_da fails. It should detect that the packet was cloned, letting pskb_expand_head reallocate the head of the skb. Please run some more tests to figure out the exact reason for the failure. There might be another hidden bug lurking there, which would get papered over by this change. - Felix
On 8/16/2024 10:54 AM, Felix Fietkau wrote: > On 16.08.24 16:30, Jeff Johnson wrote: >> On 8/16/2024 4:31 AM, Toke Høiland-Jørgensen wrote: >>> Jeff Johnson <quic_jjohnson@quicinc.com> writes: >>> >>>> The current logic in ieee80211_convert_to_unicast() uses skb_clone() >>>> to obtain an skb for each individual destination of a multicast >>>> frame, and then updates the destination address in the cloned skb's >>>> data buffer before placing that skb on the provided queue. >>>> >>>> This logic is flawed since skb_clone() shares the same data buffer >>>> with the original and the cloned skb, and hence each time the >>>> destination address is updated, it overwrites the previous destination >>>> address in this shared buffer. As a result, due to the special handing >>>> of the first valid destination, all of the skbs will eventually be >>>> sent to that first destination. >>> >>> Did you actually observe this happen in practice? ieee80211_change_da() >>> does an skb_ensure_writable() check on the Ethernet header before >>> writing it, so AFAICT it does not, in fact, overwrite the data of the >>> original frame. >> >> I'm proxying this change for our internal team, and they have observed that >> unicast frames are not being sent to the separate STAs. >> >> In response to your reply I went through the code again and it seems the >> manner in which this functionality fails isn't as it was described to me. >> >> Instead this functionality fails because we'd fail on the first >> ieee80211_change_da() call and hence goto multicast and where only the >> original skb would be queued and transmitted as a multicast frame >> >> So the original logic is still faulty, only the actual faulty behavior is not >> being described correctly: instead of sending multiple unicast frames to the >> same STA we'd send a single multicast frame. > > While I agree with switching over to skb_copy (or maybe pskb_copy to > reduce realloc on fragmented skbs), it's still not clear to me why > ieee80211_change_da fails. It should detect that the packet was cloned, > letting pskb_expand_head reallocate the head of the skb. > > Please run some more tests to figure out the exact reason for the > failure. There might be another hidden bug lurking there, which would > get papered over by this change. > > - Felix Thanks, I've thrown this back over to the development team to clarify. /jeff
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 72a9ba8bc5fd..0ee1c7df424c 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -4408,7 +4408,7 @@ ieee80211_convert_to_unicast(struct sk_buff *skb, struct net_device *dev, struct ieee80211_local *local = sdata->local; const struct ethhdr *eth = (struct ethhdr *)skb->data; struct sta_info *sta, *first = NULL; - struct sk_buff *cloned_skb; + struct sk_buff *copied_skb; rcu_read_lock(); @@ -4423,14 +4423,14 @@ ieee80211_convert_to_unicast(struct sk_buff *skb, struct net_device *dev, first = sta; continue; } - cloned_skb = skb_clone(skb, GFP_ATOMIC); - if (!cloned_skb) + copied_skb = skb_copy(skb, GFP_ATOMIC); + if (!copied_skb) goto multicast; - if (unlikely(ieee80211_change_da(cloned_skb, sta))) { - dev_kfree_skb(cloned_skb); + if (unlikely(ieee80211_change_da(copied_skb, sta))) { + dev_kfree_skb(copied_skb); goto multicast; } - __skb_queue_tail(queue, cloned_skb); + __skb_queue_tail(queue, copied_skb); } if (likely(first)) {
The current logic in ieee80211_convert_to_unicast() uses skb_clone() to obtain an skb for each individual destination of a multicast frame, and then updates the destination address in the cloned skb's data buffer before placing that skb on the provided queue. This logic is flawed since skb_clone() shares the same data buffer with the original and the cloned skb, and hence each time the destination address is updated, it overwrites the previous destination address in this shared buffer. As a result, due to the special handing of the first valid destination, all of the skbs will eventually be sent to that first destination. Fix this issue by using skb_copy() instead of skb_clone(). This will result in a duplicate data buffer being allocated for each destination, and hence each skb will be transmitted to the proper destination. Fixes: ebceec860fc3 ("mac80211: multicast to unicast conversion") Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> --- net/mac80211/tx.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) --- base-commit: ae98f5c9fd8ba84cd408b41faa77e65bf1b4cdfa change-id: 20240813-ieee80211_convert_to_unicast-1ddee968711d