From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 755F3C282C6 for ; Thu, 24 Jan 2019 14:20:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 43B58218A2 for ; Thu, 24 Jan 2019 14:20:07 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="IOtkJ8PX" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728257AbfAXOUG (ORCPT ); Thu, 24 Jan 2019 09:20:06 -0500 Received: from mail-ed1-f54.google.com ([209.85.208.54]:43454 "EHLO mail-ed1-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727665AbfAXOUE (ORCPT ); Thu, 24 Jan 2019 09:20:04 -0500 Received: by mail-ed1-f54.google.com with SMTP id f9so4685267eds.10 for ; Thu, 24 Jan 2019 06:20:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=NMcS/vYSKAO6S/HXKETtdmF6cyp8R2WuFIeLrOTYa3M=; b=IOtkJ8PX9TPzjPjX0oFPE3w5NnHNhIbQxV+TzNXRLuqzJcfFz57tiXBykEw8ePlBtP kCQvRttB3DRykCZIcAwmpDNYg6uSNtBbx4OdmoCyxfKOA5o81UDgR/cj7aatFdTD7kyR bYgUTw7l0nMeApuoS2BFBPqPk/1I8CyfiW7OsX7pnSR4lqZOaVFEWw3jLotSPp5/alWn zRVJ4hxtUsXTwWgLbdPVlQs2x/DsRPbruFglWRrtXTOcyGA1Ib0pojEZPMpiw4ymUGq/ A318FocBbZXIOUZpIw7oWZe7FCqo4dnMx8PZjWJNHaJzc4akldLEsuI+KGvbcb9lx2Od arkg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=NMcS/vYSKAO6S/HXKETtdmF6cyp8R2WuFIeLrOTYa3M=; b=tmUhZiViz8uphgRdNZ05ABZxJPUbsNx5A3IDnxEVy3Gp/6cQMifDaVqpOPoOTfoq20 X1S+Nj8M9PdyZtB1LUODupLUGZXvIEpLRinSGhsgU05hHtxVshWAJAEAkCr5rDWQ1PpH Yhk8hbohauSwskiM2u6O4QKZy+DkkTInu7IR9XIOUZQfjCIia47lXoxEQsTviih4e1/r XYwfOOtRfsHIB+81xVjZnzJHL6rwK0nP7uZw3fAOTBgSwzNLk3HxgnC9HJdVifrZcCha OsWLo8U/iCN3MTXp3CONEvAi+eO/8sogCSgP30XfyQNH4njU9FaoR0uNPwD19nJuWEDj 1a8g== X-Gm-Message-State: AJcUukcfbFBC8tZygHVOEX5Q9QM8S4QKqcWslOixpOKuoxwJNqG+kcfB juUfEKaEIjikz6rcDdJpR6mG5jd1HK/3FRs5L9o= X-Google-Smtp-Source: ALg8bN6G8FwPEVNhD+knU6iDq+Si01cZa8KnSOWLBa9MzfGhXH3uwcwCPpJ7PnWLs/q6VQcw4mrmZd50MtguNRfPhZ8= X-Received: by 2002:a17:906:3603:: with SMTP id q3-v6mr6045088ejb.164.1548339602021; Thu, 24 Jan 2019 06:20:02 -0800 (PST) MIME-Version: 1.0 References: <20190114131841.1932-1-maximmi@mellanox.com> <20190114131841.1932-2-maximmi@mellanox.com> In-Reply-To: From: Willem de Bruijn Date: Thu, 24 Jan 2019 09:19:25 -0500 Message-ID: Subject: Re: [PATCH 1/7] net: Don't set transport offset to invalid value To: Maxim Mikityanskiy Cc: "David S. Miller" , Saeed Mahameed , Willem de Bruijn , Jason Wang , Eric Dumazet , "netdev@vger.kernel.org" , Eran Ben Elisha , Tariq Toukan Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Jan 24, 2019 at 4:48 AM Maxim Mikityanskiy wrote: > > > > but the general idea is that we > > > report this status, so if you say that my version is also good for you, > > > I'll leave it as is. It was just a rationale for my decision. > > > > It's fine. But please avoid the code churn in xenvif_tx_submit > > with to extra indentation. This is equivalent: > > > > - if (skb_is_gso(skb)) { > > + if (skb_is_gso(skb) && th_set) { > > > > More fundamentally, the code has the assumption that th_set > > always holds if skb_is_gso(skb). Why add the check? This is > > another example that the return value is not really needed. > > What about > > if (skb_is_gso(skb)) { > BUG_ON(!skb_transport_header_was_set(skb)); > > ? Good idea to validate. Please on error use WARN_ON_ONCE and free the packet as with other errors in this function. BUG_ONs are problematic when a path to them is found. The other option is to stay as close to the current logic as possible in this driver and set the mac header at offset 0 if probe fails. Either through the return value or skb_transport_header_was_set. > I think it's cleaner than skipping the action here if dissect failed and > propagating a potential bug further. > > > > > If this is the only reason for the boolean return value, using > > > > skb_transport_header_was_set() is more standard (I immediately know > > > > what's happening when I read it), slightly less code change and avoids > > > > introducing a situation where the majority of callers ignore a return > > > > value. I think it's preferable. But these merits are certainly > > > > debatable, so either is fine. > > > > > > From my side, I wanted to avoid calling skb_transport_header_was_set > > > twice, so I made skb_try_probe_transport_header return whether it > > > succeeded or not. I think "try" in the function name indicates this idea > > > pretty clearly. This result status is pretty useful, it just happened > > > that it's not needed in many places, > > > > Which is an indication that it's perhaps not needed. > > Well, from the point of view of the function, it looks reasonable to > notify the caller whether the call was successful or not... You know, > many functions return error codes. > > However, this case is rather special, because it turned out that we > don't actually need that error status immediately, but it can be > requested much later, and we already have skb_transport_header_was_set > for that. > > So, considering these points, the return value can be removed, as all > use cases are "probe now, check later", not "probe now and handle errors > immediately".