[PATCH net v2] net: Reset MAC header for direct packet transmission

Reset MAC header in case of using dev_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().

Fixes: d346a3fae3ff ("packet: introduce PACKET_QDISC_BYPASS socket option")
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---

Changes since v1:

 * Move skb_reset_mac_header() to __dev_direct_xmit()
 * Add Fixes tag
 * Target net tree

Previous versions:

 * https://lkml.kernel.org/netdev/20210326154835.21296-1-kurt@linutronix.de/

net/core/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index b4c67a5be606..b5088223dc57 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -4297,6 +4297,8 @@ int __dev_direct_xmit(struct sk_buff *skb, u16 queue_id)
 		     !netif_carrier_ok(dev)))
 		goto drop;
 
+	skb_reset_mac_header(skb);
+
 	skb = validate_xmit_skb_list(skb, dev, &again);
 	if (skb != orig_skb)
 		goto drop;

-- 
2.20.1

On Mon, Mar 29, 2021 at 9:17 AM Kurt Kanzenbach [off-list ref] wrote:

Note that last year, I addressed the issue differently in commit
96cc4b69581db68efc9749ef32e9cf8e0160c509
("macvlan: do not assume mac_header is set in macvlan_broadcast()")
(amended with commit 1712b2fff8c682d145c7889d2290696647d82dab
"macvlan: use skb_reset_mac_header() in macvlan_queue_xmit()")

My reasoning was that in TX path, when ndo_start_xmit() is called, MAC
header is essentially skb->data,
so I was hoping to _remove_ skb_reset_mac_header(skb) eventually from
the fast path (aka __dev_queue_xmit),
because most drivers do not care about MAC header, they just use skb->data.

I understand it is more difficult to review drivers instead of just
adding more code in  __dev_direct_xmit()

In hsr case, I do not really see why the existing check can not be
simply reworked ?

mac_header really makes sense in input path, when some layer wants to
get it after it has been pulled.

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index ed82a470b6e154be28d7e53be57019bccd4a964d..cda495cb1471e23e6666c1f2e9d27a01694f997f

100644

--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c

