Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1520011 > unrolled thread

[PATCH] icmp: Restore resistence to abnormal messages

Back to article view | Back to linux.kernel

Contents

  [PATCH] icmp: Restore resistence to abnormal messages Vicente Jimenez Aguilar <googuy@gmail.com> - 2016-11-11 21:30 +0100
    Re: [PATCH] icmp: Restore resistence to abnormal messages David Miller <davem@davemloft.net> - 2016-11-14 19:40 +0100
      Re: [PATCH] icmp: Restore resistence to abnormal messages David Miller <davem@redhat.com> - 2016-11-15 18:00 +0100
        Re: [PATCH] icmp: Restore resistence to abnormal messages Florian Westphal <fw@strlen.de> - 2016-11-15 18:40 +0100
          Re: [PATCH] icmp: Restore resistence to abnormal messages Vicente Jiménez <googuy@gmail.com> - 2016-11-15 20:40 +0100
            Re: [PATCH] icmp: Restore resistence to abnormal messages Florian Westphal <fw@strlen.de> - 2016-11-16 02:20 +0100
              Re: [PATCH] icmp: Restore resistence to abnormal messages Vicente Jiménez <googuy@gmail.com> - 2016-11-17 02:20 +0100

#1520011 — [PATCH] icmp: Restore resistence to abnormal messages

Restore network resistance to abnormal ICMP fragmentation needed messages
with next hop MTU equal to (or exceeding) dropped packet size

Fixes: 46517008e116 ("ipv4: Kill ip_rt_frag_needed().")
Signed-off-by: Vicente Jimenez Aguilar <googuy@gmail.com>
---
 net/ipv4/icmp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 38abe70..4c90d76 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -773,6 +773,7 @@ static bool icmp_tag_validation(int proto)
 static bool icmp_unreach(struct sk_buff *skb)
 {
 	const struct iphdr *iph;
+	unsigned short old_mtu;
 	struct icmphdr *icmph;
 	struct net *net;
 	u32 info = 0;
@@ -819,6 +820,12 @@ static bool icmp_unreach(struct sk_buff *skb)
 				/* fall through */
 			case 0:
 				info = ntohs(icmph->un.frag.mtu);
+				/* Handle weird case where next hop MTU is
+				 * equal to or exceeding dropped packet size
+				 */
+				old_mtu = ntohs(iph->tot_len);
+				if (info >= old_mtu)
+					info = old_mtu - 2;
 			}
 			break;
 		case ICMP_SR_FAILED:
-- 
2.9.3

[toc] | [next] | [standalone]

#1521977

From: Vicente Jimenez Aguilar <googuy@gmail.com>
Date: Fri, 11 Nov 2016 21:20:18 +0100

> @@ -819,6 +820,12 @@ static bool icmp_unreach(struct sk_buff *skb)
>  				/* fall through */
>  			case 0:
>  				info = ntohs(icmph->un.frag.mtu);
> +				/* Handle weird case where next hop MTU is
> +				 * equal to or exceeding dropped packet size
> +				 */
> +				old_mtu = ntohs(iph->tot_len);
> +				if (info >= old_mtu)
> +					info = old_mtu - 2;

This isn't something the old code did.

The old code behaved much differently.

In the case where the new mtu was smaller than 68 or larger than
the iph->tot_len value, it would do several things:

1) First it would check for a BSD 4.2 anomaly and subtract old_mtu
   by the IP header length.

2) Second, it would try to guess the intended MTU using the
   mtu_plateau table.

I don't see any code where a subtraction by a fixed constant of 2
occurred.

Nor can I figure out what that might accomplish.  If you really
want to do this, you have to docuement what this 2 means, what
it is accomplishing, and why you have choosen to accomplish it
this way.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1522916

From: Vicente Jiménez <googuy@gmail.com>
Date: Tue, 15 Nov 2016 17:49:43 +0100

> On Mon, Nov 14, 2016 at 7:36 PM, David Miller <davem@davemloft.net> wrote:
>> From: Vicente Jimenez Aguilar <googuy@gmail.com>
>> Date: Fri, 11 Nov 2016 21:20:18 +0100
>>
>>> @@ -819,6 +820,12 @@ static bool icmp_unreach(struct sk_buff *skb)
>>>                               /* fall through */
>>>                       case 0:
>>>                               info = ntohs(icmph->un.frag.mtu);
>>> +                             /* Handle weird case where next hop MTU is
>>> +                              * equal to or exceeding dropped packet size
>>> +                              */
>>> +                             old_mtu = ntohs(iph->tot_len);
>>> +                             if (info >= old_mtu)
>>> +                                     info = old_mtu - 2;
>>
>> This isn't something the old code did.
>>
>> The old code behaved much differently.
>>
> I don't wanted to restore old behavior just fix a strange case that
> was handle by this code where the next hop MTU reported by the router
> is equal or greater than the actual path MTU. Because router
> information is wrong, we need a way to guess a good packet size
> ignoring router data. The simplest strategy that avoid odd numbers is
> reducing dropped packet size by 2.

