| Message ID | 20230224150058.149505-2-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: fix action bind logic | expand |
On Fri, Feb 24, 2023 at 12:00:56PM -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 pedit ... index 1 > tc filter add ... action pedit index 1 > > In the current code for act_pedit 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..69 ... Hi Pedro, Thanks for running the tests :) I think this patch looks good - though I am still digesting it. But I do wonder if you considered adding a test for this condition. Also, what is the failure mode? If it is that user's can't bind actions to filters, but the kernel behaves correctly with configurations it accepts. If so, then perhaps this is more of a feature than a fix. OTOH, perhaps it's a regression wrt the oldest of the two patches references below. I've haven't looked at the other patches in this series yet. But I expect my comments apply to them too. > Fixes: 71d0ed7079df ("net/act_pedit: Support using offset relative to the conventional network headers") > Fixes: f67169fef8db ("net/sched: act_pedit: fix WARN() in the traffic path") > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> ...
On 25/02/2023 10:08, Simon Horman wrote: > On Fri, Feb 24, 2023 at 12:00:56PM -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 pedit ... index 1 >> tc filter add ... action pedit index 1 >> >> In the current code for act_pedit 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..69 > > ... > > Hi Pedro, > > Thanks for running the tests :) > > I think this patch looks good - though I am still digesting it. > But I do wonder if you considered adding a test for this condition. Yes, they are in my backlog to post when net-next reopens. > > Also, what is the failure mode? When referencing actions via its indexes on filters there would be three outcomes: 1 - Action binds to filter (expected) 2 - Action fails netlink parsing in kernel 3 - Action fails parsing in iproute2 I also posted complementary iproute2 patches. > > If it is that user's can't bind actions to filters, but the kernel behaves > correctly with configurations it accepts. If so, then perhaps this is more > of a feature than a fix. I would argue it's a fix... > OTOH, perhaps it's a regression wrt the oldest of > the two patches references below. ...because filters and actions are completely separate TC objects. There shouldn't be actions that can be created independently but can't be really used. > > I've haven't looked at the other patches in this series yet. > But I expect my comments apply to them too. > >> Fixes: 71d0ed7079df ("net/act_pedit: Support using offset relative to the conventional network headers") >> Fixes: f67169fef8db ("net/sched: act_pedit: fix WARN() in the traffic path") >> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> >> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > > ...
On Sat, Feb 25, 2023 at 10:38:31AM -0300, Pedro Tammela wrote: > On 25/02/2023 10:08, Simon Horman wrote: > > On Fri, Feb 24, 2023 at 12:00:56PM -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 pedit ... index 1 > > > tc filter add ... action pedit index 1 > > > > > > In the current code for act_pedit 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..69 > > > > ... > > > > Hi Pedro, > > > > Thanks for running the tests :) > > > > I think this patch looks good - though I am still digesting it. > > But I do wonder if you considered adding a test for this condition. > > Yes, they are in my backlog to post when net-next reopens. Excellent, thanks. > > Also, what is the failure mode? > > When referencing actions via its indexes on filters there would be three > outcomes: > 1 - Action binds to filter (expected) > 2 - Action fails netlink parsing in kernel > 3 - Action fails parsing in iproute2 > > I also posted complementary iproute2 patches. > > > > > If it is that user's can't bind actions to filters, but the kernel behaves > > correctly with configurations it accepts. If so, then perhaps this is more > > of a feature than a fix. > > I would argue it's a fix... > > > OTOH, perhaps it's a regression wrt the oldest of > > the two patches references below. > > ...because filters and actions are completely separate TC objects. > There shouldn't be actions that can be created independently but can't be > really used. I agree that shouldn't be the case. For me that doesn't make it a bug, but I don't feel strongly about it. In any case, I'm now happy with this patch. Reviewed-by: Simon Horman <simon.horman@corigine.com> ...
On Sat, 25 Feb 2023 17:15:00 +0100 Simon Horman wrote: > > > OTOH, perhaps it's a regression wrt the oldest of > > > the two patches references below. > > > > ...because filters and actions are completely separate TC objects. > > There shouldn't be actions that can be created independently but can't be > > really used. > > I agree that shouldn't be the case. > For me that doesn't make it a bug, but I don't feel strongly about it. I'm with Simon - this is a long standing problem, and we weren't getting any user complaints about this. So I also prefer to route this via net-next, without the Fixes tags.
On Mon, Feb 27, 2023 at 2:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Sat, 25 Feb 2023 17:15:00 +0100 Simon Horman wrote: > > > > OTOH, perhaps it's a regression wrt the oldest of > > > > the two patches references below. > > > > > > ...because filters and actions are completely separate TC objects. > > > There shouldn't be actions that can be created independently but can't be > > > really used. > > > > I agree that shouldn't be the case. > > For me that doesn't make it a bug, but I don't feel strongly about it. > > I'm with Simon - this is a long standing problem, and we weren't getting > any user complaints about this. So I also prefer to route this via > net-next, without the Fixes tags. At minimum the pedit is a fix. The rest is toss-a-coin and put in net-next or net. cheers, jamal
On Mon, 27 Feb 2023 14:51:58 -0500 Jamal Hadi Salim wrote: > > > I agree that shouldn't be the case. > > > For me that doesn't make it a bug, but I don't feel strongly about it. > > > > I'm with Simon - this is a long standing problem, and we weren't getting > > any user complaints about this. So I also prefer to route this via > > net-next, without the Fixes tags. > > At minimum the pedit is a fix. How come? What makes pedit different? It's kinda hard to parse from the diff and the commit messages look copy/pasted. > The rest is toss-a-coin and put in net-next or net.
On Mon, 27 Feb 2023 12:04:20 -0800 Jakub Kicinski wrote: > > > I'm with Simon - this is a long standing problem, and we weren't getting > > > any user complaints about this. So I also prefer to route this via > > > net-next, without the Fixes tags. > > > > At minimum the pedit is a fix. > > How come? What makes pedit different? > It's kinda hard to parse from the diff and the commit messages > look copy/pasted. Ah, looks like DaveM already applied this (not v2), so the discussion is moot.
On Mon, Feb 27, 2023 at 4:41 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 27 Feb 2023 12:04:20 -0800 Jakub Kicinski wrote: > > > > I'm with Simon - this is a long standing problem, and we weren't getting > > > > any user complaints about this. So I also prefer to route this via > > > > net-next, without the Fixes tags. > > > > > > At minimum the pedit is a fix. > > > > How come? What makes pedit different? > > It's kinda hard to parse from the diff and the commit messages > > look copy/pasted. > > Ah, looks like DaveM already applied this (not v2), so the discussion > is moot. for completion's sake: pedit worked and then got broken at some point. The others, I believe the review missed the details unfortunately; those features are a requirement but someone cutnpasted and missed something. Note: The only reason we even found this is because some hardware (nameless at this point) capable of offloading and sharing pedit actions didnt work. Looking closely we found the other two. cheers, jamal
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index 77d288d384ae..4559a1507ea5 100644 --- a/net/sched/act_pedit.c +++ b/net/sched/act_pedit.c @@ -181,26 +181,6 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla, } parm = nla_data(pattr); - if (!parm->nkeys) { - NL_SET_ERR_MSG_MOD(extack, "Pedit requires keys to be passed"); - return -EINVAL; - } - ksize = parm->nkeys * sizeof(struct tc_pedit_key); - if (nla_len(pattr) < sizeof(*parm) + ksize) { - NL_SET_ERR_MSG_ATTR(extack, pattr, "Length of TCA_PEDIT_PARMS or TCA_PEDIT_PARMS_EX pedit attribute is invalid"); - return -EINVAL; - } - - nparms = kzalloc(sizeof(*nparms), GFP_KERNEL); - if (!nparms) - return -ENOMEM; - - nparms->tcfp_keys_ex = - tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys); - if (IS_ERR(nparms->tcfp_keys_ex)) { - ret = PTR_ERR(nparms->tcfp_keys_ex); - goto out_free; - } index = parm->index; err = tcf_idr_check_alloc(tn, &index, a, bind); @@ -209,25 +189,49 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla, &act_pedit_ops, bind, flags); if (ret) { tcf_idr_cleanup(tn, index); - goto out_free_ex; + return ret; } ret = ACT_P_CREATED; } else if (err > 0) { if (bind) - goto out_free; + return 0; if (!(flags & TCA_ACT_FLAGS_REPLACE)) { ret = -EEXIST; goto out_release; } } else { - ret = err; - goto out_free_ex; + return err; + } + + if (!parm->nkeys) { + NL_SET_ERR_MSG_MOD(extack, "Pedit requires keys to be passed"); + ret = -EINVAL; + goto out_release; + } + ksize = parm->nkeys * sizeof(struct tc_pedit_key); + if (nla_len(pattr) < sizeof(*parm) + ksize) { + NL_SET_ERR_MSG_ATTR(extack, pattr, "Length of TCA_PEDIT_PARMS or TCA_PEDIT_PARMS_EX pedit attribute is invalid"); + ret = -EINVAL; + goto out_release; + } + + nparms = kzalloc(sizeof(*nparms), GFP_KERNEL); + if (!nparms) { + ret = -ENOMEM; + goto out_release; + } + + nparms->tcfp_keys_ex = + tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys); + if (IS_ERR(nparms->tcfp_keys_ex)) { + ret = PTR_ERR(nparms->tcfp_keys_ex); + goto out_free; } err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack); if (err < 0) { ret = err; - goto out_release; + goto out_free_ex; } nparms->tcfp_off_max_hint = 0; @@ -278,12 +282,12 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla, put_chain: if (goto_ch) tcf_chain_put_by_act(goto_ch); -out_release: - tcf_idr_release(*a, bind); out_free_ex: kfree(nparms->tcfp_keys_ex); out_free: kfree(nparms); +out_release: + tcf_idr_release(*a, bind); return ret; }
The TC architecture allows filters and actions to be created independently. In filters the user can reference action objects using: tc action add action pedit ... index 1 tc filter add ... action pedit index 1 In the current code for act_pedit 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..69 ok 1 319a - Add pedit action that mangles IP TTL ok 2 7e67 - Replace pedit action with invalid goto chain ok 3 377e - Add pedit action with RAW_OP offset u32 ok 4 a0ca - Add pedit action with RAW_OP offset u32 (INVALID) ok 5 dd8a - Add pedit action with RAW_OP offset u16 u16 ok 6 53db - Add pedit action with RAW_OP offset u16 (INVALID) ok 7 5c7e - Add pedit action with RAW_OP offset u8 add value ok 8 2893 - Add pedit action with RAW_OP offset u8 quad ok 9 3a07 - Add pedit action with RAW_OP offset u8-u16-u8 ok 10 ab0f - Add pedit action with RAW_OP offset u16-u8-u8 ok 11 9d12 - Add pedit action with RAW_OP offset u32 set u16 clear u8 invert ok 12 ebfa - Add pedit action with RAW_OP offset overflow u32 (INVALID) ok 13 f512 - Add pedit action with RAW_OP offset u16 at offmask shift set ok 14 c2cb - Add pedit action with RAW_OP offset u32 retain value ok 15 1762 - Add pedit action with RAW_OP offset u8 clear value ok 16 bcee - Add pedit action with RAW_OP offset u8 retain value ok 17 e89f - Add pedit action with RAW_OP offset u16 retain value ok 18 c282 - Add pedit action with RAW_OP offset u32 clear value ok 19 c422 - Add pedit action with RAW_OP offset u16 invert value ok 20 d3d3 - Add pedit action with RAW_OP offset u32 invert value ok 21 57e5 - Add pedit action with RAW_OP offset u8 preserve value ok 22 99e0 - Add pedit action with RAW_OP offset u16 preserve value ok 23 1892 - Add pedit action with RAW_OP offset u32 preserve value ok 24 4b60 - Add pedit action with RAW_OP negative offset u16/u32 set value ok 25 a5a7 - Add pedit action with LAYERED_OP eth set src ok 26 86d4 - Add pedit action with LAYERED_OP eth set src & dst ok 27 f8a9 - Add pedit action with LAYERED_OP eth set dst ok 28 c715 - Add pedit action with LAYERED_OP eth set src (INVALID) ok 29 8131 - Add pedit action with LAYERED_OP eth set dst (INVALID) ok 30 ba22 - Add pedit action with LAYERED_OP eth type set/clear sequence ok 31 dec4 - Add pedit action with LAYERED_OP eth set type (INVALID) ok 32 ab06 - Add pedit action with LAYERED_OP eth add type ok 33 918d - Add pedit action with LAYERED_OP eth invert src ok 34 a8d4 - Add pedit action with LAYERED_OP eth invert dst ok 35 ee13 - Add pedit action with LAYERED_OP eth invert type ok 36 7588 - Add pedit action with LAYERED_OP ip set src ok 37 0fa7 - Add pedit action with LAYERED_OP ip set dst ok 38 5810 - Add pedit action with LAYERED_OP ip set src & dst ok 39 1092 - Add pedit action with LAYERED_OP ip set ihl & dsfield ok 40 02d8 - Add pedit action with LAYERED_OP ip set ttl & protocol ok 41 3e2d - Add pedit action with LAYERED_OP ip set ttl (INVALID) ok 42 31ae - Add pedit action with LAYERED_OP ip ttl clear/set ok 43 486f - Add pedit action with LAYERED_OP ip set duplicate fields ok 44 e790 - Add pedit action with LAYERED_OP ip set ce, df, mf, firstfrag, nofrag fields ok 45 cc8a - Add pedit action with LAYERED_OP ip set tos ok 46 7a17 - Add pedit action with LAYERED_OP ip set precedence ok 47 c3b6 - Add pedit action with LAYERED_OP ip add tos ok 48 43d3 - Add pedit action with LAYERED_OP ip add precedence ok 49 438e - Add pedit action with LAYERED_OP ip clear tos ok 50 6b1b - Add pedit action with LAYERED_OP ip clear precedence ok 51 824a - Add pedit action with LAYERED_OP ip invert tos ok 52 106f - Add pedit action with LAYERED_OP ip invert precedence ok 53 6829 - Add pedit action with LAYERED_OP beyond ip set dport & sport ok 54 afd8 - Add pedit action with LAYERED_OP beyond ip set icmp_type & icmp_code ok 55 3143 - Add pedit action with LAYERED_OP beyond ip set dport (INVALID) ok 56 815c - Add pedit action with LAYERED_OP ip6 set src ok 57 4dae - Add pedit action with LAYERED_OP ip6 set dst ok 58 fc1f - Add pedit action with LAYERED_OP ip6 set src & dst ok 59 6d34 - Add pedit action with LAYERED_OP ip6 dst retain value (INVALID) ok 60 94bb - Add pedit action with LAYERED_OP ip6 traffic_class ok 61 6f5e - Add pedit action with LAYERED_OP ip6 flow_lbl ok 62 6795 - Add pedit action with LAYERED_OP ip6 set payload_len, nexthdr, hoplimit ok 63 1442 - Add pedit action with LAYERED_OP tcp set dport & sport ok 64 b7ac - Add pedit action with LAYERED_OP tcp sport set (INVALID) ok 65 cfcc - Add pedit action with LAYERED_OP tcp flags set ok 66 3bc4 - Add pedit action with LAYERED_OP tcp set dport, sport & flags fields ok 67 f1c8 - Add pedit action with LAYERED_OP udp set dport & sport ok 68 d784 - Add pedit action with mixed RAW/LAYERED_OP #1 ok 69 70ca - Add pedit action with mixed RAW/LAYERED_OP #2 Fixes: 71d0ed7079df ("net/act_pedit: Support using offset relative to the conventional network headers") Fixes: f67169fef8db ("net/sched: act_pedit: fix WARN() in the traffic path") Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> --- net/sched/act_pedit.c | 58 +++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 27 deletions(-)