Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220226220639.1173594-1-Jason@zx2c4.com> <20220226220639.1173594-3-Jason@zx2c4.com>
 <CAMj1kXGRjdjbedQTU8ab+Q4jJPnN7nxc8_4QNiG3R7JE=zk-wA@mail.gmail.com> <CAHmME9rPd8Nu8Q0+R6B4Bz-caKpq5SSvLaiKdwXcyNHY8Ebfqg@mail.gmail.com>
In-Reply-To: <CAHmME9rPd8Nu8Q0+R6B4Bz-caKpq5SSvLaiKdwXcyNHY8Ebfqg@mail.gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Sun, 27 Feb 2022 11:30:06 +0100
Message-ID: <CAMj1kXEwsyPcoTvK6PZyA5L3q9u_dd_wXUiyHw7TtPM5LecP7g@mail.gmail.com>
Subject: Re: [PATCH v5 2/3] ACPI: allow longer device IDs
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     Len Brown <lenb@kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Alexander Graf <graf@amazon.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, 27 Feb 2022 at 11:03, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On 2/27/22, Ard Biesheuvel <ardb@kernel.org> wrote:
> > On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>
> >> From: Alexander Graf <graf@amazon.com>
> >>
> >
> > Please don't invent patch authors like that. Alex's patch that started
> > this discussion was completely different.
>
> Considering the investigative side ("why won't the _CID match?") and
> most the commit message were Alex's, and that those things comprise
> 95% of what this patch is, and that the code change itself isn't even
> part of anything Turing complete, I most certainly did not feel
> comfortable stripping Alex's authorship. Instead I added myself as a
> co-author at the bottom. When in doubt, err on the side of crediting
> others. Alex also took a look at this patch, I am under the impression
> of at least, before it went out. Let's minimize the paperwork
> policing, okay? I think it'd make for a much more pleasant space here.

Seriously? You want to go down the road again of poisoning the
atmosphere here, and blaming everyone else for doing it? I had enough
of that with the Zinc debacle, and I thought we had moved beyond that,
after having collaborated constructively with you on various topics
over the past couple of years.

Please stop with the ad hominems in response to criticism on factual
aspects of your code. Putting someone else's authorship on code they
did not write is not cool, and pointing that out is *not* what is
making this space unpleasant.
And 'paperwork policing' is sadly an important aspect of a high
profile open source project such as Linux.

> If Alex objects he can just simply say, "hey feel free to remove me as
> author," and it'll be simple as that, and again doesn't involve your
> policing.
>

When you ask for my review, you get my review. If there are aspects of
your contributions that are a priori exempt from criticism, I think
you're in the wrong place.

> >> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> >> entries of the respective devices. However, we squeeze them into struct
> >> acpi_device_id, which only has 9 bytes space to store the identifier. It
> >> originally had 16 bytes, but was changed to only have 9 in 6543becf26ff
> >> ("mod/file2alias: make modalias generation safe for cross compiling"),
> >> presumably on the theory that it would match the ACPI spec so it didn't
> >> matter.
> >>
> >
> > Please clarify that this applies to the module metadata side of
> > things. The ACPI subsystem already captures and exposes _HIDs and
> > _CIDs that are longer than 8 characters, which is why simply
> > increasing the size of this field is sufficient to create modules that
> > can match devices that expose a CID that is longer than 8 bytes.
>
> Good point for strengthening the argument here. Will do.
>
> >
> >> Unfortunately, while most people adhere to the ACPI specs, Microsoft
> >> decided that its VM Generation Counter device [1] should only be
> >> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> >> than 9 characters.
> >>
> >> To allow device drivers to match identifiers that exceed the 9 byte
> >> limit, this simply ups the length to 16, just like it was before the
> >> aforementioned commit. Empirical testing indicates that this
> >> doesn't actually increase vmlinux size, because the ulong in the same
> >> struct caused there to be 7 bytes of padding anyway.
> >>
> >
> > The padding situation only applies to struct acpi_device_id, whereas
> > ACPI_ID_LEN is used in other places as well. Also, the size of vmlinux
> > only covers statically allocated instances in the core kernel, and
> > most of the ACPI_ID_LEN uses are probably in drivers. So whether
> > vmlinux changes size or not is not that relevant.
>
> I actually looked at every usage in the tree (there aren't that many)
> and couldn't find a single one where behavior changed, performance
> changed, or memory usage changed. I thought we looked together on IRC
> so I'm surprised to see you mention this, but maybe I misunderstood
> you. Anyway, I can't see the size increase impacting anything at all.
> If you see a case, this would be the time to mention that you see
> something. I didn't find anything though.
>

If you did not check the rodata/data/bss sizes of all modules
depending on this macro, or checked their memory usage at runtime, you
cannot definitively say that nothing has changed.

*However*, as I argued below, using ACPI_ID_LEN to allocate a string
that needs to hold a HID or CID provided by the ACPI subsystem might
well result in a buffer overrun, as the ACPI subsystem will happily
return longer strings.

So my conclusion is that the change is ok, which is why I gave my reviewed-by.

> > Patch 6543becf26ff was wrong to change ACPI_ID_LEN, because it failed
> > to take into account any other uses of ACPI_ID_LEN, and did not bother
> > to explain why the change was necessary in the context of what it was
> > trying to achieve.
>
> I'm not sure there really were other usages back then. The commit
> message seems descriptive enough to me too. This was about cross
> compiling, so padding. But it certainly did seem to limit future
> drivers in an unintended way, as you wrote:
>
> > So, given that we need more than 8 characters to match drivers to
> > devices exposed by Hyper-V (or other VMMs adhering to the VMGENID
> > spec), I think this change is necessary and correct.
>
> Right, that's the idea.
>
>
> >
> > So, with the authorship/signoff corrected, and the commit log clarified,
> >
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>
> Thanks.
>
> Hopefully we'll hear from Rafael this week.
>
> Jason