| Message ID | 20230217223620.28508-4-paulb@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 08a0063df3aed8d76a4034279117db12dbc1050f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: cls_api: Support hardware miss to tc action | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 95 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Feb 17, 2023 at 11:36 PM Paul Blakey <paulb@nvidia.com> wrote: > > To support miss to action during hardware offload the filter's > handle is needed when setting up the actions (tcf_exts_init()), > and before offloading. > > Move filter handle initialization earlier. > > Signed-off-by: Paul Blakey <paulb@nvidia.com> > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > Reviewed-by: Simon Horman <simon.horman@corigine.com> > Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- Error path is now potentially crashing because net pointer has not been initialized. I plan fixing this issue with the following: diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index e960a46b05205bb0bca7dc0d21531e4d6a3853e3..475fe222a85566639bac75fc4a95bf649a10357d 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -2200,8 +2200,9 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, fnew->flags = nla_get_u32(tb[TCA_FLOWER_FLAGS]); if (!tc_flags_valid(fnew->flags)) { + kfree(fnew); err = -EINVAL; - goto errout; + goto errout_tb; } } @@ -2226,8 +2227,10 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, } spin_unlock(&tp->lock); - if (err) - goto errout; + if (err) { + kfree(fnew); + goto errout_tb; + } } fnew->handle = handle; @@ -2337,7 +2340,6 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, fl_mask_put(head, fnew->mask); errout_idr: idr_remove(&head->handle_idr, fnew->handle); -errout: __fl_put(fnew); errout_tb: kfree(tb);
On 23/02/2023 12:24, Eric Dumazet wrote: > On Fri, Feb 17, 2023 at 11:36 PM Paul Blakey <paulb@nvidia.com> wrote: >> >> To support miss to action during hardware offload the filter's >> handle is needed when setting up the actions (tcf_exts_init()), >> and before offloading. >> >> Move filter handle initialization earlier. >> >> Signed-off-by: Paul Blakey <paulb@nvidia.com> >> Reviewed-by: Jiri Pirko <jiri@nvidia.com> >> Reviewed-by: Simon Horman <simon.horman@corigine.com> >> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >> --- > > Error path is now potentially crashing because net pointer has not > been initialized. > > I plan fixing this issue with the following: > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > index e960a46b05205bb0bca7dc0d21531e4d6a3853e3..475fe222a85566639bac75fc4a95bf649a10357d > 100644 > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -2200,8 +2200,9 @@ static int fl_change(struct net *net, struct > sk_buff *in_skb, > fnew->flags = nla_get_u32(tb[TCA_FLOWER_FLAGS]); > > if (!tc_flags_valid(fnew->flags)) { > + kfree(fnew); > err = -EINVAL; > - goto errout; > + goto errout_tb; > } > } > > @@ -2226,8 +2227,10 @@ static int fl_change(struct net *net, struct > sk_buff *in_skb, > } > spin_unlock(&tp->lock); > > - if (err) > - goto errout; > + if (err) { > + kfree(fnew); > + goto errout_tb; > + } > } > fnew->handle = handle; > > @@ -2337,7 +2340,6 @@ static int fl_change(struct net *net, struct > sk_buff *in_skb, > fl_mask_put(head, fnew->mask); > errout_idr: > idr_remove(&head->handle_idr, fnew->handle); > -errout: > __fl_put(fnew); > errout_tb: > kfree(tb); Notice that the bug was before this patch as well. We init exts->net only in fl_set_parms()->tcf_exts_validate(exts), and before this patch we called __fl_put() on two errors before that (like if tcf_exts_init() failed). Here, its the same, we can't call __fl_put(fnew) till we called fl_set_parms(). So you're missing this "goto errorout_idr": err = tcf_exts_init_ex(&fnew->exts, net, TCA_FLOWER_ACT, 0, tp, handle, !tc_skip_hw(fnew->flags)); if (err < 0) goto errout_idr; Thanks, Paul.
On Sat, Feb 25, 2023 at 8:54 AM Paul Blakey <paulb@nvidia.com> wrote: > > > > On 23/02/2023 12:24, Eric Dumazet wrote: > > On Fri, Feb 17, 2023 at 11:36 PM Paul Blakey <paulb@nvidia.com> wrote: > >> > >> To support miss to action during hardware offload the filter's > >> handle is needed when setting up the actions (tcf_exts_init()), > >> and before offloading. > >> > >> Move filter handle initialization earlier. > >> > >> Signed-off-by: Paul Blakey <paulb@nvidia.com> > >> Reviewed-by: Jiri Pirko <jiri@nvidia.com> > >> Reviewed-by: Simon Horman <simon.horman@corigine.com> > >> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > >> --- > > > > Error path is now potentially crashing because net pointer has not > > been initialized. > > > > I plan fixing this issue with the following: > > > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > > index e960a46b05205bb0bca7dc0d21531e4d6a3853e3..475fe222a85566639bac75fc4a95bf649a10357d > > 100644 > > --- a/net/sched/cls_flower.c > > +++ b/net/sched/cls_flower.c > > @@ -2200,8 +2200,9 @@ static int fl_change(struct net *net, struct > > sk_buff *in_skb, > > fnew->flags = nla_get_u32(tb[TCA_FLOWER_FLAGS]); > > > > if (!tc_flags_valid(fnew->flags)) { > > + kfree(fnew); > > err = -EINVAL; > > - goto errout; > > + goto errout_tb; > > } > > } > > > > @@ -2226,8 +2227,10 @@ static int fl_change(struct net *net, struct > > sk_buff *in_skb, > > } > > spin_unlock(&tp->lock); > > > > - if (err) > > - goto errout; > > + if (err) { > > + kfree(fnew); > > + goto errout_tb; > > + } > > } > > fnew->handle = handle; > > > > @@ -2337,7 +2340,6 @@ static int fl_change(struct net *net, struct > > sk_buff *in_skb, > > fl_mask_put(head, fnew->mask); > > errout_idr: > > idr_remove(&head->handle_idr, fnew->handle); > > -errout: > > __fl_put(fnew); > > errout_tb: > > kfree(tb); > > > Notice that the bug was before this patch as well. > We init exts->net only in fl_set_parms()->tcf_exts_validate(exts), and > before this patch we called __fl_put() on two errors before that (like > if tcf_exts_init() failed). > > > Here, its the same, we can't call __fl_put(fnew) till we called > fl_set_parms(). So you're missing this "goto errorout_idr": > > > err = tcf_exts_init_ex(&fnew->exts, net, TCA_FLOWER_ACT, 0, tp, handle, > !tc_skip_hw(fnew->flags)); > if (err < 0) > goto errout_idr; > > Thanks, > Paul. > The bug I am talking about is triggering because ->net pointer is not initialized. ->net pointer is initialized in tcf_exts_init_ex(), before any error can be returned.
On 25/02/2023 10:14, Eric Dumazet wrote: > On Sat, Feb 25, 2023 at 8:54 AM Paul Blakey <paulb@nvidia.com> wrote: >> >> >> >> On 23/02/2023 12:24, Eric Dumazet wrote: >>> On Fri, Feb 17, 2023 at 11:36 PM Paul Blakey <paulb@nvidia.com> wrote: >>>> >>>> To support miss to action during hardware offload the filter's >>>> handle is needed when setting up the actions (tcf_exts_init()), >>>> and before offloading. >>>> >>>> Move filter handle initialization earlier. >>>> >>>> Signed-off-by: Paul Blakey <paulb@nvidia.com> >>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com> >>>> Reviewed-by: Simon Horman <simon.horman@corigine.com> >>>> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >>>> --- >>> >>> Error path is now potentially crashing because net pointer has not >>> been initialized. >>> >>> I plan fixing this issue with the following: >>> >>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c >>> index e960a46b05205bb0bca7dc0d21531e4d6a3853e3..475fe222a85566639bac75fc4a95bf649a10357d >>> 100644 >>> --- a/net/sched/cls_flower.c >>> +++ b/net/sched/cls_flower.c >>> @@ -2200,8 +2200,9 @@ static int fl_change(struct net *net, struct >>> sk_buff *in_skb, >>> fnew->flags = nla_get_u32(tb[TCA_FLOWER_FLAGS]); >>> >>> if (!tc_flags_valid(fnew->flags)) { >>> + kfree(fnew); >>> err = -EINVAL; >>> - goto errout; >>> + goto errout_tb; >>> } >>> } >>> >>> @@ -2226,8 +2227,10 @@ static int fl_change(struct net *net, struct >>> sk_buff *in_skb, >>> } >>> spin_unlock(&tp->lock); >>> >>> - if (err) >>> - goto errout; >>> + if (err) { >>> + kfree(fnew); >>> + goto errout_tb; >>> + } >>> } >>> fnew->handle = handle; >>> >>> @@ -2337,7 +2340,6 @@ static int fl_change(struct net *net, struct >>> sk_buff *in_skb, >>> fl_mask_put(head, fnew->mask); >>> errout_idr: >>> idr_remove(&head->handle_idr, fnew->handle); >>> -errout: >>> __fl_put(fnew); >>> errout_tb: >>> kfree(tb); >> >> >> Notice that the bug was before this patch as well. >> We init exts->net only in fl_set_parms()->tcf_exts_validate(exts), and >> before this patch we called __fl_put() on two errors before that (like >> if tcf_exts_init() failed). >> >> >> Here, its the same, we can't call __fl_put(fnew) till we called >> fl_set_parms(). So you're missing this "goto errorout_idr": >> >> >> err = tcf_exts_init_ex(&fnew->exts, net, TCA_FLOWER_ACT, 0, tp, handle, >> !tc_skip_hw(fnew->flags)); >> if (err < 0) >> goto errout_idr; >> >> Thanks, >> Paul. >> > > The bug I am talking about is triggering because ->net pointer is not > initialized. > > ->net pointer is initialized in tcf_exts_init_ex(), before any error > can be returned. Right, so all good!
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 885c95191ccf..be01d39dd7b9 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -2187,10 +2187,6 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, INIT_LIST_HEAD(&fnew->hw_list); refcount_set(&fnew->refcnt, 1); - err = tcf_exts_init(&fnew->exts, net, TCA_FLOWER_ACT, 0); - if (err < 0) - goto errout; - if (tb[TCA_FLOWER_FLAGS]) { fnew->flags = nla_get_u32(tb[TCA_FLOWER_FLAGS]); @@ -2200,15 +2196,45 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, } } + if (!fold) { + spin_lock(&tp->lock); + if (!handle) { + handle = 1; + err = idr_alloc_u32(&head->handle_idr, fnew, &handle, + INT_MAX, GFP_ATOMIC); + } else { + err = idr_alloc_u32(&head->handle_idr, fnew, &handle, + handle, GFP_ATOMIC); + + /* Filter with specified handle was concurrently + * inserted after initial check in cls_api. This is not + * necessarily an error if NLM_F_EXCL is not set in + * message flags. Returning EAGAIN will cause cls_api to + * try to update concurrently inserted rule. + */ + if (err == -ENOSPC) + err = -EAGAIN; + } + spin_unlock(&tp->lock); + + if (err) + goto errout; + } + fnew->handle = handle; + + err = tcf_exts_init(&fnew->exts, net, TCA_FLOWER_ACT, 0); + if (err < 0) + goto errout_idr; + err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE], tp->chain->tmplt_priv, flags, fnew->flags, extack); if (err) - goto errout; + goto errout_idr; err = fl_check_assign_mask(head, fnew, fold, mask); if (err) - goto errout; + goto errout_idr; err = fl_ht_insert_unique(fnew, fold, &in_ht); if (err) @@ -2274,29 +2300,9 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, refcount_dec(&fold->refcnt); __fl_put(fold); } else { - if (handle) { - /* user specifies a handle and it doesn't exist */ - err = idr_alloc_u32(&head->handle_idr, fnew, &handle, - handle, GFP_ATOMIC); - - /* Filter with specified handle was concurrently - * inserted after initial check in cls_api. This is not - * necessarily an error if NLM_F_EXCL is not set in - * message flags. Returning EAGAIN will cause cls_api to - * try to update concurrently inserted rule. - */ - if (err == -ENOSPC) - err = -EAGAIN; - } else { - handle = 1; - err = idr_alloc_u32(&head->handle_idr, fnew, &handle, - INT_MAX, GFP_ATOMIC); - } - if (err) - goto errout_hw; + idr_replace(&head->handle_idr, fnew, fnew->handle); refcount_inc(&fnew->refcnt); - fnew->handle = handle; list_add_tail_rcu(&fnew->list, &fnew->mask->filters); spin_unlock(&tp->lock); } @@ -2319,6 +2325,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, fnew->mask->filter_ht_params); errout_mask: fl_mask_put(head, fnew->mask); +errout_idr: + idr_remove(&head->handle_idr, fnew->handle); errout: __fl_put(fnew); errout_tb: