[PATCH] net/sched: cls_api: skip indr register on failed bind

[PATCH] net/sched: cls_api: skip indr register on failed bind

[Qihang]

Binding a device without ndo_setup_tc() to a shared block that already
offloads records the block on flow_indir_dev_list and then fails.  Nothing
removes that record, so once the block is destroyed the next
flow_indr_dev_register() splices a list into freed memory:

  tcf_block_offload_cmd()
    -> flow_indr_dev_setup_offload()
       -> indir_dev_add()   [ record: data = block,
                                cb_list = &block->flow_block.cb_list ]
    -> return -EOPNOTSUPP
  tcf_block_offload_bind()
    -> tcf_block_offload_in_use(block) is true    [ offloadcnt > 0 ]
    -> return -EOPNOTSUPP
  tcf_block_get_ext()
    -> err_block_offload_bind            [ no tcf_block_offload_unbind() ]

Only a FLOW_BLOCK_UNBIND calls indir_dev_remove(), and the failed bind
never issues one.  The next flow_indr_dev_register() replays the record
through existing_qdiscs_register(), where list_splice() writes the new
linkage into the freed block's cb_list; the callback that the replay
allocates keeps indr.data pointing at the freed block, so a later driver
unregister reaches tc_block_indr_cleanup() on it as well.

The fix was built and the reproducer re-run clean on it.

Fix by returning -EOPNOTSUPP before flow_indr_dev_setup_offload() when the
command is FLOW_BLOCK_BIND and the block is already offloading, which is
the condition tcf_block_offload_bind() rejects the bind with, so that no
record is created.  Keep the check limited to the bind: on unbind this
branch is the only caller of indir_dev_remove(), and it runs while
offloadcnt is still non-zero.

Fixes: 74fc4f828769c ("net: Fix offloading indirect devices dependency on qdisc order creation")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 net/sched/cls_api.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a9f54988561f..7acfe71853a7 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -860,6 +860,10 @@ static int tcf_block_offload_cmd(struct tcf_block *block,
 		return tcf_block_setup(block, &bo);
 	}
 
+	/* a bind that cannot succeed must not leave a record behind */
+	if (command == FLOW_BLOCK_BIND && tcf_block_offload_in_use(block))
+		return -EOPNOTSUPP;
+
 	flow_indr_dev_setup_offload(dev, sch, TC_SETUP_BLOCK, block, &bo,
 				    tc_block_indr_cleanup);
 	tcf_block_setup(block, &bo);
-- 
2.54.0 (Apple Git-157)

Re: [PATCH] net/sched: cls_api: skip indr register on failed bind

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] net/sched: cls_api: skip indr register on failed bind

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch does not fully close the failed-bind leak.

--

net/sched: cls_api: skip indr register on failed bind

This patch adds an early -EOPNOTSUPP return to tcf_block_offload_cmd().
It fires for FLOW_BLOCK_BIND on a device without ndo_setup_tc() when the
block already has offloaded filters. The aim is to stop a failed bind
from leaving a record on flow_indir_dev_list, which later causes a
use-after-free of the freed tcf_block.

> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index a9f54988561f2..7acfe71853a7c 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -860,6 +860,10 @@ static int tcf_block_offload_cmd(struct tcf_block *block,
>  		return tcf_block_setup(block, &bo);
>  	}
>  
> +	/* a bind that cannot succeed must not leave a record behind */
> +	if (command == FLOW_BLOCK_BIND && tcf_block_offload_in_use(block))
> +		return -EOPNOTSUPP;
> +
>  	flow_indr_dev_setup_offload(dev, sch, TC_SETUP_BLOCK, block, &bo,
>  				    tc_block_indr_cleanup);
>  	tcf_block_setup(block, &bo);

[Severity: High]
Is offloadcnt read too early here to fully close the leak?

The real rejection in tcf_block_offload_bind() reads offloadcnt after
flow_indr_dev_setup_offload() and tcf_block_setup() have run:

net/sched/cls_api.c:tcf_block_offload_bind() {
    ...
no_offload_dev_inc:
	if (tcf_block_offload_in_use(block))
		goto err_unlock;
    ...
}

Between the two reads, tcf_block_setup()->tcf_block_bind() replays the
existing filters to each newly allocated indirect block_cb:

tcf_block_bind()
  tcf_block_playback_offloads()
    tp->ops->reoffload()   (e.g. fl_reoffload())
      tc_setup_cb_reoffload()
        tc_cls_offload_cnt_update(add = true)
          tcf_block_offload_inc()

If the indirect driver accepts any replayed filter, offloadcnt goes
from 0 to non-zero. tcf_block_bind() then publishes the callback:

	list_splice(&bo->cb_list, &block->flow_block.cb_list);

The indirect branch still always returns -EOPNOTSUPP. So
tcf_block_offload_bind() sees tcf_block_offload_in_use() as true and
fails the bind. tcf_block_get_ext() then goes to err_block_offload_bind
without calling tcf_block_offload_unbind().

Does that still leave two things behind?

First, the indir_dev_add() record stays on flow_indir_dev_list, with
data = block and cb_list = &block->flow_block.cb_list.

Second, the indirect flow_block_cb stays on flow_block_indr_list with
indr.data = block. This second leak happens even when indir_dev_add()
returned -EEXIST.

After the block is freed, flow_indr_dev_register()->
existing_qdiscs_register() would list_splice() into the freed block's
cb_list. Also, flow_indr_dev_unregister()->tc_block_indr_cleanup() would
dereference the freed block through block_cb->indr.data. These are the
same two use-after-free paths the commit message describes.

Here is one way to reach it:

- an indirect tunnel offload driver is registered
- a shared block has flower filters (no skip_sw) that nothing has
  offloaded yet, so offloadcnt == 0
- a tunnel device without ndo_setup_tc(), such as vxlan, is bound to
  the block
- the indirect driver accepts at least one replayed filter

The commit message also describes the new predicate as:

  "the condition tcf_block_offload_bind() rejects the bind with"

Is that accurate? The actual rejection uses the offloadcnt value from
after the replay.

Would it be more robust to roll back on the failing path in
tcf_block_offload_bind() instead of predicting the post-bind offloadcnt
in advance? That could mean issuing a FLOW_BLOCK_UNBIND, or removing the
record and unbinding the spliced callbacks.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013163014.25722-1-q.h.hack.winter%40gmail.com