[PATCH net v3 0/4] net/sched: fix short-skb metadata and namespace boundaries

[PATCH net v3 0/4] net/sched: fix short-skb metadata and namespace boundaries

[Jamal Hadi Salim]

This series fixes three independent boundary problems in net/sched, and
covers the third one with tc-testing cases.

1/4 act_ife: encode a configured zero metadata value.  The u32 and u16
presence callbacks reserve an 8-byte TLV for a configured zero value, but
the encoders returned without writing it, leaking stale skb headroom bytes.

2/4 act_ife: require a full Ethernet header on egress.  An undersized
linear head can make the encoder read past the end of the skb data.

3/4 sch_teql: reject qdiscs from another network namespace.  The global
TEQL master lives in the initial netns; a qdisc created in another netns
could still mutate the master's MTU and flags.

4/4 selftests: tc-testing: cover each of the two teql attach rejections on
its own.  Case 124e moves to the initial namespace, where it creates its
own device, so only the root-only check can refuse it; a root attach from
the test namespace is refused only by the namespace check.

Origins:

1/4: reported by shaos <shawnq1@163.com> with a reproducer, which was
reproduced and verified in our test environment.
2/4: found by Sashiko (nipa and gemini) while reviewing Victor Nogueira's
parent act_ife patchset; independently reported by shaos
<shawnq1@163.com> with a reproducer, which was verified in our test
environment.
3/4: reported by shaos <shawnq1@163.com> with a reproducer, which was
reproduced and verified in our test environment.
4/4: follows Sashiko (nipa) reviews of the v2 posting, which pointed out
that a single case cannot witness both teql attach rejections.

The four patches are independent.  They target net and the three fixes
carry a stable nomination; the test cases do not.  They are sent together
because they share the short-skb and namespace-boundary review context.

Jamal Hadi Salim (4):
  net/sched: act_ife: encode configured zero metadata values
  net/sched: act_ife: require a full Ethernet header on egress
  net/sched: sch_teql: reject qdiscs from another network namespace
  selftests: tc-testing: teql: cover each attach rejection on its own

 net/sched/act_ife.c                                | 13 ++++----
 net/sched/sch_teql.c                               |  9 ++++--
 .../selftests/tc-testing/tc-tests/qdiscs/teql.json | 36 ++++++++++++++++++----
 3 files changed, 44 insertions(+), 14 deletions(-)

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---

v2 -> v3: considers comments from Sashiko (nipa) on the v2 posting.  Case 124e
ran under nsPlugin, in a namespace where the teql0 master is not present,
so the namespace check answered it and the case no longer proved that a
child attach is refused as such; the case added in v2 covered only the
namespace check.  124e now runs in the initial namespace and creates its
own device, and the namespace rejection keeps its own case.  Both cases
were verified to fail when the check they exercise is removed.

1/4's and 3/4's diffs are unchanged from the v2 posting, so neither carries
a changelog; 2/4's Fixes: tag now names the commit that introduced the
unguarded copy.

v1 -> v2: considers comments from Sashiko (nipa) on the repost2 posting.  The
cross-netns check in 3/4 ran before the "teql can only be used as root"
check, so every case it did not catch was answered by that check, masking
the assertion that tc-testing case 124e makes.  The root-parent validation
now runs first and the namespace check second, and 4/4 covers the namespace
rejection on its own as suggested by sashiko-nipa.

1/4 and 2/4 are unchanged from the repost2 posting.

[PATCH net v3 1/4] net/sched: act_ife: encode configured zero metadata values

[Jamal Hadi Salim]

Reported by shaos <shawnq1@163.com> with a reproducer for the 32-bit
case, which was reproduced and verified in our test environment.

The issue:
ife_check_meta_u32() and ife_check_meta_u16() both reserve eight bytes
of metadata for a configured value even when that value is zero, but the
matching encoder returns without calling ife_tlv_meta_encode(). The
encoder therefore writes neither the TLV header nor its value, while the
enclosing IFE length still includes the reserved bytes, exposing stale
skb headroom contents to the peer.

The 32-bit helper has been present since the IFE action was introduced
(v4.6); the 16-bit helper was added with the 16-bit metadata helpers used
by the skbtcindex callback (v4.9). Both defects predate the oldest
supported stable kernel (5.10), so they are fixed together in this patch.
Each hunk keeps its own Fixes: tag for provenance.

The fix:
Encode any configured value, including zero; skip the TLV only when
there is neither a configured nor a runtime value. The runtime fallback
is unchanged.

A raw TCA_IFE_METALST netlink request is needed to express the
configured zero: the IFE_META_SKBMARK attribute carries four zero
payload bytes for the u32 helper, and IFE_META_TCINDEX two zero payload
bytes for the 16-bit helper (not an absent or zero-length attribute).
Stock tc cannot build these requests; "use mark 0x0" emits no mark
attribute and "allow mark" emits a zero-length one.

Conditions to recreate the bug:
CONFIG_NET_ACT_IFE=y, CONFIG_NET_IFE_SKBMARK=y,
CONFIG_NET_IFE_SKBTCINDEX=y, CONFIG_NET_CLS_MATCHALL=y,
CONFIG_NET_SCH_INGRESS=y, CONFIG_USER_NS=y, CONFIG_NET_NS=y,
CONFIG_VETH=y. In a fresh user and network namespace create a veth pair
and attach clsact to the egress side. Install a matchall filter whose
ife encode action carries a zero skb mark (or a zero tcindex) in
TCA_IFE_METALST. Send a frame through the veth and inspect the IFE
metadata in the received frame. The metadata region is eight bytes but
holds neither a valid TLV header nor a value; it exposes prior headroom
contents. Setting the value to a nonzero one does not trigger the leak.
The namespace needs CAP_NET_ADMIN; no additional privilege is required.
With the fix the zero TLVs are 0001000800000000 (u32) and
0005000600000000 (u16).

Fixes: ef6980b6becb ("introduce IFE action")
Fixes: 6a5d58b67e20 ("net sched ife action: add 16 bit helpers")
Reported-by: shaos <shawnq1@163.com>
Link: https://lore.kernel.org/netdev/20261022144739.61342-1-shawnq1@163.com/
Cc: stable@vger.kernel.org
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_ife.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 3946a9589bb8..6b23a3965121 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -50,8 +50,7 @@ int ife_encode_meta_u16(u16 metaval, void *skbdata, struct tcf_meta_info *mi)
 		edata = *(u16 *)mi->metaval;
 	else if (metaval)
 		edata = metaval;
-
-	if (!edata) /* will not encode */
+	else
 		return 0;
 
 	edata = htons(edata);
@@ -94,8 +93,7 @@ int ife_encode_meta_u32(u32 metaval, void *skbdata, struct tcf_meta_info *mi)
 		edata = *(u32 *)mi->metaval;
 	else if (metaval)
 		edata = metaval;
-
-	if (!edata) /* will not encode */
+	else
 		return 0;
 
 	edata = htonl(edata);
-- 
2.43.0

[PATCH net v3 2/4] net/sched: act_ife: require a full Ethernet header on egress

[Jamal Hadi Salim]

