* [PATCH net 0/2] net/sched: sch_teql: fix shared headroom and header strip
@ 2026-09-29 10:04 Jamal Hadi Salim
2026-09-29 10:16 ` Jamal Hadi Salim
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jamal Hadi Salim @ 2026-09-29 10:04 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Victor Nogueira, Jiri Pirko, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, stable,
hybris, sashiko-bot
Two pre-existing defects on the teql transmit path, both around
teql_master_xmit() -> teql_resolve(). The first makes the slave's
headroom write corrupt a shared skb and strips a userspace-supplied link
header on retry; the second leaves skb->dev pointing at the teql master
while the skb is parked on a slave's neighbour arp_queue.
Patch 1 fixes the shared-headroom write and the header-strip on retry,
tracking the header length the slave actually added.
Patch 2 moves skb->dev = slave ahead of teql_resolve() and restores
skb->dev at the arp-queue reinjection point, so a parked skb names the
device whose neighbour took it and is delivered with a link header.
Both are follow-ups to commit dc4b95b8fee9 ("net/sched: sch_teql: restore
skb->dev on the slave failure path").
Jamal Hadi Salim (2):
net/sched: sch_teql: fix shared headroom and header strip on slave
retry
net/sched: sch_teql: keep skb->dev consistent on the arp-queue path
net/core/neighbour.c | 1 +
net/sched/sch_teql.c | 34 ++++++++++++++++++++++++++++------
2 files changed, 29 insertions(+), 6 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH net 0/2] net/sched: sch_teql: fix shared headroom and header strip 2026-09-29 10:04 [PATCH net 0/2] net/sched: sch_teql: fix shared headroom and header strip Jamal Hadi Salim @ 2026-09-29 10:16 ` Jamal Hadi Salim 2026-09-29 10:16 ` [PATCH net 1/2] net/sched: sch_teql: fix shared headroom and header strip on slave retry Jamal Hadi Salim 2026-09-29 10:16 ` [PATCH net 2/2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path Jamal Hadi Salim 2 siblings, 0 replies; 9+ messages in thread From: Jamal Hadi Salim @ 2026-09-29 10:16 UTC (permalink / raw) To: netdev Cc: Jamal Hadi Salim, Victor Nogueira, Jiri Pirko, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, stable, hybris, sashiko-bot Two pre-existing defects on the teql transmit path, both around teql_master_xmit() -> teql_resolve(). The first makes the slave's headroom write corrupt a shared skb and strips a userspace-supplied link header on retry; the second leaves skb->dev pointing at the teql master while the skb is parked on a slave's neighbour arp_queue. Patch 1 fixes the shared-headroom write and the header-strip on retry, tracking the header length the slave actually added. Patch 2 moves skb->dev = slave ahead of teql_resolve() and restores skb->dev at the arp-queue reinjection point, so a parked skb names the device whose neighbour took it and is delivered with a link header. Both are follow-ups to commit dc4b95b8fee9 ("net/sched: sch_teql: restore skb->dev on the slave failure path"). Jamal Hadi Salim (2): net/sched: sch_teql: fix shared headroom and header strip on slave retry net/sched: sch_teql: keep skb->dev consistent on the arp-queue path net/core/neighbour.c | 1 + net/sched/sch_teql.c | 34 ++++++++++++++++++++++++++++------ 2 files changed, 29 insertions(+), 6 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net 1/2] net/sched: sch_teql: fix shared headroom and header strip on slave retry 2026-09-29 10:04 [PATCH net 0/2] net/sched: sch_teql: fix shared headroom and header strip Jamal Hadi Salim 2026-09-29 10:16 ` Jamal Hadi Salim @ 2026-09-29 10:16 ` Jamal Hadi Salim 2026-09-30 13:07 ` netdev-bot+sashiko 2026-09-29 10:16 ` [PATCH net 2/2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path Jamal Hadi Salim 2 siblings, 1 reply; 9+ messages in thread From: Jamal Hadi Salim @ 2026-09-29 10:16 UTC (permalink / raw) To: netdev Cc: Jamal Hadi Salim, Victor Nogueira, Jiri Pirko, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, hybris, sashiko-bot teql_master_xmit() writes the link header of the current slave directly into the skb via teql_resolve(), and on retry undoes it with __skb_pull(skb, skb_network_offset(skb)). Two problems exist on retry: 1. When the skb is shared (for example a packet tap installed on the master makes xmit_one() clone it before teql_master_xmit()), dev_hard_header() writes into memory shared with the other clones and corrupts them. The headroom must be made private before the write. 2. The pull blindly removes skb_network_offset() bytes. For a frame that carries its own link header in the payload (AF_PACKET/SOCK_RAW with no dst entry), teql_resolve() returns 0 without adding a header, so the retry strips the userspace-supplied header and pushes a frame whose first bytes are payload. Fix this by calling skb_cow_head() before dev_hard_header(), and by pulling back only the header length that this slave actually added, tracked per iteration. An AF_PACKET/SOCK_RAW frame is then retried on the next slave with its header intact, and a shared skb is unshared before it is modified. This is a follow-up to commit dc4b95b8fee9 ("net/sched: sch_teql: restore skb->dev on the slave failure path"), which restored skb->dev to the master on the slave failure path but left these pre-existing defects on the same teql_resolve()/retry path. Conditions to recreate the bug: a teql master with at least two slaves, the first of which does not consume the skb, then send an AF_PACKET/SOCK_RAW frame (no dst entry) through the master: the frame arriving on the second slave's peer has its MAC header stripped. The shared-headroom write needs a packet tap on the master so the skb is cloned before teql_master_xmit(). Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org> Closes: https://sashiko.dev/#/patchset/20260824115928.4099988-1-victor@mojatatu.com Link: https://lore.kernel.org/netdev/20260824115928.4099988-1-victor@mojatatu.com/ Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> --- net/sched/sch_teql.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c index 409ce50cc0db..acd03f9afc6b 100644 --- a/net/sched/sch_teql.c +++ b/net/sched/sch_teql.c @@ -245,7 +245,7 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt, static int __teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res, struct net_device *dev, struct netdev_queue *txq, - struct dst_entry *dst) + struct dst_entry *dst, int *hlen) { struct neighbour *n; int err = 0; @@ -265,15 +265,28 @@ __teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res, } if (neigh_event_send(n, skb_res) == 0) { + int off = skb_network_offset(skb); char haddr[MAX_ADDR_LEN]; neigh_ha_snapshot(haddr, n, dev); + /* The skb may be shared (e.g. a packet tap clone); make the + * headroom private before dev_hard_header() writes into it. + */ + if (skb_cow_head(skb, LL_RESERVED_SPACE(dev)) < 0) { + err = -ENOMEM; + goto out; + } if (dev_hard_header(skb, dev, ntohs(skb_protocol(skb, false)), haddr, NULL, skb->len) < 0) err = -EINVAL; + /* The header, if any, is prepended above skb->data, so the + * network offset grew by exactly the bytes to undo later. + */ + *hlen = skb_network_offset(skb) - off; } else { err = (skb_res == NULL) ? -EAGAIN : 1; } +out: neigh_release(n); return err; } @@ -281,11 +294,13 @@ __teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res, static inline int teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res, struct net_device *dev, - struct netdev_queue *txq) + struct netdev_queue *txq, + int *hlen) { struct dst_entry *dst = skb_dst(skb); int res; + *hlen = 0; if (rcu_access_pointer(txq->qdisc) == &noop_qdisc) return -ENODEV; @@ -293,7 +308,7 @@ static inline int teql_resolve(struct sk_buff *skb, return 0; rcu_read_lock(); - res = __teql_resolve(skb, skb_res, dev, txq, dst); + res = __teql_resolve(skb, skb_res, dev, txq, dst, hlen); rcu_read_unlock(); return res; @@ -305,6 +320,7 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev) struct Qdisc *start, *q; int busy; int nores; + int hlen; int subq = skb_get_queue_mapping(skb); struct sk_buff *skb_res = NULL; @@ -332,7 +348,7 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev) continue; } - switch (teql_resolve(skb, skb_res, slave, slave_txq)) { + switch (teql_resolve(skb, skb_res, slave, slave_txq, &hlen)) { case 0: if (__netif_tx_trylock(slave_txq)) { unsigned int length = qdisc_pkt_len(skb); @@ -374,8 +390,14 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev) nores = 1; break; } + /* Undo only the header teql_resolve() pushed for this slave. + * Pulling skb_network_offset() instead would strip a + * userspace-supplied header when the slave added none, e.g. + * an AF_PACKET/SOCK_RAW frame with no dst entry. + */ + if (hlen > 0) + __skb_pull(skb, hlen); skb->dev = dev; - __skb_pull(skb, skb_network_offset(skb)); } while ((q = rcu_dereference(NEXT_SLAVE(q))) != start); if (nores && skb_res == NULL) { -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net 1/2] net/sched: sch_teql: fix shared headroom and header strip on slave retry 2026-09-29 10:16 ` [PATCH net 1/2] net/sched: sch_teql: fix shared headroom and header strip on slave retry Jamal Hadi Salim @ 2026-09-30 13:07 ` netdev-bot+sashiko 2026-10-03 10:43 ` Jamal Hadi Salim 2026-10-03 10:57 ` Jamal Hadi Salim 0 siblings, 2 replies; 9+ messages in thread From: netdev-bot+sashiko @ 2026-09-30 13:07 UTC (permalink / raw) To: jhs Cc: netdev, victor, jiri, davem, edumazet, kuba, pabeni, horms, hybris, sashiko-bot Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider. Critical: 0 (1) · High: 0 · Medium: 1 · Low: 0 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [Medium] The commit message and the new code comment give the wrong reason for problem 1 and the wrong way to reproduce it. Pre-existing issues: - [Critical] teql_master_xmit() in net/sched/sch_teql.c treats a slave transmit as successful only when `netdev_start_xmit(skb, slave,… -- net/sched: sch_teql: fix shared headroom and header strip on slave retry The patch adds a skb_cow_head() call before dev_hard_header() in __teql_resolve(). It also changes teql_master_xmit() so that, when a slave attempt fails, it pulls back only the header length that teql_resolve() pushed for that slave. It no longer pulls skb_network_offset() bytes. Link: https://lore.kernel.org/netdev/20260824115928.4099988-1-victor@mojatatu.com/ Closes: https://sashiko.dev/#/patchset/20260824115928.4099988-1-victor@mojatatu.com > diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c > index 409ce50cc0dbc..acd03f9afc6b8 100644 > --- a/net/sched/sch_teql.c > +++ b/net/sched/sch_teql.c [ ... ] > @@ -265,15 +265,28 @@ __teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res, > } > > if (neigh_event_send(n, skb_res) == 0) { > + int off = skb_network_offset(skb); > char haddr[MAX_ADDR_LEN]; > > neigh_ha_snapshot(haddr, n, dev); > + /* The skb may be shared (e.g. a packet tap clone); make the > + * headroom private before dev_hard_header() writes into it. > + */ > + if (skb_cow_head(skb, LL_RESERVED_SPACE(dev)) < 0) { [Severity: Medium] The skb_cow_head() call looks right. Are this comment and the commit message accurate about why it is needed? The commit message says: 1. When the skb is shared (for example a packet tap installed on the master makes xmit_one() clone it before teql_master_xmit()), and later: and a shared skb is unshared before it is modified. skb_cow_head() only handles skb_header_cloned(). It does nothing when skb_shared() is true (users > 1). If it has to expand the head, pskb_expand_head() has: BUG_ON(skb_shared(skb)); Would "header-cloned" be a more accurate term than "shared" in both places? Can the packet tap reproducer actually show corruption? teql_master_setup() never sets header_ops and uses ARPHRD_VOID, so dev_has_header() is false for the master. packet_rcv() and tpacket_rcv() only push back to the mac header when it is true: if (dev_has_header(dev)) { if (sk->sk_type != SOCK_DGRAM) skb_push(skb, skb->data - skb_mac_header(skb)); So the tap only sees bytes from skb->data onward. __teql_resolve() writes the slave header into [data - hlen, data), which the tap never reads. A trigger that does seem to cause visible corruption is a tc mirred mirror on a physical device's clsact egress, with the teql master as the target. sch_handle_egress() runs before skb_dst_drop(), so the clone keeps its dst. ARPHRD_VOID is not a mac-header-xmit type, so tcf_mirred_to_dev() pulls mac_len off the clone: expects_nh = want_ingress || !m_mac_header_xmit; at_nh = skb->data == skb_network_header(skb); if (at_nh != expects_nh) { ... skb_pull_rcsum(skb_to_send, mac_len); The original skb's Ethernet header then sits in the clone's shared headroom, and dev_hard_header() in teql overwrites it. Should the reproduction steps in the commit message describe this case instead? The subject and body also say both problems happen "on slave retry". The header write, and now the COW, happens on every slave attempt, including a first attempt that succeeds. Could the wording reflect that? [ ... ] > @@ -374,8 +390,14 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev) > nores = 1; > break; > } > + /* Undo only the header teql_resolve() pushed for this slave. > + * Pulling skb_network_offset() instead would strip a > + * userspace-supplied header when the slave added none, e.g. > + * an AF_PACKET/SOCK_RAW frame with no dst entry. > + */ > + if (hlen > 0) > + __skb_pull(skb, hlen); > skb->dev = dev; [Severity: Critical] This isn't a bug introduced by this patch, but can this __skb_pull() and the skb->dev write touch an skb that the slave has already consumed? teql_master_xmit() counts a slave transmit as successful only when it returns NETDEV_TX_OK: if (!netif_xmit_frozen_or_stopped(slave_txq) && netdev_start_xmit(skb, slave, slave_txq, false) == NETDEV_TX_OK) { Stacked slaves such as 8021q, and macvlan through macvlan_start_xmit(), return the lower device's dev_queue_xmit() result unchanged: net/8021q/vlan_dev.c:vlan_dev_hard_start_xmit() { ... ret = dev_queue_xmit(skb); ... return ret; } That result can be NET_XMIT_DROP, NET_XMIT_CN or a negative errno. dev_xmit_complete() treats all of these as consumed: if (likely(rc < NET_XMIT_MASK)) return true; For NET_XMIT_DROP the skb is already freed, for example in __dev_xmit_skb(): kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_BURST_DROP); return NET_XMIT_DROP; For NET_XMIT_CN it may still be queued on the lower qdisc. In both cases teql falls through to this block. For a VLAN slave with a resolved neighbour, hlen is ETH_HLEN + VLAN_HLEN, so __skb_pull() and skb->dev = dev write to the consumed skb. The loop then calls teql_resolve(), skb_cow_head(), dev_hard_header() and netdev_start_xmit() again on it, or it reaches the drop label: master->tx_dropped++; rcu_read_unlock(); dev_kfree_skb(skb); Would that be a double free? The function can also return NETDEV_TX_BUSY, and the core would then requeue a freed skb. The setup needs CAP_NET_ADMIN to attach teql as root on a VLAN or macvlan device. After that, ordinary congestion on the lower device is enough to hit this path. The rest of the series does not seem to fix it either; the final state still compares against NETDEV_TX_OK. Since this patch already reworks the post-failure skb handling on this path, would it make sense to check dev_xmit_complete() on the return value instead of comparing it with NETDEV_TX_OK? > } while ((q = rcu_dereference(NEXT_SLAVE(q))) != start); > > if (nores && skb_res == NULL) { -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v1.20260928064754%40mojatatu.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 1/2] net/sched: sch_teql: fix shared headroom and header strip on slave retry 2026-09-30 13:07 ` netdev-bot+sashiko @ 2026-10-03 10:43 ` Jamal Hadi Salim 2026-10-03 10:57 ` Jamal Hadi Salim 1 sibling, 0 replies; 9+ messages in thread From: Jamal Hadi Salim @ 2026-10-03 10:43 UTC (permalink / raw) To: netdev-bot+sashiko Cc: netdev, victor, jiri, davem, edumazet, kuba, pabeni, horms, hybris, sashiko-bot On Wed, Sep 30, 2026 at 9:07 AM <netdev-bot+sashiko@kernel.org> wrote: > > Thank you for your contribution! Sashiko AI review found 1 potential > issue(s) to consider. > > Critical: 0 (1) · High: 0 · Medium: 1 · Low: 0 > > If you believe none of the issues are legitimate, please reply to each > issue of Medium severity or higher and explain why the AI is wrong. > If any of them are legitimate and you plan to respin, please let patchwork > know by including "pw-bot: cr" as a separate line at the end of your reply > (one such reply per series is enough). > > - [Medium] The commit message and the new code comment give the wrong > reason for problem 1 and the wrong way to reproduce it. > > Pre-existing issues: > - [Critical] teql_master_xmit() in net/sched/sch_teql.c treats a slave > transmit as successful only when `netdev_start_xmit(skb, slave,… > We have captured this as a follouwup. > -- > > net/sched: sch_teql: fix shared headroom and header strip on slave retry > > The patch adds a skb_cow_head() call before dev_hard_header() in > __teql_resolve(). It also changes teql_master_xmit() so that, when a > slave attempt fails, it pulls back only the header length that > teql_resolve() pushed for that slave. It no longer pulls > skb_network_offset() bytes. > Given this is a doc only issue, I will not be resending this patch for this specific reason. But if i find you caught technical issues in patch2 I'll say "header-cloned", use the tc mirred mirror on a physical device's clsact egress -> teql master as the reproducer, and drop the "on slave retry" wording (the header write and the COW happen on every slave attempt, including a first attempt that succeeds) cheers, jamal > Link: https://lore.kernel.org/netdev/20260824115928.4099988-1-victor@mojatatu.com/ > Closes: https://sashiko.dev/#/patchset/20260824115928.4099988-1-victor@mojatatu.com > > > diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c > > index 409ce50cc0dbc..acd03f9afc6b8 100644 > > --- a/net/sched/sch_teql.c > > +++ b/net/sched/sch_teql.c > > [ ... ] > > > @@ -265,15 +265,28 @@ __teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res, > > } > > > > if (neigh_event_send(n, skb_res) == 0) { > > + int off = skb_network_offset(skb); > > char haddr[MAX_ADDR_LEN]; > > > > neigh_ha_snapshot(haddr, n, dev); > > + /* The skb may be shared (e.g. a packet tap clone); make the > > + * headroom private before dev_hard_header() writes into it. > > + */ > > + if (skb_cow_head(skb, LL_RESERVED_SPACE(dev)) < 0) { > > [Severity: Medium] > > The skb_cow_head() call looks right. Are this comment and the commit > message accurate about why it is needed? > > The commit message says: > > 1. When the skb is shared (for example a packet tap installed on the > master makes xmit_one() clone it before teql_master_xmit()), > > and later: > > and a shared skb is unshared before it is modified. > > skb_cow_head() only handles skb_header_cloned(). It does nothing when > skb_shared() is true (users > 1). If it has to expand the head, > pskb_expand_head() has: > > BUG_ON(skb_shared(skb)); > > Would "header-cloned" be a more accurate term than "shared" in both > places? > > Can the packet tap reproducer actually show corruption? > teql_master_setup() never sets header_ops and uses ARPHRD_VOID, so > dev_has_header() is false for the master. packet_rcv() and tpacket_rcv() > only push back to the mac header when it is true: > > if (dev_has_header(dev)) { > if (sk->sk_type != SOCK_DGRAM) > skb_push(skb, skb->data - skb_mac_header(skb)); > > So the tap only sees bytes from skb->data onward. __teql_resolve() > writes the slave header into [data - hlen, data), which the tap never > reads. > > A trigger that does seem to cause visible corruption is a tc mirred > mirror on a physical device's clsact egress, with the teql master as > the target. > > sch_handle_egress() runs before skb_dst_drop(), so the clone keeps its > dst. ARPHRD_VOID is not a mac-header-xmit type, so tcf_mirred_to_dev() > pulls mac_len off the clone: > > expects_nh = want_ingress || !m_mac_header_xmit; > at_nh = skb->data == skb_network_header(skb); > if (at_nh != expects_nh) { > ... > skb_pull_rcsum(skb_to_send, mac_len); > > The original skb's Ethernet header then sits in the clone's shared > headroom, and dev_hard_header() in teql overwrites it. > > Should the reproduction steps in the commit message describe this case > instead? > > The subject and body also say both problems happen "on slave retry". > The header write, and now the COW, happens on every slave attempt, > including a first attempt that succeeds. Could the wording reflect > that? > > [ ... ] > > > @@ -374,8 +390,14 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev) > > nores = 1; > > break; > > } > > + /* Undo only the header teql_resolve() pushed for this slave. > > + * Pulling skb_network_offset() instead would strip a > > + * userspace-supplied header when the slave added none, e.g. > > + * an AF_PACKET/SOCK_RAW frame with no dst entry. > > + */ > > + if (hlen > 0) > > + __skb_pull(skb, hlen); > > skb->dev = dev; > > [Severity: Critical] > > This isn't a bug introduced by this patch, but can this __skb_pull() > and the skb->dev write touch an skb that the slave has already > consumed? > > teql_master_xmit() counts a slave transmit as successful only when it > returns NETDEV_TX_OK: > > if (!netif_xmit_frozen_or_stopped(slave_txq) && > netdev_start_xmit(skb, slave, slave_txq, false) == > NETDEV_TX_OK) { > > Stacked slaves such as 8021q, and macvlan through macvlan_start_xmit(), > return the lower device's dev_queue_xmit() result unchanged: > > net/8021q/vlan_dev.c:vlan_dev_hard_start_xmit() { > ... > ret = dev_queue_xmit(skb); > ... > return ret; > } > > That result can be NET_XMIT_DROP, NET_XMIT_CN or a negative errno. > dev_xmit_complete() treats all of these as consumed: > > if (likely(rc < NET_XMIT_MASK)) > return true; > > For NET_XMIT_DROP the skb is already freed, for example in > __dev_xmit_skb(): > > kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_BURST_DROP); > return NET_XMIT_DROP; > > For NET_XMIT_CN it may still be queued on the lower qdisc. > > In both cases teql falls through to this block. For a VLAN slave with a > resolved neighbour, hlen is ETH_HLEN + VLAN_HLEN, so __skb_pull() and > skb->dev = dev write to the consumed skb. > > The loop then calls teql_resolve(), skb_cow_head(), dev_hard_header() > and netdev_start_xmit() again on it, or it reaches the drop label: > > master->tx_dropped++; > rcu_read_unlock(); > dev_kfree_skb(skb); > > Would that be a double free? The function can also return > NETDEV_TX_BUSY, and the core would then requeue a freed skb. > > The setup needs CAP_NET_ADMIN to attach teql as root on a VLAN or > macvlan device. After that, ordinary congestion on the lower device is > enough to hit this path. The rest of the series does not seem to fix it > either; the final state still compares against NETDEV_TX_OK. > > Since this patch already reworks the post-failure skb handling on this > path, would it make sense to check dev_xmit_complete() on the return > value instead of comparing it with NETDEV_TX_OK? > > > } while ((q = rcu_dereference(NEXT_SLAVE(q))) != start); > > > > if (nores && skb_res == NULL) { > > -- > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v1.20260928064754%40mojatatu.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 1/2] net/sched: sch_teql: fix shared headroom and header strip on slave retry 2026-09-30 13:07 ` netdev-bot+sashiko 2026-10-03 10:43 ` Jamal Hadi Salim @ 2026-10-03 10:57 ` Jamal Hadi Salim 2026-10-03 11:00 ` Jamal Hadi Salim 1 sibling, 1 reply; 9+ messages in thread From: Jamal Hadi Salim @ 2026-10-03 10:57 UTC (permalink / raw) To: netdev-bot+sashiko Cc: netdev, victor, jiri, davem, edumazet, kuba, pabeni, horms, hybris, sashiko-bot On Wed, Sep 30, 2026 at 9:07 AM <netdev-bot+sashiko@kernel.org> wrote: > > Thank you for your contribution! Sashiko AI review found 1 potential > issue(s) to consider. > > Critical: 0 (1) · High: 0 · Medium: 1 · Low: 0 > > If you believe none of the issues are legitimate, please reply to each > issue of Medium severity or higher and explain why the AI is wrong. > If any of them are legitimate and you plan to respin, please let patchwork > know by including "pw-bot: cr" as a separate line at the end of your reply > (one such reply per series is enough). > > - [Medium] The commit message and the new code comment give the wrong > reason for problem 1 and the wrong way to reproduce it. > > Pre-existing issues: > - [Critical] teql_master_xmit() in net/sched/sch_teql.c treats a slave > transmit as successful only when `netdev_start_xmit(skb, slave,… > Sorry, I missed responding to this: This is a different root cause from this patch series - we are going to send a followup fix if no one beats us to it. cheers, jamal > -- > > net/sched: sch_teql: fix shared headroom and header strip on slave retry > > The patch adds a skb_cow_head() call before dev_hard_header() in > __teql_resolve(). It also changes teql_master_xmit() so that, when a > slave attempt fails, it pulls back only the header length that > teql_resolve() pushed for that slave. It no longer pulls > skb_network_offset() bytes. > > Link: https://lore.kernel.org/netdev/20260824115928.4099988-1-victor@mojatatu.com/ > Closes: https://sashiko.dev/#/patchset/20260824115928.4099988-1-victor@mojatatu.com > > > diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c > > index 409ce50cc0dbc..acd03f9afc6b8 100644 > > --- a/net/sched/sch_teql.c > > +++ b/net/sched/sch_teql.c > > [ ... ] > > > @@ -265,15 +265,28 @@ __teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res, > > } > > > > if (neigh_event_send(n, skb_res) == 0) { > > + int off = skb_network_offset(skb); > > char haddr[MAX_ADDR_LEN]; > > > > neigh_ha_snapshot(haddr, n, dev); > > + /* The skb may be shared (e.g. a packet tap clone); make the > > + * headroom private before dev_hard_header() writes into it. > > + */ > > + if (skb_cow_head(skb, LL_RESERVED_SPACE(dev)) < 0) { > > [Severity: Medium] > > The skb_cow_head() call looks right. Are this comment and the commit > message accurate about why it is needed? > > The commit message says: > > 1. When the skb is shared (for example a packet tap installed on the > master makes xmit_one() clone it before teql_master_xmit()), > > and later: > > and a shared skb is unshared before it is modified. > > skb_cow_head() only handles skb_header_cloned(). It does nothing when > skb_shared() is true (users > 1). If it has to expand the head, > pskb_expand_head() has: > > BUG_ON(skb_shared(skb)); > > Would "header-cloned" be a more accurate term than "shared" in both > places? > > Can the packet tap reproducer actually show corruption? > teql_master_setup() never sets header_ops and uses ARPHRD_VOID, so > dev_has_header() is false for the master. packet_rcv() and tpacket_rcv() > only push back to the mac header when it is true: > > if (dev_has_header(dev)) { > if (sk->sk_type != SOCK_DGRAM) > skb_push(skb, skb->data - skb_mac_header(skb)); > > So the tap only sees bytes from skb->data onward. __teql_resolve() > writes the slave header into [data - hlen, data), which the tap never > reads. > > A trigger that does seem to cause visible corruption is a tc mirred > mirror on a physical device's clsact egress, with the teql master as > the target. > > sch_handle_egress() runs before skb_dst_drop(), so the clone keeps its > dst. ARPHRD_VOID is not a mac-header-xmit type, so tcf_mirred_to_dev() > pulls mac_len off the clone: > > expects_nh = want_ingress || !m_mac_header_xmit; > at_nh = skb->data == skb_network_header(skb); > if (at_nh != expects_nh) { > ... > skb_pull_rcsum(skb_to_send, mac_len); > > The original skb's Ethernet header then sits in the clone's shared > headroom, and dev_hard_header() in teql overwrites it. > > Should the reproduction steps in the commit message describe this case > instead? > > The subject and body also say both problems happen "on slave retry". > The header write, and now the COW, happens on every slave attempt, > including a first attempt that succeeds. Could the wording reflect > that? > > [ ... ] > > > @@ -374,8 +390,14 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev) > > nores = 1; > > break; > > } > > + /* Undo only the header teql_resolve() pushed for this slave. > > + * Pulling skb_network_offset() instead would strip a > > + * userspace-supplied header when the slave added none, e.g. > > + * an AF_PACKET/SOCK_RAW frame with no dst entry. > > + */ > > + if (hlen > 0) > > + __skb_pull(skb, hlen); > > skb->dev = dev; > > [Severity: Critical] > > This isn't a bug introduced by this patch, but can this __skb_pull() > and the skb->dev write touch an skb that the slave has already > consumed? > > teql_master_xmit() counts a slave transmit as successful only when it > returns NETDEV_TX_OK: > > if (!netif_xmit_frozen_or_stopped(slave_txq) && > netdev_start_xmit(skb, slave, slave_txq, false) == > NETDEV_TX_OK) { > > Stacked slaves such as 8021q, and macvlan through macvlan_start_xmit(), > return the lower device's dev_queue_xmit() result unchanged: > > net/8021q/vlan_dev.c:vlan_dev_hard_start_xmit() { > ... > ret = dev_queue_xmit(skb); > ... > return ret; > } > > That result can be NET_XMIT_DROP, NET_XMIT_CN or a negative errno. > dev_xmit_complete() treats all of these as consumed: > > if (likely(rc < NET_XMIT_MASK)) > return true; > > For NET_XMIT_DROP the skb is already freed, for example in > __dev_xmit_skb(): > > kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_BURST_DROP); > return NET_XMIT_DROP; > > For NET_XMIT_CN it may still be queued on the lower qdisc. > > In both cases teql falls through to this block. For a VLAN slave with a > resolved neighbour, hlen is ETH_HLEN + VLAN_HLEN, so __skb_pull() and > skb->dev = dev write to the consumed skb. > > The loop then calls teql_resolve(), skb_cow_head(), dev_hard_header() > and netdev_start_xmit() again on it, or it reaches the drop label: > > master->tx_dropped++; > rcu_read_unlock(); > dev_kfree_skb(skb); > > Would that be a double free? The function can also return > NETDEV_TX_BUSY, and the core would then requeue a freed skb. > > The setup needs CAP_NET_ADMIN to attach teql as root on a VLAN or > macvlan device. After that, ordinary congestion on the lower device is > enough to hit this path. The rest of the series does not seem to fix it > either; the final state still compares against NETDEV_TX_OK. > > Since this patch already reworks the post-failure skb handling on this > path, would it make sense to check dev_xmit_complete() on the return > value instead of comparing it with NETDEV_TX_OK? > > > } while ((q = rcu_dereference(NEXT_SLAVE(q))) != start); > > > > if (nores && skb_res == NULL) { > > -- > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v1.20260928064754%40mojatatu.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 1/2] net/sched: sch_teql: fix shared headroom and header strip on slave retry 2026-10-03 10:57 ` Jamal Hadi Salim @ 2026-10-03 11:00 ` Jamal Hadi Salim 0 siblings, 0 replies; 9+ messages in thread From: Jamal Hadi Salim @ 2026-10-03 11:00 UTC (permalink / raw) To: netdev-bot+sashiko Cc: netdev, victor, jiri, davem, edumazet, kuba, pabeni, horms, hybris, sashiko-bot On Sat, Oct 3, 2026 at 6:57 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > On Wed, Sep 30, 2026 at 9:07 AM <netdev-bot+sashiko@kernel.org> wrote: > > > > Thank you for your contribution! Sashiko AI review found 1 potential > > issue(s) to consider. > > > > Critical: 0 (1) · High: 0 · Medium: 1 · Low: 0 > > > > If you believe none of the issues are legitimate, please reply to each > > issue of Medium severity or higher and explain why the AI is wrong. > > If any of them are legitimate and you plan to respin, please let patchwork > > know by including "pw-bot: cr" as a separate line at the end of your reply > > (one such reply per series is enough). > > > > - [Medium] The commit message and the new code comment give the wrong > > reason for problem 1 and the wrong way to reproduce it. > > > > Pre-existing issues: > > - [Critical] teql_master_xmit() in net/sched/sch_teql.c treats a slave > > transmit as successful only when `netdev_start_xmit(skb, slave,… > > > > Sorry, I missed responding to this: > This is a different root cause from this patch series - we are going > to send a followup fix if no one beats us to it. > For completion: nothing on patch 2. So i dont see any reason to resend this series. cheers, jamal > cheers, > jamal > > > -- > > > > net/sched: sch_teql: fix shared headroom and header strip on slave retry > > > > The patch adds a skb_cow_head() call before dev_hard_header() in > > __teql_resolve(). It also changes teql_master_xmit() so that, when a > > slave attempt fails, it pulls back only the header length that > > teql_resolve() pushed for that slave. It no longer pulls > > skb_network_offset() bytes. > > > > Link: https://lore.kernel.org/netdev/20260824115928.4099988-1-victor@mojatatu.com/ > > Closes: https://sashiko.dev/#/patchset/20260824115928.4099988-1-victor@mojatatu.com > > > > > diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c > > > index 409ce50cc0dbc..acd03f9afc6b8 100644 > > > --- a/net/sched/sch_teql.c > > > +++ b/net/sched/sch_teql.c > > > > [ ... ] > > > > > @@ -265,15 +265,28 @@ __teql_resolve(struct sk_buff *skb, struct sk_buff *skb_res, > > > } > > > > > > if (neigh_event_send(n, skb_res) == 0) { > > > + int off = skb_network_offset(skb); > > > char haddr[MAX_ADDR_LEN]; > > > > > > neigh_ha_snapshot(haddr, n, dev); > > > + /* The skb may be shared (e.g. a packet tap clone); make the > > > + * headroom private before dev_hard_header() writes into it. > > > + */ > > > + if (skb_cow_head(skb, LL_RESERVED_SPACE(dev)) < 0) { > > > > [Severity: Medium] > > > > The skb_cow_head() call looks right. Are this comment and the commit > > message accurate about why it is needed? > > > > The commit message says: > > > > 1. When the skb is shared (for example a packet tap installed on the > > master makes xmit_one() clone it before teql_master_xmit()), > > > > and later: > > > > and a shared skb is unshared before it is modified. > > > > skb_cow_head() only handles skb_header_cloned(). It does nothing when > > skb_shared() is true (users > 1). If it has to expand the head, > > pskb_expand_head() has: > > > > BUG_ON(skb_shared(skb)); > > > > Would "header-cloned" be a more accurate term than "shared" in both > > places? > > > > Can the packet tap reproducer actually show corruption? > > teql_master_setup() never sets header_ops and uses ARPHRD_VOID, so > > dev_has_header() is false for the master. packet_rcv() and tpacket_rcv() > > only push back to the mac header when it is true: > > > > if (dev_has_header(dev)) { > > if (sk->sk_type != SOCK_DGRAM) > > skb_push(skb, skb->data - skb_mac_header(skb)); > > > > So the tap only sees bytes from skb->data onward. __teql_resolve() > > writes the slave header into [data - hlen, data), which the tap never > > reads. > > > > A trigger that does seem to cause visible corruption is a tc mirred > > mirror on a physical device's clsact egress, with the teql master as > > the target. > > > > sch_handle_egress() runs before skb_dst_drop(), so the clone keeps its > > dst. ARPHRD_VOID is not a mac-header-xmit type, so tcf_mirred_to_dev() > > pulls mac_len off the clone: > > > > expects_nh = want_ingress || !m_mac_header_xmit; > > at_nh = skb->data == skb_network_header(skb); > > if (at_nh != expects_nh) { > > ... > > skb_pull_rcsum(skb_to_send, mac_len); > > > > The original skb's Ethernet header then sits in the clone's shared > > headroom, and dev_hard_header() in teql overwrites it. > > > > Should the reproduction steps in the commit message describe this case > > instead? > > > > The subject and body also say both problems happen "on slave retry". > > The header write, and now the COW, happens on every slave attempt, > > including a first attempt that succeeds. Could the wording reflect > > that? > > > > [ ... ] > > > > > @@ -374,8 +390,14 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev) > > > nores = 1; > > > break; > > > } > > > + /* Undo only the header teql_resolve() pushed for this slave. > > > + * Pulling skb_network_offset() instead would strip a > > > + * userspace-supplied header when the slave added none, e.g. > > > + * an AF_PACKET/SOCK_RAW frame with no dst entry. > > > + */ > > > + if (hlen > 0) > > > + __skb_pull(skb, hlen); > > > skb->dev = dev; > > > > [Severity: Critical] > > > > This isn't a bug introduced by this patch, but can this __skb_pull() > > and the skb->dev write touch an skb that the slave has already > > consumed? > > > > teql_master_xmit() counts a slave transmit as successful only when it > > returns NETDEV_TX_OK: > > > > if (!netif_xmit_frozen_or_stopped(slave_txq) && > > netdev_start_xmit(skb, slave, slave_txq, false) == > > NETDEV_TX_OK) { > > > > Stacked slaves such as 8021q, and macvlan through macvlan_start_xmit(), > > return the lower device's dev_queue_xmit() result unchanged: > > > > net/8021q/vlan_dev.c:vlan_dev_hard_start_xmit() { > > ... > > ret = dev_queue_xmit(skb); > > ... > > return ret; > > } > > > > That result can be NET_XMIT_DROP, NET_XMIT_CN or a negative errno. > > dev_xmit_complete() treats all of these as consumed: > > > > if (likely(rc < NET_XMIT_MASK)) > > return true; > > > > For NET_XMIT_DROP the skb is already freed, for example in > > __dev_xmit_skb(): > > > > kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_BURST_DROP); > > return NET_XMIT_DROP; > > > > For NET_XMIT_CN it may still be queued on the lower qdisc. > > > > In both cases teql falls through to this block. For a VLAN slave with a > > resolved neighbour, hlen is ETH_HLEN + VLAN_HLEN, so __skb_pull() and > > skb->dev = dev write to the consumed skb. > > > > The loop then calls teql_resolve(), skb_cow_head(), dev_hard_header() > > and netdev_start_xmit() again on it, or it reaches the drop label: > > > > master->tx_dropped++; > > rcu_read_unlock(); > > dev_kfree_skb(skb); > > > > Would that be a double free? The function can also return > > NETDEV_TX_BUSY, and the core would then requeue a freed skb. > > > > The setup needs CAP_NET_ADMIN to attach teql as root on a VLAN or > > macvlan device. After that, ordinary congestion on the lower device is > > enough to hit this path. The rest of the series does not seem to fix it > > either; the final state still compares against NETDEV_TX_OK. > > > > Since this patch already reworks the post-failure skb handling on this > > path, would it make sense to check dev_xmit_complete() on the return > > value instead of comparing it with NETDEV_TX_OK? > > > > > } while ((q = rcu_dereference(NEXT_SLAVE(q))) != start); > > > > > > if (nores && skb_res == NULL) { > > > > -- > > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v1.20260928064754%40mojatatu.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net 2/2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path 2026-09-29 10:04 [PATCH net 0/2] net/sched: sch_teql: fix shared headroom and header strip Jamal Hadi Salim 2026-09-29 10:16 ` Jamal Hadi Salim 2026-09-29 10:16 ` [PATCH net 1/2] net/sched: sch_teql: fix shared headroom and header strip on slave retry Jamal Hadi Salim @ 2026-09-29 10:16 ` Jamal Hadi Salim 2026-09-30 13:07 ` netdev-bot+sashiko 2 siblings, 1 reply; 9+ messages in thread From: Jamal Hadi Salim @ 2026-09-29 10:16 UTC (permalink / raw) To: netdev Cc: Jamal Hadi Salim, Victor Nogueira, Jiri Pirko, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, stable, hybris, Sashiko This is a follow-up to commit dc4b95b8fee9 ("net/sched: sch_teql: restore skb->dev on the slave failure path"), which restored skb->dev to the master after each slave iteration but left the master-vs-slave mismatch on the neighbour/ARP-queue path, and left the queued skb naming the master rather than the device whose neighbour took it. teql_master_xmit() sets skb->dev = slave only right before calling the slave's ndo_start_xmit(). When the slave has no resolved neighbour, teql_resolve() hands the skb to neigh_event_send(), which queues it on the slave's neighbour arp_queue while skb->dev still points at the teql* master. skb->dev takes no netdev reference, and the master is freed unconditionally on teql_exit() while the slave is still alive, so a queued skb that runs the ARP retransmit path can dereference a freed device. Move the assignment to before teql_resolve(), so a skb parked on the slave's arp_queue carries the slave's device and is flushed with it. There is a second issue: when the neighbour resolves, neigh_update_process_arp_queue() re-looks-up the top-level neighbour so shaper, eql and teql can be re-entered, but it calls n1->output() without restoring skb->dev. An skb that was parked with skb->dev set to the slave then leaves through the slave directly, skipping teql_master_xmit() and the dev_hard_header() call that builds the slave's link-layer header, and is transmitted headerless. Restore skb->dev = n1->dev at the reinjection point; this is the identity for every other parker (neigh_resolve_output() parks with skb->dev == neigh->dev, ndisc probes carry no skb) and the needed repair for teql. Conditions to recreate the bug: a teql master with a slave whose neighbour is unresolved; send one packet through the master so the skb parks on the slave's arp_queue. On the unfixed tree the parked skb names the master (kprobe on __neigh_event_send shows skb->dev=teql0); with only the first hunk applied, the parked packet is delivered to the slave's peer without a link-layer header when ARP completes. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824115928.4099988-1-victor@mojatatu.com Link: https://lore.kernel.org/netdev/20260824115928.4099988-1-victor@mojatatu.com/ Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> --- net/core/neighbour.c | 1 + net/sched/sch_teql.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/net/core/neighbour.c b/net/core/neighbour.c index 7448320f7ad5..5823f5007812 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -1336,6 +1336,7 @@ static void neigh_update_process_arp_queue(struct neighbour *neigh) if (n2) n1 = n2; } + skb->dev = n1->dev; READ_ONCE(n1->output)(n1, skb); if (n2) neigh_release(n2); diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c index acd03f9afc6b..3fbd5af10028 100644 --- a/net/sched/sch_teql.c +++ b/net/sched/sch_teql.c @@ -348,12 +348,12 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev) continue; } + skb->dev = slave; switch (teql_resolve(skb, skb_res, slave, slave_txq, &hlen)) { case 0: if (__netif_tx_trylock(slave_txq)) { unsigned int length = qdisc_pkt_len(skb); - skb->dev = slave; if (!netif_xmit_frozen_or_stopped(slave_txq) && netdev_start_xmit(skb, slave, slave_txq, false) == NETDEV_TX_OK) { -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path 2026-09-29 10:16 ` [PATCH net 2/2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path Jamal Hadi Salim @ 2026-09-30 13:07 ` netdev-bot+sashiko 0 siblings, 0 replies; 9+ messages in thread From: netdev-bot+sashiko @ 2026-09-30 13:07 UTC (permalink / raw) To: jhs Cc: netdev, victor, jiri, davem, edumazet, kuba, pabeni, horms, hybris, sashiko-bot Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider. Critical: 0 · High: 0 · Medium: 1 · Low: 0 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [Medium] The new line `skb->dev = n1->dev;` in neigh_update_process_arp_queue() (net/core/neighbour.c:1339) runs for… -- net/sched: sch_teql: keep skb->dev consistent on the arp-queue path This moves the skb->dev = slave assignment in teql_master_xmit() to before teql_resolve(), so an skb parked on the slave's arp_queue names the slave. It also makes neigh_update_process_arp_queue() set skb->dev = n1->dev before reinjecting each queued skb through n1->output(). Link: https://lore.kernel.org/netdev/20260824115928.4099988-1-victor@mojatatu.com/ > diff --git a/net/core/neighbour.c b/net/core/neighbour.c > index 7448320f7ad52..5823f5007812b 100644 > --- a/net/core/neighbour.c > +++ b/net/core/neighbour.c > @@ -1336,6 +1336,7 @@ static void neigh_update_process_arp_queue(struct neighbour *neigh) > if (n2) > n1 = n2; > } > + skb->dev = n1->dev; > READ_ONCE(n1->output)(n1, skb); [Severity: Medium] Is the "identity for every other parker" claim in the commit message accurate? The commit message says: Restore skb->dev = n1->dev at the reinjection point; this is the identity for every other parker (neigh_resolve_output() parks with skb->dev == neigh->dev, ndisc probes carry no skb) When a dst is attached, n1 is replaced by n2 = dst_neigh_lookup_skb(dst, skb). For IPv4 that is ipv4_neigh_lookup(), which picks the neighbour on the dst's device and not on the device the skb was parked on: net/ipv4/route.c:ipv4_neigh_lookup() { ... dev = dst_dev_rcu(dst); if (likely(rt->rt_gw_family == AF_INET)) { n = ip_neigh_gw4(dev, rt->rt_gw4); ... } So this new line runs for every neighbour table, and it can change the egress device for parkers other than teql. One case is bpf_redirect_neigh() with BPF_F_NEXTHOP from tc egress. The tc egress hook runs in __dev_queue_xmit() before the IFF_XMIT_DST_RELEASE drop. __bpf_redirect_neigh_v4() replaces the dst only in the !nh branch: net/core/filter.c:__bpf_redirect_neigh_v4() { ... skb_dst_drop(skb); skb_dst_set(skb, &rt->dst); } ... } With an explicit nexthop, the original route dst (say on eth0) stays attached. bpf_out_neigh_v4() then sets skb->dev to the BPF target (say eth1) and calls neigh_output() on ip_neigh_gw4(eth1, nh). If that neighbour is unresolved, the skb is parked on eth1's neighbour. When it resolves, n2 is the neighbour on eth0, and this line sets skb->dev = eth0. Do the queued packets now leave through the route device instead of the device the BPF program chose? Before this patch they went out eth1 with eth0's L2 header, which was also wrong, but the behaviour was different. A second case is bridged DNAT in br_netfilter with pass_vlan_indev set. br_nf_pre_routing_finish() only calls br_nf_pre_routing_finish_bridge() when skb_dst(skb)->dev is the VLAN upper device (br0.VID), and that function does: net/bridge/br_netfilter_hooks.c:br_nf_pre_routing_finish_bridge() { ... skb->dev = bridge_parent(skb->dev); ... neigh = dst_neigh_lookup_skb(dst, skb); ... ret = READ_ONCE(neigh->output)(neigh, skb); ... } Here skb->dev is br0 while the neighbour is on br0.VID, so skb->dev == neigh->dev does not hold. Before this patch, parked skbs left through br0, the same as skbs that were not parked. With this change, won't parked skbs go through dev_queue_xmit(br0.VID) and vlan_dev_hard_start_xmit()? That path can rewrite the hwaccel VLAN TCI/PCP from the egress QoS map and bumps the VLAN device stats. Parked and non-parked packets would then take different paths. Could the fix be kept inside teql instead? Another option is to limit it to the teql master/slave case, where n2 differs from the parking neighbour, so other neighbour users keep their current skb->dev. [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v1.20260928064754%40mojatatu.com ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-10-03 11:00 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-29 10:04 [PATCH net 0/2] net/sched: sch_teql: fix shared headroom and header strip Jamal Hadi Salim 2026-09-29 10:16 ` Jamal Hadi Salim 2026-09-29 10:16 ` [PATCH net 1/2] net/sched: sch_teql: fix shared headroom and header strip on slave retry Jamal Hadi Salim 2026-09-30 13:07 ` netdev-bot+sashiko 2026-10-03 10:43 ` Jamal Hadi Salim 2026-10-03 10:57 ` Jamal Hadi Salim 2026-10-03 11:00 ` Jamal Hadi Salim 2026-09-29 10:16 ` [PATCH net 2/2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path Jamal Hadi Salim 2026-09-30 13:07 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox