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

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

[Kurt Kanzenbach]

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.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 net/packet/af_packet.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 118d585337d7..6325c9b7df38 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -241,6 +241,7 @@ static void __fanout_link(struct sock *sk, struct packet_sock *po);
 
 static int packet_direct_xmit(struct sk_buff *skb)
 {
+	skb_reset_mac_header(skb);
 	return dev_direct_xmit(skb, packet_pick_tx_queue(skb));
 }
 
-- 
2.20.1

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

[Willem de Bruijn]

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")

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.

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

[Kurt Kanzenbach]

[-- 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 --]