This is a followup of commit 5b483f7791b0 ("net/sched: act_ife: Only
operate on Ethernet frames") as reported by Sashiko.

The finding was raised by both Sashiko providers (nipa and gemini)
while reviewing the parent patchset. shaos <shawnq1@163.com>
independently reported the same undersized-egress path with a
reproducer, which was reproduced and verified in our test environment.

The issue:
On egress tcf_ife_is_eth_skb() only checks the device type; it does not
check that the skb carries a full Ethernet header. For an Ethernet or
loopback device it returns true even when the skb has no Ethernet
header. tcf_ife_encode() then calls ife_encode(), which copies ETH_HLEN
bytes from skb->data. A short skb therefore reads past its linear data
and the copied bytes, including stale kernel memory, are emitted in the
outer Ethernet header.

The parent commit fixed the reported ingress mac_len case but left the
egress path without a length check.

A total-length test is not sufficient: a non-GSO TUN skb can have a
large total length while only a single byte sits in the linear area, so
ife_encode() still reads past the linear header. Require instead that
the Ethernet header the encoder copies is present in the linear part of
the skb.

The Fix:
On egress require pskb_may_pull(skb, ETH_HLEN) before encoding, which
ensures the Ethernet header is present in the linear data ife_encode()
copies from; the ingress mac_len check is unchanged. A short egress skb
is now dropped instead of being encoded.

Conditions to recreate the bug:
CONFIG_NET_ACT_IFE=y, CONFIG_NET_CLS_MATCHALL=y, CONFIG_NET_SCH_INGRESS=y,
CONFIG_TUN=y, CONFIG_NET_ACT_MIRRED=y. In two user and network
namespaces (a victim and an attacker), open a TUN device in IFF_TUN mode
and attach clsact to it and to lo. Redirect ingress traffic from the TUN
device to lo egress with mirred, and install an ife encode action on lo
egress:

  tc qdisc add dev <tun> clsact
  tc qdisc add dev lo clsact
  tc filter add dev <tun> ingress matchall action mirred egress \
      redirect dev lo
  tc filter add dev lo egress matchall action ife encode use mark 1 \
      action ok

Send a TUN packet shorter than ETH_HLEN (a one-byte payload is enough)
and receive the emitted frame on the other side. Without the fix the
outer Ethernet header contains bytes copied past the short packet; with
the fix the short skb is dropped. The attacker needs CAP_NET_ADMIN in
its user namespace; no init-net privilege is required.

The same path is reachable when the total length is large but the
Ethernet header is not linear: write a non-GSO virtio header with
hdr_len=1 followed by an 8192-byte packet so that only one byte stays
linear. Without the fix the copied header reads past the linear data;
with the fix the header is pulled into the linear area before the copy.

Fixes: 295a6e06d21e ("net/sched: act_ife: Change to use ife module")
Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
Reported-by: shaos <shawnq1@163.com>
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260821164031.32824-1-victor%40mojatatu.com
Link: https://sashiko.dev/#/patchset/20260821164031.32824-1-victor@mojatatu.com
Link: https://lore.kernel.org/netdev/20261022144736.61227-1-shawnq1@163.com/
Cc: stable@vger.kernel.org
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v2 -> v3: no change to the diff; the Fixes: tag now names the commit that
introduced the unguarded copy.  This series revision considers comments
from Sashiko (nipa) on the v2 posting:
https://lore.kernel.org/netdev/179366641863.488856.17368571591466000274@kernel.org/

The tc-testing coverage of the teql rejections is reworked in 4/4 so that
each case exercises exactly one check.

 net/sched/act_ife.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 6b23a3965121..5a527da00566 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -878,13 +878,16 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
  * is an Ethernet header because the skb could've been redirected with mirred
  * from a non-Ethernet device.
  */
-static bool tcf_ife_is_eth_skb(const struct sk_buff *skb)
+static bool tcf_ife_is_eth_skb(struct sk_buff *skb)
 {
 	if (skb->dev->type != ARPHRD_ETHER &&
 	    skb->dev->type != ARPHRD_LOOPBACK)
 		return false;
 
-	return !skb_at_tc_ingress(skb) || skb->mac_len == ETH_HLEN;
+	if (skb_at_tc_ingress(skb))
+		return skb->mac_len == ETH_HLEN;
+
+	return pskb_may_pull(skb, ETH_HLEN);
 }
 
 TC_INDIRECT_SCOPE int tcf_ife_act(struct sk_buff *skb,
-- 
2.43.0

[PATCH net v3 3/4] net/sched: sch_teql: reject qdiscs from another network namespace

[Jamal Hadi Salim]

Reported by shaos <shawnq1@163.com> with a reproducer, which was
reproduced and verified in our test environment.

The issue:
The teql master devices are created once in the initial network
namespace and their Qdisc_ops are global. teql_qdisc_init() takes the
teql_master embedded in the registered sch->ops and uses it without
checking whether the new qdisc's device lives in the same network
namespace as the master. A qdisc created in another netns can therefore
attach to the init-net master and change its MTU and flags.

The Fix:
Reject a teql qdisc whose device is not in the master's network
namespace with -EOPNOTSUPP and an extack message; same-netns teql still
works.

Initialize the private skb queue before any validation so every
constructor failure path, including qdisc_create_dflt(), which resets the
qdisc before destroying it, sees an initialized empty queue.

Conditions to recreate the bug:
CONFIG_NET_SCH_TEQL=y, CONFIG_DUMMY=y, CONFIG_USER_NS=y, CONFIG_NET_NS=y.
As an unprivileged UID, unshare a user and network namespace, create a
dummy device, set its MTU to 60000, and add a root teql0 qdisc:

  unshare -Urn
  ip link add dummy0 type dummy
  ip link set dummy0 mtu 60000
  tc qdisc add dev dummy0 root teql0

The init-net teql0 device changes from MTU 1500 to 60000. A pfifo control
on the same device leaves the MTU unchanged, and a direct MTU change on
init-net teql0 is denied. The namespace needs CAP_NET_ADMIN; no init-net
privilege is required. With the fix the attach fails with
"Operation not supported" and the init-net MTU stays 1500.

Fixes: 7316ae88c43d ("net_sched: make traffic control network namespace aware")
Reported-by: shaos <shawnq1@163.com>
Link: https://lore.kernel.org/netdev/20261022144830.67948-1-shawnq1@163.com/
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v1.r2.20261031160050-3%40mojatatu.com
Link: https://lore.kernel.org/netdev/179366641863.488856.17368571591466000274@kernel.org/
Cc: stable@vger.kernel.org
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v1 -> v2: considers comments from Sashiko (nipa) on the repost2 posting:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v1.r2.20261031160050-3%40mojatatu.com

The cross-netns check ran before the "teql can only be used as root"
check, so every case it did not catch was answered by that check, masking
the assertion that tc-testing case 124e makes.  The root-parent
validation now runs first and the namespace check second; 4/4 adds a
case that covers the namespace rejection on its own.

 net/sched/sch_teql.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index 70c948a26f80..dc21cd9704a2 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -200,6 +200,8 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt,
 	struct Qdisc *first;
 	unsigned long flags;
 
+	skb_queue_head_init(&q->q);
+
 	if (dev->hard_header_len > m->dev->hard_header_len)
 		return -EINVAL;
 
@@ -211,9 +213,12 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt,
 		return -EOPNOTSUPP;
 	}
 
-	q->m = m;
+	if (dev_net(dev) != dev_net(m->dev)) {
+		NL_SET_ERR_MSG_MOD(extack, "teql master is in a different network namespace");
+		return -EOPNOTSUPP;
+	}
 
-	skb_queue_head_init(&q->q);
+	q->m = m;
 
 	raw_spin_lock_irqsave(&m->slaves_lock, flags);
 	first = rcu_dereference_protected(m->slaves, lockdep_is_held(&m->slaves_lock));
-- 
2.43.0

[PATCH net v3 4/4] selftests: tc-testing: teql: cover each attach rejection on its own

[Jamal Hadi Salim]

Found by Sashiko (nipa) while reviewing the previous posting of this
series:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v2.r0.20261101171325-4%40mojatatu.com

The issue:
Case 124e attaches teql as a child qdisc and expects the attach to be
refused. It requires nsPlugin, so its device sits in the test network
namespace while the teql0 master is in init_net. Both the root-only
check and the namespace check refuse the attach with -EOPNOTSUPP, and
the case checks the exit code and an empty dump only, so it passes
whichever check answers it. It no longer proves that a child attach is
refused as such, and the case added in the previous posting covers the
namespace check alone.

The Fix:
Run 124e in the initial namespace, creating its own device the way the
other teql cases do, so that only the root-only check can refuse it.
Attach a root qdisc from the test namespace in the second case, so that
only the namespace check can refuse that one.

Link: https://lore.kernel.org/netdev/179366641863.488856.17368571591466000274@kernel.org/
Link: https://lore.kernel.org/netdev/179366641986.488856.2193313144081673076@kernel.org/
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v2 -> v3: considers comments from Sashiko (nipa) on the v2 posting:
https://lore.kernel.org/netdev/179366641986.488856.2193313144081673076@kernel.org/

Case 124e now runs in the initial namespace and creates its own device, so
only the root-only rejection can answer it.  The case added in v2 keeps the
namespace rejection covered.  Each case now witnesses exactly one check.

v1 -> v2: new case covering the cross-netns attach rejection, following a
Sashiko (nipa) review of the v2 posting of 3/4:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v1.r2.20261031160050-3%40mojatatu.com

 .../tc-testing/tc-tests/qdiscs/teql.json      | 36 +++++++++++++++----
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
index 0179c57104ad..ab5d0881c007 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
@@ -90,12 +90,8 @@
             "ets",
             "tbf"
         ],
