LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH net] sched: fix infinite loop when creating tc filter
@ 2021-10-10 6:55 Volodymyr Mytnyk
2021-10-11 13:42 ` Vlad Buslov
0 siblings, 1 reply; 3+ messages in thread
From: Volodymyr Mytnyk @ 2021-10-10 6:55 UTC (permalink / raw)
To: netdev
Cc: Volodymyr Mytnyk, Serhiy Boiko, Jamal Hadi Salim, Cong Wang,
Jiri Pirko, David S. Miller, Jakub Kicinski, Vlad Buslov,
linux-kernel
From: Volodymyr Mytnyk <vmytnyk@marvell.com>
After running a specific set of commands tc will become unresponsive:
$ ip link add dev DEV type veth
$ tc qdisc add dev DEV clsact
$ tc chain add dev DEV chain 0 ingress
$ tc filter del dev DEV ingress
$ tc filter add dev DEV ingress flower action pass
When executing chain flush, the "chain->flushing" field is set
to true, which prevents insertion of new classifier instances.
It is unset in one place under two conditions:
`refcnt - chain->action_refcnt == 0` and `!by_act`.
Ignoring the by_act and action_refcnt arguments the `flushing procedure`
will be over when refcnt is 0.
But if the chain is explicitly created (e.g. `tc chain add .. chain 0 ..`)
refcnt is set to 1 without any classifier instances. Thus the condition
is never met and the chain->flushing field is never cleared.
And because the default chain is `flushing` new classifiers cannot
be added. tc_new_tfilter is stuck in a loop trying to find a chain
where chain->flushing is false.
By moving `chain->flushing = false` from __tcf_chain_put to the end
of tcf_chain_flush will avoid the condition and the field will always
be reset after the flush procedure.
Fixes: 91052fa1c657 ("net: sched: protect chain->explicitly_created with block->lock")
Co-developed-by: Serhiy Boiko <serhiy.boiko@plvision.eu>
Signed-off-by: Serhiy Boiko <serhiy.boiko@plvision.eu>
Signed-off-by: Volodymyr Mytnyk <vmytnyk@marvell.com>
---
net/sched/cls_api.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d73b5c5514a9..327594cce554 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -563,8 +563,6 @@ static void __tcf_chain_put(struct tcf_chain *chain, bool by_act,
if (refcnt - chain->action_refcnt == 0 && !by_act) {
tc_chain_notify_delete(tmplt_ops, tmplt_priv, chain->index,
block, NULL, 0, 0, false);
- /* Last reference to chain, no need to lock. */
- chain->flushing = false;
}
if (refcnt == 0)
@@ -615,6 +613,9 @@ static void tcf_chain_flush(struct tcf_chain *chain, bool rtnl_held)
tcf_proto_put(tp, rtnl_held, NULL);
tp = tp_next;
}
+
+ /* Last reference to chain, no need to lock. */
+ chain->flushing = false;
}
static int tcf_block_setup(struct tcf_block *block,
--
2.7.4
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] sched: fix infinite loop when creating tc filter
2021-10-10 6:55 [PATCH net] sched: fix infinite loop when creating tc filter Volodymyr Mytnyk
@ 2021-10-11 13:42 ` Vlad Buslov
2021-10-13 9:43 ` Volodymyr Mytnyk [C]
0 siblings, 1 reply; 3+ messages in thread
From: Vlad Buslov @ 2021-10-11 13:42 UTC (permalink / raw)
To: Volodymyr Mytnyk
Cc: netdev, Volodymyr Mytnyk, Serhiy Boiko, Jamal Hadi Salim,
Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski,
Vlad Buslov, linux-kernel
Hi Volodymyr,
On Sun 10 Oct 2021 at 09:55, Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu> wrote:
> From: Volodymyr Mytnyk <vmytnyk@marvell.com>
>
> After running a specific set of commands tc will become unresponsive:
>
> $ ip link add dev DEV type veth
> $ tc qdisc add dev DEV clsact
> $ tc chain add dev DEV chain 0 ingress
> $ tc filter del dev DEV ingress
> $ tc filter add dev DEV ingress flower action pass
>
> When executing chain flush, the "chain->flushing" field is set
> to true, which prevents insertion of new classifier instances.
> It is unset in one place under two conditions:
>
> `refcnt - chain->action_refcnt == 0` and `!by_act`.
>
> Ignoring the by_act and action_refcnt arguments the `flushing procedure`
> will be over when refcnt is 0.
>
> But if the chain is explicitly created (e.g. `tc chain add .. chain 0 ..`)
> refcnt is set to 1 without any classifier instances. Thus the condition
> is never met and the chain->flushing field is never cleared.
> And because the default chain is `flushing` new classifiers cannot
> be added. tc_new_tfilter is stuck in a loop trying to find a chain
> where chain->flushing is false.
>
> By moving `chain->flushing = false` from __tcf_chain_put to the end
> of tcf_chain_flush will avoid the condition and the field will always
> be reset after the flush procedure.
>
> Fixes: 91052fa1c657 ("net: sched: protect chain->explicitly_created with block->lock")
Thanks for working on this!
>
> Co-developed-by: Serhiy Boiko <serhiy.boiko@plvision.eu>
> Signed-off-by: Serhiy Boiko <serhiy.boiko@plvision.eu>
> Signed-off-by: Volodymyr Mytnyk <vmytnyk@marvell.com>
> ---
> net/sched/cls_api.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index d73b5c5514a9..327594cce554 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -563,8 +563,6 @@ static void __tcf_chain_put(struct tcf_chain *chain, bool by_act,
> if (refcnt - chain->action_refcnt == 0 && !by_act) {
> tc_chain_notify_delete(tmplt_ops, tmplt_priv, chain->index,
> block, NULL, 0, 0, false);
> - /* Last reference to chain, no need to lock. */
> - chain->flushing = false;
> }
>
> if (refcnt == 0)
> @@ -615,6 +613,9 @@ static void tcf_chain_flush(struct tcf_chain *chain, bool rtnl_held)
> tcf_proto_put(tp, rtnl_held, NULL);
> tp = tp_next;
> }
> +
> + /* Last reference to chain, no need to lock. */
But after moving the code block here you can no longer guarantee that
this is the last reference, right?
> + chain->flushing = false;
Resetting the flag here is probably correct for actual flush use-case
(e.g. RTM_DELTFILTER message with prio==0), but can cause undesired
side-effects for other users of tcf_chain_flush(). Consider following
interaction between new filter creation and explicit chain deletion that
also uses tcf_chanin_flush():
RTM_DELCHAIN RTM_NEWTFILTER
+ +
| |
| +----------v-----------+
| | |
| | __tcf_block_find |
| | |
| +----------+-----------+
| |
| |
| +----------v------------+
| | |
| | tcf_chain_get |
| | |
| +----------+------------+
| |
+--------v--------+ |
| | |
| tcf_chain_flush | |
| | |
+--------+--------+ |
| |
| +----------v------------+
| | |
| | tcf_chain_tp_find |
| | |
| +----------+------------+
| |
| |tp==NULL
| |chain->flushing==false
| |
| +---------------v----------------+
| | |
| | tp_created = 1 |
| | tcf_chain_tp_insert_unique |
| | |
| +---------------+----------------+
| |
| |
+---------------v-----------------+ |
| | |
|tcf_chain_put_explicitly_created | |
| | |
+---------------+-----------------+ |
| |
v v
In this example tc_new_tfilter() holds chain reference during flush. If
flush finishes concurrently before the check for chain->flushing, the
chain reference counter will not reach 0 (because new filter creation
code will not back off and release the reference). In the described
example tc_chain_notify_delete() will not be called which will confuse
any userland code that expects to receive delete chain notification
after sending RTM_DELCHAIN message.
With these considerations I can propose following approach to fix the
issue:
1. Extend tcf_chain_flush() with additional boolean argument and only
call it with 'true' value from tc_del_tfilter(). (or create helper
function that calls tcf_chain_flush() and then resets chain->flushing
flag)
2. Reset the 'flushing' flag when new argument is true.
3. Wrap the 'flushing' flag reset code in filter_chain_lock critical
section.
> }
>
> static int tcf_block_setup(struct tcf_block *block,
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] sched: fix infinite loop when creating tc filter
2021-10-11 13:42 ` Vlad Buslov
@ 2021-10-13 9:43 ` Volodymyr Mytnyk [C]
0 siblings, 0 replies; 3+ messages in thread
From: Volodymyr Mytnyk [C] @ 2021-10-13 9:43 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, Serhiy Boiko, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
David S. Miller, Jakub Kicinski, Vlad Buslov, linux-kernel
Hi Vlad,
Thanks for your review comments and good explanation of the problem you observe. I will
take a look at this and will back to you.
Regards,
Volodymyr
> Hi Volodymyr,
>
> On Sun 10 Oct 2021 at 09:55, Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu> wrote:
> > From: Volodymyr Mytnyk <vmytnyk@marvell.com>
> >
> > After running a specific set of commands tc will become unresponsive:
> >
> > $ ip link add dev DEV type veth
> > $ tc qdisc add dev DEV clsact
> > $ tc chain add dev DEV chain 0 ingress
> > $ tc filter del dev DEV ingress
> > $ tc filter add dev DEV ingress flower action pass
> >
> > When executing chain flush, the "chain->flushing" field is set
> > to true, which prevents insertion of new classifier instances.
> > It is unset in one place under two conditions:
> >
> > `refcnt - chain->action_refcnt == 0` and `!by_act`.
> >
> > Ignoring the by_act and action_refcnt arguments the `flushing procedure`
> > will be over when refcnt is 0.
> >
> > But if the chain is explicitly created (e.g. `tc chain add .. chain 0 ..`)
> > refcnt is set to 1 without any classifier instances. Thus the condition
> > is never met and the chain->flushing field is never cleared.
> > And because the default chain is `flushing` new classifiers cannot
> > be added. tc_new_tfilter is stuck in a loop trying to find a chain
> > where chain->flushing is false.
> >
> > By moving `chain->flushing = false` from __tcf_chain_put to the end
> > of tcf_chain_flush will avoid the condition and the field will always
> > be reset after the flush procedure.
> >
> > Fixes: 91052fa1c657 ("net: sched: protect chain->explicitly_created with block->lock")
>
> Thanks for working on this!
>
> >
> > Co-developed-by: Serhiy Boiko <serhiy.boiko@plvision.eu>
> > Signed-off-by: Serhiy Boiko <serhiy.boiko@plvision.eu>
> > Signed-off-by: Volodymyr Mytnyk <vmytnyk@marvell.com>
> > ---
> > net/sched/cls_api.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> > index d73b5c5514a9..327594cce554 100644
> > --- a/net/sched/cls_api.c
> > +++ b/net/sched/cls_api.c
> > @@ -563,8 +563,6 @@ static void __tcf_chain_put(struct tcf_chain *chain, bool by_act,
> > if (refcnt - chain->action_refcnt == 0 && !by_act) {
> > tc_chain_notify_delete(tmplt_ops, tmplt_priv, chain->index,
> > block, NULL, 0, 0, false);
> > - /* Last reference to chain, no need to lock. */
> > - chain->flushing = false;
> > }
> >
> > if (refcnt == 0)
> > @@ -615,6 +613,9 @@ static void tcf_chain_flush(struct tcf_chain *chain, bool rtnl_held)
> > tcf_proto_put(tp, rtnl_held, NULL);
> > tp = tp_next;
> > }
> > +
> > + /* Last reference to chain, no need to lock. */
>
> But after moving the code block here you can no longer guarantee that
> this is the last reference, right?
>
> > + chain->flushing = false;
>
> Resetting the flag here is probably correct for actual flush use-case
> (e.g. RTM_DELTFILTER message with prio==0), but can cause undesired
> side-effects for other users of tcf_chain_flush(). Consider following
> interaction between new filter creation and explicit chain deletion that
> also uses tcf_chanin_flush():
>
> RTM_DELCHAIN RTM_NEWTFILTER
> + +
> | |
> | +----------v-----------+
> | | |
> | | __tcf_block_find |
> | | |
> | +----------+-----------+
> | |
> | |
> | +----------v------------+
> | | |
> | | tcf_chain_get |
> | | |
> | +----------+------------+
> | |
> +--------v--------+ |
> | | |
> | tcf_chain_flush | |
> | | |
> +--------+--------+ |
> | |
> | +----------v------------+
> | | |
> | | tcf_chain_tp_find |
> | | |
> | +----------+------------+
> | |
> | |tp==NULL
> | |chain->flushing==false
> | |
> | +---------------v----------------+
> | | |
> | | tp_created = 1 |
> | | tcf_chain_tp_insert_unique |
> | | |
> | +---------------+----------------+
> | |
> | |
> +---------------v-----------------+ |
> | | |
> |tcf_chain_put_explicitly_created | |
> | | |
> +---------------+-----------------+ |
> | |
> v v
>
> In this example tc_new_tfilter() holds chain reference during flush. If
> flush finishes concurrently before the check for chain->flushing, the
> chain reference counter will not reach 0 (because new filter creation
> code will not back off and release the reference). In the described
> example tc_chain_notify_delete() will not be called which will confuse
> any userland code that expects to receive delete chain notification
> after sending RTM_DELCHAIN message.
>
> With these considerations I can propose following approach to fix the
> issue:
>
> 1. Extend tcf_chain_flush() with additional boolean argument and only
> call it with 'true' value from tc_del_tfilter(). (or create helper
> function that calls tcf_chain_flush() and then resets chain->flushing
> flag)
>
> 2. Reset the 'flushing' flag when new argument is true.
>
> 3. Wrap the 'flushing' flag reset code in filter_chain_lock critical
> section.
>
> > }
> >
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-13 9:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-10 6:55 [PATCH net] sched: fix infinite loop when creating tc filter Volodymyr Mytnyk
2021-10-11 13:42 ` Vlad Buslov
2021-10-13 9:43 ` Volodymyr Mytnyk [C]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).