This whole approach seems arbitrary.

You haven't discussed in any way, what causes this in the first place.
And what about that cause makes simply subtracting by 2 work well or
not.

You have a very locallized, specific, situation on your end you want
to fix.  But we must accept changes that handle things generically and
in a way that would help more than just your specific case.

[toc] | [prev] | [next] | [standalone]

#1522941

David Miller <davem@redhat.com> wrote:
> From: Vicente Jiménez <googuy@gmail.com>
> Date: Tue, 15 Nov 2016 17:49:43 +0100
> 
> > On Mon, Nov 14, 2016 at 7:36 PM, David Miller <davem@davemloft.net> wrote:
> >> From: Vicente Jimenez Aguilar <googuy@gmail.com>
> >> Date: Fri, 11 Nov 2016 21:20:18 +0100
> >>
> >>> @@ -819,6 +820,12 @@ static bool icmp_unreach(struct sk_buff *skb)
> >>>                               /* fall through */
> >>>                       case 0:
> >>>                               info = ntohs(icmph->un.frag.mtu);
> >>> +                             /* Handle weird case where next hop MTU is
> >>> +                              * equal to or exceeding dropped packet size
> >>> +                              */
> >>> +                             old_mtu = ntohs(iph->tot_len);
> >>> +                             if (info >= old_mtu)
> >>> +                                     info = old_mtu - 2;
> >>
> >> This isn't something the old code did.
> >>
> >> The old code behaved much differently.
> >>
> > I don't wanted to restore old behavior just fix a strange case that
> > was handle by this code where the next hop MTU reported by the router
> > is equal or greater than the actual path MTU. Because router
> > information is wrong, we need a way to guess a good packet size
> > ignoring router data. The simplest strategy that avoid odd numbers is
> > reducing dropped packet size by 2.
> 
> This whole approach seems arbitrary.
> 
> You haven't discussed in any way, what causes this in the first place.
> And what about that cause makes simply subtracting by 2 work well or
> not.
> 
> You have a very locallized, specific, situation on your end you want
> to fix.  But we must accept changes that handle things generically and
> in a way that would help more than just your specific case.

FWIW this is similar to the patch I sent a while ago:

https://patchwork.ozlabs.org/patch/493997/

I think in interest of robustness principle ("eat shit and don't die")
one of these changes should go in :-|

[toc] | [prev] | [next] | [standalone]

#1523037

I agree that both patches try to solve the same problem in a very similar way.
Florian Westphal's patch do two more things:
1- add warning with pr_warn_ratelimited. I like this idea. I also
though about adding some message but I have no kernel experience and I
preferred to have just a working solution.
2- Check if the packet size is lower than (536 + 8). I think this is
not necessary because low values (even the zero case) is already
handled by the protocol. Also I don't understand why you choose this
value, it seems to be related to TCP MSS and the compared value is IP
packet size.

Finally, both patches decrement current packet by a value: Mine by 2
and Florian's by 8 bytes. Both arbitrary values. Personally I prefer
to go by small steps. If the small step fails, it just iterate again
and with 4 iterations, my patch also decrement the original value by 8
bytes (4x2).
Basically they are the same but my patch take smaller steps and miss
the warning message.

If David Miller thinks this could be a good addition, I'll add the
warning message to my patch.

We can also discuss the amount to subtract.

