[PATCH net 1/3] net/sched: cls_fw: fix NULL pointer dereference on shared blocks

[PATCH net 1/3] net/sched: cls_fw: fix NULL pointer dereference on shared blocks

[Xiang Mei]

The old-method path in fw_classify() calls tcf_block_q() and
dereferences q->handle. Shared blocks leave block->q NULL, causing a
NULL deref when an empty cls_fw filter is attached to a shared block
and a packet with a nonzero major skb mark is classified.

Check tcf_block_shared() before accessing block->q and return -1 (no
match) for shared blocks, consistent with cls_u32's tc_u_common_ptr().

The fixed null-ptr-deref calling stack:
 KASAN: null-ptr-deref in range [0x0000000000000038-0x000000000000003f]
 RIP: 0010:fw_classify (net/sched/cls_fw.c:81)
 Call Trace:
  tcf_classify (./include/net/tc_wrapper.h:197 net/sched/cls_api.c:1764 net/sched/cls_api.c:1860)
  tc_run (net/core/dev.c:4401)
  __dev_queue_xmit (net/core/dev.c:4535 net/core/dev.c:4790)

Fixes: 1abf272022cf ("net: sched: tcindex, fw, flow: use tcf_block_q helper to get struct Qdisc")
Reported-by: Weiming Shi <bestswngs@gmail.com>
Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
 net/sched/cls_fw.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index be81c108179d..caf17ab3be87 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -74,9 +74,14 @@ TC_INDIRECT_SCOPE int fw_classify(struct sk_buff *skb,
 			}
 		}
 	} else {
-		struct Qdisc *q = tcf_block_q(tp->chain->block);
+		struct tcf_block *block = tp->chain->block;
+		struct Qdisc *q;
+
+		if (tcf_block_shared(block))
+			return -1;
 
 		/* Old method: classify the packet using its skb mark. */
+		q = tcf_block_q(block);
 		if (id && (TC_H_MAJ(id) == 0 ||
 			   !(TC_H_MAJ(id ^ q->handle)))) {
 			res->classid = id;
-- 
2.43.0

[PATCH net 2/3] net/sched: cls_flow: fix NULL pointer dereference on shared blocks

[Xiang Mei]

flow_change() calls tcf_block_q() and dereferences q->handle to derive
a default baseclass. Shared blocks leave block->q NULL, causing a NULL
deref when a flow filter without a fully qualified baseclass is created
on a shared block.

Check tcf_block_shared() before accessing block->q and return -EINVAL
for shared blocks. This avoids the null-deref shown below:

=======================================================================
KASAN: null-ptr-deref in range [0x0000000000000038-0x000000000000003f]
RIP: 0010:flow_change (net/sched/cls_flow.c:508)
Call Trace:
 tc_new_tfilter (net/sched/cls_api.c:2432)
 rtnetlink_rcv_msg (net/core/rtnetlink.c:6980)
 [...]
=======================================================================

Fixes: 1abf272022cf ("net: sched: tcindex, fw, flow: use tcf_block_q helper to get struct Qdisc")
Reported-by: Weiming Shi <bestswngs@gmail.com>
Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
 net/sched/cls_flow.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 339c664beff6..26077681c9b6 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -503,8 +503,13 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
 		}
 
 		if (TC_H_MAJ(baseclass) == 0) {
-			struct Qdisc *q = tcf_block_q(tp->chain->block);
+			struct tcf_block *block = tp->chain->block;
+			struct Qdisc *q;
 
+			if (tcf_block_shared(block))
+				goto err2;
+
+			q = tcf_block_q(block);
 			baseclass = TC_H_MAKE(q->handle, baseclass);
 		}
 		if (TC_H_MIN(baseclass) == 0)
-- 
2.43.0

[PATCH net 3/3] selftests/tc-testing: add tests for cls_fw and cls_flow on shared blocks

[Xiang Mei]

Regression tests for the shared-block NULL derefs fixed in the previous
two patches:
  - fw: attach an empty fw filter to a shared block and send traffic.
  - flow: create a flow filter on a shared block without a baseclass.

Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
 .../tc-testing/tc-tests/infra/filter.json     | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/infra/filter.json b/tools/testing/selftests/tc-testing/tc-tests/infra/filter.json
index 8d10042b489b..b749087d1691 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/filter.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/filter.json
@@ -22,5 +22,50 @@
         "teardown": [
             "$TC qdisc del dev $DUMMY root handle 1: htb default 1"
         ]
+    },
+    {
+        "id": "b7e3",
+        "name": "Empty fw filter on shared block - no NULL deref in fw_classify",
+        "category": [
+            "filter",
+            "fw"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$TC qdisc add dev $DEV1 ingress_block 1 clsact",
+            "$TC filter add block 1 protocol ip prio 1 fw"
+        ],
+        "cmdUnderTest": "$IP addr add 10.10.10.1/24 dev $DEV1 && ping -I$DEV1 -c1 -W1 10.10.10.1 || true",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DEV1",
+        "matchPattern": "clsact",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DEV1 clsact"
+        ]
+    },
+    {
+        "id": "c8f4",
+        "name": "Flow filter on shared block without baseclass - no NULL deref in flow_change",
+        "category": [
+            "filter",
+            "flow"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$TC qdisc add dev $DEV1 ingress_block 1 clsact"
+        ],
+        "cmdUnderTest": "$TC filter add block 1 protocol ip prio 1 handle 1 flow map key dst",
+        "expExitCode": "2",
+        "verifyCmd": "$TC filter show block 1",
+        "matchPattern": "flow",
+        "matchCount": "0",
+        "teardown": [
+            "$TC qdisc del dev $DEV1 clsact"
+        ]
     }
 ]