-        "plugins": {
-            "requires": [
-                "nsPlugin"
-            ]
-        },
         "setup": [
+            "$IP link add dev $DUMMY type dummy",
             "$TC qdisc add dev $DUMMY root handle 1: qfq",
             "$TC class add dev $DUMMY parent 1: classid 1:1 qfq weight 15 maxpkt 16384"
         ],
@@ -104,7 +100,35 @@
         "verifyCmd": "$TC -s -j qdisc ls dev $DUMMY parent 1:1",
         "matchJSON": [],
         "teardown": [
-            "$TC qdisc del dev $DUMMY root handle 1:"
+            "$TC qdisc del dev $DUMMY root handle 1:",
+            "$IP link del dev $DUMMY"
+        ]
+    },
+    {
+        "id": "6a06",
+        "name": "Reject TEQL attach from a different network namespace",
+        "category": [
+            "qdisc",
+            "teql"
+        ],
+        "plugins": {
+            "requires": [
+                "nsPlugin"
+            ]
+        },
+        "setup": [],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY root handle 3: teql0",
+        "expExitCode": "2",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc teql0 3: root",
+        "matchCount": "0",
+        "teardown": [
+            [
+                "$TC qdisc del dev $DUMMY root handle 3:",
+                0,
+                1,
+                2
+            ]
         ]
     }
 ]
-- 
2.43.0

Re: [PATCH net v3 2/4] net/sched: act_ife: require a full Ethernet header on egress

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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 is titled "require a full Ethernet header on
  egress".
- [Low] The commit message says "A short egress skb is now dropped
  instead of being encoded." The patch actually drops every egress skb…

--

net/sched: act_ife: require a full Ethernet header on egress

On egress, tcf_ife_is_eth_skb() now requires pskb_may_pull(skb, ETH_HLEN)
to succeed. Without that check, ife_encode() could copy ETH_HLEN bytes from
past the linear data of a short skb. The ingress mac_len check is unchanged.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260821164031.32824-1-victor%40mojatatu.com
Link: https://sashiko.dev/#/patchset/20260821164031.32824-1-victor@mojatatu.com
Link: https://lore.kernel.org/netdev/20261022144736.61227-1-shawnq1@163.com/

> diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
> index 6b23a3965121e..5a527da005661 100644
> --- a/net/sched/act_ife.c
> +++ b/net/sched/act_ife.c
> @@ -878,13 +878,16 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
>   * is an Ethernet header because the skb could've been redirected with mirred
>   * from a non-Ethernet device.
>   */
> -static bool tcf_ife_is_eth_skb(const struct sk_buff *skb)
> +static bool tcf_ife_is_eth_skb(struct sk_buff *skb)
>  {
>  	if (skb->dev->type != ARPHRD_ETHER &&
>  	    skb->dev->type != ARPHRD_LOOPBACK)
>  		return false;
>  
> -	return !skb_at_tc_ingress(skb) || skb->mac_len == ETH_HLEN;
> +	if (skb_at_tc_ingress(skb))
> +		return skb->mac_len == ETH_HLEN;
> +
> +	return pskb_may_pull(skb, ETH_HLEN);

[Severity: Medium]
The subject says "require a full Ethernet header on egress". The commit
message also says tcf_ife_is_eth_skb() "returns true even when the skb has
no Ethernet header".

Does the new egress branch check that the skb has an Ethernet header, or
only that ETH_HLEN bytes are in the linear area?

Take the reproducer setup from the commit message and send an IP packet of
14 bytes or more instead of a short one:

tun_get_user() (IFF_TUN)
    skb_reset_network_header(skb);       /* mac_len is 0 */
tcf_mirred_to_dev()
    mac_len = at_ingress ? skb->mac_len : skb_network_offset(skb);
    skb_push_rcsum(skb_to_send, mac_len); /* pushes 0 bytes */
lo egress: tcf_ife_act()
    tcf_ife_is_eth_skb()
        pskb_may_pull(skb, ETH_HLEN)      /* succeeds */
    tcf_ife_encode()->ife_encode()
        iethh = (struct ethhdr *) skb->data;
        __skb_push(skb, total_push);
        memcpy(skb->data, iethh, ETH_HLEN);

At this point skb->data still points at the IP header. The first 14 IP
header bytes are copied as the inner "original" Ethernet header. They also
end up in the outer h_dest/h_source unless eth_dst/eth_src are configured.

The over-read past the linear data is fixed, and these bytes are packet
data the sender controls, not kernel memory. This behaviour for L3 skbs of
14 bytes or more was already there after commit 5b483f7791b0. Still, a
non-Ethernet frame can be encoded on egress as if it had an Ethernet
header, which is what the subject and issue statement say this patch
prevents.

The comment above the function also still reads as a pure check:

    IFE encapsulates the original Ethernet header and, on decode, expects
    to find one, so it can only ever work on skbs that carry one.

It does not say that the egress branch only checks linear length, or that
the call can now pull and so modify the skb.

Should the subject, issue statement and function comment say that egress
only checks linear length? Or should the egress branch also check that the
L2 header really is Ethernet?

[Severity: Low]
The commit message says "A short egress skb is now dropped instead of
being encoded." Is that the only behaviour change?

tcf_ife_act() calls tcf_ife_is_eth_skb() before choosing between encode
and decode. So a short egress skb is now dropped even when tcf_ife_encode()
would never have encoded it.

ife_get_sz() returns 0 when no metadata is present. One example is
ife_check_meta_u32() with no skb->mark and no configured value. In that
case tcf_ife_encode() returned before touching skb->data:

net/sched/act_ife.c:tcf_ife_encode() {
    ...
	if (!metalen) {		/* no metadata to send */
		...
		qstats_cpu_overlimit_inc(ife->common.cpu_qstats);
		return action;
	}
    ...
}

These sub-ETH_HLEN frames used to pass with the configured action and were
counted as overlimits. Now tcf_ife_act() drops them with TC_ACT_SHOT and
counts them as drops.

Could the commit message mention this change in behaviour and statistics?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v3.r0.20261103064310%40mojatatu.com

Re: [PATCH net v3 3/4] net/sched: sch_teql: reject qdiscs from another network namespace

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

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 fix checks that the slave device and the master are in the
  same network namespace only once, in teql_qdisc_init()…
- [Low] The commit message says only that a qdisc from another netns
  could attach to the init-net master "and change its MTU and flags".
- [Low] Moving skb_queue_head_init(&q->q) to the top of teql_qdisc_init()
  (sch_teql.c:203) is presented as groundwork for the new netns failure…

--

net/sched: sch_teql: reject qdiscs from another network namespace

This patch makes teql_qdisc_init() reject a teql qdisc whose device is
in a different network namespace than the teql master. It returns
-EOPNOTSUPP with an extack message. It also moves skb_queue_head_init()
of the private queue to the top of teql_qdisc_init(), so every
constructor failure path sees an initialized queue.

Link: https://lore.kernel.org/netdev/20261022144830.67948-1-shawnq1@163.com/
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v1.r2.20261031160050-3%40mojatatu.com
Link: https://lore.kernel.org/netdev/179366641863.488856.17368571591466000274@kernel.org/

> The issue:
> The teql master devices are created once in the initial network
> namespace and their Qdisc_ops are global. teql_qdisc_init() takes the
> teql_master embedded in the registered sch->ops and uses it without
> checking whether the new qdisc's device lives in the same network
> namespace as the master. A qdisc created in another netns can therefore
> attach to the init-net master and change its MTU and flags.

[Severity: Low]
Does this description understate the impact of the bug?

Besides changing the MTU and flags, teql_qdisc_init() links the foreign
qdisc into the master's slave ring, or makes it the first slave:

    rcu_assign_pointer(NEXT_SLAVE(first), sch);

>From then on, teql_master_xmit() round-robins init_net traffic sent
through teqlN onto the device from the other netns. There is no netns
check and no skb scrubbing:

    skb->dev = slave;
    if (!netif_xmit_frozen_or_stopped(slave_txq))
        rc = netdev_start_xmit(skb, slave, slave_txq, false);

If teql0 is already up with slaves, the attach still succeeds as long as
the foreign device has compatible flags and an MTU at least as large as
the master's. A CAP_NET_ADMIN holder in a user namespace can set both.

With a veth slave, the netns owner could capture or drop part of the
host's teql traffic. It could also answer ARP that __teql_resolve()
sends on that device.

The code change closes this path as well. Since stable and CVE triage
depend on the stated impact, could the commit message describe the
cross-netns traffic interception?

> Initialize the private skb queue before any validation so every
> constructor failure path, including qdisc_create_dflt(), which resets the
> qdisc before destroying it, sees an initialized empty queue.

[Severity: Low]
This isn't a bug, but should the commit message say that this reorder
also fixes an existing NULL pointer dereference?

Before this patch, three returns happened before skb_queue_head_init():
-EINVAL (hard_header_len), -ELOOP (m->dev == dev) and -EOPNOTSUPP
(non-root parent). On those paths q->q was still zeroed. When init
fails, qdisc_create_dflt() calls qdisc_put(), which leads to:

    qdisc_put()
      __qdisc_destroy()
        qdisc_reset()
          teql_reset()
            skb_queue_purge(&dat->q)

skb_queue_purge_reason() sees list->next == NULL. That is not equal to
list, so it treats the queue as non-empty, and skb_queue_splice_init()
dereferences a NULL first pointer.

This looks reachable with net.core.default_qdisc=teql0. mq, mqprio and
taprio create children with a non-root parent, and
attach_one_default_qdisc() on teql0 itself returns -ELOOP. Setting
default_qdisc needs CAP_NET_ADMIN in init_net.

Would it be worth calling out the crash fix explicitly, for backports
and Fixes: attribution?

> diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> index 70c948a26f809..dc21cd9704a25 100644
> --- a/net/sched/sch_teql.c
> +++ b/net/sched/sch_teql.c

[ ... ]

> @@ -211,9 +213,12 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	q->m = m;
> +	if (dev_net(dev) != dev_net(m->dev)) {
> +		NL_SET_ERR_MSG_MOD(extack, "teql master is in a different network namespace");
> +		return -EOPNOTSUPP;
> +	}

[Severity: Medium]
Is a netns check at attach time enough to keep the master and its
slaves in the same namespace?

teql_master_setup() never sets dev->netns_immutable. That means
__dev_change_net_namespace() lets teqlN move to another netns while
slaves are attached, because this is the only guard:

    if (dev->netns_immutable) {
        NL_SET_ERR_MSG(extack, "The interface netns is immutable");
        goto out;
    }

The move runs dev_shutdown() only on teqlN's own qdisc. The teql slave
qdiscs on the init-net devices stay linked in m->slaves:
teql_master_close() only stops the queue, and teql registers no netdev
notifier.

Once the master is in netns B, three things can happen:

The owner of B, who may be root in an unprivileged user namespace,
passes the new dev_net(dev) == dev_net(m->dev) check. It can then add
its own devices to a ring that still holds init-net slaves.

teql_master_open() takes the MTU and flags from the init-net slaves.

Traffic sent through teqlN in B reaches teql_master_xmit(), which sets
skb->dev = slave and calls netdev_start_xmit() on the init-net slave.
Through __teql_resolve(), it also calls ipv4_neigh_create() on the
init-net slave.

Moving teqlN out of init_net needs CAP_NET_ADMIN in init_net, so the
unprivileged user cannot do this alone. After this patch, though,
moving the master seems to be the only way a container can use teql.

Should the master be marked netns_immutable? Alternatively, should a
netns change of the master be refused, or detach the existing slaves?

> -	skb_queue_head_init(&q->q);
> +	q->m = m;
>  
>  	raw_spin_lock_irqsave(&m->slaves_lock, flags);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v3.r0.20261103064310%40mojatatu.com

Re: [PATCH net v3 4/4] selftests: tc-testing: teql: cover each attach rejection on its own

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit body describes the patch as a change relative to an
  unmerged v2 posting, not as a change to the tree.
- [Low] Both rejection cases check only that tc exits with 2 and that no
  matching qdisc is dumped.

--

selftests: tc-testing: teql: cover each attach rejection on its own

This commit moves tdc case 124e out of the nsPlugin test namespace and
into init_net with its own dummy device. Only the root-only check in
teql_qdisc_init() can then refuse it. It also adds case 6a06, which
attaches teql0 at root from the test namespace, so that only the
namespace check can refuse that one.

Link: https://lore.kernel.org/netdev/179366641863.488856.17368571591466000274@kernel.org/
Link: https://lore.kernel.org/netdev/179366641986.488856.2193313144081673076@kernel.org/

> It no longer proves that a child attach is
> refused as such, and the case added in the previous posting covers the
> namespace check alone.

[ ... ]

> Attach a root qdisc from the test namespace in the second case, so that
> only the namespace check can refuse that one.

[Severity: Low]
This isn't a bug, but these sentences read as if a namespace rejection
case is already in the tree. Is that the case?

Case 6a06 ("Reject TEQL attach from a different network namespace") is
added in full by this commit. The parent commit, "net/sched: sch_teql:
reject qdiscs from another network namespace", changes nothing under
tools/testing/selftests/tc-testing/.

The only place that says a new case is added is the v1 -> v2 and
v2 -> v3 changelog. That text sits below the "---", so it is dropped
when the patch is applied.

Could the body say outright that this commit adds new coverage for the
namespace rejection and moves 124e into init_net?

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
> index 0179c57104adb..ab5d0881c0070 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json

[ ... ]

> @@ -104,7 +100,35 @@
>          "verifyCmd": "$TC -s -j qdisc ls dev $DUMMY parent 1:1",
>          "matchJSON": [],
>          "teardown": [
> -            "$TC qdisc del dev $DUMMY root handle 1:"
> +            "$TC qdisc del dev $DUMMY root handle 1:",
> +            "$IP link del dev $DUMMY"
> +        ]
> +    },
> +    {
> +        "id": "6a06",
> +        "name": "Reject TEQL attach from a different network namespace",

[ ... ]

> +        "setup": [],
> +        "cmdUnderTest": "$TC qdisc add dev $DUMMY root handle 3: teql0",
> +        "expExitCode": "2",
> +        "verifyCmd": "$TC qdisc show dev $DUMMY",
> +        "matchPattern": "qdisc teql0 3: root",
> +        "matchCount": "0",

[Severity: Low]
Can this case pass even when the namespace check is not what refused
the attach?

tc exits with 2 for any netlink error. The verify step only checks that
no teql0 qdisc shows up in the dump. Case 124e has the same weakness:
it checks only expExitCode "2" and matchJSON [].

In teql_qdisc_init(), both rejections return the same errno. Only the
extack text tells them apart:

net/sched/sch_teql.c:teql_qdisc_init() {
    ...
	if (sch->parent != TC_H_ROOT) {
		NL_SET_ERR_MSG_MOD(extack, "teql can only be used as root");
		return -EOPNOTSUPP;
	}

	if (dev_net(dev) != dev_net(m->dev)) {
		NL_SET_ERR_MSG_MOD(extack, "teql master is in a different network namespace");
		return -EOPNOTSUPP;
	}
    ...
}

The tc-testing config sets CONFIG_NET_SCH_TEQL=m. Unlike the other
sch_* modules, sch_teql.c has no MODULE_ALIAS_NET_SCH(). So the
alias-based request_module() never autoloads it, and the lookup fails:

net/sched/sch_api.c:qdisc_create() {
    ...
	ops = qdisc_lookup_ops(kind);
	if (!ops) {
		err = -ENOENT;
		NL_SET_ERR_MSG(extack, "Specified qdisc kind is unknown");
		goto err_out;
	}
    ...
}

When that happens, tc still exits with 2 and nothing named teql0
appears. Both 6a06 and 124e then pass, whether or not the kernel has
either check. That includes a kernel without the namespace fix.

The module is loaded only by try_modprobe() in tdc.sh. That function
just prints "skipping" when modprobe fails. Running tdc.py directly,
for example with -e 6a06 or -c teql, never loads it.

The commit message says "only the root-only check can refuse it" and
"only the namespace check can refuse that one". That only holds when
sch_teql is loaded.

Would it make sense for each case to match the specific extack
message? Another option is to fail when the teql0 kind is unavailable.

The positive teql cases would fail in the same setup, so the suite as a
whole would still show that something is wrong.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v3.r0.20261103064310%40mojatatu.com

Re: [PATCH net v3 2/4] net/sched: act_ife: require a full Ethernet header on egress

[Jamal Hadi Salim]

On Sat, Nov 7, 2026 at 9:50 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
> 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 is titled "require a full Ethernet header on
>   egress".

Yeah, that is a bit of an overclaim  - but i will not be resending
just for this. If there are code changes needed in the next patches i
will resend.

> - [Low] The commit message says "A short egress skb is now dropped
>   instead of being encoded." The patch actually drops every egress skb…

Same here. No resend coming.

cheers,
jamal
> --
>
> net/sched: act_ife: require a full Ethernet header on egress
>
> On egress, tcf_ife_is_eth_skb() now requires pskb_may_pull(skb, ETH_HLEN)
> to succeed. Without that check, ife_encode() could copy ETH_HLEN bytes from
> past the linear data of a short skb. The ingress mac_len check is unchanged.
>
> Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260821164031.32824-1-victor%40mojatatu.com
> Link: https://sashiko.dev/#/patchset/20260821164031.32824-1-victor@mojatatu.com
> Link: https://lore.kernel.org/netdev/20261022144736.61227-1-shawnq1@163.com/
>
> > diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
> > index 6b23a3965121e..5a527da005661 100644
> > --- a/net/sched/act_ife.c
> > +++ b/net/sched/act_ife.c
> > @@ -878,13 +878,16 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
> >   * is an Ethernet header because the skb could've been redirected with mirred
> >   * from a non-Ethernet device.
> >   */
> > -static bool tcf_ife_is_eth_skb(const struct sk_buff *skb)
> > +static bool tcf_ife_is_eth_skb(struct sk_buff *skb)
> >  {
> >       if (skb->dev->type != ARPHRD_ETHER &&
> >           skb->dev->type != ARPHRD_LOOPBACK)
> >               return false;
> >
> > -     return !skb_at_tc_ingress(skb) || skb->mac_len == ETH_HLEN;
> > +     if (skb_at_tc_ingress(skb))
> > +             return skb->mac_len == ETH_HLEN;
> > +
> > +     return pskb_may_pull(skb, ETH_HLEN);
>
> [Severity: Medium]
> The subject says "require a full Ethernet header on egress". The commit
> message also says tcf_ife_is_eth_skb() "returns true even when the skb has
> no Ethernet header".
>
> Does the new egress branch check that the skb has an Ethernet header, or
> only that ETH_HLEN bytes are in the linear area?
>
> Take the reproducer setup from the commit message and send an IP packet of
> 14 bytes or more instead of a short one:
>
> tun_get_user() (IFF_TUN)
>     skb_reset_network_header(skb);       /* mac_len is 0 */
> tcf_mirred_to_dev()
>     mac_len = at_ingress ? skb->mac_len : skb_network_offset(skb);
>     skb_push_rcsum(skb_to_send, mac_len); /* pushes 0 bytes */
> lo egress: tcf_ife_act()
>     tcf_ife_is_eth_skb()
>         pskb_may_pull(skb, ETH_HLEN)      /* succeeds */
>     tcf_ife_encode()->ife_encode()
>         iethh = (struct ethhdr *) skb->data;
>         __skb_push(skb, total_push);
>         memcpy(skb->data, iethh, ETH_HLEN);
>
> At this point skb->data still points at the IP header. The first 14 IP
> header bytes are copied as the inner "original" Ethernet header. They also
> end up in the outer h_dest/h_source unless eth_dst/eth_src are configured.
>
> The over-read past the linear data is fixed, and these bytes are packet
> data the sender controls, not kernel memory. This behaviour for L3 skbs of
> 14 bytes or more was already there after commit 5b483f7791b0. Still, a
> non-Ethernet frame can be encoded on egress as if it had an Ethernet
> header, which is what the subject and issue statement say this patch
> prevents.
>
> The comment above the function also still reads as a pure check:
>
>     IFE encapsulates the original Ethernet header and, on decode, expects
>     to find one, so it can only ever work on skbs that carry one.
>
> It does not say that the egress branch only checks linear length, or that
> the call can now pull and so modify the skb.
>
> Should the subject, issue statement and function comment say that egress
> only checks linear length? Or should the egress branch also check that the
> L2 header really is Ethernet?
>
> [Severity: Low]
> The commit message says "A short egress skb is now dropped instead of
> being encoded." Is that the only behaviour change?
>
> tcf_ife_act() calls tcf_ife_is_eth_skb() before choosing between encode
> and decode. So a short egress skb is now dropped even when tcf_ife_encode()
> would never have encoded it.
>
> ife_get_sz() returns 0 when no metadata is present. One example is
> ife_check_meta_u32() with no skb->mark and no configured value. In that
> case tcf_ife_encode() returned before touching skb->data:
>
> net/sched/act_ife.c:tcf_ife_encode() {
>     ...
>         if (!metalen) {         /* no metadata to send */
>                 ...
>                 qstats_cpu_overlimit_inc(ife->common.cpu_qstats);
>                 return action;
>         }
>     ...
> }
>
> These sub-ETH_HLEN frames used to pass with the configured action and were
> counted as overlimits. Now tcf_ife_act() drops them with TC_ACT_SHOT and
> counts them as drops.
>
> Could the commit message mention this change in behaviour and statistics?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v3.r0.20261103064310%40mojatatu.com

Re: [PATCH net v3 3/4] net/sched: sch_teql: reject qdiscs from another network namespace

[Jamal Hadi Salim]

On Sat, Nov 7, 2026 at 9:50 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 2
>
> 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 fix checks that the slave device and the master are in the
>   same network namespace only once, in teql_qdisc_init()…
> - [Low] The commit message says only that a qdisc from another netns
>   could attach to the init-net master "and change its MTU and flags".
> - [Low] Moving skb_queue_head_init(&q->q) to the top of teql_qdisc_init()
>   (sch_teql.c:203) is presented as groundwork for the new netns failure…
>

Again, cosmetics i will fix if i have to resend.
The netns_immutable gap is pre-existing and needs CAP_NET_ADMIN in
init_net, so it stays the net-next follow-up.
The reorder keeps this patch's Fixes: because the namespace refusal is
itself reachable through qdisc_create_dflt() -> qdisc_reset() ->
teql_reset(), and if i have to resend then I will state the
interception impact in the body...


cheers,
jamal
> --
>
> net/sched: sch_teql: reject qdiscs from another network namespace
>
> This patch makes teql_qdisc_init() reject a teql qdisc whose device is
> in a different network namespace than the teql master. It returns
> -EOPNOTSUPP with an extack message. It also moves skb_queue_head_init()
> of the private queue to the top of teql_qdisc_init(), so every
> constructor failure path sees an initialized queue.
>
> Link: https://lore.kernel.org/netdev/20261022144830.67948-1-shawnq1@163.com/
> Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v1.r2.20261031160050-3%40mojatatu.com
> Link: https://lore.kernel.org/netdev/179366641863.488856.17368571591466000274@kernel.org/
>
> > The issue:
> > The teql master devices are created once in the initial network
> > namespace and their Qdisc_ops are global. teql_qdisc_init() takes the
> > teql_master embedded in the registered sch->ops and uses it without
> > checking whether the new qdisc's device lives in the same network
> > namespace as the master. A qdisc created in another netns can therefore
> > attach to the init-net master and change its MTU and flags.
>
> [Severity: Low]
> Does this description understate the impact of the bug?
>
> Besides changing the MTU and flags, teql_qdisc_init() links the foreign
> qdisc into the master's slave ring, or makes it the first slave:
>
>     rcu_assign_pointer(NEXT_SLAVE(first), sch);
>
> >From then on, teql_master_xmit() round-robins init_net traffic sent
> through teqlN onto the device from the other netns. There is no netns
> check and no skb scrubbing:
>
>     skb->dev = slave;
>     if (!netif_xmit_frozen_or_stopped(slave_txq))
>         rc = netdev_start_xmit(skb, slave, slave_txq, false);
>
> If teql0 is already up with slaves, the attach still succeeds as long as
> the foreign device has compatible flags and an MTU at least as large as
> the master's. A CAP_NET_ADMIN holder in a user namespace can set both.
>
> With a veth slave, the netns owner could capture or drop part of the
> host's teql traffic. It could also answer ARP that __teql_resolve()
> sends on that device.
>
> The code change closes this path as well. Since stable and CVE triage
> depend on the stated impact, could the commit message describe the
> cross-netns traffic interception?
>
> > Initialize the private skb queue before any validation so every
> > constructor failure path, including qdisc_create_dflt(), which resets the
> > qdisc before destroying it, sees an initialized empty queue.
>
> [Severity: Low]
> This isn't a bug, but should the commit message say that this reorder
> also fixes an existing NULL pointer dereference?
>
> Before this patch, three returns happened before skb_queue_head_init():
> -EINVAL (hard_header_len), -ELOOP (m->dev == dev) and -EOPNOTSUPP
> (non-root parent). On those paths q->q was still zeroed. When init
> fails, qdisc_create_dflt() calls qdisc_put(), which leads to:
>
>     qdisc_put()
>       __qdisc_destroy()
>         qdisc_reset()
>           teql_reset()
>             skb_queue_purge(&dat->q)
>
> skb_queue_purge_reason() sees list->next == NULL. That is not equal to
> list, so it treats the queue as non-empty, and skb_queue_splice_init()
> dereferences a NULL first pointer.
>
> This looks reachable with net.core.default_qdisc=teql0. mq, mqprio and
> taprio create children with a non-root parent, and
> attach_one_default_qdisc() on teql0 itself returns -ELOOP. Setting
> default_qdisc needs CAP_NET_ADMIN in init_net.
>
> Would it be worth calling out the crash fix explicitly, for backports
> and Fixes: attribution?
>
> > diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> > index 70c948a26f809..dc21cd9704a25 100644
> > --- a/net/sched/sch_teql.c
> > +++ b/net/sched/sch_teql.c
>
> [ ... ]
>
> > @@ -211,9 +213,12 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt,
> >               return -EOPNOTSUPP;
> >       }
> >
> > -     q->m = m;
> > +     if (dev_net(dev) != dev_net(m->dev)) {
> > +             NL_SET_ERR_MSG_MOD(extack, "teql master is in a different network namespace");
> > +             return -EOPNOTSUPP;
> > +     }
>
> [Severity: Medium]
> Is a netns check at attach time enough to keep the master and its
> slaves in the same namespace?
>
> teql_master_setup() never sets dev->netns_immutable. That means
> __dev_change_net_namespace() lets teqlN move to another netns while
> slaves are attached, because this is the only guard:
>
>     if (dev->netns_immutable) {
>         NL_SET_ERR_MSG(extack, "The interface netns is immutable");
>         goto out;
>     }
>
> The move runs dev_shutdown() only on teqlN's own qdisc. The teql slave
> qdiscs on the init-net devices stay linked in m->slaves:
> teql_master_close() only stops the queue, and teql registers no netdev
> notifier.
>
> Once the master is in netns B, three things can happen:
>
> The owner of B, who may be root in an unprivileged user namespace,
> passes the new dev_net(dev) == dev_net(m->dev) check. It can then add
> its own devices to a ring that still holds init-net slaves.
>
> teql_master_open() takes the MTU and flags from the init-net slaves.
>
> Traffic sent through teqlN in B reaches teql_master_xmit(), which sets
> skb->dev = slave and calls netdev_start_xmit() on the init-net slave.
> Through __teql_resolve(), it also calls ipv4_neigh_create() on the
> init-net slave.
>
> Moving teqlN out of init_net needs CAP_NET_ADMIN in init_net, so the
> unprivileged user cannot do this alone. After this patch, though,
> moving the master seems to be the only way a container can use teql.
>
> Should the master be marked netns_immutable? Alternatively, should a
> netns change of the master be refused, or detach the existing slaves?
>
> > -     skb_queue_head_init(&q->q);
> > +     q->m = m;
> >
> >       raw_spin_lock_irqsave(&m->slaves_lock, flags);
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v3.r0.20261103064310%40mojatatu.com

Re: [PATCH net v3 4/4] selftests: tc-testing: teql: cover each attach rejection on its own

[Jamal Hadi Salim]

On Sat, Nov 7, 2026 at 9:50 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The commit body describes the patch as a change relative to an
>   unmerged v2 posting, not as a change to the tree.
> - [Low] Both rejection cases check only that tc exits with 2 and that no
>   matching qdisc is dumped.
>

Again cosmetics - so far no reason to resend.
6a06 is added by this commit, not by a previous posting, if i resend i
would reword to state that.
The module point cannot cause a pass here - it would fail several other tests..


cheers,
jamal
> --
>
> selftests: tc-testing: teql: cover each attach rejection on its own
>
> This commit moves tdc case 124e out of the nsPlugin test namespace and
> into init_net with its own dummy device. Only the root-only check in
> teql_qdisc_init() can then refuse it. It also adds case 6a06, which
> attaches teql0 at root from the test namespace, so that only the
> namespace check can refuse that one.
>
> Link: https://lore.kernel.org/netdev/179366641863.488856.17368571591466000274@kernel.org/
> Link: https://lore.kernel.org/netdev/179366641986.488856.2193313144081673076@kernel.org/
>
> > It no longer proves that a child attach is
> > refused as such, and the case added in the previous posting covers the
> > namespace check alone.
>
> [ ... ]
>
> > Attach a root qdisc from the test namespace in the second case, so that
> > only the namespace check can refuse that one.
>
> [Severity: Low]
> This isn't a bug, but these sentences read as if a namespace rejection
> case is already in the tree. Is that the case?
>
> Case 6a06 ("Reject TEQL attach from a different network namespace") is
> added in full by this commit. The parent commit, "net/sched: sch_teql:
> reject qdiscs from another network namespace", changes nothing under
> tools/testing/selftests/tc-testing/.
>
> The only place that says a new case is added is the v1 -> v2 and
> v2 -> v3 changelog. That text sits below the "---", so it is dropped
> when the patch is applied.
>
> Could the body say outright that this commit adds new coverage for the
> namespace rejection and moves 124e into init_net?
>
> > diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
> > index 0179c57104adb..ab5d0881c0070 100644
> > --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
> > +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/teql.json
>
> [ ... ]
>
> > @@ -104,7 +100,35 @@
> >          "verifyCmd": "$TC -s -j qdisc ls dev $DUMMY parent 1:1",
> >          "matchJSON": [],
> >          "teardown": [
> > -            "$TC qdisc del dev $DUMMY root handle 1:"
> > +            "$TC qdisc del dev $DUMMY root handle 1:",
> > +            "$IP link del dev $DUMMY"
> > +        ]
> > +    },
> > +    {
> > +        "id": "6a06",
> > +        "name": "Reject TEQL attach from a different network namespace",
>
> [ ... ]
>
> > +        "setup": [],
> > +        "cmdUnderTest": "$TC qdisc add dev $DUMMY root handle 3: teql0",
> > +        "expExitCode": "2",
> > +        "verifyCmd": "$TC qdisc show dev $DUMMY",
> > +        "matchPattern": "qdisc teql0 3: root",
> > +        "matchCount": "0",
>
> [Severity: Low]
> Can this case pass even when the namespace check is not what refused
> the attach?
>
> tc exits with 2 for any netlink error. The verify step only checks that
> no teql0 qdisc shows up in the dump. Case 124e has the same weakness:
> it checks only expExitCode "2" and matchJSON [].
>
> In teql_qdisc_init(), both rejections return the same errno. Only the
> extack text tells them apart:
>
> net/sched/sch_teql.c:teql_qdisc_init() {
>     ...
>         if (sch->parent != TC_H_ROOT) {
>                 NL_SET_ERR_MSG_MOD(extack, "teql can only be used as root");
>                 return -EOPNOTSUPP;
>         }
>
>         if (dev_net(dev) != dev_net(m->dev)) {
>                 NL_SET_ERR_MSG_MOD(extack, "teql master is in a different network namespace");
>                 return -EOPNOTSUPP;
>         }
>     ...
> }
>
> The tc-testing config sets CONFIG_NET_SCH_TEQL=m. Unlike the other
> sch_* modules, sch_teql.c has no MODULE_ALIAS_NET_SCH(). So the
> alias-based request_module() never autoloads it, and the lookup fails:
>
> net/sched/sch_api.c:qdisc_create() {
>     ...
>         ops = qdisc_lookup_ops(kind);
>         if (!ops) {
>                 err = -ENOENT;
>                 NL_SET_ERR_MSG(extack, "Specified qdisc kind is unknown");
>                 goto err_out;
>         }
>     ...
> }
>
> When that happens, tc still exits with 2 and nothing named teql0
> appears. Both 6a06 and 124e then pass, whether or not the kernel has
> either check. That includes a kernel without the namespace fix.
>
> The module is loaded only by try_modprobe() in tdc.sh. That function
> just prints "skipping" when modprobe fails. Running tdc.py directly,
> for example with -e 6a06 or -c teql, never loads it.
>
> The commit message says "only the root-only check can refuse it" and
> "only the namespace check can refuse that one". That only holds when
> sch_teql is loaded.
>
> Would it make sense for each case to match the specific extack
> message? Another option is to fail when the teql0 kind is unavailable.
>
> The positive teql cases would fail in the same setup, so the suite as a
> whole would still show that something is wrong.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v3.r0.20261103064310%40mojatatu.com

Re: [PATCH net v3 1/4] net/sched: act_ife: encode configured zero metadata values

[Simon Horman]

On Tue, Nov 03, 2026 at 06:50:29AM -0500, Jamal Hadi Salim wrote:
> Reported by shaos <shawnq1@163.com> with a reproducer for the 32-bit
> case, which was reproduced and verified in our test environment.
> 
> The issue:
> ife_check_meta_u32() and ife_check_meta_u16() both reserve eight bytes
> of metadata for a configured value even when that value is zero, but the
> matching encoder returns without calling ife_tlv_meta_encode(). The
> encoder therefore writes neither the TLV header nor its value, while the
> enclosing IFE length still includes the reserved bytes, exposing stale
> skb headroom contents to the peer.
> 
> The 32-bit helper has been present since the IFE action was introduced
> (v4.6); the 16-bit helper was added with the 16-bit metadata helpers used
> by the skbtcindex callback (v4.9). Both defects predate the oldest
> supported stable kernel (5.10), so they are fixed together in this patch.
> Each hunk keeps its own Fixes: tag for provenance.
> 
> The fix:
> Encode any configured value, including zero; skip the TLV only when
> there is neither a configured nor a runtime value. The runtime fallback
> is unchanged.
> 
> A raw TCA_IFE_METALST netlink request is needed to express the
> configured zero: the IFE_META_SKBMARK attribute carries four zero
> payload bytes for the u32 helper, and IFE_META_TCINDEX two zero payload
> bytes for the 16-bit helper (not an absent or zero-length attribute).
> Stock tc cannot build these requests; "use mark 0x0" emits no mark
> attribute and "allow mark" emits a zero-length one.
> 
> Conditions to recreate the bug:
> CONFIG_NET_ACT_IFE=y, CONFIG_NET_IFE_SKBMARK=y,
> CONFIG_NET_IFE_SKBTCINDEX=y, CONFIG_NET_CLS_MATCHALL=y,
> CONFIG_NET_SCH_INGRESS=y, CONFIG_USER_NS=y, CONFIG_NET_NS=y,
> CONFIG_VETH=y. In a fresh user and network namespace create a veth pair
> and attach clsact to the egress side. Install a matchall filter whose
> ife encode action carries a zero skb mark (or a zero tcindex) in
> TCA_IFE_METALST. Send a frame through the veth and inspect the IFE
> metadata in the received frame. The metadata region is eight bytes but
> holds neither a valid TLV header nor a value; it exposes prior headroom
> contents. Setting the value to a nonzero one does not trigger the leak.
> The namespace needs CAP_NET_ADMIN; no additional privilege is required.
> With the fix the zero TLVs are 0001000800000000 (u32) and
> 0005000600000000 (u16).
> 
> Fixes: ef6980b6becb ("introduce IFE action")
> Fixes: 6a5d58b67e20 ("net sched ife action: add 16 bit helpers")
> Reported-by: shaos <shawnq1@163.com>
> Link: https://lore.kernel.org/netdev/20261022144739.61342-1-shawnq1@163.com/
> Cc: stable@vger.kernel.org
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net v3 2/4] net/sched: act_ife: require a full Ethernet header on egress

