| Message ID | 20210329071716.12235-1-kurt@linutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: 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 |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: bjorn@kernel.org |
| 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: 10 this patch: 10 |
| 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, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 10 this patch: 10 |
| netdev/header_inline | success | Link |
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;
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; - } -
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; > - } > -
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?
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.
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;
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(+)