[PATCH] IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

[PATCH] IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

[Julius Volz]

Add handling of incoming ICMPv6 Packet Too Big messages. This message
is received when a realserver sends a packet >PMTU to the client. The
hop on this path with insufficient MTU will generate an ICMPv6 Packet
Too Big message back to the VIP. The LVS server receives this message,
but the call to the function handling this has been missing. Thus, IPVS
fails to forward the message to the real server, which then does not
adjust the path MTU. This patch adds the missing call to
ip_vs_in_icmp_v6() in ip_vs_in() to handle this situation.

Thanks to Rob Gallagher from HEAnet for reporting this issue and for
testing this patch in production (with direct routing mode).

Signed-off-by: Julius Volz <julius.volz@gmail.com>
Tested-by: Rob Gallagher <robert.gallagher@heanet.ie>
---
 net/netfilter/ipvs/ip_vs_core.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 8dddb17..5750800 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1274,13 +1274,24 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb,
 		return NF_ACCEPT;
 	}
 
-	if (unlikely(iph.protocol == IPPROTO_ICMP)) {
-		int related, verdict = ip_vs_in_icmp(skb, &related, hooknum);
+#ifdef CONFIG_IP_VS_IPV6
+	if (af == AF_INET6) {
+		if (unlikely(iph.protocol == IPPROTO_ICMPV6)) {
+			int related, verdict = ip_vs_in_icmp_v6(skb, &related, hooknum);
 
-		if (related)
-			return verdict;
-		ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
-	}
+			if (related)
+				return verdict;
+			ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
+		}
+	} else
+#endif
+		if (unlikely(iph.protocol == IPPROTO_ICMP)) {
+			int related, verdict = ip_vs_in_icmp(skb, &related, hooknum);
+
+			if (related)
+				return verdict;
+			ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
+		}
 
 	/* Protocol supported? */
 	pp = ip_vs_proto_get(iph.protocol);
-- 
1.6.0.4

Re: [PATCH] IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

[Simon Horman]

On Wed, Jun 24, 2009 at 03:22:32PM +0200, Julius Volz wrote:
> Add handling of incoming ICMPv6 Packet Too Big messages. This message
> is received when a realserver sends a packet >PMTU to the client. The
> hop on this path with insufficient MTU will generate an ICMPv6 Packet
> Too Big message back to the VIP. The LVS server receives this message,
> but the call to the function handling this has been missing. Thus, IPVS
> fails to forward the message to the real server, which then does not
> adjust the path MTU. This patch adds the missing call to
> ip_vs_in_icmp_v6() in ip_vs_in() to handle this situation.
> 
> Thanks to Rob Gallagher from HEAnet for reporting this issue and for
> testing this patch in production (with direct routing mode).
> 
> Signed-off-by: Julius Volz <julius.volz@gmail.com>
> Tested-by: Rob Gallagher <robert.gallagher@heanet.ie>

Hi Julius, Hi Rob,

this seems reasonable to me, although it seems that the following
code is common. I wonder if its repetition could be removed.

			if (related)
				return verdict;
			ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);

On a not very related note, I'm currently on holidays and
my net access is very sporadic. I'll be back at my desk on the 8th.

> ---
>  net/netfilter/ipvs/ip_vs_core.c |   23 +++++++++++++++++------
>  1 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 8dddb17..5750800 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1274,13 +1274,24 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb,
>  		return NF_ACCEPT;
>  	}
>  
> -	if (unlikely(iph.protocol == IPPROTO_ICMP)) {
> -		int related, verdict = ip_vs_in_icmp(skb, &related, hooknum);
> +#ifdef CONFIG_IP_VS_IPV6
> +	if (af == AF_INET6) {
> +		if (unlikely(iph.protocol == IPPROTO_ICMPV6)) {
> +			int related, verdict = ip_vs_in_icmp_v6(skb, &related, hooknum);
>  
> -		if (related)
> -			return verdict;
> -		ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
> -	}
> +			if (related)
> +				return verdict;
> +			ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
> +		}
> +	} else
> +#endif
> +		if (unlikely(iph.protocol == IPPROTO_ICMP)) {
> +			int related, verdict = ip_vs_in_icmp(skb, &related, hooknum);
> +
> +			if (related)
> +				return verdict;
> +			ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
> +		}
>  
>  	/* Protocol supported? */
>  	pp = ip_vs_proto_get(iph.protocol);
> -- 
> 1.6.0.4

