[PATCH net] net/sched: cls_api: unlink indr cb on a failed bind

[PATCH net] net/sched: cls_api: unlink indr cb on a failed bind

[Qihang]

Binding a device without ndo_setup_tc() to a shared block goes through the
indirect path, and when that bind fails tcf_block_bind()'s error path
frees the driver callback while it is still linked on the global
flow_block_indr_list.  The next flow_indr_dev_unregister() of that driver
walks the freed node:

  tcf_block_get_ext()
    -> tcf_block_offload_cmd()
       -> flow_indr_dev_setup_offload()  [ allocates block_cb and links it
                                            on flow_block_indr_list ]
       -> tcf_block_setup() -> tcf_block_bind()
          -> err_unroll: list_del(&block_cb->driver_list);
                         flow_block_cb_free(block_cb);   [ kfree() ]
    -> err_block_offload_bind            [ no tcf_block_offload_unbind() ]

The only unlink is flow_indr_block_cb_remove(), which a driver calls while
handling FLOW_BLOCK_UNBIND, and a failed bind never delivers that command,
so nothing removes the node before the callback is freed.

commit da94a7781fc3 ("net/sched: cls_api: remove block_cb from driver_list
before freeing") fixed this loop for driver_list; indr.list is the link
that change still misses.

Fix by unlinking the callback before it is freed, and by initialising
indr.list in flow_block_cb_alloc(), so an unlinked callback carries an
empty list instead of an all-NULL one.  That is what makes the
list_empty() test exact for the direct and the indirect path alike.

Fixes: 1fac52da5942c ("net: flow_offload: consolidate indirect flow_block infrastructure")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 net/core/flow_offload.c | 2 ++
 net/sched/cls_api.c     | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 5071d7fe6ce2..64451129a230 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -266,6 +266,8 @@ struct flow_block_cb *flow_block_cb_alloc(flow_setup_cb_t *cb,
 	if (!block_cb)
 		return ERR_PTR(-ENOMEM);
 
+	INIT_LIST_HEAD(&block_cb->indr.list);
+
 	block_cb->cb = cb;
 	block_cb->cb_ident = cb_ident;
 	block_cb->cb_priv = cb_priv;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a9f54988561f..ffbb8353251b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1665,6 +1665,9 @@ static int tcf_block_bind(struct tcf_block *block,
 err_unroll:
 	list_for_each_entry_safe(block_cb, next, &bo->cb_list, list) {
 		list_del(&block_cb->driver_list);
+		/* linked only for an indirect bind */
+		if (!list_empty(&block_cb->indr.list))
+			list_del_init(&block_cb->indr.list);
 		if (i-- > 0) {
 			list_del(&block_cb->list);
 			tcf_block_playback_offloads(block, block_cb->cb,
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net] net/sched: cls_api: unlink indr cb on a 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.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

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.