[PATCH net 0/5] net: sched: Undo tcf_bind_filter in case of errors in set callbacks

[PATCH net 0/5] net: sched: Undo tcf_bind_filter in case of errors in set callbacks

[Victor Nogueira]

Five different classifier (fw, bpf, u32, matchall, and flower) are
calling tcf_bind_filter in their callbacks, but weren't undoing it by
calling tcf_unbind_filter if their was an error after binding.

This patch set fixes all this by calling tcf_unbind_filter in such
cases.

This set also undoes a refcount decrement in cls_u32 when an update
fails under specific conditions which are described in patch #4.

Victor Nogueira (5):
  net: sched: cls_bpf: Undo tcf_bind_filter in case of an error
  net: sched: cls_matchall: Undo tcf_bind_filter in case of failure
    after mall_set_parms
  net: sched: cls_u32: Undo tcf_bind_filter if u32_replace_hw_knode
  net: sched: cls_u32: Undo refcount decrement in case update failed
  net: sched: cls_flower: Undo tcf_bind_filter if fl_set_key fails

 net/sched/cls_bpf.c      |  8 ++++++--
 net/sched/cls_flower.c   | 29 +++++++++++++++++++++++++----
 net/sched/cls_matchall.c |  8 ++++++--
 net/sched/cls_u32.c      | 32 ++++++++++++++++++++++++++------
 4 files changed, 63 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH net 1/5] net: sched: cls_bpf: Undo tcf_bind_filter in case of an error

[Victor Nogueira]

If cls_bpf_offload errors out, we must also undo tcf_bind_filter that
was done in cls_bpf_set_parms.

Fix that by calling tcf_unbind_filter in errout_parms.

Fixes: eadb41489fd2 ("net: cls_bpf: add support for marking filters as hardware-only")

Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/cls_bpf.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 466c26df853a..4d9974b1b29d 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -409,7 +409,7 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
 static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
 			     struct cls_bpf_prog *prog, unsigned long base,
 			     struct nlattr **tb, struct nlattr *est, u32 flags,
