| Message ID | 20230224150058.149505-3-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 11 of 11 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, 110 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:57PM -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 mpls ... index 1 > tc filter add ... action mpls index 1 > > In the current code for act_mpls 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..53 > ok 1 a933 - Add MPLS dec_ttl action with pipe opcode > ok 2 08d1 - Add mpls dec_ttl action with pass opcode > ok 3 d786 - Add mpls dec_ttl action with drop opcode > ok 4 f334 - Add mpls dec_ttl action with reclassify opcode > ok 5 29bd - Add mpls dec_ttl action with continue opcode > ok 6 48df - Add mpls dec_ttl action with jump opcode > ok 7 62eb - Add mpls dec_ttl action with trap opcode > ok 8 09d2 - Add mpls dec_ttl action with opcode and cookie > ok 9 c170 - Add mpls dec_ttl action with opcode and cookie of max length > ok 10 9118 - Add mpls dec_ttl action with invalid opcode > ok 11 6ce1 - Add mpls dec_ttl action with label (invalid) > ok 12 352f - Add mpls dec_ttl action with tc (invalid) > ok 13 fa1c - Add mpls dec_ttl action with ttl (invalid) > ok 14 6b79 - Add mpls dec_ttl action with bos (invalid) > ok 15 d4c4 - Add mpls pop action with ip proto > ok 16 91fb - Add mpls pop action with ip proto and cookie > ok 17 92fe - Add mpls pop action with mpls proto > ok 18 7e23 - Add mpls pop action with no protocol (invalid) > ok 19 6182 - Add mpls pop action with label (invalid) > ok 20 6475 - Add mpls pop action with tc (invalid) > ok 21 067b - Add mpls pop action with ttl (invalid) > ok 22 7316 - Add mpls pop action with bos (invalid) > ok 23 38cc - Add mpls push action with label > ok 24 c281 - Add mpls push action with mpls_mc protocol > ok 25 5db4 - Add mpls push action with label, tc and ttl > ok 26 7c34 - Add mpls push action with label, tc ttl and cookie of max length > ok 27 16eb - Add mpls push action with label and bos > ok 28 d69d - Add mpls push action with no label (invalid) > ok 29 e8e4 - Add mpls push action with ipv4 protocol (invalid) > ok 30 ecd0 - Add mpls push action with out of range label (invalid) > ok 31 d303 - Add mpls push action with out of range tc (invalid) > ok 32 fd6e - Add mpls push action with ttl of 0 (invalid) > ok 33 19e9 - Add mpls mod action with mpls label > ok 34 1fde - Add mpls mod action with max mpls label > ok 35 0c50 - Add mpls mod action with mpls label exceeding max (invalid) > ok 36 10b6 - Add mpls mod action with mpls label of MPLS_LABEL_IMPLNULL (invalid) > ok 37 57c9 - Add mpls mod action with mpls min tc > ok 38 6872 - Add mpls mod action with mpls max tc > ok 39 a70a - Add mpls mod action with mpls tc exceeding max (invalid) > ok 40 6ed5 - Add mpls mod action with mpls ttl > ok 41 77c1 - Add mpls mod action with mpls ttl and cookie > ok 42 b80f - Add mpls mod action with mpls max ttl > ok 43 8864 - Add mpls mod action with mpls min ttl > ok 44 6c06 - Add mpls mod action with mpls ttl of 0 (invalid) > ok 45 b5d8 - Add mpls mod action with mpls ttl exceeding max (invalid) > ok 46 451f - Add mpls mod action with mpls max bos > ok 47 a1ed - Add mpls mod action with mpls min bos > ok 48 3dcf - Add mpls mod action with mpls bos exceeding max (invalid) > ok 49 db7c - Add mpls mod action with protocol (invalid) > ok 50 b070 - Replace existing mpls push action with new ID > ok 51 95a9 - Replace existing mpls push action with new label, tc, ttl and cookie > ok 52 6cce - Delete mpls pop action > ok 53 d138 - Flush mpls actions > > Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC") > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> Minor not notwithstanding, Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > net/sched/act_mpls.c | 66 +++++++++++++++++++++++++------------------- > 1 file changed, 37 insertions(+), 29 deletions(-) > > diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c > index 6b26bdb999d7..809f7928a1be 100644 > --- a/net/sched/act_mpls.c > +++ b/net/sched/act_mpls.c > @@ -190,40 +190,67 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla, > parm = nla_data(tb[TCA_MPLS_PARMS]); > index = parm->index; > > + err = tcf_idr_check_alloc(tn, &index, a, bind); > + if (err < 0) > + return err; > + exists = err; > + if (exists && bind) > + return 0; > + > + if (!exists) { > + ret = tcf_idr_create(tn, index, est, a, &act_mpls_ops, bind, > + true, flags); > + if (ret) { > + tcf_idr_cleanup(tn, index); > + return ret; > + } > + > + ret = ACT_P_CREATED; > + } else if (!(flags & TCA_ACT_FLAGS_REPLACE)) { > + tcf_idr_release(*a, bind); > + return -EEXIST; nit: I think it would be more consistent to goto release_idr here. > + } > + > /* Verify parameters against action type. */ > switch (parm->m_action) { > case TCA_MPLS_ACT_POP: > if (!tb[TCA_MPLS_PROTO]) { > NL_SET_ERR_MSG_MOD(extack, "Protocol must be set for MPLS pop"); > - return -EINVAL; > + err = -EINVAL; > + goto release_idr; > } > if (!eth_proto_is_802_3(nla_get_be16(tb[TCA_MPLS_PROTO]))) { > NL_SET_ERR_MSG_MOD(extack, "Invalid protocol type for MPLS pop"); > - return -EINVAL; > + err = -EINVAL; > + goto release_idr; > } > if (tb[TCA_MPLS_LABEL] || tb[TCA_MPLS_TTL] || tb[TCA_MPLS_TC] || > tb[TCA_MPLS_BOS]) { > NL_SET_ERR_MSG_MOD(extack, "Label, TTL, TC or BOS cannot be used with MPLS pop"); > - return -EINVAL; > + err = -EINVAL; > + goto release_idr; > } > break; > case TCA_MPLS_ACT_DEC_TTL: > if (tb[TCA_MPLS_PROTO] || tb[TCA_MPLS_LABEL] || > tb[TCA_MPLS_TTL] || tb[TCA_MPLS_TC] || tb[TCA_MPLS_BOS]) { > NL_SET_ERR_MSG_MOD(extack, "Label, TTL, TC, BOS or protocol cannot be used with MPLS dec_ttl"); > - return -EINVAL; > + err = -EINVAL; > + goto release_idr; > } > break; > case TCA_MPLS_ACT_PUSH: > case TCA_MPLS_ACT_MAC_PUSH: > if (!tb[TCA_MPLS_LABEL]) { > NL_SET_ERR_MSG_MOD(extack, "Label is required for MPLS push"); > - return -EINVAL; > + err = -EINVAL; > + goto release_idr; > } > if (tb[TCA_MPLS_PROTO] && > !eth_p_mpls(nla_get_be16(tb[TCA_MPLS_PROTO]))) { > NL_SET_ERR_MSG_MOD(extack, "Protocol must be an MPLS type for MPLS push"); > - return -EPROTONOSUPPORT; > + err = -EPROTONOSUPPORT; > + goto release_idr; > } > /* Push needs a TTL - if not specified, set a default value. */ > if (!tb[TCA_MPLS_TTL]) { > @@ -238,33 +265,14 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla, > case TCA_MPLS_ACT_MODIFY: > if (tb[TCA_MPLS_PROTO]) { > NL_SET_ERR_MSG_MOD(extack, "Protocol cannot be used with MPLS modify"); > - return -EINVAL; > + err = -EINVAL; > + goto release_idr; > } > break; > default: > NL_SET_ERR_MSG_MOD(extack, "Unknown MPLS action"); > - return -EINVAL; > - } > - > - err = tcf_idr_check_alloc(tn, &index, a, bind); > - if (err < 0) > - return err; > - exists = err; > - if (exists && bind) > - return 0; > - > - if (!exists) { > - ret = tcf_idr_create(tn, index, est, a, > - &act_mpls_ops, bind, true, flags); > - if (ret) { > - tcf_idr_cleanup(tn, index); > - return ret; > - } > - > - ret = ACT_P_CREATED; > - } else if (!(flags & TCA_ACT_FLAGS_REPLACE)) { > - tcf_idr_release(*a, bind); > - return -EEXIST; > + err = -EINVAL; > + goto release_idr; > } > > err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack); > -- > 2.34.1 >
diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c index 6b26bdb999d7..809f7928a1be 100644 --- a/net/sched/act_mpls.c +++ b/net/sched/act_mpls.c @@ -190,40 +190,67 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla, parm = nla_data(tb[TCA_MPLS_PARMS]); index = parm->index; + err = tcf_idr_check_alloc(tn, &index, a, bind); + if (err < 0) + return err; + exists = err; + if (exists && bind) + return 0; + + if (!exists) { + ret = tcf_idr_create(tn, index, est, a, &act_mpls_ops, bind, + true, flags); + if (ret) { + tcf_idr_cleanup(tn, index); + return ret; + } + + ret = ACT_P_CREATED; + } else if (!(flags & TCA_ACT_FLAGS_REPLACE)) { + tcf_idr_release(*a, bind); + return -EEXIST; + } + /* Verify parameters against action type. */ switch (parm->m_action) { case TCA_MPLS_ACT_POP: if (!tb[TCA_MPLS_PROTO]) { NL_SET_ERR_MSG_MOD(extack, "Protocol must be set for MPLS pop"); - return -EINVAL; + err = -EINVAL; + goto release_idr; } if (!eth_proto_is_802_3(nla_get_be16(tb[TCA_MPLS_PROTO]))) { NL_SET_ERR_MSG_MOD(extack, "Invalid protocol type for MPLS pop"); - return -EINVAL; + err = -EINVAL; + goto release_idr; } if (tb[TCA_MPLS_LABEL] || tb[TCA_MPLS_TTL] || tb[TCA_MPLS_TC] || tb[TCA_MPLS_BOS]) { NL_SET_ERR_MSG_MOD(extack, "Label, TTL, TC or BOS cannot be used with MPLS pop"); - return -EINVAL; + err = -EINVAL; + goto release_idr; } break; case TCA_MPLS_ACT_DEC_TTL: if (tb[TCA_MPLS_PROTO] || tb[TCA_MPLS_LABEL] || tb[TCA_MPLS_TTL] || tb[TCA_MPLS_TC] || tb[TCA_MPLS_BOS]) { NL_SET_ERR_MSG_MOD(extack, "Label, TTL, TC, BOS or protocol cannot be used with MPLS dec_ttl"); - return -EINVAL; + err = -EINVAL; + goto release_idr; } break; case TCA_MPLS_ACT_PUSH: case TCA_MPLS_ACT_MAC_PUSH: if (!tb[TCA_MPLS_LABEL]) { NL_SET_ERR_MSG_MOD(extack, "Label is required for MPLS push"); - return -EINVAL; + err = -EINVAL; + goto release_idr; } if (tb[TCA_MPLS_PROTO] && !eth_p_mpls(nla_get_be16(tb[TCA_MPLS_PROTO]))) { NL_SET_ERR_MSG_MOD(extack, "Protocol must be an MPLS type for MPLS push"); - return -EPROTONOSUPPORT; + err = -EPROTONOSUPPORT; + goto release_idr; } /* Push needs a TTL - if not specified, set a default value. */ if (!tb[TCA_MPLS_TTL]) { @@ -238,33 +265,14 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla, case TCA_MPLS_ACT_MODIFY: if (tb[TCA_MPLS_PROTO]) { NL_SET_ERR_MSG_MOD(extack, "Protocol cannot be used with MPLS modify"); - return -EINVAL; + err = -EINVAL; + goto release_idr; } break; default: NL_SET_ERR_MSG_MOD(extack, "Unknown MPLS action"); - return -EINVAL; - } - - err = tcf_idr_check_alloc(tn, &index, a, bind); - if (err < 0) - return err; - exists = err; - if (exists && bind) - return 0; - - if (!exists) { - ret = tcf_idr_create(tn, index, est, a, - &act_mpls_ops, bind, true, flags); - if (ret) { - tcf_idr_cleanup(tn, index); - return ret; - } - - ret = ACT_P_CREATED; - } else if (!(flags & TCA_ACT_FLAGS_REPLACE)) { - tcf_idr_release(*a, bind); - return -EEXIST; + err = -EINVAL; + goto release_idr; } err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
The TC architecture allows filters and actions to be created independently. In filters the user can reference action objects using: tc action add action mpls ... index 1 tc filter add ... action mpls index 1 In the current code for act_mpls 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..53 ok 1 a933 - Add MPLS dec_ttl action with pipe opcode ok 2 08d1 - Add mpls dec_ttl action with pass opcode ok 3 d786 - Add mpls dec_ttl action with drop opcode ok 4 f334 - Add mpls dec_ttl action with reclassify opcode ok 5 29bd - Add mpls dec_ttl action with continue opcode ok 6 48df - Add mpls dec_ttl action with jump opcode ok 7 62eb - Add mpls dec_ttl action with trap opcode ok 8 09d2 - Add mpls dec_ttl action with opcode and cookie ok 9 c170 - Add mpls dec_ttl action with opcode and cookie of max length ok 10 9118 - Add mpls dec_ttl action with invalid opcode ok 11 6ce1 - Add mpls dec_ttl action with label (invalid) ok 12 352f - Add mpls dec_ttl action with tc (invalid) ok 13 fa1c - Add mpls dec_ttl action with ttl (invalid) ok 14 6b79 - Add mpls dec_ttl action with bos (invalid) ok 15 d4c4 - Add mpls pop action with ip proto ok 16 91fb - Add mpls pop action with ip proto and cookie ok 17 92fe - Add mpls pop action with mpls proto ok 18 7e23 - Add mpls pop action with no protocol (invalid) ok 19 6182 - Add mpls pop action with label (invalid) ok 20 6475 - Add mpls pop action with tc (invalid) ok 21 067b - Add mpls pop action with ttl (invalid) ok 22 7316 - Add mpls pop action with bos (invalid) ok 23 38cc - Add mpls push action with label ok 24 c281 - Add mpls push action with mpls_mc protocol ok 25 5db4 - Add mpls push action with label, tc and ttl ok 26 7c34 - Add mpls push action with label, tc ttl and cookie of max length ok 27 16eb - Add mpls push action with label and bos ok 28 d69d - Add mpls push action with no label (invalid) ok 29 e8e4 - Add mpls push action with ipv4 protocol (invalid) ok 30 ecd0 - Add mpls push action with out of range label (invalid) ok 31 d303 - Add mpls push action with out of range tc (invalid) ok 32 fd6e - Add mpls push action with ttl of 0 (invalid) ok 33 19e9 - Add mpls mod action with mpls label ok 34 1fde - Add mpls mod action with max mpls label ok 35 0c50 - Add mpls mod action with mpls label exceeding max (invalid) ok 36 10b6 - Add mpls mod action with mpls label of MPLS_LABEL_IMPLNULL (invalid) ok 37 57c9 - Add mpls mod action with mpls min tc ok 38 6872 - Add mpls mod action with mpls max tc ok 39 a70a - Add mpls mod action with mpls tc exceeding max (invalid) ok 40 6ed5 - Add mpls mod action with mpls ttl ok 41 77c1 - Add mpls mod action with mpls ttl and cookie ok 42 b80f - Add mpls mod action with mpls max ttl ok 43 8864 - Add mpls mod action with mpls min ttl ok 44 6c06 - Add mpls mod action with mpls ttl of 0 (invalid) ok 45 b5d8 - Add mpls mod action with mpls ttl exceeding max (invalid) ok 46 451f - Add mpls mod action with mpls max bos ok 47 a1ed - Add mpls mod action with mpls min bos ok 48 3dcf - Add mpls mod action with mpls bos exceeding max (invalid) ok 49 db7c - Add mpls mod action with protocol (invalid) ok 50 b070 - Replace existing mpls push action with new ID ok 51 95a9 - Replace existing mpls push action with new label, tc, ttl and cookie ok 52 6cce - Delete mpls pop action ok 53 d138 - Flush mpls actions Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC") Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> --- net/sched/act_mpls.c | 66 +++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 29 deletions(-)