[PATCH v2] ipv6: Fix protocol resubmission

[PATCH v2] ipv6: Fix protocol resubmission

[Josh Hunt]

I came across this problem while trying to use UDP encapsulation with IPv6. The
change below fixes that, but it was not immediately apparent if there are any
other protocols relying on this broken behavior. FWIW the behavior below now
matches IPv4.

Josh

v2: Actually sets nexthdr so we can use it ;)

---

UDP encapsulation is broken on IPv6 because it expects when it returns a negative
value that the packet will be resubmitted to the stack with the handler corresponding
to the return value. The check currently looks for return values > 0 and then resubmits.

This patch fixes that check and also moves the resubmit label to take advantage of
the return code identifying the next protocol we want to process.

Signed-off-by: Josh Hunt <johunt@akamai.com>
---
 net/ipv6/ip6_input.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index f2e464e..e16c289 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,9 +246,10 @@ 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) {
-- 
1.7.9.5

Re: [PATCH v2] ipv6: Fix protocol resubmission

[Sergei Shtylyov]

Hello.

On 05/29/2015 11:04 PM, Josh Hunt wrote:

> I came across this problem while trying to use UDP encapsulation with IPv6. The
> change below fixes that, but it was not immediately apparent if there are any
> other protocols relying on this broken behavior. FWIW the behavior below now
> matches IPv4.

> Josh

    Need "Signed-off-by:" line insted of this.

> v2: Actually sets nexthdr so we can use it ;)

    Normally goes after ---, not before.

> ---

> UDP encapsulation is broken on IPv6 because it expects when it returns a negative
> value that the packet will be resubmitted to the stack with the handler corresponding
> to the return value. The check currently looks for return values > 0 and then resubmits.

> This patch fixes that check and also moves the resubmit label to take advantage of
> the return code identifying the next protocol we want to process.

> Signed-off-by: Josh Hunt <johunt@akamai.com>
> ---
>   net/ipv6/ip6_input.c |    7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> index f2e464e..e16c289 100644
> --- a/net/ipv6/ip6_input.c
> +++ b/net/ipv6/ip6_input.c
[...]
> @@ -246,9 +246,10 @@ 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);

    The CodingStyle dictates that all branches must have {} if at least one 
has them.

[...]

WBR, Sergei

Re: [PATCH v2] ipv6: Fix protocol resubmission

[Josh Hunt]

On 05/29/2015 03:27 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 05/29/2015 11:04 PM, Josh Hunt wrote:
>
>> I came across this problem while trying to use UDP encapsulation with
>> IPv6. The
>> change below fixes that, but it was not immediately apparent if there
>> are any
>> other protocols relying on this broken behavior. FWIW the behavior
>> below now
>> matches IPv4.
>
>> Josh
>
>     Need "Signed-off-by:" line insted of this.
>
>> v2: Actually sets nexthdr so we can use it ;)
>
>     Normally goes after ---, not before.
>
>> ---
>
>> UDP encapsulation is broken on IPv6 because it expects when it returns
>> a negative
>> value that the packet will be resubmitted to the stack with the
>> handler corresponding
>> to the return value. The check currently looks for return values > 0
>> and then resubmits.
>
>> This patch fixes that check and also moves the resubmit label to take
>> advantage of
>> the return code identifying the next protocol we want to process.
>
>> Signed-off-by: Josh Hunt <johunt@akamai.com>
>> ---
>>   net/ipv6/ip6_input.c |    7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>
>> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
>> index f2e464e..e16c289 100644
>> --- a/net/ipv6/ip6_input.c
>> +++ b/net/ipv6/ip6_input.c
> [...]
>> @@ -246,9 +246,10 @@ 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);
>
>     The CodingStyle dictates that all branches must have {} if at least
> one has them.

Thanks Sergei, I appreciate the review. I will make these changes in a 
v3 after I get feedback this is the correct fix for the problem.

Josh

Re: [PATCH v2] ipv6: Fix protocol resubmission

[Tom Herbert]

Hi Josh,

Why did you need to move the resubmit label?


