| Message ID | 20210326154835.21296-1-kurt@linutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net/packet: Reset MAC header for direct packet transmission | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: john.ogness@linutronix.de orcohen@paloaltonetworks.com willemb@google.com wanghai38@huawei.com eyal.birger@gmail.com tannerlove@google.com xie.he.0141@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/header_inline | success | Link |
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.
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
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)); }
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(+)