Netdev List
 help / color / mirror / Atom feed
* [PATCH net 0/2 repost2] net/sched: sch_teql: fix shared headroom and header strip
@ 2026-10-20 10:38 Jamal Hadi Salim
  2026-10-20 10:38 ` [PATCH net 1/2 repost2] net/sched: sch_teql: fix shared headroom and header strip on slave retry Jamal Hadi Salim
  0 siblings, 1 reply; 9+ messages in thread
From: Jamal Hadi Salim @ 2026-10-20 10:38 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,
	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").

This is a repost of the same two patches; there is no code change.  The
previous posting went out unthreaded (patch 2 was not a reply to patch 1),
so this posting is sent as one threaded series: cover -> patch 1 ->
patch 2.

Changes since repost1:
- re-threaded as a single series (no code change).

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 repost2] net/sched: sch_teql: fix shared headroom and header strip on slave retry
  2026-10-20 10:38 [PATCH net 0/2 repost2] net/sched: sch_teql: fix shared headroom and header strip Jamal Hadi Salim
@ 2026-10-20 10:38 ` Jamal Hadi Salim
  2026-10-20 10:38   ` [PATCH net 2/2 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path Jamal Hadi Salim
  0 siblings, 1 reply; 9+ messages in thread
From: Jamal Hadi Salim @ 2026-10-20 10:38 UTC (permalink / raw)
  To: netdev
  Cc: Jamal Hadi Salim, Victor Nogueira, Jiri Pirko, David S . Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
	sashiko-bot, stable

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 the
retry path:

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")
Cc: stable@vger.kernel.org
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

* [PATCH net 2/2 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path
  2026-10-20 10:38 ` [PATCH net 1/2 repost2] net/sched: sch_teql: fix shared headroom and header strip on slave retry Jamal Hadi Salim
@ 2026-10-20 10:38   ` Jamal Hadi Salim
  2026-10-26 16:07     ` Simon Horman
  0 siblings, 1 reply; 9+ messages in thread
From: Jamal Hadi Salim @ 2026-10-20 10:38 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,
	Sashiko

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.

That alone is not enough: 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. A 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.

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.

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")
Cc: stable@vger.kernel.org
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 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path
  2026-10-20 10:38   ` [PATCH net 2/2 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path Jamal Hadi Salim
@ 2026-10-26 16:07     ` Simon Horman
  2026-10-26 23:51       ` Jamal Hadi Salim
  0 siblings, 1 reply; 9+ messages in thread
From: Simon Horman @ 2026-10-26 16:07 UTC (permalink / raw)
  To: jhs
  Cc: Simon Horman, netdev, victor, jiri, davem, edumazet, kuba, pabeni,
	stable, sashiko-bot

This is an AI-generated review of your patch. The human sending this
email says "Hi Jamal, passing this on for your consideration as it
seems to have got stuck somewhere."
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v1.r2.20261020051718-1@mojatatu.com?part=2
---
net/sched: sch_teql: keep skb->dev consistent on the arp-queue path

The patch sets skb->dev = slave before teql_resolve(). An skb parked on
the slave's neighbour arp_queue then names the slave, not the teql master.
It also adds skb->dev = n1->dev at the reinjection point in
neigh_update_process_arp_queue(), so parked teql skbs go back through
teql_master_xmit() when the neighbour resolves.

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);
>  		if (n2)
>  			neigh_release(n2);

[Severity: Medium]
The commit message says this assignment "is the identity for every other
parker (neigh_resolve_output() parks with skb->dev == neigh->dev, ndisc
probes carry no skb)". Does that hold when the skb's dst points at a
different device than the neighbour it was parked on?

If the dst is not DST_OBSOLETE_DEAD, n1 is not the parking neighbour. It is
replaced by n2 from dst_neigh_lookup_skb(), and ipv4_neigh_lookup() looks
that neighbour up on the dst's device:

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 the new line is only a no-op when dst->dev == skb->dev.
skb->dev == neigh->dev is not enough.

At least two parkers seem to break that. The first is bpf_redirect_neigh()
with explicit nexthop params. bpf_out_neigh_v4() moves skb->dev to the
redirect target B and resolves the nexthop on B:

net/core/filter.c:bpf_out_neigh_v4() {
    ...
	skb->dev = dev;
    ...
	} else if (nh->nh_family == AF_INET) {
		neigh = ip_neigh_gw4(dev, nh->ipv4_nh);
    ...
}

However, __bpf_redirect_neigh_v4() only replaces the dst in the !nh branch,
so the routed dst for the original device A stays attached:

net/core/filter.c:__bpf_redirect_neigh_v4() {
    ...
		skb_dst_drop(skb);
		skb_dst_set(skb, &rt->dst);
	}
    ...
}

bpf_out_neigh_v6() follows the same pattern with ip_neigh_gw6().

The second is nft fwd ... neigh in a netdev egress chain.
nft_fwd_neigh_eval() changes skb->dev and parks the skb without touching
the dst:

net/netfilter/nft_fwd_netdev.c:nft_fwd_neigh_eval() {
    ...
	skb->dev = dev;
	skb_clear_tstamp(skb);

	nf_dev_xmit_recursion_inc();
	neigh_xmit(neigh_table, dev, addr, skb);
    ...
}

In __dev_queue_xmit(), nf_hook_egress() and sch_handle_egress() run before
the IFF_XMIT_DST_RELEASE skb_dst_drop(), so A's routed dst is still
attached at these hooks. If B's neighbour is incomplete, the skb is parked
on B's arp_queue with skb->dev == B and dst->dev == A.

When B's neighbour resolves, n2 is A's neighbour. With this patch skb->dev
becomes A, and n1->output() builds A's link-layer header and calls
dev_queue_xmit() on A.

Before the patch, the frame still left on B, though with A's link-layer
header, which was already a pre-existing problem. After the patch, would
the packets queued during resolution go through A's egress qdisc, tc and
nft hooks a second time?

That could mean nft_fwd_neigh_eval() calls ip_decrease_ttl() or
decrements hop_limit again. Non-idempotent BPF programs (encap,
adjust_room) would run again, and counting or stateful rules would see
the packet twice. If the redirect is not applied again on A's path, the
packets would leave on A and bypass the configured BPF or nft steering.

Since this is a change to core neighbour code tagged for stable, could the
reinjection fix be limited to the teql case? Alternatively, could the
commit message describe the behaviour change for these other parkers?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 2/2 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path
  2026-10-26 16:07     ` Simon Horman
