| Message ID | 20190626115855.13241-3-nikolay@cumulusnetworks.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | em_ipt: add support for addrtype | expand |
Hi Nikolay, On Wed, 26 Jun 2019 14:58:52 +0300 Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > Set the family based on the protocol otherwise protocol-neutral > matches will have wrong information (e.g. NFPROTO_UNSPEC). In > preparation for using NFPROTO_UNSPEC xt matches. > > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > --- > net/sched/em_ipt.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c > index 64dbafe4e94c..23965a071177 100644 > --- a/net/sched/em_ipt.c > +++ b/net/sched/em_ipt.c > @@ -189,10 +189,12 @@ static int em_ipt_match(struct sk_buff *skb, > struct tcf_ematch *em, case htons(ETH_P_IP): > if (!pskb_network_may_pull(skb, sizeof(struct > iphdr))) return 0; > + state.pf = NFPROTO_IPV4; > break; > case htons(ETH_P_IPV6): > if (!pskb_network_may_pull(skb, sizeof(struct > ipv6hdr))) return 0; > + state.pf = NFPROTO_IPV6; > break; > default: > return 0; > @@ -203,7 +205,7 @@ static int em_ipt_match(struct sk_buff *skb, > struct tcf_ematch *em, if (skb->skb_iif) > indev = dev_get_by_index_rcu(em->net, skb->skb_iif); > > - nf_hook_state_init(&state, im->hook, im->match->family, > + nf_hook_state_init(&state, im->hook, state.pf, > indev ?: skb->dev, skb->dev, NULL, > em->net, NULL); > acpar.match = im->match; I think this change is incompatible with current behavior. Consider the 'policy' match which matches the packet's xfrm state (sec_path) with the provided user space parameters. The sec_path includes information about the encapsulating packet's parameters whereas the current skb points to the encapsulated packet, and the match is done on the encapsulating packet's info. So if you have an IPv6 packet encapsulated within an IPv4 packet, the match parameters should be done using IPv4 parameters, not IPv6. Maybe use the packet's family only if the match family is UNSPEC? Eyal.
On 26/06/2019 16:33, Eyal Birger wrote: > Hi Nikolay, > > On Wed, 26 Jun 2019 14:58:52 +0300 > Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > >> Set the family based on the protocol otherwise protocol-neutral >> matches will have wrong information (e.g. NFPROTO_UNSPEC). In >> preparation for using NFPROTO_UNSPEC xt matches. >> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> >> --- >> net/sched/em_ipt.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c >> index 64dbafe4e94c..23965a071177 100644 >> --- a/net/sched/em_ipt.c >> +++ b/net/sched/em_ipt.c >> @@ -189,10 +189,12 @@ static int em_ipt_match(struct sk_buff *skb, >> struct tcf_ematch *em, case htons(ETH_P_IP): >> if (!pskb_network_may_pull(skb, sizeof(struct >> iphdr))) return 0; >> + state.pf = NFPROTO_IPV4; >> break; >> case htons(ETH_P_IPV6): >> if (!pskb_network_may_pull(skb, sizeof(struct >> ipv6hdr))) return 0; >> + state.pf = NFPROTO_IPV6; >> break; >> default: >> return 0; >> @@ -203,7 +205,7 @@ static int em_ipt_match(struct sk_buff *skb, >> struct tcf_ematch *em, if (skb->skb_iif) >> indev = dev_get_by_index_rcu(em->net, skb->skb_iif); >> >> - nf_hook_state_init(&state, im->hook, im->match->family, >> + nf_hook_state_init(&state, im->hook, state.pf, >> indev ?: skb->dev, skb->dev, NULL, >> em->net, NULL); >> acpar.match = im->match; > > I think this change is incompatible with current behavior. > > Consider the 'policy' match which matches the packet's xfrm state (sec_path) > with the provided user space parameters. The sec_path includes information > about the encapsulating packet's parameters whereas the current skb points to > the encapsulated packet, and the match is done on the encapsulating > packet's info. > > So if you have an IPv6 packet encapsulated within an IPv4 packet, the match > parameters should be done using IPv4 parameters, not IPv6. > > Maybe use the packet's family only if the match family is UNSPEC? > > Eyal. > Hi Eyal, I see your point, I was wondering about the xfrm cases. :) In such case I think we can simplify the set and do it only on UNSPEC matches as you suggest. Maybe we should enforce the tc protocol based on the user-specified nfproto at least from iproute2 otherwise people can add mismatching rules (e.g. nfproto == v6, tc proto == v4). Thanks, Nik
On Wed, 26 Jun 2019 16:45:28 +0300 Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > On 26/06/2019 16:33, Eyal Birger wrote: > > Hi Nikolay, > > > > On Wed, 26 Jun 2019 14:58:52 +0300 > > Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > > > >> Set the family based on the protocol otherwise protocol-neutral > >> matches will have wrong information (e.g. NFPROTO_UNSPEC). In > >> preparation for using NFPROTO_UNSPEC xt matches. > >> > >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > >> --- > >> net/sched/em_ipt.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> ... > >> - nf_hook_state_init(&state, im->hook, im->match->family, > >> + nf_hook_state_init(&state, im->hook, state.pf, > >> indev ?: skb->dev, skb->dev, NULL, > >> em->net, NULL); > >> acpar.match = im->match; > > > > I think this change is incompatible with current behavior. > > > > Consider the 'policy' match which matches the packet's xfrm state > > (sec_path) with the provided user space parameters. The sec_path > > includes information about the encapsulating packet's parameters > > whereas the current skb points to the encapsulated packet, and the > > match is done on the encapsulating packet's info. > > > > So if you have an IPv6 packet encapsulated within an IPv4 packet, > > the match parameters should be done using IPv4 parameters, not IPv6. > > > > Maybe use the packet's family only if the match family is UNSPEC? > > > > Eyal. > > > > Hi Eyal, > I see your point, I was wondering about the xfrm cases. :) > In such case I think we can simplify the set and do it only on UNSPEC > matches as you suggest. > > Maybe we should enforce the tc protocol based on the user-specified > nfproto at least from iproute2 otherwise people can add mismatching > rules (e.g. nfproto == v6, tc proto == v4). > Hi Nik, I think for iproute2 the issue is the same. For encapsulated IPv6 in IPv4 for example, tc proto will be IPv6 (tc sees the encapsulated packet after decryption) whereas nfproto will be IPv4 (policy match is done on the encapsulating state metadata which is IPv4). I think the part missing in iproute2 is the ability to specify NFPROTO_UNSPEC. Thanks, Eyal
On 26 June 2019 19:22:54 EEST, Eyal Birger <eyal.birger@gmail.com> wrote: >On Wed, 26 Jun 2019 16:45:28 +0300 >Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: > >> On 26/06/2019 16:33, Eyal Birger wrote: >> > Hi Nikolay, >> > >> > On Wed, 26 Jun 2019 14:58:52 +0300 >> > Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote: >> > >> >> Set the family based on the protocol otherwise protocol-neutral >> >> matches will have wrong information (e.g. NFPROTO_UNSPEC). In >> >> preparation for using NFPROTO_UNSPEC xt matches. >> >> >> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> >> >> --- >> >> net/sched/em_ipt.c | 4 +++- >> >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> >... >> >> - nf_hook_state_init(&state, im->hook, im->match->family, >> >> + nf_hook_state_init(&state, im->hook, state.pf, >> >> indev ?: skb->dev, skb->dev, NULL, >> >> em->net, NULL); >> >> acpar.match = im->match; >> > >> > I think this change is incompatible with current behavior. >> > >> > Consider the 'policy' match which matches the packet's xfrm state >> > (sec_path) with the provided user space parameters. The sec_path >> > includes information about the encapsulating packet's parameters >> > whereas the current skb points to the encapsulated packet, and the >> > match is done on the encapsulating packet's info. >> > >> > So if you have an IPv6 packet encapsulated within an IPv4 packet, >> > the match parameters should be done using IPv4 parameters, not >IPv6. >> > >> > Maybe use the packet's family only if the match family is UNSPEC? >> > >> > Eyal. >> > >> >> Hi Eyal, >> I see your point, I was wondering about the xfrm cases. :) >> In such case I think we can simplify the set and do it only on UNSPEC >> matches as you suggest. >> >> Maybe we should enforce the tc protocol based on the user-specified >> nfproto at least from iproute2 otherwise people can add mismatching >> rules (e.g. nfproto == v6, tc proto == v4). >> >Hi Nik, > >I think for iproute2 the issue is the same. For encapsulated IPv6 in >IPv4 for example, tc proto will be IPv6 (tc sees the encapsulated >packet after decryption) whereas nfproto will be IPv4 (policy match is >done on the encapsulating state metadata which is IPv4). > >I think the part missing in iproute2 is the ability to specify >NFPROTO_UNSPEC. > >Thanks, >Eyal Right, I answered too quickly, it makes sense to mix them for xt policy. I also plan to add support for clsact, it should be trivial and iproute2-only change. Thanks, Nik
diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c index 64dbafe4e94c..23965a071177 100644 --- a/net/sched/em_ipt.c +++ b/net/sched/em_ipt.c @@ -189,10 +189,12 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em, case htons(ETH_P_IP): if (!pskb_network_may_pull(skb, sizeof(struct iphdr))) return 0; + state.pf = NFPROTO_IPV4; break; case htons(ETH_P_IPV6): if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr))) return 0; + state.pf = NFPROTO_IPV6; break; default: return 0; @@ -203,7 +205,7 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em, if (skb->skb_iif) indev = dev_get_by_index_rcu(em->net, skb->skb_iif); - nf_hook_state_init(&state, im->hook, im->match->family, + nf_hook_state_init(&state, im->hook, state.pf, indev ?: skb->dev, skb->dev, NULL, em->net, NULL); acpar.match = im->match;
Set the family based on the protocol otherwise protocol-neutral matches will have wrong information (e.g. NFPROTO_UNSPEC). In preparation for using NFPROTO_UNSPEC xt matches. Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> --- net/sched/em_ipt.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)