| Message ID | 20230704151456.52334-2-victor@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sched: Undo tcf_bind_filter in case of errors in set callbacks | expand |
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)
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 >
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.
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
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.
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)
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(-)