-- 
2.43.0

Re: [PATCH net 1/3] net/sched: cls_fw: fix NULL pointer dereference on shared blocks

[Jamal Hadi Salim]

On Fri, Mar 27, 2026 at 2:13 AM Xiang Mei <xmei5@asu.edu> wrote:
>
> The old-method path in fw_classify() calls tcf_block_q() and
> dereferences q->handle. Shared blocks leave block->q NULL, causing a
> NULL deref when an empty cls_fw filter is attached to a shared block
> and a packet with a nonzero major skb mark is classified.
>
> Check tcf_block_shared() before accessing block->q and return -1 (no
> match) for shared blocks, consistent with cls_u32's tc_u_common_ptr().
>
> The fixed null-ptr-deref calling stack:
>  KASAN: null-ptr-deref in range [0x0000000000000038-0x000000000000003f]
>  RIP: 0010:fw_classify (net/sched/cls_fw.c:81)
>  Call Trace:
>   tcf_classify (./include/net/tc_wrapper.h:197 net/sched/cls_api.c:1764 net/sched/cls_api.c:1860)
>   tc_run (net/core/dev.c:4401)
>   __dev_queue_xmit (net/core/dev.c:4535 net/core/dev.c:4790)
>
> Fixes: 1abf272022cf ("net: sched: tcindex, fw, flow: use tcf_block_q helper to get struct Qdisc")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Signed-off-by: Xiang Mei <xmei5@asu.edu>

Thanks for also providing the tdc tests. Looks like a bizarre bug—and
in my mind the question is does fw or flow even factor into tc blocks?
Please give me time to review if the approach makes sense - perhaps
this weekend.

cheers,
jamal

