[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAM0EoMnV3-o_4L3Vv=TuEqC=iNKhNnW0c4HQiRqrJD5NtjKeOQ@mail.gmail.com>
Date: Thu, 24 Oct 2024 11:39:51 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
Pedro Tammela <pctammela@...atatu.com>, Victor Nogueira <victor@...atatu.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net/sched: sch_api: fix xa_insert() error path in tcf_block_get_ext()
On Wed, Oct 23, 2024 at 6:05 AM Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> This command:
>
> $ tc qdisc replace dev eth0 ingress_block 1 egress_block 1 clsact
> Error: block dev insert failed: -EBUSY.
>
> fails because user space requests the same block index to be set for
> both ingress and egress.
>
> [ side note, I don't think it even failed prior to commit 913b47d3424e
> ("net/sched: Introduce tc block netdev tracking infra"), because this
> is a command from an old set of notes of mine which used to work, but
> alas, I did not scientifically bisect this ]
>
What would be the use case for having both share the same index?
Mirror action for example could be used to target a group of ports
grouped by blockid in which case a unique blockid simplifies.
> The problem is not that it fails, but rather, that the second time
> around, it fails differently (and irrecoverably):
>
> $ tc qdisc replace dev eth0 ingress_block 1 egress_block 1 clsact
> Error: dsa_core: Flow block cb is busy.
>
> [ another note: the extack is added by me for illustration purposes.
> the context of the problem is that clsact_init() obtains the same
> &q->ingress_block pointer as &q->egress_block, and since we call
> tcf_block_get_ext() on both of them, "dev" will be added to the
> block->ports xarray twice, thus failing the operation: once through
> the ingress block pointer, and once again through the egress block
> pointer. the problem itself is that when xa_insert() fails, we have
> emitted a FLOW_BLOCK_BIND command through ndo_setup_tc(), but the
> offload never sees a corresponding FLOW_BLOCK_UNBIND. ]
>
> Even correcting the bad user input, we still cannot recover:
>
> $ tc qdisc replace dev swp3 ingress_block 1 egress_block 2 clsact
> Error: dsa_core: Flow block cb is busy.
>
> Basically the only way to recover is to reboot the system, or unbind and
> rebind the net device driver.
>
> To fix the bug, we need to fill the correct error teardown path which
> was missed during code movement, and call tcf_block_offload_unbind()
> when xa_insert() fails.
>
> [ last note, fundamentally I blame the label naming convention in
> tcf_block_get_ext() for the bug. The labels should be named after what
> they do, not after the error path that jumps to them. This way, it is
> obviously wrong that two labels pointing to the same code mean
> something is wrong, and checking the code correctness at the goto site
> is also easier ]
>
> Fixes: 94e2557d086a ("net: sched: move block device tracking into tcf_block_get/put_ext()")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
Fix makes sense.
Acked-by: Jamal Hadi Salim <jhs@...atatu.com>
I am also hoping you did run the tdc tests (despite this not looking
like it breaks any existing feature)
cheers,
jamal
> ---
> net/sched/cls_api.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 7637f979d689..2a7d856cc334 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -1518,6 +1518,7 @@ int tcf_block_get_ext(struct tcf_block **p_block, struct Qdisc *q,
> return 0;
>
> err_dev_insert:
> + tcf_block_offload_unbind(block, q, ei);
> err_block_offload_bind:
> tcf_chain0_head_change_cb_del(block, ei);
> err_chain0_head_change_cb_add:
> --
> 2.43.0
>
Powered by blists - more mailing lists