@ 2026-10-26 23:51       ` Jamal Hadi Salim
  2026-10-27  0:46         ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Jamal Hadi Salim @ 2026-10-26 23:51 UTC (permalink / raw)
  To: Simon Horman
  Cc: netdev, victor, jiri, davem, edumazet, kuba, pabeni, stable,
	sashiko-bot

On Mon, Oct 26, 2026 at 12:08 PM Simon Horman <horms@kernel.org> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email says "Hi Jamal, passing this on for your consideration as it
> seems to have got stuck somewhere."

There was nothing wrong with the headers this time - so not sure what
is going on. A bit frustrating.

In any case these are complaints about commit messages in both 1/2 and
this i.e there is nothing to change in the code.
If necessary, I can resend repost 3 with the fixed commit clarifying
the "identity for every other parker"

cheers,
jamal

> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-6HS9.v1.r2.20261020051718-1@mojatatu.com?part=2
> ---
> net/sched: sch_teql: keep skb->dev consistent on the arp-queue path
>
> The patch sets skb->dev = slave before teql_resolve(). An skb parked on
> the slave's neighbour arp_queue then names the slave, not the teql master.
> It also adds skb->dev = n1->dev at the reinjection point in
> neigh_update_process_arp_queue(), so parked teql skbs go back through
> teql_master_xmit() when the neighbour resolves.
>
> 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);
> >               if (n2)
> >                       neigh_release(n2);
>
> [Severity: Medium]
> The commit message says this assignment "is the identity for every other
> parker (neigh_resolve_output() parks with skb->dev == neigh->dev, ndisc
> probes carry no skb)". Does that hold when the skb's dst points at a
> different device than the neighbour it was parked on?
>
> If the dst is not DST_OBSOLETE_DEAD, n1 is not the parking neighbour. It is
> replaced by n2 from dst_neigh_lookup_skb(), and ipv4_neigh_lookup() looks
> that neighbour up on the dst's device:
>
> 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 the new line is only a no-op when dst->dev == skb->dev.
> skb->dev == neigh->dev is not enough.
>
> At least two parkers seem to break that. The first is bpf_redirect_neigh()
> with explicit nexthop params. bpf_out_neigh_v4() moves skb->dev to the
> redirect target B and resolves the nexthop on B:
>
> net/core/filter.c:bpf_out_neigh_v4() {
>     ...
>         skb->dev = dev;
>     ...
>         } else if (nh->nh_family == AF_INET) {
>                 neigh = ip_neigh_gw4(dev, nh->ipv4_nh);
>     ...
> }
>
> However, __bpf_redirect_neigh_v4() only replaces the dst in the !nh branch,
> so the routed dst for the original device A stays attached:
>
> net/core/filter.c:__bpf_redirect_neigh_v4() {
>     ...
>                 skb_dst_drop(skb);
>                 skb_dst_set(skb, &rt->dst);
>         }
>     ...
> }
>
> bpf_out_neigh_v6() follows the same pattern with ip_neigh_gw6().
>
> The second is nft fwd ... neigh in a netdev egress chain.
> nft_fwd_neigh_eval() changes skb->dev and parks the skb without touching
> the dst:
>
> net/netfilter/nft_fwd_netdev.c:nft_fwd_neigh_eval() {
>     ...
>         skb->dev = dev;
>         skb_clear_tstamp(skb);
>
>         nf_dev_xmit_recursion_inc();
>         neigh_xmit(neigh_table, dev, addr, skb);
>     ...
> }
>
> In __dev_queue_xmit(), nf_hook_egress() and sch_handle_egress() run before
> the IFF_XMIT_DST_RELEASE skb_dst_drop(), so A's routed dst is still
> attached at these hooks. If B's neighbour is incomplete, the skb is parked
> on B's arp_queue with skb->dev == B and dst->dev == A.
>
> When B's neighbour resolves, n2 is A's neighbour. With this patch skb->dev
> becomes A, and n1->output() builds A's link-layer header and calls
> dev_queue_xmit() on A.
>
> Before the patch, the frame still left on B, though with A's link-layer
> header, which was already a pre-existing problem. After the patch, would
> the packets queued during resolution go through A's egress qdisc, tc and
> nft hooks a second time?
>
> That could mean nft_fwd_neigh_eval() calls ip_decrease_ttl() or
> decrements hop_limit again. Non-idempotent BPF programs (encap,
> adjust_room) would run again, and counting or stateful rules would see
> the packet twice. If the redirect is not applied again on A's path, the
> packets would leave on A and bypass the configured BPF or nft steering.
>
> Since this is a change to core neighbour code tagged for stable, could the
> reinjection fix be limited to the teql case? Alternatively, could the
> commit message describe the behaviour change for these other parkers?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 2/2 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path
  2026-10-26 23:51       ` Jamal Hadi Salim