[Simon Horman]

On Sat, Nov 07, 2026 at 05:10:07PM -0500, Jamal Hadi Salim wrote:
> On Sat, Nov 7, 2026 at 9:50 AM <netdev-bot+sashiko@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 2 potential
> > issue(s) to consider.
> >
> > Critical: 0 · High: 0 · Medium: 1 · Low: 1
> >
> > 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 is titled "require a full Ethernet header on
> >   egress".
> 
> Yeah, that is a bit of an overclaim  - but i will not be resending
> just for this. If there are code changes needed in the next patches i
> will resend.
> 
> > - [Low] The commit message says "A short egress skb is now dropped
> >   instead of being encoded." The patch actually drops every egress skb…
> 
> Same here. No resend coming.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net v3 3/4] net/sched: sch_teql: reject qdiscs from another network namespace

[Simon Horman]

On Sat, Nov 07, 2026 at 05:13:10PM -0500, Jamal Hadi Salim wrote:
> On Sat, Nov 7, 2026 at 9:50 AM <netdev-bot+sashiko@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 3 potential
> > issue(s) to consider.
> >
> > Critical: 0 · High: 0 · Medium: 1 · Low: 2
> >
> > 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 fix checks that the slave device and the master are in the
> >   same network namespace only once, in teql_qdisc_init()…
> > - [Low] The commit message says only that a qdisc from another netns
> >   could attach to the init-net master "and change its MTU and flags".
> > - [Low] Moving skb_queue_head_init(&q->q) to the top of teql_qdisc_init()
> >   (sch_teql.c:203) is presented as groundwork for the new netns failure…
> >
> 
> Again, cosmetics i will fix if i have to resend.
> The netns_immutable gap is pre-existing and needs CAP_NET_ADMIN in
> init_net, so it stays the net-next follow-up.
> The reorder keeps this patch's Fixes: because the namespace refusal is
> itself reachable through qdisc_create_dflt() -> qdisc_reset() ->
> teql_reset(), and if i have to resend then I will state the
> interception impact in the body...

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net v3 4/4] selftests: tc-testing: teql: cover each attach rejection on its own

