| Message ID | 20190626155615.16639-4-nikolay@cumulusnetworks.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | em_ipt: add support for addrtype | expand |
Hi Nik, On Wed, 26 Jun 2019 18:56:14 +0300 Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > For NFPROTO_UNSPEC xt_matches there's no way to restrict the matching > to a specific family, in order to do so we record the user-specified > family and later enforce it while doing the match. > > v2: adjust changes to missing patch, was patch 04 in v1 > > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > --- > net/sched/em_ipt.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > ..snip.. > @@ -182,8 +195,8 @@ static int em_ipt_match(struct sk_buff *skb, > struct tcf_ematch *em, const struct em_ipt_match *im = (const void > *)em->data; struct xt_action_param acpar = {}; > struct net_device *indev = NULL; > - u8 nfproto = im->match->family; > struct nf_hook_state state; > + u8 nfproto = im->nfproto; Maybe I'm missing something now - but it's not really clear to me now why keeping im->nfproto would be useful: If NFPROTO_UNSPEC was provided by userspace then the actual nfproto used will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified from userspace then it will equal im->match->family. Is there any case where the resulting nfproto would differ as a result of this patch? Otherwise the patchset looks excellent to me. Thanks! Eyal.
On 26 June 2019 19:18:35 EEST, Eyal Birger <eyal.birger@gmail.com> wrote: >Hi Nik, > >On Wed, 26 Jun 2019 18:56:14 +0300 >Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > >> For NFPROTO_UNSPEC xt_matches there's no way to restrict the matching >> to a specific family, in order to do so we record the user-specified >> family and later enforce it while doing the match. >> >> v2: adjust changes to missing patch, was patch 04 in v1 >> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> >> --- >> net/sched/em_ipt.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >..snip.. >> @@ -182,8 +195,8 @@ static int em_ipt_match(struct sk_buff *skb, >> struct tcf_ematch *em, const struct em_ipt_match *im = (const void >> *)em->data; struct xt_action_param acpar = {}; >> struct net_device *indev = NULL; >> - u8 nfproto = im->match->family; >> struct nf_hook_state state; >> + u8 nfproto = im->nfproto; > >Maybe I'm missing something now - but it's not really clear to me now >why keeping im->nfproto would be useful: > >If NFPROTO_UNSPEC was provided by userspace then the actual nfproto >used >will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified >from userspace then it will equal im->match->family. > >Is there any case where the resulting nfproto would differ as a result >of this patch? > >Otherwise the patchset looks excellent to me. > >Thanks! >Eyal. Hi, It's needed to limit the match only to the user-specified family for unspec xt matches. The problem is otherwise im->match->family stays at nfproto_unspec regardless of the user choice. Thanks for reviewing the set. Cheers, Nik
On 26 June 2019 19:33:48 EEST, nikolay@cumulusnetworks.com wrote: >On 26 June 2019 19:18:35 EEST, Eyal Birger <eyal.birger@gmail.com> >wrote: >>Hi Nik, >> >>On Wed, 26 Jun 2019 18:56:14 +0300 >>Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: >> >>> For NFPROTO_UNSPEC xt_matches there's no way to restrict the >matching >>> to a specific family, in order to do so we record the user-specified >>> family and later enforce it while doing the match. >>> >>> v2: adjust changes to missing patch, was patch 04 in v1 >>> >>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> >>> --- >>> net/sched/em_ipt.c | 17 +++++++++++++++-- >>> 1 file changed, 15 insertions(+), 2 deletions(-) >>> >>..snip.. >>> @@ -182,8 +195,8 @@ static int em_ipt_match(struct sk_buff *skb, >>> struct tcf_ematch *em, const struct em_ipt_match *im = (const void >>> *)em->data; struct xt_action_param acpar = {}; >>> struct net_device *indev = NULL; >>> - u8 nfproto = im->match->family; >>> struct nf_hook_state state; >>> + u8 nfproto = im->nfproto; >> >>Maybe I'm missing something now - but it's not really clear to me now >>why keeping im->nfproto would be useful: >> >>If NFPROTO_UNSPEC was provided by userspace then the actual nfproto >>used >>will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified >>from userspace then it will equal im->match->family. >> >>Is there any case where the resulting nfproto would differ as a result >>of this patch? >> >>Otherwise the patchset looks excellent to me. >> >>Thanks! >>Eyal. > >Hi, >It's needed to limit the match only to the user-specified family >for unspec xt matches. The problem is otherwise im->match->family >stays at nfproto_unspec regardless of the user choice. > >Thanks for reviewing the set. > >Cheers, > Nik Hm, while that is true, thinking more about it - mixing the user proto and the real proto could be problematic since we no longer enforce them to be equal, but we check the network header len based on the packet only and we can end up checking v4 len and parsing it as nfproto v6. I'll spin v3 with unspec only and we can restrict it later if needed.
diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c index fd7f5b288c31..ce91f3cea0bd 100644 --- a/net/sched/em_ipt.c +++ b/net/sched/em_ipt.c @@ -21,6 +21,7 @@ struct em_ipt_match { const struct xt_match *match; u32 hook; + u8 nfproto; u8 match_data[0] __aligned(8); }; @@ -115,6 +116,7 @@ static int em_ipt_change(struct net *net, void *data, int data_len, struct em_ipt_match *im = NULL; struct xt_match *match; int mdata_len, ret; + u8 nfproto; ret = nla_parse_deprecated(tb, TCA_EM_IPT_MAX, data, data_len, em_ipt_policy, NULL); @@ -125,6 +127,16 @@ static int em_ipt_change(struct net *net, void *data, int data_len, !tb[TCA_EM_IPT_MATCH_DATA] || !tb[TCA_EM_IPT_NFPROTO]) return -EINVAL; + nfproto = nla_get_u8(tb[TCA_EM_IPT_NFPROTO]); + switch (nfproto) { + case NFPROTO_IPV4: + case NFPROTO_IPV6: + case NFPROTO_UNSPEC: + break; + default: + return -EINVAL; + } + match = get_xt_match(tb); if (IS_ERR(match)) { pr_err("unable to load match\n"); @@ -140,6 +152,7 @@ static int em_ipt_change(struct net *net, void *data, int data_len, im->match = match; im->hook = nla_get_u32(tb[TCA_EM_IPT_HOOK]); + im->nfproto = nfproto; nla_memcpy(im->match_data, tb[TCA_EM_IPT_MATCH_DATA], mdata_len); ret = check_match(net, im, mdata_len); @@ -182,8 +195,8 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em, const struct em_ipt_match *im = (const void *)em->data; struct xt_action_param acpar = {}; struct net_device *indev = NULL; - u8 nfproto = im->match->family; struct nf_hook_state state; + u8 nfproto = im->nfproto; int ret; switch (tc_skb_protocol(skb)) { @@ -231,7 +244,7 @@ static int em_ipt_dump(struct sk_buff *skb, struct tcf_ematch *em) return -EMSGSIZE; if (nla_put_u8(skb, TCA_EM_IPT_MATCH_REVISION, im->match->revision) < 0) return -EMSGSIZE; - if (nla_put_u8(skb, TCA_EM_IPT_NFPROTO, im->match->family) < 0) + if (nla_put_u8(skb, TCA_EM_IPT_NFPROTO, im->nfproto) < 0) return -EMSGSIZE; if (nla_put(skb, TCA_EM_IPT_MATCH_DATA, im->match->usersize ?: im->match->matchsize,
For NFPROTO_UNSPEC xt_matches there's no way to restrict the matching to a specific family, in order to do so we record the user-specified family and later enforce it while doing the match. v2: adjust changes to missing patch, was patch 04 in v1 Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> --- net/sched/em_ipt.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)