[PATCH] sit: correct IP protocol used in ipip6_err

[PATCH] sit: correct IP protocol used in ipip6_err

[Simon Horman]

Since 32b8a8e59c9c ("sit: add IPv4 over IPv4 support")
ipip6_err() may be called for packets whose IP protocol is
IPPROTO_IPIP as well as those whose IP protocol is IPPROTO_IPV6.

In the case of IPPROTO_IPIP packets the correct protocol value is not
passed to ipv4_update_pmtu() or ipv4_redirect().

This patch resolves this problem by using the IP protocol of the packet
rather than a hard-coded value. This appears to be consistent
with the usage of the protocol of a packet by icmp_socket_deliver()
the caller of ipip6_err().

I was able to exercise the redirect case by using a setup where an ICMP
redirect was received for the destination of the encapsulated packet.
However, it appears that although incorrect the protocol field is not used
in this case and thus no problem manifests.  On inspection it does not
appear that a problem will manifest in the fragmentation needed/update pmtu
case either.

In short I believe this is a cosmetic fix. None the less, the use of
IPPROTO_IPV6 seems wrong and confusing.

Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 net/ipv6/sit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index d9f2bd6ef72d..f4356bb13f4b 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -560,13 +560,13 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
 
 	if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
 		ipv4_update_pmtu(skb, dev_net(skb->dev), info,
-				 t->parms.link, 0, IPPROTO_IPV6, 0);
+				 t->parms.link, 0, iph->protocol, 0);
 		err = 0;
 		goto out;
 	}
 	if (type == ICMP_REDIRECT) {
 		ipv4_redirect(skb, dev_net(skb->dev), t->parms.link, 0,
-			      IPPROTO_IPV6, 0);
+			      iph->protocol, 0);
 		err = 0;
 		goto out;
 	}
-- 
2.1.4

Re: [PATCH] sit: correct IP protocol used in ipip6_err

[Simon Horman]

On Thu, Jun 16, 2016 at 05:06:19PM +0900, Simon Horman wrote:
> Since 32b8a8e59c9c ("sit: add IPv4 over IPv4 support")
> ipip6_err() may be called for packets whose IP protocol is
> IPPROTO_IPIP as well as those whose IP protocol is IPPROTO_IPV6.
> 
> In the case of IPPROTO_IPIP packets the correct protocol value is not
> passed to ipv4_update_pmtu() or ipv4_redirect().
> 
> This patch resolves this problem by using the IP protocol of the packet
> rather than a hard-coded value. This appears to be consistent
> with the usage of the protocol of a packet by icmp_socket_deliver()
> the caller of ipip6_err().
> 
> I was able to exercise the redirect case by using a setup where an ICMP
> redirect was received for the destination of the encapsulated packet.
> However, it appears that although incorrect the protocol field is not used
> in this case and thus no problem manifests.  On inspection it does not
> appear that a problem will manifest in the fragmentation needed/update pmtu
> case either.
> 
> In short I believe this is a cosmetic fix. None the less, the use of
> IPPROTO_IPV6 seems wrong and confusing.
> 
> Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>

Apologies for not making this more obvious, this is a "net-next" patch.

> ---
>  net/ipv6/sit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index d9f2bd6ef72d..f4356bb13f4b 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -560,13 +560,13 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
>  
>  	if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
>  		ipv4_update_pmtu(skb, dev_net(skb->dev), info,
> -				 t->parms.link, 0, IPPROTO_IPV6, 0);
> +				 t->parms.link, 0, iph->protocol, 0);
>  		err = 0;
>  		goto out;
>  	}
>  	if (type == ICMP_REDIRECT) {
>  		ipv4_redirect(skb, dev_net(skb->dev), t->parms.link, 0,
> -			      IPPROTO_IPV6, 0);
> +			      iph->protocol, 0);
>  		err = 0;
>  		goto out;
>  	}
> -- 
> 2.1.4
> 

Re: [PATCH] sit: correct IP protocol used in ipip6_err

[YOSHIFUJI Hideaki]

Hi, Simon,

Simon Horman wrote:
> On Thu, Jun 16, 2016 at 05:06:19PM +0900, Simon Horman wrote:
>> Since 32b8a8e59c9c ("sit: add IPv4 over IPv4 support")
>> ipip6_err() may be called for packets whose IP protocol is
>> IPPROTO_IPIP as well as those whose IP protocol is IPPROTO_IPV6.
>>
>> In the case of IPPROTO_IPIP packets the correct protocol value is not
>> passed to ipv4_update_pmtu() or ipv4_redirect().
>>
>> This patch resolves this problem by using the IP protocol of the packet
>> rather than a hard-coded value. This appears to be consistent
>> with the usage of the protocol of a packet by icmp_socket_deliver()
>> the caller of ipip6_err().
>>
>> I was able to exercise the redirect case by using a setup where an ICMP
>> redirect was received for the destination of the encapsulated packet.
>> However, it appears that although incorrect the protocol field is not used
>> in this case and thus no problem manifests.  On inspection it does not
>> appear that a problem will manifest in the fragmentation needed/update pmtu
>> case either.
>>
>> In short I believe this is a cosmetic fix. None the less, the use of
>> IPPROTO_IPV6 seems wrong and confusing.
>>
>> Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com>
>> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> 
> Apologies for not making this more obvious, this is a "net-next" patch.

