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=-7.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 0E783C433DB for ; Sun, 7 Feb 2021 16:15:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D8CB564E3E for ; Sun, 7 Feb 2021 16:14:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229716AbhBGQOl (ORCPT ); Sun, 7 Feb 2021 11:14:41 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39744 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229684AbhBGQOj (ORCPT ); Sun, 7 Feb 2021 11:14:39 -0500 Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 135DFC06174A for ; Sun, 7 Feb 2021 08:13:58 -0800 (PST) Received: by mail-wm1-x332.google.com with SMTP id i9so10893305wmq.1 for ; Sun, 07 Feb 2021 08:13:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chrisdown.name; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=f37oj4SSGjJfGtwkQjnruEKUcZ9jnJkZgRlS8GvfDpk=; b=VjnOiaVP9qw4+aXLb3VabaMjxoNmVuHAsR3OCyqZ/I91l4V/FRz78g69nwUOo0QROL dlU9D/owyWqjoqI9lAViV916GWcsWhQX7LFOxUeMaPSUYFlfBQMGNMgOzPxoEVhv7wWi nU9vOrAQpYnmdwbyKkBh5Vb+Bdo6GUREdio6I= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=f37oj4SSGjJfGtwkQjnruEKUcZ9jnJkZgRlS8GvfDpk=; b=PRpbzzc6QDaX0KdIXOnxDz/wkuWE9XB0nr0jW+zbknnGvdsPmY81Jevvn8SrxevsyO YEzVt6AAjxi+a7GN1+0rtzJJ1R1bEhbUt5vdo0Lg44fvrCw80bn8bxY3a9EpVq1B+tkH GBGZc+0Jam0KIjEvqDayz+7wTrh+8VhjYkvTBMvvBATHRqaYeWjjW5zVnCQ8ckG5qBid EiC485boAVkOu49F7O/I4jsJuU77k3lt7YUMc5WZ9xE6t9ye4Xe0oNBBeuxZry/PKFGw v+DFo7avSYes2qIzdMfhpYj0AUmEwyblY9SXUJV8GZ/zrhzPGWC/x6i2wc9ryBUzC56J Awag== X-Gm-Message-State: AOAM532UjY12ZsU8nfI+6H5Ch6LZTt5P/xnYqJRs3owH/JYPo7jwR1Gl P2lzMOglvTJhss8iCQtoadPIdw== X-Google-Smtp-Source: ABdhPJy/G6UTTnVN7G8hSm4cI5wnEUspUGa1gKabuiqwlIqZPGMWhGgLTEcznMyRnlOAaCxUGYFdbQ== X-Received: by 2002:a1c:32c4:: with SMTP id y187mr11517942wmy.120.1612714436498; Sun, 07 Feb 2021 08:13:56 -0800 (PST) Received: from localhost ([2a01:4b00:8432:8a00:63de:dd93:20be:f460]) by smtp.gmail.com with ESMTPSA id u3sm25613442wre.54.2021.02.07.08.13.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 07 Feb 2021 08:13:55 -0800 (PST) Date: Sun, 7 Feb 2021 16:13:55 +0000 From: Chris Down To: Joe Perches Cc: Petr Mladek , linux-kernel@vger.kernel.org, Sergey Senozhatsky , John Ogness , Johannes Weiner , Andrew Morton , kernel-team@fb.com, Steven Rostedt , Alexey Dobriyan , Greg Kroah-Hartman , Jason Baron , Kees Cook , linux-api@vger.kernel.org Subject: Re: [PATCH] printk: Userspace format enumeration support Message-ID: References: <49124db60cdc88c4e9fcca1bbc9767432ad5a93b.camel@perches.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/2.0.5 (da5e3282) (2021-01-21) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Joe Perches writes: >> There are several issues with your proposed approach that make it unsuitable >> for use as part of a reliable production environment: >> >> 1. It misses printk() formats without KERN_SOH >> >> printk() formats without KERN_SOH are legal and use MESSAGE_LOGLEVEL_DEFAULT. >> On my test kernel, your proposed patch loses >5% of printk formats -- over 200 >> messages -- due to this, including critical ones like those about hardware or >> other errors. > >There are _very_ few of those printks without KERN_ and those >very few are not generally being changed. I already specified how many are lost: 5%. That's not "very few". That's a huge proportion of the coverage afforded by this patch, including several important cases. Relying on "they generally don't change" is not a recipe for reliability or success (and they do change, more data on that below). >> 2. Users don't always have the kernel image available >> >> Many of our machines and many of the machines of others like us do not boot >> using local storage, but instead use PXE or other technologies where the kernel >> may not be stored during runtime. >> >> As is described in the changelog, it is necessary to be able to vary >> remediations not only based on what is already in /dev/kmsg, but also to be >> able to make decisions about our methodology based on what's _supported_ in the >> running kernel at runtime, and your proposed approach makes this not viable. > >Indirection would alway work. > >You could load a separate file with output strings along with your >kernel image. You're moving the goalposts quite quickly here, which makes it harder to reply to your points. Now you're proposing an entirely separate distribution path, compared to interfaces that we already have precedent for in the kernel (eg. trace_printk). That requires a strong justification, and I'm not seeing one here. >> 3. `KERN_SOH + level' can appear in other places than just printk strings >> >> KERN_SOH is just ASCII '\001' -- it's not distinctive or unique, even when >> paired with a check for something that looks like a level after it. For this >> reason, your proposed patch results in a non-trivial amount of non-printk >> related garbage in its output. For example: >> >>      % binutils/strings -k /tmp/vmlinux | head -5 >>      3L)s >>      3L)s >>      c,[]A\ >>      c(L)c >>      d$pL)d$`u4 >> >> Fundamentally, one cannot use a tool which just determines whether something is >> printable to determine semantic intent. > >$ kernel_strings --kernel --section ".rodata" vmlinux > >I got exactly 0. "It works on my computer" is not a valid testing methodology, especially for something as complex as the Linux kernel. It's especially not a valid rebuttal to someone demonstrating that it clearly doesn't work on theirs. Even filtering to the .rodata section, there's plenty of garbage just in the first five cases: % binutils/strings --kernel --section ".rodata" /tmp/vmlinux | head -5 3******* Your BIOS seems to not contain a fix for K8 errata #93 1>pBC) dTRAC 6Run %s as init process 7calling %pS @ %i Clearly there are cases that you are not considering. My kernel config is attached if you want to try and replicate, but regardless, it's really not valid to say "it works for me" in response to someone showing that it doesn't. >> 4. strings(1) output cannot differentiate embedded newlines and new formats >> >> The following has exactly the same output from strings(1), but will manifest >> completely differently at printk() time: >> >>      printk(KERN_ERR "line one\nline two\nline three\n"); >>      printk("line four\n"); > >This is not the preferred output style and is only done in old and >unchanging code. > >Your use case in your commit log is looking for _changed_ formats. Joe, it's fine to present alternatives to people's patches, but please do your research before spouting things like this. It's a waste of everyone's time to refute things which are so easily demonstrated to be false. Here are a bunch of recent changes to printk I found just from literally 2 minutes of looking through `git log`: - ea34f78f3df6: 2020, printk site deleted (which of course we also need to know.) - a0f6d924cada: 2020, new callsite. the level is printed dynamically, so your proposed patch would not match. - bf13718bc57a: 2020, existing printk changed. - 994388f228c6: 2020, printk site changed to au0828_isocdbg, reworded entirely. - a8b62fd08505: 2020, new callsite, dynamic level. I could find literally pages and pages of these just from the last few years. Your belief that these printks are only in "unchanging" code does not match reality. >On Thu, 2021-02-04 at 15:37 +0000, Chris Down wrote: >> This patch provides a solution to the issue of silently changed or >> deleted printks: > >Exactly _how_ many of these use cases do you think exist? > >The generally preferred style for the example above would be: > > pr_err("line one\n"); > pr_err("line two\n"); > pr_err("line three\n"); > pr_err("line four\n"); I have no idea why you think this is so rare -- we have mixed pr_* and unadorned printk() all over the codebase. A number of the patches I just gave above are in files with mixed calls. >> The originally posted patch _does_ differentiate between these cases, using \0 >> as a reliable separator. Its outputs are, respectively: >> >>      \0013line one\nline two\nline three\0\nline four\n\0 >>      \0013line one\nline two\n\0line three\nline four\n\0 >> >> This isn't just a theoretical concern -- there are plenty of places which use >> multiline printks, and we must be able to distinguish between that and >> _multiple_ printks. > >Just like there are many places that use buffered printks as the >example I gave earlier. None of which your proposed solution would find. There are always going to be cases which are not caught. The point is that the patch proposed in this thread captures significantly more cases than the `strings` case (not to mention that it avoids outputting garbage from .rodata), not that it covers every imaginable scenario. >> 5. strings(1) is not contextually aware, and cannot be made to act as if it is >> >> strings has no idea about what it is reading, which is why it is more than >> happy to output the kind of meaningless output shown in #3. There are plenty of >> places across the kernel where there might be a sequence of bytes which the >> strings utility happens to interpret as being semantically meaningful, but in >> reality just happens to be an unrelated sequence of coincidentally printable >> bytes that just happens to contain a \001. >> >> I appreciate your willingness to propose other solutions, but for these >> reasons, the proposed strings(1) patch would not suffice as an interface for >> printk enumeration. > >I think you are on a path to try to make printk output immutable. >I think that's a _very_ bad path. That's literally the opposite of what this patchset does. This patchset offloads the responsibility of worrying about userspace parsers breaking because of changes to kernel printks, because those userspace parsers and maintainers now have a mechanism to detect changes. If anything, it _reduces_ the risk of what you're describing.