Re: [PATCH] wifi: mac80211: Fix ieee80211_convert_to_unicast() logic

[Jeff Johnson <quic_jjohnson@quicinc.com>]

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