Acked-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

BTW, we should have similar fix in -net, -stable etc. as well, no?

> 
>> ---
>>  net/ipv6/sit.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
>> index d9f2bd6ef72d..f4356bb13f4b 100644
>> --- a/net/ipv6/sit.c
>> +++ b/net/ipv6/sit.c
>> @@ -560,13 +560,13 @@ static int ipip6_err(struct sk_buff *skb, u32 info)
>>  
>>  	if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) {
>>  		ipv4_update_pmtu(skb, dev_net(skb->dev), info,
>> -				 t->parms.link, 0, IPPROTO_IPV6, 0);
>> +				 t->parms.link, 0, iph->protocol, 0);
>>  		err = 0;
>>  		goto out;
>>  	}
>>  	if (type == ICMP_REDIRECT) {
>>  		ipv4_redirect(skb, dev_net(skb->dev), t->parms.link, 0,
>> -			      IPPROTO_IPV6, 0);
>> +			      iph->protocol, 0);
>>  		err = 0;
>>  		goto out;
>>  	}
>> -- 
>> 2.1.4
>>

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

Re: [PATCH] sit: correct IP protocol used in ipip6_err

[Simon Horman]

On Thu, Jun 16, 2016 at 05:23:13PM +0900, YOSHIFUJI Hideaki wrote:
> Hi, Simon,
> 
> Simon Horman wrote:
> > On Thu, Jun 16, 2016 at 05:06:19PM +0900, Simon Horman wrote:
> >> Since 32b8a8e59c9c ("sit: add IPv4 over IPv4 support")
> >> ipip6_err() may be called for packets whose IP protocol is
> >> IPPROTO_IPIP as well as those whose IP protocol is IPPROTO_IPV6.
> >>
> >> In the case of IPPROTO_IPIP packets the correct protocol value is not
> >> passed to ipv4_update_pmtu() or ipv4_redirect().
> >>
> >> This patch resolves this problem by using the IP protocol of the packet
> >> rather than a hard-coded value. This appears to be consistent
> >> with the usage of the protocol of a packet by icmp_socket_deliver()
> >> the caller of ipip6_err().
> >>
> >> I was able to exercise the redirect case by using a setup where an ICMP
> >> redirect was received for the destination of the encapsulated packet.
> >> However, it appears that although incorrect the protocol field is not used
> >> in this case and thus no problem manifests.  On inspection it does not
> >> appear that a problem will manifest in the fragmentation needed/update pmtu
> >> case either.
> >>
> >> In short I believe this is a cosmetic fix. None the less, the use of
> >> IPPROTO_IPV6 seems wrong and confusing.
> >>
> >> Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com>
> >> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> > 
> > Apologies for not making this more obvious, this is a "net-next" patch.
> 
> Acked-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
> 
> BTW, we should have similar fix in -net, -stable etc. as well, no?

I am not opposed to this patch going there and probably it can do so
verbatim but I haven't found any run-time problems resolved by the patch.

Re: [PATCH] sit: correct IP protocol used in ipip6_err

[David Miller]

From: Simon Horman <simon.horman@netronome.com>
Date: Thu, 16 Jun 2016 17:06:19 +0900

> Since 32b8a8e59c9c ("sit: add IPv4 over IPv4 support")
> ipip6_err() may be called for packets whose IP protocol is
> IPPROTO_IPIP as well as those whose IP protocol is IPPROTO_IPV6.
> 
> In the case of IPPROTO_IPIP packets the correct protocol value is not
> passed to ipv4_update_pmtu() or ipv4_redirect().
> 
> This patch resolves this problem by using the IP protocol of the packet
> rather than a hard-coded value. This appears to be consistent
> with the usage of the protocol of a packet by icmp_socket_deliver()
> the caller of ipip6_err().
> 
> I was able to exercise the redirect case by using a setup where an ICMP
> redirect was received for the destination of the encapsulated packet.
> However, it appears that although incorrect the protocol field is not used
> in this case and thus no problem manifests.  On inspection it does not
> appear that a problem will manifest in the fragmentation needed/update pmtu
> case either.
> 
> In short I believe this is a cosmetic fix. None the less, the use of
> IPPROTO_IPV6 seems wrong and confusing.
> 
> Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>

Applied to 'net' and queued up for -stable, thanks Simon.