On Fri, May 29, 2015 at 2:43 PM, Josh Hunt <johunt@akamai.com> wrote:
> On 05/29/2015 03:27 PM, Sergei Shtylyov wrote:
>>
>> Hello.
>>
>> On 05/29/2015 11:04 PM, Josh Hunt wrote:
>>
>>> I came across this problem while trying to use UDP encapsulation with
>>> IPv6. The
>>> change below fixes that, but it was not immediately apparent if there
>>> are any
>>> other protocols relying on this broken behavior. FWIW the behavior
>>> below now
>>> matches IPv4.
>>
>>
>>> Josh
>>
>>
>>     Need "Signed-off-by:" line insted of this.
>>
>>> v2: Actually sets nexthdr so we can use it ;)
>>
>>
>>     Normally goes after ---, not before.
>>
>>> ---
>>
>>
>>> UDP encapsulation is broken on IPv6 because it expects when it returns
>>> a negative
>>> value that the packet will be resubmitted to the stack with the
>>> handler corresponding
>>> to the return value. The check currently looks for return values > 0
>>> and then resubmits.
>>
>>
>>> This patch fixes that check and also moves the resubmit label to take
>>> advantage of
>>> the return code identifying the next protocol we want to process.
>>
>>
>>> Signed-off-by: Josh Hunt <johunt@akamai.com>
>>> ---
>>>   net/ipv6/ip6_input.c |    7 ++++---
>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>>
>>> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
>>> index f2e464e..e16c289 100644
>>> --- a/net/ipv6/ip6_input.c
>>> +++ b/net/ipv6/ip6_input.c
>>
>> [...]
>>>
>>> @@ -246,9 +246,10 @@ 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);
>>
>>
>>     The CodingStyle dictates that all branches must have {} if at least
>> one has them.
>
>
> Thanks Sergei, I appreciate the review. I will make these changes in a v3
> after I get feedback this is the correct fix for the problem.
>
> Josh

Re: [PATCH v2] ipv6: Fix protocol resubmission

[Josh Hunt]

On 05/29/2015 04:48 PM, Tom Herbert wrote:
> Hi Josh,
>
> Why did you need to move the resubmit label?

Grabbing nhoff out of the skb's cb didn't seem relevant anymore, unless 
we're requiring the decapsulating code to update the control block 
before it returns. Also, since we are returning nexthdr with the return 
value it seems unnecessary to go through that work when we already have it.

Not that there has to be symmetry here, but for example, the v4 code 
doesn't look up the protocol field again in the inner ip header. It just 
uses the protocol value returned from the protocol handler.

Also, I'm skipping the pskb_pull(), which I assumed was left up to the 
decapsulating code to setup the data pointer properly before returning. 
Again, this is how the v4 code behaves.

The only thing left is the idev which I'm not sure about. Could that 
change b/t calls?

Josh

Re: [PATCH v2] ipv6: Fix protocol resubmission

[Josh Hunt]

On 05/29/2015 09:37 PM, Josh Hunt wrote:
> On 05/29/2015 04:48 PM, Tom Herbert wrote:
>> Hi Josh,
>>
>> Why did you need to move the resubmit label?
>
> Grabbing nhoff out of the skb's cb didn't seem relevant anymore, unless
> we're requiring the decapsulating code to update the control block
> before it returns. Also, since we are returning nexthdr with the return
> value it seems unnecessary to go through that work when we already have it.
>
> Not that there has to be symmetry here, but for example, the v4 code
> doesn't look up the protocol field again in the inner ip header. It just
> uses the protocol value returned from the protocol handler.
>
> Also, I'm skipping the pskb_pull(), which I assumed was left up to the
> decapsulating code to setup the data pointer properly before returning.
> Again, this is how the v4 code behaves.
>
> The only thing left is the idev which I'm not sure about. Could that
> change b/t calls?
>
Tom

Do you think the above is incorrect and we should just leave the 
resubmit label where it currently is? If so, do you think we should just 
bypass the nhoff/nexthdr checks and use the nexthdr value returned by 
the protocol handler?

Thanks
Josh