-			     struct netlink_ext_ack *extack)
+			     bool *bound_to_filter, struct netlink_ext_ack *extack)
 {
 	bool is_bpf, is_ebpf, have_exts = false;
 	u32 gen_flags = 0;
@@ -451,6 +451,7 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
 	if (tb[TCA_BPF_CLASSID]) {
 		prog->res.classid = nla_get_u32(tb[TCA_BPF_CLASSID]);
 		tcf_bind_filter(tp, &prog->res, base);
+		*bound_to_filter = true;
 	}
 
 	return 0;
@@ -464,6 +465,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 {
 	struct cls_bpf_head *head = rtnl_dereference(tp->root);
 	struct cls_bpf_prog *oldprog = *arg;
+	bool bound_to_filter = false;
 	struct nlattr *tb[TCA_BPF_MAX + 1];
 	struct cls_bpf_prog *prog;
 	int ret;
@@ -505,7 +507,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	prog->handle = handle;
 
 	ret = cls_bpf_set_parms(net, tp, prog, base, tb, tca[TCA_RATE], flags,
-				extack);
+				&bound_to_filter, extack);
 	if (ret < 0)
 		goto errout_idr;
 
@@ -530,6 +532,8 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	return 0;
 
 errout_parms:
+	if (bound_to_filter)
+		tcf_unbind_filter(tp, &prog->res);
 	cls_bpf_free_parms(prog);
 errout_idr:
 	if (!oldprog)
-- 
2.25.1

Re: [PATCH net 1/5] net: sched: cls_bpf: Undo tcf_bind_filter in case of an error

[Simon Horman]

On Tue, Jul 04, 2023 at 12:14:52PM -0300, Victor Nogueira wrote:
> If cls_bpf_offload errors out, we must also undo tcf_bind_filter that
> was done in cls_bpf_set_parms.
> 
> Fix that by calling tcf_unbind_filter in errout_parms.
> 
> Fixes: eadb41489fd2 ("net: cls_bpf: add support for marking filters as hardware-only")
> 

nit: no blank line here.

> Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
> ---
>  net/sched/cls_bpf.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index 466c26df853a..4d9974b1b29d 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -409,7 +409,7 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
>  static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
>  			     struct cls_bpf_prog *prog, unsigned long base,
>  			     struct nlattr **tb, struct nlattr *est, u32 flags,
> -			     struct netlink_ext_ack *extack)
> +			     bool *bound_to_filter, struct netlink_ext_ack *extack)
>  {
>  	bool is_bpf, is_ebpf, have_exts = false;
>  	u32 gen_flags = 0;
> @@ -451,6 +451,7 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
>  	if (tb[TCA_BPF_CLASSID]) {
>  		prog->res.classid = nla_get_u32(tb[TCA_BPF_CLASSID]);
>  		tcf_bind_filter(tp, &prog->res, base);
> +		*bound_to_filter = true;
>  	}
>  
>  	return 0;
> @@ -464,6 +465,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
>  {
>  	struct cls_bpf_head *head = rtnl_dereference(tp->root);
>  	struct cls_bpf_prog *oldprog = *arg;
> +	bool bound_to_filter = false;
>  	struct nlattr *tb[TCA_BPF_MAX + 1];
>  	struct cls_bpf_prog *prog;
>  	int ret;

Please use reverse xmas tree - longest line to shortest - for
local variable declarations in Networking code.

> @@ -505,7 +507,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
>  	prog->handle = handle;
>  
>  	ret = cls_bpf_set_parms(net, tp, prog, base, tb, tca[TCA_RATE], flags,
> -				extack);
> +				&bound_to_filter, extack);
>  	if (ret < 0)
>  		goto errout_idr;
>  
> @@ -530,6 +532,8 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
>  	return 0;
>  
>  errout_parms:
> +	if (bound_to_filter)
> +		tcf_unbind_filter(tp, &prog->res);
>  	cls_bpf_free_parms(prog);
>  errout_idr:
>  	if (!oldprog)

-- 
pw-bot: changes-requested

Re: [PATCH net 1/5] net: sched: cls_bpf: Undo tcf_bind_filter in case of an error

[Jamal Hadi Salim]

On Tue, Jul 4, 2023 at 4:48 PM Simon Horman <simon.horman@corigine.com> wrote:
>
> On Tue, Jul 04, 2023 at 12:14:52PM -0300, Victor Nogueira wrote:
> > If cls_bpf_offload errors out, we must also undo tcf_bind_filter that
> > was done in cls_bpf_set_parms.
> >
> > Fix that by calling tcf_unbind_filter in errout_parms.
> >
> > Fixes: eadb41489fd2 ("net: cls_bpf: add support for marking filters as hardware-only")
> >
>
> nit: no blank line here.
>
> > Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
> > ---
> >  net/sched/cls_bpf.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> > index 466c26df853a..4d9974b1b29d 100644
> > --- a/net/sched/cls_bpf.c
> > +++ b/net/sched/cls_bpf.c
> > @@ -409,7 +409,7 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
> >  static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
> >                            struct cls_bpf_prog *prog, unsigned long base,
> >                            struct nlattr **tb, struct nlattr *est, u32 flags,
> > -                          struct netlink_ext_ack *extack)
> > +                          bool *bound_to_filter, struct netlink_ext_ack *extack)
> >  {
> >       bool is_bpf, is_ebpf, have_exts = false;
> >       u32 gen_flags = 0;
> > @@ -451,6 +451,7 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
> >       if (tb[TCA_BPF_CLASSID]) {
> >               prog->res.classid = nla_get_u32(tb[TCA_BPF_CLASSID]);
> >               tcf_bind_filter(tp, &prog->res, base);
> > +             *bound_to_filter = true;
> >       }
> >
> >       return 0;
> > @@ -464,6 +465,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
> >  {
> >       struct cls_bpf_head *head = rtnl_dereference(tp->root);
> >       struct cls_bpf_prog *oldprog = *arg;
> > +     bool bound_to_filter = false;
> >       struct nlattr *tb[TCA_BPF_MAX + 1];
> >       struct cls_bpf_prog *prog;
> >       int ret;
>
> Please use reverse xmas tree - longest line to shortest - for
> local variable declarations in Networking code.
>

I think Ed's tool is actually wrong on this Simon.
The rule I know of is: initializations first then declarations -
unless it is documented elsewhere as not the case.

cheers,
jamal


> > @@ -505,7 +507,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
> >       prog->handle = handle;
> >
> >       ret = cls_bpf_set_parms(net, tp, prog, base, tb, tca[TCA_RATE], flags,
> > -                             extack);
> > +                             &bound_to_filter, extack);
> >       if (ret < 0)
> >               goto errout_idr;
> >
> > @@ -530,6 +532,8 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
> >       return 0;
> >
> >  errout_parms:
> > +     if (bound_to_filter)
> > +             tcf_unbind_filter(tp, &prog->res);
> >       cls_bpf_free_parms(prog);
> >  errout_idr:
> >       if (!oldprog)
>
> --
> pw-bot: changes-requested
>

Re: [PATCH net 1/5] net: sched: cls_bpf: Undo tcf_bind_filter in case of an error

[Simon Horman]

On Tue, Jul 04, 2023 at 04:55:25PM -0400, Jamal Hadi Salim wrote:
> On Tue, Jul 4, 2023 at 4:48 PM Simon Horman <simon.horman@corigine.com> wrote:
> >
> > On Tue, Jul 04, 2023 at 12:14:52PM -0300, Victor Nogueira wrote:
> > > If cls_bpf_offload errors out, we must also undo tcf_bind_filter that
> > > was done in cls_bpf_set_parms.
> > >
> > > Fix that by calling tcf_unbind_filter in errout_parms.
> > >
> > > Fixes: eadb41489fd2 ("net: cls_bpf: add support for marking filters as hardware-only")
> > >
> >
> > nit: no blank line here.
> >
> > > Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > > Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
> > > ---
> > >  net/sched/cls_bpf.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> > > index 466c26df853a..4d9974b1b29d 100644
> > > --- a/net/sched/cls_bpf.c
> > > +++ b/net/sched/cls_bpf.c
> > > @@ -409,7 +409,7 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
> > >  static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
> > >                            struct cls_bpf_prog *prog, unsigned long base,
> > >                            struct nlattr **tb, struct nlattr *est, u32 flags,
> > > -                          struct netlink_ext_ack *extack)
> > > +                          bool *bound_to_filter, struct netlink_ext_ack *extack)
> > >  {
> > >       bool is_bpf, is_ebpf, have_exts = false;
> > >       u32 gen_flags = 0;
> > > @@ -451,6 +451,7 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
> > >       if (tb[TCA_BPF_CLASSID]) {
> > >               prog->res.classid = nla_get_u32(tb[TCA_BPF_CLASSID]);
> > >               tcf_bind_filter(tp, &prog->res, base);
> > > +             *bound_to_filter = true;
> > >       }
> > >
> > >       return 0;
> > > @@ -464,6 +465,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
> > >  {
> > >       struct cls_bpf_head *head = rtnl_dereference(tp->root);
> > >       struct cls_bpf_prog *oldprog = *arg;
> > > +     bool bound_to_filter = false;
> > >       struct nlattr *tb[TCA_BPF_MAX + 1];
> > >       struct cls_bpf_prog *prog;
> > >       int ret;
> >
> > Please use reverse xmas tree - longest line to shortest - for
> > local variable declarations in Networking code.
> >
> 
> I think Ed's tool is actually wrong on this Simon.
> The rule I know of is: initializations first then declarations -
> unless it is documented elsewhere as not the case.

Hi Jamal,

That is not my understanding of the rule.

Re: [PATCH net 1/5] net: sched: cls_bpf: Undo tcf_bind_filter in case of an error

[Jamal Hadi Salim]

On Tue, Jul 4, 2023 at 5:20 PM Simon Horman <simon.horman@corigine.com> wrote:
>
> On Tue, Jul 04, 2023 at 04:55:25PM -0400, Jamal Hadi Salim wrote:
> > On Tue, Jul 4, 2023 at 4:48 PM Simon Horman <simon.horman@corigine.com> wrote:
> > >
> > > On Tue, Jul 04, 2023 at 12:14:52PM -0300, Victor Nogueira wrote:
> > > > If cls_bpf_offload errors out, we must also undo tcf_bind_filter that
> > > > was done in cls_bpf_set_parms.
> > > >
> > > > Fix that by calling tcf_unbind_filter in errout_parms.
> > > >
> > > > Fixes: eadb41489fd2 ("net: cls_bpf: add support for marking filters as hardware-only")
> > > >
> > >
> > > nit: no blank line here.
> > >
> > > > Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> > > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > > > Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
> > > > ---
> > > >  net/sched/cls_bpf.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> > > > index 466c26df853a..4d9974b1b29d 100644
> > > > --- a/net/sched/cls_bpf.c
> > > > +++ b/net/sched/cls_bpf.c
> > > > @@ -409,7 +409,7 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
> > > >  static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
> > > >                            struct cls_bpf_prog *prog, unsigned long base,
> > > >                            struct nlattr **tb, struct nlattr *est, u32 flags,
> > > > -                          struct netlink_ext_ack *extack)
> > > > +                          bool *bound_to_filter, struct netlink_ext_ack *extack)
> > > >  {
> > > >       bool is_bpf, is_ebpf, have_exts = false;
> > > >       u32 gen_flags = 0;
> > > > @@ -451,6 +451,7 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
> > > >       if (tb[TCA_BPF_CLASSID]) {
> > > >               prog->res.classid = nla_get_u32(tb[TCA_BPF_CLASSID]);
> > > >               tcf_bind_filter(tp, &prog->res, base);
> > > > +             *bound_to_filter = true;
> > > >       }
> > > >
> > > >       return 0;
> > > > @@ -464,6 +465,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
> > > >  {
> > > >       struct cls_bpf_head *head = rtnl_dereference(tp->root);
> > > >       struct cls_bpf_prog *oldprog = *arg;
> > > > +     bool bound_to_filter = false;
> > > >       struct nlattr *tb[TCA_BPF_MAX + 1];
> > > >       struct cls_bpf_prog *prog;
> > > >       int ret;
> > >
> > > Please use reverse xmas tree - longest line to shortest - for
> > > local variable declarations in Networking code.
> > >
> >
> > I think Ed's tool is actually wrong on this Simon.
> > The rule I know of is: initializations first then declarations -
> > unless it is documented elsewhere as not the case.
>
> Hi Jamal,
>
> That is not my understanding of the rule.

Something about mixing assignments and declarations being
cplusplusish. That's always how my fingers think.

So how would this have been done differently? This is the current patch:
----
        struct cls_bpf_head *head = rtnl_dereference(tp->root);
        struct cls_bpf_prog *oldprog = *arg;
+       bool bound_to_filter = false;
        struct nlattr *tb[TCA_BPF_MAX + 1];
        struct cls_bpf_prog *prog;
        int ret;
----

Should the change be?
---
        struct cls_bpf_head *head = rtnl_dereference(tp->root);
        struct cls_bpf_prog *oldprog = *arg;
        struct nlattr *tb[TCA_BPF_MAX + 1];
+      bool bound_to_filter = false;
        struct cls_bpf_prog *prog;
        int ret;
---

I dont think my gut or brain would let me type that - but if those are
them rules then it is Victor doing the typing ;->

cheers,
jamal

Re: [PATCH net 1/5] net: sched: cls_bpf: Undo tcf_bind_filter in case of an error

[Simon Horman]

On Tue, Jul 04, 2023 at 05:42:29PM -0400, Jamal Hadi Salim wrote:
> On Tue, Jul 4, 2023 at 5:20 PM Simon Horman <simon.horman@corigine.com> wrote:
> >
> > On Tue, Jul 04, 2023 at 04:55:25PM -0400, Jamal Hadi Salim wrote:
> > > On Tue, Jul 4, 2023 at 4:48 PM Simon Horman <simon.horman@corigine.com> wrote:
> > > >
> > > > On Tue, Jul 04, 2023 at 12:14:52PM -0300, Victor Nogueira wrote:
> > > > > If cls_bpf_offload errors out, we must also undo tcf_bind_filter that
> > > > > was done in cls_bpf_set_parms.
> > > > >
> > > > > Fix that by calling tcf_unbind_filter in errout_parms.
> > > > >
> > > > > Fixes: eadb41489fd2 ("net: cls_bpf: add support for marking filters as hardware-only")
> > > > >
> > > >
> > > > nit: no blank line here.
> > > >
> > > > > Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> > > > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > > > > Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
> > > > > ---
> > > > >  net/sched/cls_bpf.c | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> > > > > index 466c26df853a..4d9974b1b29d 100644
> > > > > --- a/net/sched/cls_bpf.c
> > > > > +++ b/net/sched/cls_bpf.c
> > > > > @@ -409,7 +409,7 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
> > > > >  static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
> > > > >                            struct cls_bpf_prog *prog, unsigned long base,
> > > > >                            struct nlattr **tb, struct nlattr *est, u32 flags,
> > > > > -                          struct netlink_ext_ack *extack)
> > > > > +                          bool *bound_to_filter, struct netlink_ext_ack *extack)
> > > > >  {
> > > > >       bool is_bpf, is_ebpf, have_exts = false;
> > > > >       u32 gen_flags = 0;
> > > > > @@ -451,6 +451,7 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
> > > > >       if (tb[TCA_BPF_CLASSID]) {
> > > > >               prog->res.classid = nla_get_u32(tb[TCA_BPF_CLASSID]);
> > > > >               tcf_bind_filter(tp, &prog->res, base);
> > > > > +             *bound_to_filter = true;
> > > > >       }
> > > > >
> > > > >       return 0;
> > > > > @@ -464,6 +465,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
> > > > >  {
> > > > >       struct cls_bpf_head *head = rtnl_dereference(tp->root);
> > > > >       struct cls_bpf_prog *oldprog = *arg;
> > > > > +     bool bound_to_filter = false;
> > > > >       struct nlattr *tb[TCA_BPF_MAX + 1];
> > > > >       struct cls_bpf_prog *prog;
> > > > >       int ret;
> > > >
> > > > Please use reverse xmas tree - longest line to shortest - for
> > > > local variable declarations in Networking code.
> > > >
> > >
> > > I think Ed's tool is actually wrong on this Simon.
> > > The rule I know of is: initializations first then declarations -
> > > unless it is documented elsewhere as not the case.
> >
> > Hi Jamal,
> >
> > That is not my understanding of the rule.
> 
> Something about mixing assignments and declarations being
> cplusplusish. That's always how my fingers think.
> 
> So how would this have been done differently? This is the current patch:
> ----
>         struct cls_bpf_head *head = rtnl_dereference(tp->root);
>         struct cls_bpf_prog *oldprog = *arg;
> +       bool bound_to_filter = false;
>         struct nlattr *tb[TCA_BPF_MAX + 1];
>         struct cls_bpf_prog *prog;
>         int ret;
> ----
> 
> Should the change be?
> ---
>         struct cls_bpf_head *head = rtnl_dereference(tp->root);
>         struct cls_bpf_prog *oldprog = *arg;
>         struct nlattr *tb[TCA_BPF_MAX + 1];
> +       bool bound_to_filter = false;
>         struct cls_bpf_prog *prog;
>         int ret;
> ---
> 
> I dont think my gut or brain would let me type that - but if those are
> them rules then it is Victor doing the typing ;->

Hi Jamal,

Let's not drag this out too long.
But, FWIIW, my understanding is that your 2nd example matches the guidelines.

-- 
pw-bot: under-review

[PATCH net 2/5] net: sched: cls_matchall: Undo tcf_bind_filter in case of failure after mall_set_parms

[Victor Nogueira]

In case an error occurred after mall_set_parms executed successfully, we
must undo the tcf_bind_filter call it issues.

Fix that by calling tcf_unbind_filter in err_replace_hw_filter label.

Fixes: ec2507d2a306 ("net/sched: cls_matchall: Fix error path")

Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/cls_matchall.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index fa3bbd187eb9..02830409e013 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -163,7 +163,7 @@ static int mall_set_parms(struct net *net, struct tcf_proto *tp,
 			  struct cls_mall_head *head,
 			  unsigned long base, struct nlattr **tb,
 			  struct nlattr *est, u32 flags, u32 fl_flags,
-			  struct netlink_ext_ack *extack)
+			  bool *bound_to_filter, struct netlink_ext_ack *extack)
 {
 	int err;
 
@@ -175,6 +175,7 @@ static int mall_set_parms(struct net *net, struct tcf_proto *tp,
 	if (tb[TCA_MATCHALL_CLASSID]) {
 		head->res.classid = nla_get_u32(tb[TCA_MATCHALL_CLASSID]);
 		tcf_bind_filter(tp, &head->res, base);
+		*bound_to_filter = true;
 	}
 	return 0;
 }
@@ -186,6 +187,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 		       struct netlink_ext_ack *extack)
 {
 	struct cls_mall_head *head = rtnl_dereference(tp->root);
+	bool bound_to_filter = false;
 	struct nlattr *tb[TCA_MATCHALL_MAX + 1];
 	struct cls_mall_head *new;
 	u32 userflags = 0;
@@ -227,7 +229,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 	}
 
 	err = mall_set_parms(net, tp, new, base, tb, tca[TCA_RATE],
-			     flags, new->flags, extack);
+			     flags, new->flags, &bound_to_filter, extack);
 	if (err)
 		goto err_set_parms;
 
