[PATCH v3] ipv6: Fix protocol resubmission

[PATCH v3] ipv6: Fix protocol resubmission

[Josh Hunt]

UDP encapsulation is broken on IPv6. This is because the logic to resubmit
the nexthdr is inverted, checking for a ret value > 0 instead of < 0. Also,
the resubmit label is in the wrong position since we already get the
nexthdr value when performing decapsulation. In addition the skb pull is no
longer necessary either.

This changes the return value check to look for < 0, using it for the
nexthdr on the next iteration, and moves the resubmit label to the proper
location.

With these changes the v6 code now matches what we do in the v4 ip input
code wrt resubmitting when decapsulating.

Signed-off-by: Josh Hunt <johunt@akamai.com>
---

v3: Fix formatting issues found by Sergei Shtylyov
v2: Use returned nexthdr value

 net/ipv6/ip6_input.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index f2e464e..41a73da 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -212,13 +212,13 @@ static int ip6_input_finish(struct sock *sk, struct sk_buff *skb)
 	 */
 
 	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) {
@@ -246,10 +246,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_BH(net, idev, IPSTATS_MIB_INDELIVERS);
+		}
 	} else {
 		if (!raw) {
 			if (xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb)) {
-- 
1.7.9.5

Re: [PATCH v3] ipv6: Fix protocol resubmission

[Tom Herbert]

On Mon, Jun 8, 2015 at 9:00 AM, Josh Hunt <johunt@akamai.com> wrote:
> UDP encapsulation is broken on IPv6. This is because the logic to resubmit
> the nexthdr is inverted, checking for a ret value > 0 instead of < 0. Also,
> the resubmit label is in the wrong position since we already get the
> nexthdr value when performing decapsulation. In addition the skb pull is no
> longer necessary either.
>
> This changes the return value check to look for < 0, using it for the
> nexthdr on the next iteration, and moves the resubmit label to the proper
> location.
>
> With these changes the v6 code now matches what we do in the v4 ip input
> code wrt resubmitting when decapsulating.
>
Acked-by: "Tom Herbert" <tom@herbertland.com>

> Signed-off-by: Josh Hunt <johunt@akamai.com>
> ---
>
> v3: Fix formatting issues found by Sergei Shtylyov
> v2: Use returned nexthdr value
>
>  net/ipv6/ip6_input.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> index f2e464e..41a73da 100644
> --- a/net/ipv6/ip6_input.c
> +++ b/net/ipv6/ip6_input.c
> @@ -212,13 +212,13 @@ static int ip6_input_finish(struct sock *sk, struct sk_buff *skb)
>          */
>
>         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) {
> @@ -246,10 +246,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_BH(net, idev, IPSTATS_MIB_INDELIVERS);
> +               }
>         } else {
>                 if (!raw) {
>                         if (xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb)) {
> --
> 1.7.9.5
>

Re: [PATCH v3] ipv6: Fix protocol resubmission

[David Miller]

From: Josh Hunt <johunt@akamai.com>
Date: Mon,  8 Jun 2015 12:00:59 -0400

> UDP encapsulation is broken on IPv6. This is because the logic to resubmit
> the nexthdr is inverted, checking for a ret value > 0 instead of < 0. Also,
> the resubmit label is in the wrong position since we already get the
> nexthdr value when performing decapsulation. In addition the skb pull is no
> longer necessary either.
> 
> This changes the return value check to look for < 0, using it for the
> nexthdr on the next iteration, and moves the resubmit label to the proper
> location.
> 
> With these changes the v6 code now matches what we do in the v4 ip input
> code wrt resubmitting when decapsulating.
> 
> Signed-off-by: Josh Hunt <johunt@akamai.com>

Applied.

Re: [PATCH v3] ipv6: Fix protocol resubmission

[Hajime Tazaki]

Hello Josh, Dave,

my mobile ipv6 test on libos failed with this commit.

This commit makes a destination option header handling (i.e.,
ipprot->handler == ipv6_destopt_rcv) failed since
ipv6_destopt_rcv() seems to return a positive value to
indicate to goto resubmission label.

I will look for more detail.

-- Hajime

At Mon, 08 Jun 2015 12:16:01 -0700 (PDT),
David Miller wrote:
> 
> From: Josh Hunt <johunt@akamai.com>
> Date: Mon,  8 Jun 2015 12:00:59 -0400
> 
> > UDP encapsulation is broken on IPv6. This is because the logic to resubmit
> > the nexthdr is inverted, checking for a ret value > 0 instead of < 0. Also,
> > the resubmit label is in the wrong position since we already get the
> > nexthdr value when performing decapsulation. In addition the skb pull is no
> > longer necessary either.
> > 
> > This changes the return value check to look for < 0, using it for the
> > nexthdr on the next iteration, and moves the resubmit label to the proper
> > location.
> > 
> > With these changes the v6 code now matches what we do in the v4 ip input
> > code wrt resubmitting when decapsulating.
> > 
> > Signed-off-by: Josh Hunt <johunt@akamai.com>
> 
> Applied.

Re: [PATCH v3] ipv6: Fix protocol resubmission

[YOSHIFUJI Hideaki]

Hi,

Hajime Tazaki wrote:
> 
> Hello Josh, Dave,
> 
> my mobile ipv6 test on libos failed with this commit.
> 
> This commit makes a destination option header handling (i.e.,
> ipprot->handler == ipv6_destopt_rcv) failed since
> ipv6_destopt_rcv() seems to return a positive value to
> indicate to goto resubmission label.
> 
> I will look for more detail.

I think we should look into other protocol handlers as well...

--yoshfuji

> 
> -- Hajime
> 
> At Mon, 08 Jun 2015 12:16:01 -0700 (PDT),
> David Miller wrote:
>>
>> From: Josh Hunt <johunt@akamai.com>
>> Date: Mon,  8 Jun 2015 12:00:59 -0400
>>
>>> UDP encapsulation is broken on IPv6. This is because the logic to resubmit
>>> the nexthdr is inverted, checking for a ret value > 0 instead of < 0. Also,
>>> the resubmit label is in the wrong position since we already get the
>>> nexthdr value when performing decapsulation. In addition the skb pull is no
>>> longer necessary either.
>>>
>>> This changes the return value check to look for < 0, using it for the
>>> nexthdr on the next iteration, and moves the resubmit label to the proper
>>> location.
>>>
>>> With these changes the v6 code now matches what we do in the v4 ip input
>>> code wrt resubmitting when decapsulating.
>>>
>>> Signed-off-by: Josh Hunt <johunt@akamai.com>
>>
>> Applied.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Hideaki Yoshifuji <hideaki.yoshifuji@miraclelinux.com>
Technical Division, MIRACLE LINUX CORPORATION

Re: [PATCH v3] ipv6: Fix protocol resubmission

[Josh Hunt]

On 06/09/2015 11:24 PM, Hajime Tazaki wrote:
>
> Hello Josh, Dave,
>
> my mobile ipv6 test on libos failed with this commit.
>
> This commit makes a destination option header handling (i.e.,
> ipprot->handler == ipv6_destopt_rcv) failed since
> ipv6_destopt_rcv() seems to return a positive value to
> indicate to goto resubmission label.
>
> I will look for more detail.
>
> -- Hajime

Hajime

Thanks for the report. I mentioned in an earlier post this might be a 
problem.

Dave, what if we restore the old behavior, but add a new label to handle 
the case where the decapsulating protocol returns the nexthdr value? 
Allowing for migration over to this method over time. I've pasted in a 
patch doing so below.

The other solution I guess is to change how the udp handler works, but I 
was hoping to keep it behaving the same as v4.

Josh

diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index 41a73da..a4fab24 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -212,13 +212,13 @@ static int ip6_input_finish(struct sock *sk, 
struct sk_buff *skb)
          */

         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:
+resubmit_nexthdr:
         raw = raw6_local_deliver(skb, nexthdr);
         ipprot = rcu_dereference(inet6_protos[nexthdr]);
         if (ipprot) {
@@ -246,9 +246,11 @@ resubmit:
                         goto discard;

                 ret = ipprot->handler(skb);
-               if (ret < 0) {
-                       nexthdr = -ret;
+               if (ret > 0) {
                         goto resubmit;
+               } else if (ret < 0) {
+                       nexthdr = -ret;
+                       goto resubmit_nexthdr;
                 } else if (ret == 0) {
                         IP6_INC_STATS_BH(net, idev, 
IPSTATS_MIB_INDELIVERS);
                 }

Re: [PATCH v3] ipv6: Fix protocol resubmission

[YOSHIFUJI Hideaki]

Hi,

Josh Hunt wrote:
> On 06/09/2015 11:24 PM, Hajime Tazaki wrote:
>>
>> Hello Josh, Dave,
>>
>> my mobile ipv6 test on libos failed with this commit.
>>
>> This commit makes a destination option header handling (i.e.,
>> ipprot->handler == ipv6_destopt_rcv) failed since
>> ipv6_destopt_rcv() seems to return a positive value to
>> indicate to goto resubmission label.
>>
>> I will look for more detail.
>>
>> -- Hajime
> 
> Hajime
> 
> Thanks for the report. I mentioned in an earlier post this might be a problem.
> 
> Dave, what if we restore the old behavior, but add a new label to handle the case where the decapsulating protocol returns the nexthdr value? Allowing for migration over to this method over time. I've pasted in a patch doing so below.

I think it is insufficient because IPv6 stack already uses
positive value, 0 and negative values.

> 
> The other solution I guess is to change how the udp handler works, but I was hoping to keep it behaving the same as v4.

xfrm returns different value for IPv4 and IPv6, for example,
so udp can do in the same way.  And we can use the fact that
the size of next header field is 8-bit.


> 
> Josh
> 
> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> index 41a73da..a4fab24 100644
> --- a/net/ipv6/ip6_input.c
> +++ b/net/ipv6/ip6_input.c
> @@ -212,13 +212,13 @@ static int ip6_input_finish(struct sock *sk, struct sk_buff *skb)
>          */
> 
>         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:
> +resubmit_nexthdr:
>         raw = raw6_local_deliver(skb, nexthdr);
>         ipprot = rcu_dereference(inet6_protos[nexthdr]);
>         if (ipprot) {
> @@ -246,9 +246,11 @@ resubmit:
>                         goto discard;
> 
>                 ret = ipprot->handler(skb);
> -               if (ret < 0) {
> -                       nexthdr = -ret;
> +               if (ret > 0) {
>                         goto resubmit;
> +               } else if (ret < 0) {
> +                       nexthdr = -ret;
> +                       goto resubmit_nexthdr;
>                 } else if (ret == 0) {
>                         IP6_INC_STATS_BH(net, idev, IPSTATS_MIB_INDELIVERS);
>                 }

Re: [PATCH v3] ipv6: Fix protocol resubmission

[Josh Hunt]

On 06/10/2015 10:16 AM, YOSHIFUJI Hideaki wrote:
> Hi,
>
> Josh Hunt wrote:
>>
>> Hajime
>>
>> Thanks for the report. I mentioned in an earlier post this might be a problem.
>>
>> Dave, what if we restore the old behavior, but add a new label to handle the case where the decapsulating protocol returns the nexthdr value? Allowing for migration over to this method over time. I've pasted in a patch doing so below.
>
> I think it is insufficient because IPv6 stack already uses
> positive value, 0 and negative values.
>

Where does it use a negative value?

ret is only checked to be > or == to 0. I don't see any checks or code 
handling a return value of < 0 prior to my patch.

If something does return a negative value should it since nothing 
happens with it?

Josh

Re: [PATCH v3] ipv6: Fix protocol resubmission

[Josh Hunt]

On 06/10/2015 10:23 AM, Josh Hunt wrote:
> On 06/10/2015 10:16 AM, YOSHIFUJI Hideaki wrote:
>> Hi,
>>
>> Josh Hunt wrote:
>>>
>>> Hajime
>>>
>>> Thanks for the report. I mentioned in an earlier post this might be a
>>> problem.
>>>
>>> Dave, what if we restore the old behavior, but add a new label to
>>> handle the case where the decapsulating protocol returns the nexthdr
>>> value? Allowing for migration over to this method over time. I've
>>> pasted in a patch doing so below.
>>
>> I think it is insufficient because IPv6 stack already uses
>> positive value, 0 and negative values.
>>
>
> Where does it use a negative value?
>
> ret is only checked to be > or == to 0. I don't see any checks or code
> handling a return value of < 0 prior to my patch.
>
> If something does return a negative value should it since nothing
> happens with it?

Looking at the code again, I guess we don't increment
IPSTATS_MIB_INDELIVERS if we get a negative return value prior to my patch.

If so, then I guess we have to fix this in the udp path like you were 
suggesting. I will look at this and propose a patch to revert my 
original change and update the v6 udp code.

Josh

Re: [PATCH v3] ipv6: Fix protocol resubmission

[Josh Hunt]

On 06/10/2015 10:16 AM, YOSHIFUJI Hideaki wrote:
> Hi,
>
> Josh Hunt wrote:
>> On 06/09/2015 11:24 PM, Hajime Tazaki wrote:
>>>
>>> Hello Josh, Dave,
>>>
>>> my mobile ipv6 test on libos failed with this commit.
>>>
>>> This commit makes a destination option header handling (i.e.,
>>> ipprot->handler == ipv6_destopt_rcv) failed since
>>> ipv6_destopt_rcv() seems to return a positive value to
>>> indicate to goto resubmission label.
>>>
>>> I will look for more detail.
>>>
>>> -- Hajime
>>
>> Hajime
>>
>> Thanks for the report. I mentioned in an earlier post this might be a problem.
>>
>> Dave, what if we restore the old behavior, but add a new label to handle the case where the decapsulating protocol returns the nexthdr value? Allowing for migration over to this method over time. I've pasted in a patch doing so below.
>
> I think it is insufficient because IPv6 stack already uses
> positive value, 0 and negative values.
>
>>
>> The other solution I guess is to change how the udp handler works, but I was hoping to keep it behaving the same as v4.
>
> xfrm returns different value for IPv4 and IPv6, for example,
> so udp can do in the same way.  And we can use the fact that
> the size of next header field is 8-bit.
>

Dave

Can you please revert this change?

commit 0243508edd317ff1fa63b495643a7c192fbfcd92
Author: Josh Hunt <johunt@akamai.com>
Date:   Mon Jun 8 12:00:59 2015 -0400

     ipv6: Fix protocol resubmission

Let me know if you need a patch to do this and I will submit something.

I will fix the original issue in the UDP code in another patch.

Josh

Re: [PATCH v3] ipv6: Fix protocol resubmission

[David Miller]

From: Josh Hunt <johunt@akamai.com>
Date: Wed, 10 Jun 2015 16:57:25 -0500

> Can you please revert this change?
> 
> commit 0243508edd317ff1fa63b495643a7c192fbfcd92
> Author: Josh Hunt <johunt@akamai.com>
> Date:   Mon Jun 8 12:00:59 2015 -0400
> 
>     ipv6: Fix protocol resubmission

Done, thanks.

Re: [PATCH v3] ipv6: Fix protocol resubmission

[Hajime Tazaki]

At Wed, 10 Jun 2015 16:57:25 -0500,
Josh Hunt wrote:

> Dave
> 
> Can you please revert this change?
> 
> commit 0243508edd317ff1fa63b495643a7c192fbfcd92
> Author: Josh Hunt <johunt@akamai.com>
> Date:   Mon Jun 8 12:00:59 2015 -0400
> 
>      ipv6: Fix protocol resubmission
> 
> Let me know if you need a patch to do this and I will submit something.
> 
> I will fix the original issue in the UDP code in another patch.

feel free to Cc me if you would like me to test the new patch.

thanks.

-- Hajime

Re: [PATCH v3] ipv6: Fix protocol resubmission

[Josh Hunt]

On 06/11/2015 08:05 AM, Hajime Tazaki wrote:
>
> At Wed, 10 Jun 2015 16:57:25 -0500,
> Josh Hunt wrote:
>
>> Dave
>>
>> Can you please revert this change?
>>
>> commit 0243508edd317ff1fa63b495643a7c192fbfcd92
>> Author: Josh Hunt <johunt@akamai.com>
>> Date:   Mon Jun 8 12:00:59 2015 -0400
>>
>>       ipv6: Fix protocol resubmission
>>
>> Let me know if you need a patch to do this and I will submit something.
>>
>> I will fix the original issue in the UDP code in another patch.
>
> feel free to Cc me if you would like me to test the new patch.

I will definitely take you up on that :)

Thanks
Josh