Re: [PATCH] IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

[Julius Volz]

Hi Simon,

On Sun, Jun 28, 2009 at 5:43 PM, Simon Horman<horms@verge.net.au> wrote:
> On Wed, Jun 24, 2009 at 03:22:32PM +0200, Julius Volz wrote:
>> Add handling of incoming ICMPv6 Packet Too Big messages. This message
>> is received when a realserver sends a packet >PMTU to the client. The
>> hop on this path with insufficient MTU will generate an ICMPv6 Packet
>> Too Big message back to the VIP. The LVS server receives this message,
>> but the call to the function handling this has been missing. Thus, IPVS
>> fails to forward the message to the real server, which then does not
>> adjust the path MTU. This patch adds the missing call to
>> ip_vs_in_icmp_v6() in ip_vs_in() to handle this situation.
>>
>> Thanks to Rob Gallagher from HEAnet for reporting this issue and for
>> testing this patch in production (with direct routing mode).
>>
>> Signed-off-by: Julius Volz <julius.volz@gmail.com>
>> Tested-by: Rob Gallagher <robert.gallagher@heanet.ie>
>
> Hi Julius, Hi Rob,
>
> this seems reasonable to me, although it seems that the following
> code is common. I wonder if its repetition could be removed.
>
>                        if (related)
>                                return verdict;
>                        ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);

I agree, though I see no "nice" way to remove this duplication
considering the ifs and #ifdefs around this. You could move the
related and verdict variables to the top of the function and then
recheck afterwards whether one of these ICMP-handling branches was
entered and put the common code in there. But this seems more
cumbersome to me than repeating the code. Maybe you see a nicer way?
Btw., exactly this structure already exists in ip_vs_out(), which is
why I adopted it like this for ip_vs_in().

Cheers,
Julius

Re: [PATCH] IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

[Simon Horman]

On Thu, Jul 02, 2009 at 04:43:39PM +0200, Julius Volz wrote:
> Hi Simon,
> 
> On Sun, Jun 28, 2009 at 5:43 PM, Simon Horman<horms@verge.net.au> wrote:
> > On Wed, Jun 24, 2009 at 03:22:32PM +0200, Julius Volz wrote:
> >> Add handling of incoming ICMPv6 Packet Too Big messages. This message
> >> is received when a realserver sends a packet >PMTU to the client. The
> >> hop on this path with insufficient MTU will generate an ICMPv6 Packet
> >> Too Big message back to the VIP. The LVS server receives this message,
> >> but the call to the function handling this has been missing. Thus, IPVS
> >> fails to forward the message to the real server, which then does not
> >> adjust the path MTU. This patch adds the missing call to
> >> ip_vs_in_icmp_v6() in ip_vs_in() to handle this situation.
> >>
> >> Thanks to Rob Gallagher from HEAnet for reporting this issue and for
> >> testing this patch in production (with direct routing mode).
> >>
> >> Signed-off-by: Julius Volz <julius.volz@gmail.com>
> >> Tested-by: Rob Gallagher <robert.gallagher@heanet.ie>
> >
> > Hi Julius, Hi Rob,
> >
> > this seems reasonable to me, although it seems that the following
> > code is common. I wonder if its repetition could be removed.
> >
> >                        if (related)
> >                                return verdict;
> >                        ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
> 
> I agree, though I see no "nice" way to remove this duplication
> considering the ifs and #ifdefs around this. You could move the
> related and verdict variables to the top of the function and then
> recheck afterwards whether one of these ICMP-handling branches was
> entered and put the common code in there. But this seems more
> cumbersome to me than repeating the code. Maybe you see a nicer way?
> Btw., exactly this structure already exists in ip_vs_out(), which is
> why I adopted it like this for ip_vs_in().