[Simon Horman]

On Sat, Nov 07, 2026 at 05:15:49PM -0500, Jamal Hadi Salim wrote:
> On Sat, Nov 7, 2026 at 9:50 AM <netdev-bot+sashiko@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found more severe
> > issue(s) in other patches of this series. This mail is only an FYI, in case
> > you want to address the nit picks below when you respin.
> >
> > Low priority / nit picks for your consideration:
> >
> > - [Low] The commit body describes the patch as a change relative to an
> >   unmerged v2 posting, not as a change to the tree.
> > - [Low] Both rejection cases check only that tc exits with 2 and that no
> >   matching qdisc is dumped.
> >
> 
> Again cosmetics - so far no reason to resend.
> 6a06 is added by this commit, not by a previous posting, if i resend i
> would reword to state that.
> The module point cannot cause a pass here - it would fail several other tests..

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net v3 0/4] net/sched: fix short-skb metadata and namespace boundaries

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  3 Nov 2026 06:50:28 -0500 you wrote:
> This series fixes three independent boundary problems in net/sched, and
> covers the third one with tc-testing cases.
> 
> 1/4 act_ife: encode a configured zero metadata value.  The u32 and u16
> presence callbacks reserve an 8-byte TLV for a configured zero value, but
> the encoders returned without writing it, leaking stale skb headroom bytes.
> 
> [...]

Here is the summary with links:
  - [net,v3,1/4] net/sched: act_ife: encode configured zero metadata values
    https://git.kernel.org/netdev/net/c/7d02c07c738b
  - [net,v3,2/4] net/sched: act_ife: require a full Ethernet header on egress
    https://git.kernel.org/netdev/net/c/11dbce090834
  - [net,v3,3/4] net/sched: sch_teql: reject qdiscs from another network namespace
    https://git.kernel.org/netdev/net/c/1ae84bdd736f
  - [net,v3,4/4] selftests: tc-testing: teql: cover each attach rejection on its own
    https://git.kernel.org/netdev/net/c/367a50c3233e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html