> ---
>  net/sched/cls_fw.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
> index be81c108179d..caf17ab3be87 100644
> --- a/net/sched/cls_fw.c
> +++ b/net/sched/cls_fw.c
> @@ -74,9 +74,14 @@ TC_INDIRECT_SCOPE int fw_classify(struct sk_buff *skb,
>                         }
>                 }
>         } else {
> -               struct Qdisc *q = tcf_block_q(tp->chain->block);
> +               struct tcf_block *block = tp->chain->block;
> +               struct Qdisc *q;
> +
> +               if (tcf_block_shared(block))
> +                       return -1;
>
>                 /* Old method: classify the packet using its skb mark. */
> +               q = tcf_block_q(block);
>                 if (id && (TC_H_MAJ(id) == 0 ||
>                            !(TC_H_MAJ(id ^ q->handle)))) {
>                         res->classid = id;
> --
> 2.43.0
>

Re: [PATCH net 1/3] net/sched: cls_fw: fix NULL pointer dereference on shared blocks

[Jamal Hadi Salim]

On Fri, Mar 27, 2026 at 10:11 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Fri, Mar 27, 2026 at 2:13 AM Xiang Mei <xmei5@asu.edu> wrote:
> >
> > The old-method path in fw_classify() calls tcf_block_q() and
> > dereferences q->handle. Shared blocks leave block->q NULL, causing a
> > NULL deref when an empty cls_fw filter is attached to a shared block
> > and a packet with a nonzero major skb mark is classified.
> >
> > Check tcf_block_shared() before accessing block->q and return -1 (no
> > match) for shared blocks, consistent with cls_u32's tc_u_common_ptr().
> >
> > The fixed null-ptr-deref calling stack:
> >  KASAN: null-ptr-deref in range [0x0000000000000038-0x000000000000003f]
> >  RIP: 0010:fw_classify (net/sched/cls_fw.c:81)
> >  Call Trace:
> >   tcf_classify (./include/net/tc_wrapper.h:197 net/sched/cls_api.c:1764 net/sched/cls_api.c:1860)
> >   tc_run (net/core/dev.c:4401)
> >   __dev_queue_xmit (net/core/dev.c:4535 net/core/dev.c:4790)
> >
> > Fixes: 1abf272022cf ("net: sched: tcindex, fw, flow: use tcf_block_q helper to get struct Qdisc")
> > Reported-by: Weiming Shi <bestswngs@gmail.com>
> > Signed-off-by: Xiang Mei <xmei5@asu.edu>
>
> Thanks for also providing the tdc tests. Looks like a bizarre bug—and
> in my mind the question is does fw or flow even factor into tc blocks?
> Please give me time to review if the approach makes sense - perhaps
> this weekend.
>
Since the fix is exactly as what u32 does - why not move
tc_u_common_ptr() to a header file (pkt_cls.h)? then reuse it in u32,
fw, and flow.

I am still questioning the value of using blocks with these two
classifiers and unfortunately the commit message of  1abf272022cf was
not helpful.
The testcases look good. Please add a letter head, perhaps quote the
comment in tc_u_common_ptr() since it gives a good description of this
whole dance.

cheers,
jamal

[PATCH] net/sched: skip reverse bind walk after failed class delete

[Qi Tang]

__tc_ctl_tclass() unconditionally calls tc_bind_tclass() after
RTM_DELTCLASS even when tclass_del_notify() fails.  On
ingress/clsact qdiscs with shared blocks, the delete always fails
with -EOPNOTSUPP, but the subsequent bind walk still runs.
tc_bind_tclass() reaches tcf_node_bind() which calls tcf_block_q()
on the shared block and dereferences the NULL qdisc pointer.

Only perform the reverse bind walk when the class delete succeeds.

Triggered by issuing RTM_DELTCLASS on an ingress/clsact pseudo-class
that carries a shared block with a classifier using bind_class:

  $ tc qdisc add dev veth0 egress_block 22 clsact
  $ tc filter add block 22 pref 1 protocol ip handle 0x1 \
      fw classid ffff:fff3 action drop
  $ tc class del dev veth0 classid ffff:fff3
  RTNETLINK answers: Operation not supported
  Kernel panic - not syncing: Attempted to kill init!
  exitcode=0x00000700

The NULL dereference occurs in tcf_node_bind() when sch_tree_lock()
is called with the NULL return from tcf_block_q() on a shared block.

Fixes: 07d79fc7d94e ("net_sched: add reverse binding for tc class")
Signed-off-by: Qi Tang <tpluszz77@gmail.com>
---
 net/sched/sch_api.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index cc43e3f7574f..50d311ff1f2d 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -2251,7 +2251,8 @@ static int __tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
 		case RTM_DELTCLASS:
 			err = tclass_del_notify(net, cops, skb, n, q, cl, extack);
 			/* Unbind the class with flilters with 0 */
-			tc_bind_tclass(q, portid, clid, 0);
+			if (!err)
+				tc_bind_tclass(q, portid, clid, 0);
 			goto out;
 		case RTM_GETTCLASS:
 			err = tclass_get_notify(net, skb, n, q, cl, extack);
-- 
2.43.0

Re: [PATCH] net/sched: skip reverse bind walk after failed class delete

[Jamal Hadi Salim]

On Sun, Mar 29, 2026 at 12:53 PM Qi Tang <tpluszz77@gmail.com> wrote:
>
> __tc_ctl_tclass() unconditionally calls tc_bind_tclass() after
> RTM_DELTCLASS even when tclass_del_notify() fails.  On
> ingress/clsact qdiscs with shared blocks, the delete always fails
> with -EOPNOTSUPP, but the subsequent bind walk still runs.
> tc_bind_tclass() reaches tcf_node_bind() which calls tcf_block_q()
> on the shared block and dereferences the NULL qdisc pointer.
>
> Only perform the reverse bind walk when the class delete succeeds.
>
> Triggered by issuing RTM_DELTCLASS on an ingress/clsact pseudo-class
> that carries a shared block with a classifier using bind_class:
>
>   $ tc qdisc add dev veth0 egress_block 22 clsact
>   $ tc filter add block 22 pref 1 protocol ip handle 0x1 \
>       fw classid ffff:fff3 action drop
>   $ tc class del dev veth0 classid ffff:fff3
>   RTNETLINK answers: Operation not supported
>   Kernel panic - not syncing: Attempted to kill init!
>   exitcode=0x00000700
>
> The NULL dereference occurs in tcf_node_bind() when sch_tree_lock()
> is called with the NULL return from tcf_block_q() on a shared block.
>

Try these patches:
https://lore.kernel.org/netdev/CAM0EoMmVey4ayJBZgogMO4S3Z-_n5oDfasdZyiVryEqCNqu6bw@mail.gmail.com/T/#t

Same for your other issue.

cheers,
jamal

> Fixes: 07d79fc7d94e ("net_sched: add reverse binding for tc class")
> Signed-off-by: Qi Tang <tpluszz77@gmail.com>
> ---
>  net/sched/sch_api.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index cc43e3f7574f..50d311ff1f2d 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -2251,7 +2251,8 @@ static int __tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
>                 case RTM_DELTCLASS:
>                         err = tclass_del_notify(net, cops, skb, n, q, cl, extack);
>                         /* Unbind the class with flilters with 0 */
> -                       tc_bind_tclass(q, portid, clid, 0);
> +                       if (!err)
> +                               tc_bind_tclass(q, portid, clid, 0);
>                         goto out;
>                 case RTM_GETTCLASS:
>                         err = tclass_get_notify(net, skb, n, q, cl, extack);
> --
> 2.43.0
>

Re: [PATCH net 1/3] net/sched: cls_fw: fix NULL pointer dereference on shared blocks

[Xiang Mei]

On Sun, Mar 29, 2026 at 08:15:17AM -0400, Jamal Hadi Salim wrote:
> On Fri, Mar 27, 2026 at 10:11 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Fri, Mar 27, 2026 at 2:13 AM Xiang Mei <xmei5@asu.edu> wrote:
> > >
> > > The old-method path in fw_classify() calls tcf_block_q() and
> > > dereferences q->handle. Shared blocks leave block->q NULL, causing a
> > > NULL deref when an empty cls_fw filter is attached to a shared block
> > > and a packet with a nonzero major skb mark is classified.
> > >
> > > Check tcf_block_shared() before accessing block->q and return -1 (no
> > > match) for shared blocks, consistent with cls_u32's tc_u_common_ptr().
> > >
> > > The fixed null-ptr-deref calling stack:
> > >  KASAN: null-ptr-deref in range [0x0000000000000038-0x000000000000003f]
> > >  RIP: 0010:fw_classify (net/sched/cls_fw.c:81)
> > >  Call Trace:
> > >   tcf_classify (./include/net/tc_wrapper.h:197 net/sched/cls_api.c:1764 net/sched/cls_api.c:1860)
> > >   tc_run (net/core/dev.c:4401)
> > >   __dev_queue_xmit (net/core/dev.c:4535 net/core/dev.c:4790)
> > >
> > > Fixes: 1abf272022cf ("net: sched: tcindex, fw, flow: use tcf_block_q helper to get struct Qdisc")
> > > Reported-by: Weiming Shi <bestswngs@gmail.com>
> > > Signed-off-by: Xiang Mei <xmei5@asu.edu>
> >
> > Thanks for also providing the tdc tests. Looks like a bizarre bug—and
> > in my mind the question is does fw or flow even factor into tc blocks?
> > Please give me time to review if the approach makes sense - perhaps
> > this weekend.
> >
> Since the fix is exactly as what u32 does - why not move
> tc_u_common_ptr() to a header file (pkt_cls.h)? then reuse it in u32,
> fw, and flow.

Thank you for the suggestion, I appreciate the review.

I spent some time exploring how to reuse tc_u_common_ptr() in cls_fw and
cls_flow, but I'm not sure I see a clean way to make it work across all 
three classifiers. The core issue is that they need different things 
when the block is shared:

 - cls_u32 needs a pointer value (block or block->q) as a hash key for 
   tc_u_common lookup
 - cls_fw and cls_flow need to bail out and return -1 (no match)

If we keep tc_u_common_ptr() as-is (returning void *), the usage in 
fw/flow would be tc_u_common_ptr(tp) == block to detect shared blocks,
which is less clear than tcf_block_shared(block) and doesn't save any 
code.

If we rework it to return struct Qdisc * (NULL for shared), it would
break cls_u32's current usage where it needs the block pointer itself
as a hash key for the shared case.

I may be misunderstanding your suggestion though — could you share 
what you had in mind? 

Thank you again for your time,
Xiang
> 
> I am still questioning the value of using blocks with these two
> classifiers and unfortunately the commit message of  1abf272022cf was
> not helpful.
> The testcases look good. Please add a letter head, perhaps quote the
> comment in tc_u_common_ptr() since it gives a good description of this
> whole dance.
> 
> cheers,
> jamal

Re: [PATCH] net/sched: skip reverse bind walk after failed class delete

[Qi Tang]

On Mon, Mar 30, 2026, Jamal Hadi Salim wrote:
> Try these patches:
> https://lore.kernel.org/netdev/CAM0EoMmVey4ayJBZgogMO4S3Z-_n5oDfasdZyiVryEqCNqu6bw@mail.gmail.com/T/#t
>
> Same for your other issue.

Thanks for the pointer, Jamal. Please disregard my two net/sched
patches.

Qi Tang

Re: [PATCH net 1/3] net/sched: cls_fw: fix NULL pointer dereference on shared blocks

[Jamal Hadi Salim]

On Sun, Mar 29, 2026 at 2:52 PM Xiang Mei <xmei5@asu.edu> wrote:
>
> On Sun, Mar 29, 2026 at 08:15:17AM -0400, Jamal Hadi Salim wrote:
> > On Fri, Mar 27, 2026 at 10:11 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Fri, Mar 27, 2026 at 2:13 AM Xiang Mei <xmei5@asu.edu> wrote:
> > > >
> > > > The old-method path in fw_classify() calls tcf_block_q() and
> > > > dereferences q->handle. Shared blocks leave block->q NULL, causing a
> > > > NULL deref when an empty cls_fw filter is attached to a shared block
> > > > and a packet with a nonzero major skb mark is classified.
> > > >
> > > > Check tcf_block_shared() before accessing block->q and return -1 (no
> > > > match) for shared blocks, consistent with cls_u32's tc_u_common_ptr().
> > > >
> > > > The fixed null-ptr-deref calling stack:
> > > >  KASAN: null-ptr-deref in range [0x0000000000000038-0x000000000000003f]
> > > >  RIP: 0010:fw_classify (net/sched/cls_fw.c:81)
> > > >  Call Trace:
> > > >   tcf_classify (./include/net/tc_wrapper.h:197 net/sched/cls_api.c:1764 net/sched/cls_api.c:1860)
> > > >   tc_run (net/core/dev.c:4401)
> > > >   __dev_queue_xmit (net/core/dev.c:4535 net/core/dev.c:4790)
> > > >
> > > > Fixes: 1abf272022cf ("net: sched: tcindex, fw, flow: use tcf_block_q helper to get struct Qdisc")
> > > > Reported-by: Weiming Shi <bestswngs@gmail.com>
> > > > Signed-off-by: Xiang Mei <xmei5@asu.edu>
> > >
> > > Thanks for also providing the tdc tests. Looks like a bizarre bug—and
> > > in my mind the question is does fw or flow even factor into tc blocks?
> > > Please give me time to review if the approach makes sense - perhaps
> > > this weekend.
> > >
> > Since the fix is exactly as what u32 does - why not move
> > tc_u_common_ptr() to a header file (pkt_cls.h)? then reuse it in u32,
> > fw, and flow.
>
> Thank you for the suggestion, I appreciate the review.
>

Actually on second thought - why dont you fix the fw one so it catches
things at control time? Something like:

--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -247,8 +247,18 @@ static int fw_change(struct net *net, struct
sk_buff *in_skb,
        struct nlattr *tb[TCA_FW_MAX + 1];
        int err;

-       if (!opt)
-               return handle ? -EINVAL : 0; /* Succeed if it is old method. */
+       if (!opt) {
+               if (handle)
+                       return -EINVAL;
+
+               if (tcf_block_shared(tp->chain->block)) {
+                       NL_SET_ERR_MSG(extack,
+                                      "Must specify mark when
attaching fw filter to block");
+                       return -EINVAL;
+               }
+
+               return 0; /* Succeed if it is old method. */
+       }

        err = nla_parse_nested_deprecated(tb, TCA_FW_MAX, opt, fw_policy,
                                          NULL);

Also for the flow one, can you add an extack like above.
Note: This will change the nature of your tdc test, you dont need to
send an skb marked packet anymore since your config will be reject the
setup..

cheers,
jamal

> I spent some time exploring how to reuse tc_u_common_ptr() in cls_fw and
> cls_flow, but I'm not sure I see a clean way to make it work across all
> three classifiers. The core issue is that they need different things
> when the block is shared:
>
>  - cls_u32 needs a pointer value (block or block->q) as a hash key for
>    tc_u_common lookup
>  - cls_fw and cls_flow need to bail out and return -1 (no match)
>
> If we keep tc_u_common_ptr() as-is (returning void *), the usage in
> fw/flow would be tc_u_common_ptr(tp) == block to detect shared blocks,
> which is less clear than tcf_block_shared(block) and doesn't save any
> code.
>
> If we rework it to return struct Qdisc * (NULL for shared), it would
> break cls_u32's current usage where it needs the block pointer itself
> as a hash key for the shared case.
>
> I may be misunderstanding your suggestion though — could you share
> what you had in mind?
>
> Thank you again for your time,
> Xiang
> >
> > I am still questioning the value of using blocks with these two
> > classifiers and unfortunately the commit message of  1abf272022cf was
> > not helpful.
> > The testcases look good. Please add a letter head, perhaps quote the
> > comment in tc_u_common_ptr() since it gives a good description of this
> > whole dance.
> >
> > cheers,
> > jamal

Re: [PATCH net 1/3] net/sched: cls_fw: fix NULL pointer dereference on shared blocks

[Xiang Mei]

Thanks for the review and advice. It's a better patch. I have sent the v3:
- for cls_fw and cls_flow, we check in fw/flow_change
- Adjust the tests

Thanks,
Xiang

On Mon, Mar 30, 2026 at 9:46 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Sun, Mar 29, 2026 at 2:52 PM Xiang Mei <xmei5@asu.edu> wrote:
> >
> > On Sun, Mar 29, 2026 at 08:15:17AM -0400, Jamal Hadi Salim wrote:
> > > On Fri, Mar 27, 2026 at 10:11 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > >
> > > > On Fri, Mar 27, 2026 at 2:13 AM Xiang Mei <xmei5@asu.edu> wrote:
> > > > >
> > > > > The old-method path in fw_classify() calls tcf_block_q() and
> > > > > dereferences q->handle. Shared blocks leave block->q NULL, causing a
> > > > > NULL deref when an empty cls_fw filter is attached to a shared block
> > > > > and a packet with a nonzero major skb mark is classified.
> > > > >
> > > > > Check tcf_block_shared() before accessing block->q and return -1 (no
> > > > > match) for shared blocks, consistent with cls_u32's tc_u_common_ptr().
> > > > >
> > > > > The fixed null-ptr-deref calling stack:
> > > > >  KASAN: null-ptr-deref in range [0x0000000000000038-0x000000000000003f]
> > > > >  RIP: 0010:fw_classify (net/sched/cls_fw.c:81)
> > > > >  Call Trace:
> > > > >   tcf_classify (./include/net/tc_wrapper.h:197 net/sched/cls_api.c:1764 net/sched/cls_api.c:1860)
> > > > >   tc_run (net/core/dev.c:4401)
> > > > >   __dev_queue_xmit (net/core/dev.c:4535 net/core/dev.c:4790)
> > > > >
> > > > > Fixes: 1abf272022cf ("net: sched: tcindex, fw, flow: use tcf_block_q helper to get struct Qdisc")
> > > > > Reported-by: Weiming Shi <bestswngs@gmail.com>
> > > > > Signed-off-by: Xiang Mei <xmei5@asu.edu>
> > > >
> > > > Thanks for also providing the tdc tests. Looks like a bizarre bug—and
> > > > in my mind the question is does fw or flow even factor into tc blocks?
> > > > Please give me time to review if the approach makes sense - perhaps
> > > > this weekend.
> > > >
> > > Since the fix is exactly as what u32 does - why not move
> > > tc_u_common_ptr() to a header file (pkt_cls.h)? then reuse it in u32,
> > > fw, and flow.
> >
> > Thank you for the suggestion, I appreciate the review.
> >
>
> Actually on second thought - why dont you fix the fw one so it catches
> things at control time? Something like:
>
> --- a/net/sched/cls_fw.c
> +++ b/net/sched/cls_fw.c
> @@ -247,8 +247,18 @@ static int fw_change(struct net *net, struct
> sk_buff *in_skb,
>         struct nlattr *tb[TCA_FW_MAX + 1];
>         int err;
>
> -       if (!opt)
> -               return handle ? -EINVAL : 0; /* Succeed if it is old method. */
> +       if (!opt) {
> +               if (handle)
> +                       return -EINVAL;
> +
> +               if (tcf_block_shared(tp->chain->block)) {
> +                       NL_SET_ERR_MSG(extack,
> +                                      "Must specify mark when
> attaching fw filter to block");
> +                       return -EINVAL;
> +               }
> +
> +               return 0; /* Succeed if it is old method. */
> +       }
>
>         err = nla_parse_nested_deprecated(tb, TCA_FW_MAX, opt, fw_policy,
>                                           NULL);
>
> Also for the flow one, can you add an extack like above.
> Note: This will change the nature of your tdc test, you dont need to
> send an skb marked packet anymore since your config will be reject the
> setup..
>
> cheers,
> jamal
>
> > I spent some time exploring how to reuse tc_u_common_ptr() in cls_fw and
> > cls_flow, but I'm not sure I see a clean way to make it work across all
> > three classifiers. The core issue is that they need different things
> > when the block is shared:
> >
> >  - cls_u32 needs a pointer value (block or block->q) as a hash key for
> >    tc_u_common lookup
> >  - cls_fw and cls_flow need to bail out and return -1 (no match)
> >
> > If we keep tc_u_common_ptr() as-is (returning void *), the usage in
> > fw/flow would be tc_u_common_ptr(tp) == block to detect shared blocks,
> > which is less clear than tcf_block_shared(block) and doesn't save any
> > code.
> >
> > If we rework it to return struct Qdisc * (NULL for shared), it would
> > break cls_u32's current usage where it needs the block pointer itself
> > as a hash key for the shared case.
> >
> > I may be misunderstanding your suggestion though — could you share
> > what you had in mind?
> >
> > Thank you again for your time,
> > Xiang
> > >
> > > I am still questioning the value of using blocks with these two
> > > classifiers and unfortunately the commit message of  1abf272022cf was
> > > not helpful.
> > > The testcases look good. Please add a letter head, perhaps quote the
> > > comment in tc_u_common_ptr() since it gives a good description of this
> > > whole dance.
> > >
> > > cheers,
> > > jamal