* [PATCH net v2] net: Reset MAC header for direct packet transmission
@ 2021-03-29 7:17 Kurt Kanzenbach
2021-03-29 8:51 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Kurt Kanzenbach @ 2021-03-29 7:17 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Antoine Tenart, Eric Dumazet, Wei Wang, Cong Wang, Taehee Yoo,
Jesper Dangaard Brouer, Sebastian Andrzej Siewior,
Willem de Bruijn, netdev, Kurt Kanzenbach
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net v2] net: Reset MAC header for direct packet transmission 2021-03-29 7:17 [PATCH net v2] net: Reset MAC header for direct packet transmission Kurt Kanzenbach @ 2021-03-29 8:51 ` Eric Dumazet 2021-03-29 10:30 ` Kurt Kanzenbach 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2021-03-29 8:51 UTC (permalink / raw) To: Kurt Kanzenbach Cc: David S. Miller, Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Antoine Tenart, Wei Wang, Cong Wang, Taehee Yoo, Jesper Dangaard Brouer, Sebastian Andrzej Siewior, Willem de Bruijn, netdev On Mon, Mar 29, 2021 at 9:17 AM Kurt Kanzenbach <kurt@linutronix.de> wrote: > > 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 > 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; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: Reset MAC header for direct packet transmission 2021-03-29 8:51 ` Eric Dumazet @ 2021-03-29 10:30 ` Kurt Kanzenbach 2021-03-29 12:13 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Kurt Kanzenbach @ 2021-03-29 10:30 UTC (permalink / raw) To: Eric Dumazet Cc: David S. Miller, Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Antoine Tenart, Wei Wang, Cong Wang, Taehee Yoo, Jesper Dangaard Brouer, Sebastian Andrzej Siewior, Willem de Bruijn, netdev [-- Attachment #1: Type: text/plain, Size: 3027 bytes --] 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. > > 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? 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; - } - [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: Reset MAC header for direct packet transmission 2021-03-29 10:30 ` Kurt Kanzenbach @ 2021-03-29 12:13 ` Eric Dumazet 2021-03-29 12:41 ` Julian Wiedmann 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2021-03-29 12:13 UTC (permalink / raw) To: Kurt Kanzenbach Cc: David S. Miller, Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Antoine Tenart, Wei Wang, Cong Wang, Taehee Yoo, Jesper Dangaard Brouer, Sebastian Andrzej Siewior, Willem de Bruijn, netdev On Mon, Mar 29, 2021 at 12:30 PM Kurt Kanzenbach <kurt@linutronix.de> wrote: > > 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. 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. > > > > > 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. > > 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; > - } > - ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: Reset MAC header for direct packet transmission 2021-03-29 12:13 ` Eric Dumazet @ 2021-03-29 12:41 ` Julian Wiedmann 2021-03-29 13:17 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Julian Wiedmann @ 2021-03-29 12:41 UTC (permalink / raw) To: Eric Dumazet, Kurt Kanzenbach Cc: David S. Miller, Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Antoine Tenart, Wei Wang, Cong Wang, Taehee Yoo, Jesper Dangaard Brouer, Sebastian Andrzej Siewior, Willem de Bruijn, netdev On 29.03.21 14:13, Eric Dumazet wrote: > On Mon, Mar 29, 2021 at 12:30 PM Kurt Kanzenbach <kurt@linutronix.de> wrote: >> >> 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. > > 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: Reset MAC header for direct packet transmission 2021-03-29 12:41 ` Julian Wiedmann @ 2021-03-29 13:17 ` Eric Dumazet 0 siblings, 0 replies; 6+ messages in thread From: Eric Dumazet @ 2021-03-29 13:17 UTC (permalink / raw) To: Julian Wiedmann Cc: Kurt Kanzenbach, David S. Miller, Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Antoine Tenart, Wei Wang, Cong Wang, Taehee Yoo, Jesper Dangaard Brouer, Sebastian Andrzej Siewior, Willem de Bruijn, netdev On Mon, Mar 29, 2021 at 2:42 PM Julian Wiedmann <jwi@linux.ibm.com> wrote: > > On 29.03.21 14:13, Eric Dumazet wrote: > > On Mon, Mar 29, 2021 at 12:30 PM Kurt Kanzenbach <kurt@linutronix.de> wrote: > >> > >> 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. > > > > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-29 13:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-03-29 7:17 [PATCH net v2] net: Reset MAC header for direct packet transmission Kurt Kanzenbach 2021-03-29 8:51 ` Eric Dumazet 2021-03-29 10:30 ` Kurt Kanzenbach 2021-03-29 12:13 ` Eric Dumazet 2021-03-29 12:41 ` Julian Wiedmann 2021-03-29 13:17 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).