On Tue, Nov 15, 2016 at 6:30 PM, Florian Westphal <fw@strlen.de> wrote:
> David Miller <davem@redhat.com> wrote:
>> From: Vicente Jiménez <googuy@gmail.com>
>> Date: Tue, 15 Nov 2016 17:49:43 +0100
>>
>> > On Mon, Nov 14, 2016 at 7:36 PM, David Miller <davem@davemloft.net> wrote:
>> >> From: Vicente Jimenez Aguilar <googuy@gmail.com>
>> >> Date: Fri, 11 Nov 2016 21:20:18 +0100
>> >>
>> >>> @@ -819,6 +820,12 @@ static bool icmp_unreach(struct sk_buff *skb)
>> >>>                               /* fall through */
>> >>>                       case 0:
>> >>>                               info = ntohs(icmph->un.frag.mtu);
>> >>> +                             /* Handle weird case where next hop MTU is
>> >>> +                              * equal to or exceeding dropped packet size
>> >>> +                              */
>> >>> +                             old_mtu = ntohs(iph->tot_len);
>> >>> +                             if (info >= old_mtu)
>> >>> +                                     info = old_mtu - 2;
>> >>
>> >> This isn't something the old code did.
>> >>
>> >> The old code behaved much differently.
>> >>
>> > I don't wanted to restore old behavior just fix a strange case that
>> > was handle by this code where the next hop MTU reported by the router
>> > is equal or greater than the actual path MTU. Because router
>> > information is wrong, we need a way to guess a good packet size
>> > ignoring router data. The simplest strategy that avoid odd numbers is
>> > reducing dropped packet size by 2.
>>
>> This whole approach seems arbitrary.
>>
>> You haven't discussed in any way, what causes this in the first place.
>> And what about that cause makes simply subtracting by 2 work well or
>> not.
>>
>> You have a very locallized, specific, situation on your end you want
>> to fix.  But we must accept changes that handle things generically and
>> in a way that would help more than just your specific case.
>
> FWIW this is similar to the patch I sent a while ago:
>
> https://patchwork.ozlabs.org/patch/493997/
>
> I think in interest of robustness principle ("eat shit and don't die")
> one of these changes should go in :-|



-- 
saludos
vicente

[toc] | [prev] | [next] | [standalone]

#1523169

Vicente Jiménez <googuy@gmail.com> wrote:
> I agree that both patches try to solve the same problem in a very similar way.
> Florian Westphal's patch do two more things:
> 1- add warning with pr_warn_ratelimited. I like this idea. I also
> though about adding some message but I have no kernel experience and I
> preferred to have just a working solution.

I added this only to show whats happening.

I don't like such printks because end users can't do anything about it.

> 2- Check if the packet size is lower than (536 + 8). I think this is
> not necessary because low values (even the zero case) is already
> handled by the protocol. Also I don't understand why you choose this
> value, it seems to be related to TCP MSS and the compared value is IP
> packet size.

Right, no need for this check.

> Finally, both patches decrement current packet by a value: Mine by 2
> and Florian's by 8 bytes. Both arbitrary values. Personally I prefer
> to go by small steps. If the small step fails, it just iterate again
> and with 4 iterations, my patch also decrement the original value by 8
> bytes (4x2).
> Basically they are the same but my patch take smaller steps and miss
> the warning message.

IIRC I chose 8 because connection recovered faster in my case.

I have not experienced this issue again (I dropped the patch from
my kernel at some point and the connection stalls did not reappear so
this got fixed elsewhere).

I'd just apply your patch, possibly with an additional comment that
says that we're grasping at straws because some middlebox is evidently
feeding bogus pmtu information.

[toc] | [prev] | [next] | [standalone]

#1523998

On Wed, Nov 16, 2016 at 2:14 AM, Florian Westphal <fw@strlen.de> wrote:
> Vicente Jiménez <googuy@gmail.com> wrote:
>> 1- add warning with pr_warn_ratelimited. I like this idea. I also
>> though about adding some message but I have no kernel experience and I
>> preferred to have just a working solution.
>
> I added this only to show whats happening.
>
> I don't like such printks because end users can't do anything about it.
>
What about using net_dbg_ratelimited macro? it only adds messages if
debug is enabled.

>
>> Finally, both patches decrement current packet by a value: Mine by 2
>> and Florian's by 8 bytes. Both arbitrary values. Personally I prefer
>> to go by small steps. If the small step fails, it just iterate again
>> and with 4 iterations, my patch also decrement the original value by 8
>> bytes (4x2).
>> Basically they are the same but my patch take smaller steps and miss
>> the warning message.
>
> IIRC I chose 8 because connection recovered faster in my case.
>
> I have not experienced this issue again (I dropped the patch from
> my kernel at some point and the connection stalls did not reappear so
> this got fixed elsewhere).
My issue is permanent for now in various locations. We have to
decrease MTU manually on all devices with newer kernels. We don't have
direct access to those abnormal routers because they are managed by a
communication provider that think the network had no problem because
all their Windows machines apparently work perfectly.

-- 
cheers
vicente

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web