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=-0.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no 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 355FDC433DF for ; Tue, 19 May 2020 16:26:21 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id DD104207FB for ; Tue, 19 May 2020 16:26:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=linux-foundation.org header.i=@linux-foundation.org header.b="a8S6Fimr" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DD104207FB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux-foundation.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 3FCAA900003; Tue, 19 May 2020 12:26:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3AD1A900002; Tue, 19 May 2020 12:26:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 29BC7900003; Tue, 19 May 2020 12:26:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0204.hostedemail.com [216.40.44.204]) by kanga.kvack.org (Postfix) with ESMTP id 0E16F900002 for ; Tue, 19 May 2020 12:26:20 -0400 (EDT) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id BAFE887D8 for ; Tue, 19 May 2020 16:26:19 +0000 (UTC) X-FDA: 76833996078.04.plane76_544384bd9a63c X-HE-Tag: plane76_544384bd9a63c X-Filterd-Recvd-Size: 5592 Received: from mail-lj1-f193.google.com (mail-lj1-f193.google.com [209.85.208.193]) by imf16.hostedemail.com (Postfix) with ESMTP for ; Tue, 19 May 2020 16:26:18 +0000 (UTC) Received: by mail-lj1-f193.google.com with SMTP id z6so332104ljm.13 for ; Tue, 19 May 2020 09:26:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=dhbRZ27gTq3LlamCeWaOIagumkyuEgHdNj01JBr2vyU=; b=a8S6FimrljQCYdqdMRjHudj5xuwSAYeW17sDBke8Wp/wO6Vv10u9cXvAcweW0ZSEYT O3hXN68ol8TooTR2xUI2YNd5y+PctNRhxhENJnhmYB+aBitMMtqsXQFMvUY86xEYH5PA jFICERjG8bgZGMW0GbVN6qDt50ZsFp9Lq9zCI= 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=dhbRZ27gTq3LlamCeWaOIagumkyuEgHdNj01JBr2vyU=; b=YIf1O6Y/k9r1Ty4H+tK9a3oodUsj9+i39SLgSepoCsslgzPT1FkZVJ3eniJSLqpeQG ioMJmDVsnQhGdtE7rMqtbc51/pw2WKZ+h/KFtpVIRWWuyclNcIksdd1MEf6R2nlI1UP0 rla0/ykLfMiCsi/GeubZzbv9zAvCspla+4FvzNp3OYKRIOX3ZsEarHi74OLneMDKNL8y WNppHhYWOiavWGXF7iB9q+YCIfi7sORZt/qTWl+VcBCjhcxc49fs9FkrKOViT21kRge8 QSpz4bDxwxiZeNFtzqprL41xzOWMR/gSZaVOjGPj5SlDtxvmCEOdRkH9cdPgAJdudCOS vlUQ== X-Gm-Message-State: AOAM531YoQ9ME1NzZTllgtHJ29s3QR+RV22ABjac2H2v88HgTvHVo8Lr TYRUekcBuGvh+QxrvQWlAT50Z9T9z90= X-Google-Smtp-Source: ABdhPJzlgLZRBHQdKGgqxegsYaMiSg0aO3lFRtiCQVZ/njx4t+SQL2Te2eY2JFihLTq8XgCzCyV/ug== X-Received: by 2002:a2e:9f43:: with SMTP id v3mr112061ljk.285.1589905575692; Tue, 19 May 2020 09:26:15 -0700 (PDT) Received: from mail-lj1-f181.google.com (mail-lj1-f181.google.com. [209.85.208.181]) by smtp.gmail.com with ESMTPSA id y8sm47924ljh.83.2020.05.19.09.26.13 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 19 May 2020 09:26:14 -0700 (PDT) Received: by mail-lj1-f181.google.com with SMTP id b6so418201ljj.1 for ; Tue, 19 May 2020 09:26:13 -0700 (PDT) X-Received: by 2002:a05:651c:1183:: with SMTP id w3mr128485ljo.265.1589905573160; Tue, 19 May 2020 09:26:13 -0700 (PDT) MIME-Version: 1.0 References: <20200519134449.1466624-1-hch@lst.de> <20200519134449.1466624-13-hch@lst.de> In-Reply-To: <20200519134449.1466624-13-hch@lst.de> From: Linus Torvalds Date: Tue, 19 May 2020 09:25:57 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 12/20] maccess: remove strncpy_from_unsafe To: Christoph Hellwig Cc: "the arch/x86 maintainers" , Alexei Starovoitov , Daniel Borkmann , Masami Hiramatsu , Andrew Morton , linux-parisc@vger.kernel.org, linux-um , Netdev , bpf@vger.kernel.org, Linux-MM , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, May 19, 2020 at 6:45 AM Christoph Hellwig wrote: > > + if (IS_ENABLED(CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE) && > + compat && (unsigned long)unsafe_ptr < TASK_SIZE) > + ret = strncpy_from_user_nofault(dst, user_ptr, size); > + else > + ret = strncpy_from_kernel_nofault(dst, unsafe_ptr, size); These conditionals are completely illegible. That's true in the next patch too. Stop using "IS_ENABLED(config)" to make very complex conditionals. A clear #ifdef is much better if the alternative is a conditional that is completely impossible to actually understand and needs multiple lines to read. If you made this a simple helper (called "bpf_strncpy_from_unsafe()" with that "compat" flag, perhaps?), it would be much more legible as /* * Big comment goes here about the compat behavior and * non-overlapping address spaces and ambiguous pointers. */ static long bpf_strncpy_from_legacy(void *dest, const void *unsafe_ptr, long size, bool legacy) { #ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE if (legacy && addr < TASK_SIZE) return strncpy_from_user_nofault(dst, (const void __user *) unsafe_ptr, size); #endif return strncpy_from_kernel_nofault(dst, unsafe_ptr, size); } and then you'd just use if (bpf_strncpy_from_unsafe(dst, unsafe_ptr, size, compat) < 0) memset(dst, 0, size); and avoid any complicated conditionals, goto's, and make the code much easier to understand thanks to having a big comment about the legacy case. In fact, separately I'd probably want that "compat" naming to be scrapped entirely in that file. "compat" generally means something very specific and completely different in the kernel: it's the "I'm a 32-bit binary on a 64-bit kernel" compatibility case. Here, it's literally "BPF legacy behavior", not that kind of "compat" thing. But that renaming is separate, although I'd start the ball rolling with that "bpf_strncpy_from_legacy()" helper. Linus