From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [Patch net] net: make pskb_trim_rcsum_slow() robust Date: Mon, 29 Oct 2018 20:08:44 -0700 Message-ID: References: <20181030003515.12075-1-xiyou.wangcong@gmail.com> <5466e637-37d2-a11e-c30c-fc17a4dbeaf1@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Linux Kernel Network Developers , Eric Dumazet To: Cong Wang , Eric Dumazet Return-path: Received: from mail-pf1-f177.google.com ([209.85.210.177]:38921 "EHLO mail-pf1-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725988AbeJ3MAX (ORCPT ); Tue, 30 Oct 2018 08:00:23 -0400 Received: by mail-pf1-f177.google.com with SMTP id c25-v6so5043042pfe.6 for ; Mon, 29 Oct 2018 20:08:46 -0700 (PDT) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 10/29/2018 07:41 PM, Cong Wang wrote: > On Mon, Oct 29, 2018 at 7:25 PM Eric Dumazet wrote: >> >> >> >> On 10/29/2018 07:21 PM, Cong Wang wrote: >>> On Mon, Oct 29, 2018 at 7:14 PM Eric Dumazet wrote: >>>> >>>> Would not it be simpler to set ip_summed to CHECKSUM_NONE (no need to save old_csum) ? >>> >>> For !CHECKSUM_COMPLETE, ip_summed should be untouched, right? >>> >>> If you mean only setting to CHECKSUM_NONE for CHECKSUM_COMPLETE case, >>> the end result may not be simpler. >> >> I meant to reinstate what was there before my patch in this error case >> >> if (skb->ip_summed == CHECKSUM_COMPLETE) >> skb->ip_summed = CHECKSUM_NONE; >> >> That would only be run in error (quite unlikely) path, instead of saving old_csum in all cases. > > I know your point, however, I am not sure that is a desired behavior. > > On failure, I think the whole skb should be restored to its previous state > before entering this function, changing it to CHECKSUM_NONE on failure > is inconsistent with success case. > Before my patch, we were changing skb->ip_summed to CHECKSUM_NONE, so why suddenly we need to be consistent ? In any case, ip_check_defrag() should really drop this skb, as for other allocation failures (like skb_share_check()), if really we want consistency.