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