@@ -555,11 +555,7 @@ void hsr_forward_skb(struct sk_buff *skb, struct

hsr_port *port)
 {
        struct hsr_frame_info frame;

-       if (skb_mac_header(skb) != skb->data) {
-               WARN_ONCE(1, "%s:%d: Malformed frame (port_src %s)\n",
-                         __FILE__, __LINE__, port->dev->name);
-               goto out_drop;
-       }
+       skb_reset_mac_header(skb);

        if (fill_frame_info(&frame, skb, port) < 0)
                goto out_drop;

On Mon Mar 29 2021, Eric Dumazet wrote:

Note that last year, I addressed the issue differently in commit
96cc4b69581db68efc9749ef32e9cf8e0160c509
("macvlan: do not assume mac_header is set in macvlan_broadcast()")
(amended with commit 1712b2fff8c682d145c7889d2290696647d82dab
"macvlan: use skb_reset_mac_header() in macvlan_queue_xmit()")

My reasoning was that in TX path, when ndo_start_xmit() is called, MAC
header is essentially skb->data,
so I was hoping to _remove_ skb_reset_mac_header(skb) eventually from
the fast path (aka __dev_queue_xmit),
because most drivers do not care about MAC header, they just use skb->data.

I understand it is more difficult to review drivers instead of just
adding more code in  __dev_direct_xmit()

In hsr case, I do not really see why the existing check can not be
simply reworked ?

It can be reworked, no problem. I just thought it might be better to add
it to the generic code just in case there are more drivers suffering
from the issue.

hsr_forward_skb() has four call sites. Three of them make sure that the
header is properly set. The Tx path does not. So, maybe something like
below?

Thanks,
Kurt

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 7444ec6e298e..bfcdc75fc01e 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c

@@ -217,6 +217,7 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
        master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
        if (master) {
                skb->dev = master->dev;
+               skb_reset_mac_header(skb);
                hsr_forward_skb(skb, master);
        } else {
                atomic_long_inc(&dev->tx_dropped);

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index ed82a470b6e1..b218e4594009 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c

@@ -555,12 +555,6 @@ void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port)
 {
        struct hsr_frame_info frame;

-       if (skb_mac_header(skb) != skb->data) {
-               WARN_ONCE(1, "%s:%d: Malformed frame (port_src %s)\n",
-                         __FILE__, __LINE__, port->dev->name);
-               goto out_drop;
-       }
-

On Mon, Mar 29, 2021 at 12:30 PM Kurt Kanzenbach [off-list ref] wrote:

On Mon Mar 29 2021, Eric Dumazet wrote:

quoted

Note that last year, I addressed the issue differently in commit
96cc4b69581db68efc9749ef32e9cf8e0160c509
("macvlan: do not assume mac_header is set in macvlan_broadcast()")
(amended with commit 1712b2fff8c682d145c7889d2290696647d82dab
"macvlan: use skb_reset_mac_header() in macvlan_queue_xmit()")

My reasoning was that in TX path, when ndo_start_xmit() is called, MAC
header is essentially skb->data,
so I was hoping to _remove_ skb_reset_mac_header(skb) eventually from
the fast path (aka __dev_queue_xmit),
because most drivers do not care about MAC header, they just use skb->data.

I understand it is more difficult to review drivers instead of just
adding more code in  __dev_direct_xmit()

In hsr case, I do not really see why the existing check can not be
simply reworked ?

It can be reworked, no problem. I just thought it might be better to add
it to the generic code just in case there are more drivers suffering
from the issue.

Note that I have a similar issue pending in ipvlan.

Still, I think I prefer the non easy way to not add more stuff in fast path.

quoted

mac_header really makes sense in input path, when some layer wants to
get it after it has been pulled.

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index ed82a470b6e154be28d7e53be57019bccd4a964d..cda495cb1471e23e6666c1f2e9d27a01694f997f

100644

--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c

@@ -555,11 +555,7 @@ void hsr_forward_skb(struct sk_buff *skb, struct

hsr_port *port)
 {
        struct hsr_frame_info frame;

-       if (skb_mac_header(skb) != skb->data) {
-               WARN_ONCE(1, "%s:%d: Malformed frame (port_src %s)\n",
-                         __FILE__, __LINE__, port->dev->name);
-               goto out_drop;
-       }
+       skb_reset_mac_header(skb);

hsr_forward_skb() has four call sites. Three of them make sure that the
header is properly set. The Tx path does not. So, maybe something like
below?

Yes, this should be fine.

On 29.03.21 14:13, Eric Dumazet wrote:

On Mon, Mar 29, 2021 at 12:30 PM Kurt Kanzenbach [off-list ref] wrote:

quoted

On Mon Mar 29 2021, Eric Dumazet wrote:

quoted

Note that last year, I addressed the issue differently in commit
96cc4b69581db68efc9749ef32e9cf8e0160c509
("macvlan: do not assume mac_header is set in macvlan_broadcast()")
(amended with commit 1712b2fff8c682d145c7889d2290696647d82dab
"macvlan: use skb_reset_mac_header() in macvlan_queue_xmit()")

My reasoning was that in TX path, when ndo_start_xmit() is called, MAC
header is essentially skb->data,
so I was hoping to _remove_ skb_reset_mac_header(skb) eventually from
the fast path (aka __dev_queue_xmit),
because most drivers do not care about MAC header, they just use skb->data.

I understand it is more difficult to review drivers instead of just
adding more code in  __dev_direct_xmit()

In hsr case, I do not really see why the existing check can not be
simply reworked ?

It can be reworked, no problem. I just thought it might be better to add
it to the generic code just in case there are more drivers suffering
from the issue.

Note that I have a similar issue pending in ipvlan.

Still, I think I prefer the non easy way to not add more stuff in fast path.

Can we apply this fix (and propagate it to stable), and then remove the
skb_reset_mac_header() from _both_ xmit paths through net-next?

On Mon, Mar 29, 2021 at 2:42 PM Julian Wiedmann [off-list ref] wrote:

On 29.03.21 14:13, Eric Dumazet wrote:

quoted

On Mon, Mar 29, 2021 at 12:30 PM Kurt Kanzenbach [off-list ref] wrote:

quoted

On Mon Mar 29 2021, Eric Dumazet wrote:

quoted

Note that last year, I addressed the issue differently in commit
96cc4b69581db68efc9749ef32e9cf8e0160c509
("macvlan: do not assume mac_header is set in macvlan_broadcast()")
(amended with commit 1712b2fff8c682d145c7889d2290696647d82dab
"macvlan: use skb_reset_mac_header() in macvlan_queue_xmit()")

My reasoning was that in TX path, when ndo_start_xmit() is called, MAC
header is essentially skb->data,
so I was hoping to _remove_ skb_reset_mac_header(skb) eventually from
the fast path (aka __dev_queue_xmit),
because most drivers do not care about MAC header, they just use skb->data.

I understand it is more difficult to review drivers instead of just
adding more code in  __dev_direct_xmit()

In hsr case, I do not really see why the existing check can not be
simply reworked ?

It can be reworked, no problem. I just thought it might be better to add
it to the generic code just in case there are more drivers suffering
from the issue.

Note that I have a similar issue pending in ipvlan.

Still, I think I prefer the non easy way to not add more stuff in fast path.

Can we apply this fix (and propagate it to stable), and then remove the
skb_reset_mac_header() from _both_ xmit paths through net-next?

This is the plan, but as I said a full audit is needed.

Currently ipvlan still needs a fix.