Re: [PATCH net-next] net/packet: Reset MAC header for direct packet transmission

[Kurt Kanzenbach <kurt@linutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

On Sun Mar 28 2021, Willem de Bruijn wrote:
> On Fri, Mar 26, 2021 at 11:49 AM Kurt Kanzenbach <kurt@linutronix.de> wrote:
>>
>> Reset MAC header in case of using packet_direct_xmit(), e.g. by specifying
>> PACKET_QDISC_BYPASS. This is needed, because other code such as the HSR layer
>> expects the MAC header to be correctly set.
>>
>> This has been observed using the following setup:
>>
>> |$ ip link add name hsr0 type hsr slave1 lan0 slave2 lan1 supervision 45 version 1
>> |$ ifconfig hsr0 up
>> |$ ./test hsr0
>>
>> The test binary is using mmap'ed sockets and is specifying the
>> PACKET_QDISC_BYPASS socket option.
>>
>> This patch resolves the following warning on a non-patched kernel:
>>
>> |[  112.725394] ------------[ cut here ]------------
>> |[  112.731418] WARNING: CPU: 1 PID: 257 at net/hsr/hsr_forward.c:560 hsr_forward_skb+0x484/0x568
>> |[  112.739962] net/hsr/hsr_forward.c:560: Malformed frame (port_src hsr0)
>>
>> The MAC header is also reset unconditionally in case of PACKET_QDISC_BYPASS is
>> not used.
>
> At the top of __dev_queue_xmit.
>
> I think it is reasonable to expect the mac header to be set in
> ndo_start_xmit. Not sure which other devices besides hsr truly
> requires it.
>
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>
> If this fixes a bug, it should target net.
>
> Fixes: d346a3fae3ff ("packet: introduce PACKET_QDISC_BYPASS socket
> option")

OK.

>
> This change belongs in __dev_direct_xmit unless all callers except
> packet_direct_xmit do correctly set the mac header. xsk_generic_xmit
> appears to miss it, too, so would equally trigger this warning.

It seems like there are only two callers and the XDP part is missing it
as well. I'll move it to __dev_direct_xmit().

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]