Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754927AbcKOTc4 (ORCPT ); Tue, 15 Nov 2016 14:32:56 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:34757 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752703AbcKOTcw (ORCPT ); Tue, 15 Nov 2016 14:32:52 -0500 MIME-Version: 1.0 In-Reply-To: <20161115173024.GD30581@breakpoint.cc> References: <20161111202018.13795-1-googuy@gmail.com> <20161114.133646.1687576478968660327.davem@davemloft.net> <20161115.115657.798577230951109692.davem@redhat.com> <20161115173024.GD30581@breakpoint.cc> From: =?UTF-8?Q?Vicente_Jim=C3=A9nez?= Date: Tue, 15 Nov 2016 20:32:49 +0100 Message-ID: Subject: Re: [PATCH] icmp: Restore resistence to abnormal messages To: Florian Westphal Cc: David Miller , Alexey Kuznetsov , James Morris , Hideaki YOSHIFUJI , Patrick McHardy , netdev@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAFJX0cB030788 Content-Length: 3295 Lines: 77 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 wrote: > David Miller wrote: >> From: Vicente Jiménez >> Date: Tue, 15 Nov 2016 17:49:43 +0100 >> >> > On Mon, Nov 14, 2016 at 7:36 PM, David Miller wrote: >> >> From: Vicente Jimenez Aguilar >> >> 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