Hi Julius,

sorry for the delay in responding, I've been off-line / recovering from
being off-line.

I couldn't see an obvious way either, though I was hoping that
you might :-) As you can't I'm happy to go with what you originally
posted.

Acked-by: Simon Horman <horms@verge.net.au>

Re: [PATCH] IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

[Simon Horman]

From: Julius Volz <julius.volz@gmail.com>

IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

Add handling of incoming ICMPv6 Packet Too Big messages. This message
is received when a realserver sends a packet >PMTU to the client. The
hop on this path with insufficient MTU will generate an ICMPv6 Packet
Too Big message back to the VIP. The LVS server receives this message,
but the call to the function handling this has been missing. Thus, IPVS
fails to forward the message to the real server, which then does not
adjust the path MTU. This patch adds the missing call to
ip_vs_in_icmp_v6() in ip_vs_in() to handle this situation.

Thanks to Rob Gallagher from HEAnet for reporting this issue and for
testing this patch in production (with direct routing mode).

Signed-off-by: Julius Volz <julius.volz@gmail.com>
Tested-by: Rob Gallagher <robert.gallagher@heanet.ie>
Signed-off-by: Simon Horman <horms@verge.net.au>

---
 net/netfilter/ipvs/ip_vs_core.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

Dave, please consider applying this change.

I'm ok with it not going into 2.6.31 as I don't think that
many people are affected by this problem.

diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 8dddb17..5750800 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1274,13 +1274,24 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb,
 		return NF_ACCEPT;
 	}
 
-	if (unlikely(iph.protocol == IPPROTO_ICMP)) {
-		int related, verdict = ip_vs_in_icmp(skb, &related, hooknum);
+#ifdef CONFIG_IP_VS_IPV6
+	if (af == AF_INET6) {
+		if (unlikely(iph.protocol == IPPROTO_ICMPV6)) {
+			int related, verdict = ip_vs_in_icmp_v6(skb, &related, hooknum);
 
-		if (related)
-			return verdict;
-		ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
-	}
+			if (related)
+				return verdict;
+			ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
+		}
+	} else
+#endif
+		if (unlikely(iph.protocol == IPPROTO_ICMP)) {
+			int related, verdict = ip_vs_in_icmp(skb, &related, hooknum);
+
+			if (related)
+				return verdict;
+			ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
+		}
 
 	/* Protocol supported? */
 	pp = ip_vs_proto_get(iph.protocol);

Re: [PATCH] IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

[Eric Dumazet]

