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@xxxxxxxxxxx> 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
