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

[netdev-bot+sashiko@kernel.org]

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