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=-13.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable 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 A61B8C32751 for ; Wed, 7 Aug 2019 21:12:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 713D2217D9 for ; Wed, 7 Aug 2019 21:12:31 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="rAUbgR3z" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730375AbfHGVM2 (ORCPT ); Wed, 7 Aug 2019 17:12:28 -0400 Received: from mail-pl1-f193.google.com ([209.85.214.193]:38800 "EHLO mail-pl1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730365AbfHGVM1 (ORCPT ); Wed, 7 Aug 2019 17:12:27 -0400 Received: by mail-pl1-f193.google.com with SMTP id m12so3959286plt.5 for ; Wed, 07 Aug 2019 14:12:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=fd+dofbpCV/6k5WcFl4oZMCWdF8fj4kR4/a+wjxKmsA=; b=rAUbgR3zhEw1fjUVF8KM3rovMm6jms4eUTwDrpb/7hk+mJRmY+GevkEEjZ8jd9WxJ8 ebboqjF0o1XIe0OyOtHde46MsTIXqNDzKbfU14DIZd/Xg3c2k72+RFur9R2NQ6CghXkb kvSyjMG5CjfRXi8FIkIWOq1aMaubFv5QFuFCjjJdeAsifVfE6p6WjJcnUmgtWFYkBGVZ m1LPTv3MkQd6vdZv9sdS0bQAe2kA/7mk1TFwOGPkDQzcMC/5KjUC/P+kZhx2g/JMS5NV /GJkm5fugsOWIlIhHvsuntFuhFO1K2oR3ulrDn3XIO2GAdfJxdQO/P+tKaNbuLtzqKRt uosA== 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=fd+dofbpCV/6k5WcFl4oZMCWdF8fj4kR4/a+wjxKmsA=; b=mz5GDfTefHwT5M69jR7O55JDEOYeVjHX6iioFuOPpIBsgzRNZEcAlInCU/5iuu1p0T cJsslWIKy45wZ4WtYxQwDejr2CS4pzkzW05YHWj/JKe7TWuVJTaf/jfkZEeVb9xK/XrB SMIBodnqG5k7dZDcIMV2dlnPsPSbm6D7713wixxFyvUE0xRwm7NyJ1jDd+eVbt14egLX VtmsBiGya6CVkLvXji8s9/xfOqO4TRrlPnaFbhJUvttJIilIZ0+qJY36M1dv3ULV9NjK rtDf9G73TmRHZ6JH+/H31iGWvgAwwq82aTL58rZiF0GFoyN9QSWw/STyWO/9bSOjYEne epTg== X-Gm-Message-State: APjAAAWMO3hD7HwBvj/NEBelfYjwjeWsGCOm/HVcwE2mJcN7MsHSunk4 hJ0GP1yPf8sw4CL7WAqsR/XRtQejFfiwS9v44rkjMQ== X-Google-Smtp-Source: APXvYqznwJRoAcnn7fVAEd4l+avqz5PXq39u3baHCNe66I0Lo+mKBr+Sgwoiq+RvXzYN4HtITPL6iOWmEQ73ykipzGY= X-Received: by 2002:a17:902:3363:: with SMTP id a90mr9572730plc.119.1565212346434; Wed, 07 Aug 2019 14:12:26 -0700 (PDT) MIME-Version: 1.0 References: <20190729211014.39333-1-ndesaulniers@google.com> In-Reply-To: From: Nick Desaulniers Date: Wed, 7 Aug 2019 14:12:15 -0700 Message-ID: Subject: Re: [PATCH] mips: avoid explicit UB in assignment of mips_io_port_base To: "Maciej W. Rozycki" Cc: Ralf Baechle , Paul Burton , James Hogan , Nathan Chancellor , Eli Friedman , Hassan Naveed , Stephen Kitt , Serge Semin , Mike Rapoport , Andrew Morton , Michal Hocko , linux-mips@vger.kernel.org, LKML , clang-built-linux , regehr@cs.utah.edu, Philip Reames , Alexander Potapenko Content-Type: text/plain; charset="UTF-8" Sender: linux-mips-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-mips@vger.kernel.org Sorry for the delayed response, literally sent the patch then went on vacation. On Mon, Jul 29, 2019 at 3:16 PM Maciej W. Rozycki wrote: > > On Mon, 29 Jul 2019, Nick Desaulniers wrote: > > > The code in question is modifying a variable declared const through > > pointer manipulation. Such code is explicitly undefined behavior, and > > is the lone issue preventing malta_defconfig from booting when built > > with Clang: > > > > If an attempt is made to modify an object defined with a const-qualified > > type through use of an lvalue with non-const-qualified type, the > > behavior is undefined. > > > > LLVM is removing such assignments. A simple fix is to not declare > > variables const that you plan on modifying. Limiting the scope would be > > a better method of preventing unwanted writes to such a variable. This is now documented in the LLVM release notes for Clang-9: https://github.com/llvm/llvm-project/commit/e39e79358fcdd5d8ad809defaa821f0bbfa809a5 > > > > Further, the code in question mentions "compiler bugs" without any links > > to bug reports, so it is difficult to know if the issue is resolved in > > GCC. The patch was authored in 2006, which would have been GCC 4.0.3 or > > 4.1.1. The minimal supported version of GCC in the Linux kernel is > > currently 4.6. > > It's somewhat older than that. My investigation points to: > > commit c94e57dcd61d661749d53ee876ab265883b0a103 > Author: Ralf Baechle > Date: Sun Nov 25 09:25:53 2001 +0000 > > Cleanup of include/asm-mips/io.h. Now looks neat and harmless. Oh indeed, great find! So it looks to me like the order of events is: 1. https://github.com/jaaron/linux-mips-ip30/commit/c94e57dcd61d661749d53ee876ab265883b0a103 in 2001 first introduces the UB. mips_io_port_base is defined non-const in arch/mips/kernel/setup.c, but then declared extern const (and modified via UB) in include/asm-mips/io.h. A setter is created, but not a getter (I'll revisit this below). This appears to work (due to luck) for a few years until: 2. https://github.com/mpe/linux-fullhistory/commit/966f4406d903a4214fdc74bec54710c6232a95b8 in 2006 adds a compiler barrier (reload all variables) and this appears to work. The commit message mentions that reads after modification of the const variable were buggy (likely GCC started taking advantage of the explicit UB around this time as well). This isn't a fix for UB (more thoughts below), but appears to work. 3. https://github.com/llvm/llvm-project/commit/b45631090220b732e614b5530bbd1d230eb9d38e in 2019 removes writes to const variables in LLVM as that's explicit UB. We observe the boot failure in mips and narrow it down to this instance. I can see how throwing a compiler barrier in there made subsequent reads after UB writes appear to work, but that was more due to luck and implementation details of GCC than the heart of the issue (ie. not writing code that is explicitly undefined behavior)(and could change in future versions of GCC). Stated another way, the fix for explicit UB is not hacks, but avoiding the UB by rewriting the problematic code. > However the purpose of the arrangement does not appear to me to be > particularly specific to a compiler version. > > > For what its worth, there was UB before the commit in question, it just > > added a barrier and got lucky IRT codegen. I don't think there's any > > actual compiler bugs related, just runtime bugs due to UB. > > Does your solution preserves the original purpose of the hack though as > documented in the comment you propose to be removed? The function modified simply writes to a global variable. It's not clear to my why the value about to be modified would EVER be loaded before modification. > Clearly it was defined enough to work for almost 18 years, so it would be > good to keep the optimisation functionally by using different means that > do not rely on UB. "Defined enough" ??? https://youtu.be/Aq_1l316ow8?t=17 > This variable is assigned at most once throughout the > life of the kernel and then early on, so considering it r/w with all the > consequences for all accesses does not appear to me to be a good use of > it. Note: it's not possible to express the semantics of a "write once variable" in C short of static initialization (AFAIK, without explicit violation of UB, but Cunningham's Law may apply). (set_io_port_base is called in ~20 places) Thinking more about this while I was away, I think what this code has needed since 2001 is proper encapsulation. If you want a variable that is written from one place only, but readable throughout, then the pattern I'd use is: 1. declare a getter in a .h file. 2. define/qualify `mips_io_port_base` as `static` and non-const in a .c file where it's modified. 3. define the getter and setter in the above .c file. That would rely on linkage to limit the visibility of the symbol for modification. But, we'd then need to export the getter, vs the symbol itself. There's also on the order of ~20 call sites that would need to be changed to invoke the getter rather than read the raw variable. Also, it's unlikely the getter gets inlined across translation units (short of LTO, which the mainline kernel doesn't support today). I think my patch here (https://lkml.org/lkml/2019/7/29/1636) is minimally and much less invasive. > Maybe a piece of inline asm to hide the initialisation or suchlike then? I think that would still be UB as the definition would not be changed; you'd still be modifying a variable declared const. -- Thanks, ~Nick Desaulniers