| Message ID | 1462985253-2380625-7-git-send-email-tom@herbertland.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Tom, On Wed, 11 May 2016 09:47:26 -0700 Tom Herbert <tom@herbertland.com> wrote: > In ip6_input_finish the protocol handle returns a value greater than > zero the packet needs to be resubmitted using the returned protocol. > The returned protocol is being ignored and each time through resubmit > nexthdr is taken from an offest in the packet. This patch fixes that > so that nexthdr is taken from return value of the protocol handler. > > Signed-off-by: Tom Herbert <tom@herbertland.com> > --- > net/ipv6/ip6_input.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c > index 6ed5601..2a0258a 100644 > --- a/net/ipv6/ip6_input.c > +++ b/net/ipv6/ip6_input.c > @@ -222,13 +222,14 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk > */ > > rcu_read_lock(); > -resubmit: > + > idev = ip6_dst_idev(skb_dst(skb)); > if (!pskb_pull(skb, skb_transport_offset(skb))) > goto discard; > nhoff = IP6CB(skb)->nhoff; > nexthdr = skb_network_header(skb)[nhoff]; > > +resubmit: This has already been attempted in 0243508edd "ipv6: Fix protocol resubmission" and reverted in 1b0ccfe54a. It looks that in some genuine extension header handling cases of ipv6 (not related to encapsulation), the original resubmission code REALLY requires one to re-read IP6CB(skb)->nhoff and refetch the nexthdr. See ipv6_rthdr_rcv and ipv6_destopt_rcv for example: Snip from ipv6_rthdr_rcv: struct inet6_skb_parm *opt = IP6CB(skb); opt->lastopt = opt->srcrt = skb_network_header_len(skb); skb->transport_header += (hdr->hdrlen + 1) << 3; opt->dst0 = opt->dst1; opt->dst1 = 0; opt->nhoff = (&hdr->nexthdr) - skb_network_header(skb); return 1; Snip from ipv6_destopt_rcv: opt = IP6CB(skb); #if IS_ENABLED(CONFIG_IPV6_MIP6) opt->nhoff = dstbuf; #else opt->nhoff = opt->dst1; #endif return 1; I think there are two "resubmission" cases: 1. original ipv6 extension header handling, which seem to require nexthdr re-read (after IP6CB(skb)->nhoff re-assigned), as seen above 2. encapsulation resubmission (e.g. fou) One suggestion: we may identify the encapsulation case by returning the negative of the proto number. Another suggestion: we can take your approach, but execute the nexthdr re-read within ipv6_rthdr_rcv and ipv6_destopt_rcv (and similar inet6_protocol.handler functions). Regards, Shmulik
On Thu, May 12, 2016 at 1:23 PM, Shmulik Ladkani <shmulik.ladkani@gmail.com> wrote: > Hi Tom, > > On Wed, 11 May 2016 09:47:26 -0700 Tom Herbert <tom@herbertland.com> wrote: >> In ip6_input_finish the protocol handle returns a value greater than >> zero the packet needs to be resubmitted using the returned protocol. >> The returned protocol is being ignored and each time through resubmit >> nexthdr is taken from an offest in the packet. This patch fixes that >> so that nexthdr is taken from return value of the protocol handler. >> >> Signed-off-by: Tom Herbert <tom@herbertland.com> >> --- >> net/ipv6/ip6_input.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c >> index 6ed5601..2a0258a 100644 >> --- a/net/ipv6/ip6_input.c >> +++ b/net/ipv6/ip6_input.c >> @@ -222,13 +222,14 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk >> */ >> >> rcu_read_lock(); >> -resubmit: >> + >> idev = ip6_dst_idev(skb_dst(skb)); >> if (!pskb_pull(skb, skb_transport_offset(skb))) >> goto discard; >> nhoff = IP6CB(skb)->nhoff; >> nexthdr = skb_network_header(skb)[nhoff]; >> >> +resubmit: > > This has already been attempted in 0243508edd "ipv6: Fix protocol > resubmission" and reverted in 1b0ccfe54a. > > It looks that in some genuine extension header handling cases of ipv6 > (not related to encapsulation), the original resubmission code REALLY > requires one to re-read IP6CB(skb)->nhoff and refetch the nexthdr. > Is there any reason why the EH handlers can't read the nexthdr and return that? Tom > See ipv6_rthdr_rcv and ipv6_destopt_rcv for example: > > Snip from ipv6_rthdr_rcv: > > struct inet6_skb_parm *opt = IP6CB(skb); > > opt->lastopt = opt->srcrt = skb_network_header_len(skb); > skb->transport_header += (hdr->hdrlen + 1) << 3; > opt->dst0 = opt->dst1; > opt->dst1 = 0; > opt->nhoff = (&hdr->nexthdr) - skb_network_header(skb); > return 1; > > Snip from ipv6_destopt_rcv: > > opt = IP6CB(skb); > #if IS_ENABLED(CONFIG_IPV6_MIP6) > opt->nhoff = dstbuf; > #else > opt->nhoff = opt->dst1; > #endif > return 1; > > I think there are two "resubmission" cases: > 1. original ipv6 extension header handling, which seem to require > nexthdr re-read (after IP6CB(skb)->nhoff re-assigned), as seen above > 2. encapsulation resubmission (e.g. fou) > > One suggestion: we may identify the encapsulation case by returning the > negative of the proto number. > > Another suggestion: we can take your approach, but execute the nexthdr > re-read within ipv6_rthdr_rcv and ipv6_destopt_rcv (and similar > inet6_protocol.handler functions). > > Regards, > Shmulik
Hi Tom, On Thu, 12 May 2016 14:45:36 -0700 Tom Herbert <tom@herbertland.com> wrote: > On Thu, May 12, 2016 at 1:23 PM, Shmulik Ladkani > <shmulik.ladkani@gmail.com> wrote: > > On Wed, 11 May 2016 09:47:26 -0700 Tom Herbert <tom@herbertland.com> wrote: > >> In ip6_input_finish the protocol handle returns a value greater than > >> zero the packet needs to be resubmitted using the returned protocol. > >> The returned protocol is being ignored and each time through resubmit > >> nexthdr is taken from an offest in the packet. This patch fixes that > >> so that nexthdr is taken from return value of the protocol handler. > >> > >> Signed-off-by: Tom Herbert <tom@herbertland.com> > >> --- > >> net/ipv6/ip6_input.c | 9 ++++++--- > >> 1 file changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c > >> index 6ed5601..2a0258a 100644 > >> --- a/net/ipv6/ip6_input.c > >> +++ b/net/ipv6/ip6_input.c > >> @@ -222,13 +222,14 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk > >> */ > >> > >> rcu_read_lock(); > >> -resubmit: > >> + > >> idev = ip6_dst_idev(skb_dst(skb)); > >> if (!pskb_pull(skb, skb_transport_offset(skb))) > >> goto discard; > >> nhoff = IP6CB(skb)->nhoff; > >> nexthdr = skb_network_header(skb)[nhoff]; > >> > >> +resubmit: > > > > This has already been attempted in 0243508edd "ipv6: Fix protocol > > resubmission" and reverted in 1b0ccfe54a. > > > > It looks that in some genuine extension header handling cases of ipv6 > > (not related to encapsulation), the original resubmission code REALLY > > requires one to re-read IP6CB(skb)->nhoff and refetch the nexthdr. > > > Is there any reason why the EH handlers can't read the nexthdr and return > that? Unaware of any reason; It does looks like we can simply do so for the EH handlers (this was my 2nd suggestion). Note also, that after resubmission, if the new nexthdr proto isn't found at inet6_protos, there's a later: icmpv6_send(skb, ICMPV6_PARAMPROB, ICMPV6_UNK_NEXTHDR, nhoff); whose purpose is to send icmpv6 "Parameter Problem, Unrecognized Next Header", with the "Pointer info" field set to value of 'nhoff'. Thus for the EH handlers case we need to pass the updated 'IP6CB(skb)->nhoff' as the last argument. OTOH, if the nexthdr proto was due to encapsulation resubmission, I'm not sure what's the right "pointer info" value to specify :) Regards, Shmulik
Hi, On Thu, 12 May 2016 14:45:36 -0700 Tom Herbert <tom@herbertland.com> wrote: > On Thu, May 12, 2016 at 1:23 PM, Shmulik Ladkani > <shmulik.ladkani@gmail.com> wrote: > >> @@ -222,13 +222,14 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk > >> */ > >> > >> rcu_read_lock(); > >> -resubmit: > >> + > >> idev = ip6_dst_idev(skb_dst(skb)); > >> if (!pskb_pull(skb, skb_transport_offset(skb))) > >> goto discard; > >> nhoff = IP6CB(skb)->nhoff; > >> nexthdr = skb_network_header(skb)[nhoff]; > >> > >> +resubmit: > > > > This has already been attempted in 0243508edd "ipv6: Fix protocol > > resubmission" and reverted in 1b0ccfe54a. > > > > It looks that in some genuine extension header handling cases of ipv6 > > (not related to encapsulation), the original resubmission code REALLY > > requires one to re-read IP6CB(skb)->nhoff and refetch the nexthdr. > > > Is there any reason why the EH handlers can't read the nexthdr and return that? One additional thing: Seems the if (!pskb_pull(skb, skb_transport_offset(skb))) located at the original resubmit label was also necessary, as the EH handlers may increment skb->transport_header (both ipv6_destopt_rcv and ipv6_rthdr_rcv do so). So if we'd like to read the nexthdr at the EH handlers we should repeat the "skb pull; read nexthdr from skb_network_header(skb)[new nhoff]; return nexhdr;" prior each positive return from EH handlers. Thanks Shmulik
Hi, On Fri, 13 May 2016 09:28:50 +0300 Shmulik Ladkani <shmulik.ladkani@gmail.com> wrote: > On Thu, 12 May 2016 14:45:36 -0700 Tom Herbert <tom@herbertland.com> wrote: > > Is there any reason why the EH handlers can't read the nexthdr and return that? > > One additional thing: > > Seems the > > if (!pskb_pull(skb, skb_transport_offset(skb))) > > located at the original resubmit label was also necessary, as the EH > handlers may increment skb->transport_header (both ipv6_destopt_rcv and > ipv6_rthdr_rcv do so). > > So if we'd like to read the nexthdr at the EH handlers we should repeat > the "skb pull; read nexthdr from skb_network_header(skb)[new nhoff]; > return nexhdr;" prior each positive return from EH handlers. Alternatively, instead of modifying EH handlers adding this repeated logic, we can rearrange ip6_input_finish code, under the premise that "EH handling iff !INET6_PROTO_FINAL", as follows: @@ -229,13 +229,14 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk */ rcu_read_lock(); -resubmit: +nexthdr_read: idev = ip6_dst_idev(skb_dst(skb)); if (!pskb_pull(skb, skb_transport_offset(skb))) goto discard; nhoff = IP6CB(skb)->nhoff; nexthdr = skb_network_header(skb)[nhoff]; +resubmit: raw = raw6_local_deliver(skb, nexthdr); ipprot = rcu_dereference(inet6_protos[nexthdr]); if (ipprot) { @@ -263,8 +264,12 @@ resubmit: goto discard; ret = ipprot->handler(skb); - if (ret > 0) + if (ret > 0) { + if (!(ipprot->flags & INET6_PROTO_FINAL)) + goto nexthdr_read; + nexthdr = ret; goto resubmit; + } else if (ret == 0) __IP6_INC_STATS(net, idev, IPSTATS_MIB_INDELIVERS); Meaning, for EH (identified by !INET6_PROTO_FINAL), act as originally was (skbpull and refetch nexthdr); For non INET6_PROTO_FINAL, ret code is the proto itself, so go directly to resubmit. Less modifications, but (1) creates a coupling (wasn't there already?) between EH handlers and the !INET6_PROTO_FINAL flag, (2) anchors the dual semantics WRT ret code of inet6_protocol->handler. Regards, Shmulik
diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c index 6ed5601..2a0258a 100644 --- a/net/ipv6/ip6_input.c +++ b/net/ipv6/ip6_input.c @@ -222,13 +222,14 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk */ rcu_read_lock(); -resubmit: + idev = ip6_dst_idev(skb_dst(skb)); if (!pskb_pull(skb, skb_transport_offset(skb))) goto discard; nhoff = IP6CB(skb)->nhoff; nexthdr = skb_network_header(skb)[nhoff]; +resubmit: raw = raw6_local_deliver(skb, nexthdr); ipprot = rcu_dereference(inet6_protos[nexthdr]); if (ipprot) { @@ -256,10 +257,12 @@ resubmit: goto discard; ret = ipprot->handler(skb); - if (ret > 0) + if (ret > 0) { + nexthdr = ret; goto resubmit; - else if (ret == 0) + } else if (ret == 0) { __IP6_INC_STATS(net, idev, IPSTATS_MIB_INDELIVERS); + } } else { if (!raw) { if (xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb)) {
In ip6_input_finish the protocol handle returns a value greater than zero the packet needs to be resubmitted using the returned protocol. The returned protocol is being ignored and each time through resubmit nexthdr is taken from an offest in the packet. This patch fixes that so that nexthdr is taken from return value of the protocol handler. Signed-off-by: Tom Herbert <tom@herbertland.com> --- net/ipv6/ip6_input.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)