Simon Horman a écrit :
> From: Julius Volz <julius.volz@gmail.com>
> 
> IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages
> 
> Add handling of incoming ICMPv6 Packet Too Big messages. This message
> is received when a realserver sends a packet >PMTU to the client. The
> hop on this path with insufficient MTU will generate an ICMPv6 Packet
> Too Big message back to the VIP. The LVS server receives this message,
> but the call to the function handling this has been missing. Thus, IPVS
> fails to forward the message to the real server, which then does not
> adjust the path MTU. This patch adds the missing call to
> ip_vs_in_icmp_v6() in ip_vs_in() to handle this situation.
> 
> Thanks to Rob Gallagher from HEAnet for reporting this issue and for
> testing this patch in production (with direct routing mode).
> 
> Signed-off-by: Julius Volz <julius.volz@gmail.com>
> Tested-by: Rob Gallagher <robert.gallagher@heanet.ie>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> ---
>  net/netfilter/ipvs/ip_vs_core.c |   23 +++++++++++++++++------
>  1 files changed, 17 insertions(+), 6 deletions(-)
> 
> Dave, please consider applying this change.
> 
> I'm ok with it not going into 2.6.31 as I don't think that
> many people are affected by this problem.
> 
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 8dddb17..5750800 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1274,13 +1274,24 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb,
>  		return NF_ACCEPT;
>  	}
>  
> -	if (unlikely(iph.protocol == IPPROTO_ICMP)) {
> -		int related, verdict = ip_vs_in_icmp(skb, &related, hooknum);
> +#ifdef CONFIG_IP_VS_IPV6
> +	if (af == AF_INET6) {
> +		if (unlikely(iph.protocol == IPPROTO_ICMPV6)) {
> +			int related, verdict = ip_vs_in_icmp_v6(skb, &related, hooknum);
>  
> -		if (related)
> -			return verdict;
> -		ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
> -	}
> +			if (related)
> +				return verdict;
> +			ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
> +		}
> +	} else
> +#endif
> +		if (unlikely(iph.protocol == IPPROTO_ICMP)) {
> +			int related, verdict = ip_vs_in_icmp(skb, &related, hooknum);
> +
> +			if (related)
> +				return verdict;
> +			ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
> +		}
>  
>  	/* Protocol supported? */
>  	pp = ip_vs_proto_get(iph.protocol);

I see no reference to ICMPV6_PKT_TOOBIG in this patch, so ChangeLog might be
misleading or uncomplete, since other ICMPV6 message types 
(ICMPV6_DEST_UNREACH/ICMPV6_TIME_EXCEED) will also be forwarded/handled ?

Re: [PATCH] IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 24 Jul 2009 06:25:25 +0200

> I see no reference to ICMPV6_PKT_TOOBIG in this patch, so ChangeLog might be
> misleading or uncomplete, since other ICMPV6 message types 
> (ICMPV6_DEST_UNREACH/ICMPV6_TIME_EXCEED) will also be forwarded/handled ?

Agreed, this commit message could use a tidy.  What the patch
actually is doing is adding the handling of ipv6 icmp messages
at all.

Simon could you clarify the commit message a bit and resubmit?

Thanks!

Re: [PATCH] IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

[Simon Horman]

On Sun, Jul 26, 2009 at 07:19:05PM -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 24 Jul 2009 06:25:25 +0200
> 
> > I see no reference to ICMPV6_PKT_TOOBIG in this patch, so ChangeLog
> > might be misleading or uncomplete, since other ICMPV6 message types
> > (ICMPV6_DEST_UNREACH/ICMPV6_TIME_EXCEED) will also be forwarded/handled
> > ?
> 
> Agreed, this commit message could use a tidy.  What the patch actually is
> doing is adding the handling of ipv6 icmp messages at all.
> 
> Simon could you clarify the commit message a bit and resubmit?

Will do.

Re: [PATCH] IPVS: Add handling of incoming ICMPV6_PKT_TOOBIG messages

[Julius Volz]

On Mon, Jul 27, 2009 at 4:37 AM, Simon Horman<horms@verge.net.au> wrote:
> On Sun, Jul 26, 2009 at 07:19:05PM -0700, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Fri, 24 Jul 2009 06:25:25 +0200
>>
>> > I see no reference to ICMPV6_PKT_TOOBIG in this patch, so ChangeLog
>> > might be misleading or uncomplete, since other ICMPV6 message types
>> > (ICMPV6_DEST_UNREACH/ICMPV6_TIME_EXCEED) will also be forwarded/handled
>> > ?
>>
>> Agreed, this commit message could use a tidy.  What the patch actually is
>> doing is adding the handling of ipv6 icmp messages at all.
>>
>> Simon could you clarify the commit message a bit and resubmit?
>
> Will do.

Oops, yes. For some weird reason, only the specific problem that
caused the patch was in my head when writing that commit message. Feel
free to edit it to include ICMPv6 handling for in-out packets in
general.

Julius