@ 2026-10-27  0:46         ` Jakub Kicinski
  2026-10-27  7:25           ` Jamal Hadi Salim
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2026-10-27  0:46 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: Simon Horman, netdev, victor, jiri, davem, edumazet, pabeni,
	stable, sashiko-bot

On Mon, 26 Oct 2026 19:51:33 -0400 Jamal Hadi Salim wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email says "Hi Jamal, passing this on for your consideration as it
> > seems to have got stuck somewhere."  
> 
> There was nothing wrong with the headers this time - so not sure what
> is going on. A bit frustrating.

It is frustrating, you are the only one with these issues.
Patch 2 is posted in reply to patch 1. IDK what else is wrong.

> In any case these are complaints about commit messages in both 1/2 and
> this i.e there is nothing to change in the code.
> If necessary, I can resend repost 3 with the fixed commit clarifying
> the "identity for every other parker"

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 2/2 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path
  2026-10-27  0:46         ` Jakub Kicinski
@ 2026-10-27  7:25           ` Jamal Hadi Salim
  2026-10-27  7:44             ` Jamal Hadi Salim
  0 siblings, 1 reply; 9+ messages in thread
From: Jamal Hadi Salim @ 2026-10-27  7:25 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Simon Horman, netdev, victor, jiri, davem, edumazet, pabeni,
	stable, sashiko-bot

On Mon, Oct 26, 2026 at 8:46 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 26 Oct 2026 19:51:33 -0400 Jamal Hadi Salim wrote:
> > > This is an AI-generated review of your patch. The human sending this
> > > email says "Hi Jamal, passing this on for your consideration as it
> > > seems to have got stuck somewhere."
> >
> > There was nothing wrong with the headers this time - so not sure what
> > is going on. A bit frustrating.
>
> It is frustrating, you are the only one with these issues.
> Patch 2 is posted in reply to patch 1. IDK what else is wrong.
>

Let me send a repost3 - will take the opportunity to pedantically fix
the commit message (no code change).

cheers,
jamal

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 2/2 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path
  2026-10-27  7:25           ` Jamal Hadi Salim
