Received: by 2002:ac0:a5a6:0:0:0:0:0 with SMTP id m35-v6csp859800imm; Fri, 28 Sep 2018 08:01:46 -0700 (PDT) X-Google-Smtp-Source: ACcGV63I/LuRplC0R6ZjM2590DdS2BR1jxmY2sT1yvnMMvOaDEpp4Lhi/XCKryZVFgq3IqgbOrI/ X-Received: by 2002:a63:e943:: with SMTP id q3-v6mr15422767pgj.42.1538146906281; Fri, 28 Sep 2018 08:01:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1538146906; cv=none; d=google.com; s=arc-20160816; b=TZ6KJesVUSvAvSLtwGwdh6A/GDNouRHMaHCB7R0BXv4GKO6VOpi02Or0Tv0DkzJGSv QcIFysqZ7oIpIfuM0ZK1FwFf84xLqpTGrkWyHewsScYBxhmMS8Gn4bMDx3VsZgniUidX LoGGUG5/VxJFMongdkk2vbWTZBaZmpTOWNAYy4zjBk5CsqHRC8GU6oVklVnAqI+Tr3bk ektCA1HyVOy7/8xDiHL27O8WpaJxJyoEzh1n4u1fxzGFtHh77TC5eX9zwN/fyCLrzyJp 1eRp+JVa3bFxW183CNLccgctYIo+YxEJCRKwe8Eam3wmjC4bXKDBYCCuNHT9MgdeYDT2 XAKA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=8p7gmvve/5J6P4WOrn5cgWCYYH8eMqcrnbPKjwPq4+w=; b=lCQ1KaFWBFmJOj6s3UUEquvFj5v7QkIVk4IXIy4CgY0P7/pw9qSKv66W6J107x9X/R 81ya5wp81Wr4T0c/ckZ5XnLwvWp5mYuHUFon/tCknE4DXktomiIWguXQjywvsJS3sE7E Za/Bo9LDGsk1xyWjQZHdj0TaZNExlV1Tnk2jZlrcdP8MTzVhTZZB4cU4CW4vyz33LpVs Y1Hb7JgNPdQeymtTzePSckSKw4kiMEgrMtif7mJSuUyKntoL4Vi4HlLxGrVs5Pvp2UsU sQZTBzZn25xYeDMdL2nKVxQxB5Hw1loH9lLZVA1HyHSdxyX9QrbYecVr5BBirBp8DY7n sJhQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@lunn.ch header.s=20171124 header.b=xfvI+0xe; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id h69-v6si4830238pfc.121.2018.09.28.08.01.27; Fri, 28 Sep 2018 08:01:46 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@lunn.ch header.s=20171124 header.b=xfvI+0xe; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729025AbeI1VZb (ORCPT + 99 others); Fri, 28 Sep 2018 17:25:31 -0400 Received: from vps0.lunn.ch ([185.16.172.187]:48708 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726345AbeI1VZb (ORCPT ); Fri, 28 Sep 2018 17:25:31 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=8p7gmvve/5J6P4WOrn5cgWCYYH8eMqcrnbPKjwPq4+w=; b=xfvI+0xesghkWJrNfgm8VdimdUQcku+0LCuXBaxzyhC3RJtA+CDko8h139P2OZDbycXAbsy0ZkLvAvJLzwpenMomoFzYYTgKojsyq8Gz07TuaeweRvWwpVmc7a2fbBzsvBBwMiIGIghaMwPolOsAwkCsQkELqRBYR7GHt93xK4k=; Received: from andrew by vps0.lunn.ch with local (Exim 4.84_2) (envelope-from ) id 1g5uGb-0005K4-Al; Fri, 28 Sep 2018 17:01:17 +0200 Date: Fri, 28 Sep 2018 17:01:17 +0200 From: Andrew Lunn To: "Jason A. Donenfeld" Cc: LKML , Netdev , Linux Crypto Mailing List , David Miller , Greg Kroah-Hartman Subject: Re: [PATCH net-next v6 23/23] net: WireGuard secure network tunnel Message-ID: <20180928150117.GA19396@lunn.ch> References: <20180925145622.29959-1-Jason@zx2c4.com> <20180925145622.29959-24-Jason@zx2c4.com> <20180927011526.GB1193@lunn.ch> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Sep 28, 2018 at 12:37:03AM +0200, Jason A. Donenfeld wrote: > Hi Andrew, > > Thanks for following up with this. > > On Thu, Sep 27, 2018 at 3:15 AM Andrew Lunn wrote: > > I know you have been concentrating on the crypto code, so i'm not > > expecting too many changes at the moment in the network code. > > I should be addressing things in parallel, actually, so I'm happy to > work on this. > > > WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON() > > #2984: FILE: drivers/net/wireguard/noise.c:293: > > + BUG_ON(first_len > BLAKE2S_HASH_SIZE || second_len > BLAKE2S_HASH_SIZE || > > I was actually going to ask you about this, because it applies > similarly in another context too that I'm trying to refine. The above > function you quote has the following properties: We have the above case, and we have the general case. The general case is, only use BUG_ON() when you know things have already gone bad and there is no recovery for the machine. I doubt that ever applies to wireguard. So i expect all the BUG_ON() to be removed. This is something which Linus keeps ranting about.... In this case: You have it inside a #ifdef. Meaning you don't really care, you can keep going anyway if debugging is turned of. So just turn it into a WARN_ON() so you get the splat, but the kernel keeps running. You have some other options. first_len, second_len, third_len are all parameter coming from #defines. As you suggested, you could do BUILD_BUG_ON(), but you have to do it at the caller. Which is fine, this is debug code, not user input validation code... BUILD_BUG_ON(NOISE_HASH_LEN > BLAKE2S_HAS_SIZE) BUILD_BUG_ON(NOISE_SYMMETRIC_KEY_LEN > BLAKE2S_HAS_SIZE) BUILD_BUG_ON(NOISE_PUBLIC_KEY_LEN > BLAKE2S_HAS_SIZE) kdf(chaining_key, key, NULL, dh_calculation, NOISE_HASH_LEN, NOISE_SYMMETRIC_KEY_LEN, 0, NOISE_PUBLIC_KEY_LEN, chaining_key); > > WARNING: Macros with flow control statements should be avoided > > #5471: FILE: drivers/net/wireguard/selftest/allowedips.h:456: > > +#define init_peer(name) do { \ > > + name = kzalloc(sizeof(*name), GFP_KERNEL); \ > > + if (unlikely(!name)) { \ > > + pr_info("allowedips self-test: out of memory\n"); \ > > + goto free; \ > > + } \ > > + kref_init(&name->refcount); \ > > + } while (0) > > This is part of a debugging selftest, where I'm initing a bunch of > peers one after another, and this macro helps keep the test clear > while offloading the actual irrelevant coding part to this macro. The > test itself then just has code like: Please don't focus on just the examples i give you. Look at the bigger issue. We cannot see the woods for the trees. checkpatch is giving out lots of warnings. There are so many, it is hard to see the interesting ones from the simple ones where you need to add an extra space, or add missing {}. If checkpatch was just issues one or two warnings about a goto in a macro, and nothing else, i probably would let it pass. This is bad, but it is not terrible. I personally have fallen into a trap caused by a goto in a macro. I changed the locking, not knowing about the goto, causing an error path not to unlock the lock. It took a while, but eventually the error happened, and soon after the machine deadlocked. At the time static checkers did not expand macros, so they did not detect the issue. > On a slightly related note, out of curiosity, any idea what's up with > the future of LTO in the kernel? Sorry, i've no idea. Not my corner of the kernel. > It sounds like that'd be nice to have > on a module-by-module basis. IOW, I'd love to LTO all of my .c files > in wireguard together, and then only ever expose mod_init/exit and > whatever I explicitly EXPORT_SYMBOL The namespace is more than just about the linker. I see an Opps stack trace with wg_ symbols, i know i need to talk to Jason. Without any prefix, i have to go digging into the code to find out who i need to talk to. This is one reason why often every symbol has the prefix, not just the global scope ones. Go look at other code in driver/net. You will find that most drivers use a prefix everywhere, both local and global. Andrew