@@ -246,6 +248,8 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 	return 0;
 
 err_replace_hw_filter:
+	if (bound_to_filter)
+		tcf_unbind_filter(tp, &new->res);
 err_set_parms:
 	free_percpu(new->pf);
 err_alloc_percpu:
-- 
2.25.1

[PATCH net 3/5] net: sched: cls_u32: Undo tcf_bind_filter if u32_replace_hw_knode

[Victor Nogueira]

When u32_replace_hw_knode fails, we need to undo the tcf_bind_filter
operation done at u32_set_parms.

Fixes: d34e3e181395 ("net: cls_u32: Add support for skip-sw flag to tc u32 classifier.")

Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/cls_u32.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index d15d50de7980..e193db39bee2 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -712,11 +712,13 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
 	[TCA_U32_FLAGS]		= { .type = NLA_U32 },
 };
 
+#define U32_SET_FLAGS_BOUND 0x1
+
 static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 			 unsigned long base,
 			 struct tc_u_knode *n, struct nlattr **tb,
 			 struct nlattr *est, u32 flags, u32 fl_flags,
-			 struct netlink_ext_ack *extack)
+			 u8 *set_flags, struct netlink_ext_ack *extack)
 {
 	int err, ifindex = -1;
 
@@ -763,6 +765,7 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 	if (tb[TCA_U32_CLASSID]) {
 		n->res.classid = nla_get_u32(tb[TCA_U32_CLASSID]);
 		tcf_bind_filter(tp, &n->res, base);
+		*set_flags |= U32_SET_FLAGS_BOUND;
 	}
 
 	if (ifindex >= 0)
@@ -853,6 +856,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		      struct netlink_ext_ack *extack)
 {
 	struct tc_u_common *tp_c = tp->data;
+	u8 set_flags = 0;
 	struct tc_u_hnode *ht;
 	struct tc_u_knode *n;
 	struct tc_u32_sel *s;
@@ -905,7 +909,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 
 		err = u32_set_parms(net, tp, base, new, tb,
 				    tca[TCA_RATE], flags, new->flags,
-				    extack);
+				    &set_flags, extack);
 
 		if (err) {
 			__u32_destroy_key(new);
@@ -914,6 +918,9 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 
 		err = u32_replace_hw_knode(tp, new, flags, extack);
 		if (err) {
+			if (set_flags & U32_SET_FLAGS_BOUND)
+				tcf_unbind_filter(tp, &new->res);
+
 			__u32_destroy_key(new);
 			return err;
 		}
@@ -1075,14 +1082,14 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 #endif
 
 	err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE],