@ 2026-10-27  7:44             ` Jamal Hadi Salim
  2026-10-27  8:04               ` Jamal Hadi Salim
  0 siblings, 1 reply; 9+ messages in thread
From: Jamal Hadi Salim @ 2026-10-27  7:44 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Simon Horman, netdev, victor, jiri, davem, edumazet, pabeni,
	stable, sashiko-bot

On Tue, Oct 27, 2026 at 3:25 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Mon, Oct 26, 2026 at 8:46 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Mon, 26 Oct 2026 19:51:33 -0400 Jamal Hadi Salim wrote:
> > > > This is an AI-generated review of your patch. The human sending this
> > > > email says "Hi Jamal, passing this on for your consideration as it
> > > > seems to have got stuck somewhere."
> > >
> > > There was nothing wrong with the headers this time - so not sure what
> > > is going on. A bit frustrating.
> >
> > It is frustrating, you are the only one with these issues.
> > Patch 2 is posted in reply to patch 1. IDK what else is wrong.
> >
>
> Let me send a repost3 - will take the opportunity to pedantically fix
> the commit message (no code change).

BTW, checking i see the threading was:
     0/2 cover  (root)
       1/2      (reply to 0/2)
         2/2    (reply to 1/2)

I checked other postings that didnt have issues and it was fine. I
also found the approach
   0/2 cover  (root)
       1/2      (reply to 0/2)
        2/2    (reply to 0/2)

was fine. So not sure if it is the threading. Likely I have to make a
change to not confuse it. I am making a commit text change this time.
If you can please just apply that instead of waiting for sashiko, in
posting 1 it didnt have issues with 1/2 commit phrasing. In repost 2
it did. And i am sure if we wait for it to review again it will find
something about a code comment or who knows even code.
The pre-existing issues it complains about are what a parallel teql
patch resolves.

cheers,
jamal

cheers,
jamal

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 2/2 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path
  2026-10-27  7:44             ` Jamal Hadi Salim
@ 2026-10-27  8:04               ` Jamal Hadi Salim
  0 siblings, 0 replies; 9+ messages in thread
From: Jamal Hadi Salim @ 2026-10-27  8:04 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Simon Horman, netdev, victor, jiri, davem, edumazet, pabeni,
	stable, sashiko-bot

On Tue, Oct 27, 2026 at 3:44 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Tue, Oct 27, 2026 at 3:25 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Mon, Oct 26, 2026 at 8:46 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Mon, 26 Oct 2026 19:51:33 -0400 Jamal Hadi Salim wrote:
> > > > > This is an AI-generated review of your patch. The human sending this
> > > > > email says "Hi Jamal, passing this on for your consideration as it
> > > > > seems to have got stuck somewhere."
> > > >
> > > > There was nothing wrong with the headers this time - so not sure what
> > > > is going on. A bit frustrating.
> > >
> > > It is frustrating, you are the only one with these issues.
> > > Patch 2 is posted in reply to patch 1. IDK what else is wrong.
> > >
> >
> > Let me send a repost3 - will take the opportunity to pedantically fix
> > the commit message (no code change).
>
> BTW, checking i see the threading was:
>      0/2 cover  (root)
>        1/2      (reply to 0/2)
>          2/2    (reply to 1/2)
>
> I checked other postings that didnt have issues and it was fine. I
> also found the approach
>    0/2 cover  (root)
>        1/2      (reply to 0/2)
>         2/2    (reply to 0/2)
>

And for repost3:
- 0/2 (root)
     1/2  In-Reply-To/References: 0/2
     2/2  In-Reply-To: 1/2; References: 0/2 + 1/2

I am sticking to this approach going forward.

cheers,
jamal

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-10-27  8:04 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-20 10:38 [PATCH net 0/2 repost2] net/sched: sch_teql: fix shared headroom and header strip Jamal Hadi Salim
2026-10-20 10:38 ` [PATCH net 1/2 repost2] net/sched: sch_teql: fix shared headroom and header strip on slave retry Jamal Hadi Salim
2026-10-20 10:38   ` [PATCH net 2/2 repost2] net/sched: sch_teql: keep skb->dev consistent on the arp-queue path Jamal Hadi Salim
2026-10-26 16:07     ` Simon Horman
2026-10-26 23:51       ` Jamal Hadi Salim
2026-10-27  0:46         ` Jakub Kicinski
2026-10-27  7:25           ` Jamal Hadi Salim
2026-10-27  7:44             ` Jamal Hadi Salim
2026-10-27  8:04               ` Jamal Hadi Salim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox