Re: [PATCH] icmp: Restore resistence to abnormal messages

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 [off-list ref] wrote:

David Miller [off-list ref] wrote:

quoted

From: Vicente Jiménez <redacted>
Date: Tue, 15 Nov 2016 17:49:43 +0100

quoted

On Mon, Nov 14, 2016 at 7:36 PM, David Miller [off-list ref] wrote:

quoted

From: Vicente Jimenez Aguilar <redacted>
Date: Fri, 11 Nov 2016 21:20:18 +0100

quoted

@@ -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