-			    flags, n->flags, extack);
+			    flags, n->flags, &set_flags, extack);
 	if (err == 0) {
 		struct tc_u_knode __rcu **ins;
 		struct tc_u_knode *pins;
 
 		err = u32_replace_hw_knode(tp, n, flags, extack);
 		if (err)
-			goto errhw;
+			goto errunbind;
 
 		if (!tc_in_hw(n->flags))
 			n->flags |= TCA_CLS_FLAGS_NOT_IN_HW;
@@ -1100,7 +1107,10 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		return 0;
 	}
 
-errhw:
+errunbind:
+	if (set_flags & U32_SET_FLAGS_BOUND)
+		tcf_unbind_filter(tp, &n->res);
+
 #ifdef CONFIG_CLS_U32_MARK
 	free_percpu(n->pcpu_success);
 #endif
-- 
2.25.1

[PATCH net 4/5] net: sched: cls_u32: Undo refcount decrement in case update failed

[Victor Nogueira]

In the case of an update, when TCA_U32_LINK is set, u32_set_parms will
decrement the refcount of the ht_down (struct tc_u_hnode) pointer
present in the older u32 filter which we are replacing. However, if
u32_replace_hw_knode errors out, the update command fails and that
ht_down pointer continues decremented. To fix that, when
u32_replace_hw_knode fails, check if ht_down's refcount was decremented
and undo the decrement.

Fixes: d34e3e181395 ("net: cls_u32: Add support for skip-sw flag to tc u32 classifier.")

Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/cls_u32.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index e193db39bee2..5dc401e4baa6 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -713,6 +713,7 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
 };
 
 #define U32_SET_FLAGS_BOUND 0x1
+#define U32_SET_FLAGS_DECR_HTDOWN 0x2
 
 static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 			 unsigned long base,
@@ -759,8 +760,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 		ht_old = rtnl_dereference(n->ht_down);
 		rcu_assign_pointer(n->ht_down, ht_down);
 
-		if (ht_old)
+		if (ht_old) {
+			*set_flags |= U32_SET_FLAGS_DECR_HTDOWN;
 			ht_old->refcnt--;
+		}
 	}
 	if (tb[TCA_U32_CLASSID]) {
 		n->res.classid = nla_get_u32(tb[TCA_U32_CLASSID]);
@@ -921,6 +924,13 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 			if (set_flags & U32_SET_FLAGS_BOUND)
 				tcf_unbind_filter(tp, &new->res);
 
+			if (set_flags & U32_SET_FLAGS_DECR_HTDOWN) {
+				struct tc_u_hnode *ht_old;
+
+				ht_old = rtnl_dereference(n->ht_down);
+				if (ht_old)
+					ht_old->refcnt++;
+			}
 			__u32_destroy_key(new);
 			return err;
 		}
-- 
2.25.1

[PATCH net 5/5] net: sched: cls_flower: Undo tcf_bind_filter if fl_set_key fails

[Victor Nogueira]

if TCA_FLOWER_CLASSID is specified in the netlink message, the code will
call tcf_bind_filter. However, if any error occurs after that, the code
should undo this by calling tcf_unbind_filter.

When checking for TCA_FLOWER_CLASSID attribute, the code is calling for
tcf_bind_fitler.

Fixes: 77b9900ef53a ("tc: introduce Flower classifier")

Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/cls_flower.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 56065cc5a661..644b0097e6ae 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -2169,7 +2169,7 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
 			struct nlattr *est,
 			struct fl_flow_tmplt *tmplt,
 			u32 flags, u32 fl_flags,
-			struct netlink_ext_ack *extack)
+			bool *bound_to_filter, struct netlink_ext_ack *extack)
 {
 	int err;
 
@@ -2185,18 +2185,20 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
 		tcf_bind_filter(tp, &f->res, base);
 		if (flags & TCA_ACT_FLAGS_NO_RTNL)
 			rtnl_unlock();
+		*bound_to_filter = true;
 	}
 
 	err = fl_set_key(net, tb, &f->key, &mask->key, extack);
 	if (err)
-		return err;
+		goto unbind_filter;
 
 	fl_mask_update_range(mask);
 	fl_set_masked_key(&f->mkey, &f->key, mask);
 
 	if (!fl_mask_fits_tmplt(tmplt, mask)) {
 		NL_SET_ERR_MSG_MOD(extack, "Mask does not fit the template");
-		return -EINVAL;
+		err = -EINVAL;
+		goto unbind_filter;
 	}
 
 	/* Enable tc skb extension if filter matches on data extracted from
@@ -2208,6 +2210,17 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
 	}
 
 	return 0;
+
+unbind_filter:
+	if (*bound_to_filter) {
+		if (flags & TCA_ACT_FLAGS_NO_RTNL)
+			rtnl_lock();
+		tcf_unbind_filter(tp, &f->res);
+		if (flags & TCA_ACT_FLAGS_NO_RTNL)
+			rtnl_unlock();
+		*bound_to_filter = false;
+	}
+	return err;
 }
 
 static int fl_ht_insert_unique(struct cls_fl_filter *fnew,
@@ -2241,6 +2254,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 	struct cls_fl_head *head = fl_head_dereference(tp);
 	bool rtnl_held = !(flags & TCA_ACT_FLAGS_NO_RTNL);
 	struct cls_fl_filter *fold = *arg;
+	bool bound_to_filter = false;
 	struct cls_fl_filter *fnew;
 	struct fl_flow_mask *mask;
 	struct nlattr **tb;
@@ -2327,7 +2341,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 
 	err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE],
 			   tp->chain->tmplt_priv, flags, fnew->flags,
-			   extack);
+			   &bound_to_filter, extack);
 	if (err)
 		goto errout_idr;
 
@@ -2425,6 +2439,13 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 errout_mask:
 	fl_mask_put(head, fnew->mask);
 errout_idr:
+	if (bound_to_filter) {
+		if (flags & TCA_ACT_FLAGS_NO_RTNL)
+			rtnl_lock();
+		tcf_unbind_filter(tp, &fnew->res);
+		if (flags & TCA_ACT_FLAGS_NO_RTNL)
+			rtnl_unlock();
+	}
 	if (!fold)
 		idr_remove(&head->handle_idr, fnew->handle);
 	__fl_put(fnew);
-- 
2.25.1