| Message ID | 20230224150058.149505-4-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: fix action bind logic | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-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 10 of 10 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 | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 23 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Feb 24, 2023 at 12:00:58PM -0300, Pedro Tammela wrote: > The TC architecture allows filters and actions to be created independently. > In filters the user can reference action objects using: > tc action add action sample ... index 1 > tc filter add ... action pedit index 1 > > In the current code for act_sample this is broken as it checks netlink > attributes for create/update before actually checking if we are binding to an > existing action. > > tdc results: > 1..29 > ok 1 9784 - Add valid sample action with mandatory arguments > ok 2 5c91 - Add valid sample action with mandatory arguments and continue control action > ok 3 334b - Add valid sample action with mandatory arguments and drop control action > ok 4 da69 - Add valid sample action with mandatory arguments and reclassify control action > ok 5 13ce - Add valid sample action with mandatory arguments and pipe control action > ok 6 1886 - Add valid sample action with mandatory arguments and jump control action > ok 7 7571 - Add sample action with invalid rate > ok 8 b6d4 - Add sample action with mandatory arguments and invalid control action > ok 9 a874 - Add invalid sample action without mandatory arguments > ok 10 ac01 - Add invalid sample action without mandatory argument rate > ok 11 4203 - Add invalid sample action without mandatory argument group > ok 12 14a7 - Add invalid sample action without mandatory argument group > ok 13 8f2e - Add valid sample action with trunc argument > ok 14 45f8 - Add sample action with maximum rate argument > ok 15 ad0c - Add sample action with maximum trunc argument > ok 16 83a9 - Add sample action with maximum group argument > ok 17 ed27 - Add sample action with invalid rate argument > ok 18 2eae - Add sample action with invalid group argument > ok 19 6ff3 - Add sample action with invalid trunc size > ok 20 2b2a - Add sample action with invalid index > ok 21 dee2 - Add sample action with maximum allowed index > ok 22 560e - Add sample action with cookie > ok 23 704a - Replace existing sample action with new rate argument > ok 24 60eb - Replace existing sample action with new group argument > ok 25 2cce - Replace existing sample action with new trunc argument > ok 26 59d1 - Replace existing sample action with new control argument > ok 27 0a6e - Replace sample action with invalid goto chain control > ok 28 3872 - Delete sample action with valid index > ok 29 a394 - Delete sample action with invalid index > > Fixes: 5c5670fae430 ("net/sched: Introduce sample tc action") > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> I have a minor comment below. But in the context of this series, this patch looks good to me. Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > net/sched/act_sample.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c > index f7416b5598e0..4c670e7568dc 100644 > --- a/net/sched/act_sample.c > +++ b/net/sched/act_sample.c > @@ -55,8 +55,8 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla, > sample_policy, NULL); > if (ret < 0) > return ret; > - if (!tb[TCA_SAMPLE_PARMS] || !tb[TCA_SAMPLE_RATE] || > - !tb[TCA_SAMPLE_PSAMPLE_GROUP]) > + > + if (!tb[TCA_SAMPLE_PARMS]) > return -EINVAL; > > parm = nla_data(tb[TCA_SAMPLE_PARMS]); > @@ -80,6 +80,13 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla, > tcf_idr_release(*a, bind); > return -EEXIST; nit: not for this patch, but I think it would be more consistent to use goto release_idr here. > } > + > + if (!tb[TCA_SAMPLE_RATE] || !tb[TCA_SAMPLE_PSAMPLE_GROUP]) { > + NL_SET_ERR_MSG(extack, "sample rate and group are required"); > + err = -EINVAL; > + goto release_idr; > + } > + > err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack); > if (err < 0) > goto release_idr; > -- > 2.34.1 >
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c index f7416b5598e0..4c670e7568dc 100644 --- a/net/sched/act_sample.c +++ b/net/sched/act_sample.c @@ -55,8 +55,8 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla, sample_policy, NULL); if (ret < 0) return ret; - if (!tb[TCA_SAMPLE_PARMS] || !tb[TCA_SAMPLE_RATE] || - !tb[TCA_SAMPLE_PSAMPLE_GROUP]) + + if (!tb[TCA_SAMPLE_PARMS]) return -EINVAL; parm = nla_data(tb[TCA_SAMPLE_PARMS]); @@ -80,6 +80,13 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla, tcf_idr_release(*a, bind); return -EEXIST; } + + if (!tb[TCA_SAMPLE_RATE] || !tb[TCA_SAMPLE_PSAMPLE_GROUP]) { + NL_SET_ERR_MSG(extack, "sample rate and group are required"); + err = -EINVAL; + goto release_idr; + } + err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack); if (err < 0) goto release_idr;
The TC architecture allows filters and actions to be created independently. In filters the user can reference action objects using: tc action add action sample ... index 1 tc filter add ... action pedit index 1 In the current code for act_sample this is broken as it checks netlink attributes for create/update before actually checking if we are binding to an existing action. tdc results: 1..29 ok 1 9784 - Add valid sample action with mandatory arguments ok 2 5c91 - Add valid sample action with mandatory arguments and continue control action ok 3 334b - Add valid sample action with mandatory arguments and drop control action ok 4 da69 - Add valid sample action with mandatory arguments and reclassify control action ok 5 13ce - Add valid sample action with mandatory arguments and pipe control action ok 6 1886 - Add valid sample action with mandatory arguments and jump control action ok 7 7571 - Add sample action with invalid rate ok 8 b6d4 - Add sample action with mandatory arguments and invalid control action ok 9 a874 - Add invalid sample action without mandatory arguments ok 10 ac01 - Add invalid sample action without mandatory argument rate ok 11 4203 - Add invalid sample action without mandatory argument group ok 12 14a7 - Add invalid sample action without mandatory argument group ok 13 8f2e - Add valid sample action with trunc argument ok 14 45f8 - Add sample action with maximum rate argument ok 15 ad0c - Add sample action with maximum trunc argument ok 16 83a9 - Add sample action with maximum group argument ok 17 ed27 - Add sample action with invalid rate argument ok 18 2eae - Add sample action with invalid group argument ok 19 6ff3 - Add sample action with invalid trunc size ok 20 2b2a - Add sample action with invalid index ok 21 dee2 - Add sample action with maximum allowed index ok 22 560e - Add sample action with cookie ok 23 704a - Replace existing sample action with new rate argument ok 24 60eb - Replace existing sample action with new group argument ok 25 2cce - Replace existing sample action with new trunc argument ok 26 59d1 - Replace existing sample action with new control argument ok 27 0a6e - Replace sample action with invalid goto chain control ok 28 3872 - Delete sample action with valid index ok 29 a394 - Delete sample action with invalid index Fixes: 5c5670fae430 ("net/sched: Introduce sample tc action") Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> --- net/sched/act_sample.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)