diff mbox series

[v5,2/3] ACPI: allow longer device IDs

Message ID 20220226220639.1173594-3-Jason@zx2c4.com (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series ACPI: VM fork detection for RNG | expand

Commit Message

Jason A. Donenfeld Feb. 26, 2022, 10:06 p.m. UTC
From: Alexander Graf <graf@amazon.com>

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.

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.

This patch is a prerequisite to add support for VMGenID in Linux, the
subsequent patch in this series. It has been confirmed to also work on
the udev/modalias side in userspace.

[1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx

Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Alexander Graf <graf@amazon.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Hi Rafael & Len,

This patchset is directed toward you two specifically. Patches 1/3 and
3/3 have been through the ringer of review a bit already and do not
specifically require your attention, but in v4 we wound up getting hung
up on an ACPI API limitation. This v5 fixes that limitation with this
2/3 patch that you see here, with a trivial one line fix, which does
require your attention.

Patches 1/3 and 3/3 will go through my random.git tree. However, 3/3
actually depends on this one here, 2/3, in order to compile without
warnings (and be functional at all). Therefore, it would be nice if you
would provide an "Acked-by" on it and permit me to /also/ take it
through my random.git tree (if it looks like a correct patch to you, of
course). This would make the merge logistics a lot easier. Plus it's a
small +1/-1 line change.

Please have a look and let me know what you think.

Thanks,
Jason

 include/linux/mod_devicetable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ard Biesheuvel Feb. 27, 2022, 7:31 a.m. UTC | #1
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.

> 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.

> 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.


> This patch is a prerequisite to add support for VMGenID in Linux, the
> subsequent patch in this series. It has been confirmed to also work on
> the udev/modalias side in userspace.
>
> [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Alexander Graf <graf@amazon.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

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.

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.

So, with the authorship/signoff corrected, and the commit log clarified,

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>


> ---
> Hi Rafael & Len,
>
> This patchset is directed toward you two specifically. Patches 1/3 and
> 3/3 have been through the ringer of review a bit already and do not
> specifically require your attention, but in v4 we wound up getting hung
> up on an ACPI API limitation. This v5 fixes that limitation with this
> 2/3 patch that you see here, with a trivial one line fix, which does
> require your attention.
>
> Patches 1/3 and 3/3 will go through my random.git tree. However, 3/3
> actually depends on this one here, 2/3, in order to compile without
> warnings (and be functional at all). Therefore, it would be nice if you
> would provide an "Acked-by" on it and permit me to /also/ take it
> through my random.git tree (if it looks like a correct patch to you, of
> course). This would make the merge logistics a lot easier. Plus it's a
> small +1/-1 line change.
>
> Please have a look and let me know what you think.
>
> Thanks,
> Jason
>
>  include/linux/mod_devicetable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 4bb71979a8fd..5da5d990ff58 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -211,7 +211,7 @@ struct css_device_id {
>         kernel_ulong_t driver_data;
>  };
>
> -#define ACPI_ID_LEN    9
> +#define ACPI_ID_LEN    16
>
>  struct acpi_device_id {
>         __u8 id[ACPI_ID_LEN];
> --
> 2.35.1
>
Ard Biesheuvel Feb. 27, 2022, 7:37 a.m. UTC | #2
On Sun, 27 Feb 2022 at 08:31, 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.
>
> > 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.
>
> > 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.
>
>
> > This patch is a prerequisite to add support for VMGenID in Linux, the
> > subsequent patch in this series. It has been confirmed to also work on
> > the udev/modalias side in userspace.
> >
> > [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
> >
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > Signed-off-by: Alexander Graf <graf@amazon.com>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
> 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.
>

Hmm, actually, ACPI_ID_LEN wasn't used outside of
linux/mod_device_table.h before 6543becf26ff, so changing it at that
point was fine.

I do wonder how much code is out there that blindly assumes the ACPI
core will never deliver more than 8 bytes' worth of _HID/_CID, and
subsequently runs off the end of a statically sized buffer.
Jason A. Donenfeld Feb. 27, 2022, 10:03 a.m. UTC | #3
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.
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.

>
>> 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.

> 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
Ard Biesheuvel Feb. 27, 2022, 10:30 a.m. UTC | #4
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
Ard Biesheuvel Feb. 27, 2022, 10:47 a.m. UTC | #5
On Sun, 27 Feb 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> 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.
>
...
>
> 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.
>

I typed this before reading your message on IRC, which reads:

"Alex looked at that patch before i sent it out and did not object to
me keeping his authorship. I wouldn't have sent it out otherwise."

and so I stand corrected if this is true. But please, next time,
please be more clear about these things.
Alexander Graf Feb. 27, 2022, 11:38 a.m. UTC | #6
On 27.02.22 11:47, Ard Biesheuvel wrote:
> On Sun, 27 Feb 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
>> 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.
> ...
>> 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.
>>
> I typed this before reading your message on IRC, which reads:
>
> "Alex looked at that patch before i sent it out and did not object to
> me keeping his authorship. I wouldn't have sent it out otherwise."
>
> and so I stand corrected if this is true. But please, next time,
> please be more clear about these things.


Yes, he did reach out to me on a separate channel and I told him to go 
for it :). Sorry if I created some confusion with that.


Thanks,

Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Alexander Graf Feb. 27, 2022, 11:42 a.m. UTC | #7
On 26.02.22 23:06, Jason A. Donenfeld wrote:
> From: Alexander Graf <graf@amazon.com>
>
> 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.
>
> 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


This is only true for 64bit systems where padding automatically bloated 
to 9 byte array to 16. I still believe the patch is fine as it is, but 
there will be minor .rodata overhead on 32bit targets which you may want 
to quantify in the patch description.


Thanks a lot for sending this out! And let's hope that 16 bytes is 
enough for everyone.

Alex


> doesn't actually increase vmlinux size, because the ulong in the same
> struct caused there to be 7 bytes of padding anyway.
>
> This patch is a prerequisite to add support for VMGenID in Linux, the
> subsequent patch in this series. It has been confirmed to also work on
> the udev/modalias side in userspace.
>
> [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Alexander Graf <graf@amazon.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Hi Rafael & Len,
>
> This patchset is directed toward you two specifically. Patches 1/3 and
> 3/3 have been through the ringer of review a bit already and do not
> specifically require your attention, but in v4 we wound up getting hung
> up on an ACPI API limitation. This v5 fixes that limitation with this
> 2/3 patch that you see here, with a trivial one line fix, which does
> require your attention.
>
> Patches 1/3 and 3/3 will go through my random.git tree. However, 3/3
> actually depends on this one here, 2/3, in order to compile without
> warnings (and be functional at all). Therefore, it would be nice if you
> would provide an "Acked-by" on it and permit me to /also/ take it
> through my random.git tree (if it looks like a correct patch to you, of
> course). This would make the merge logistics a lot easier. Plus it's a
> small +1/-1 line change.
>
> Please have a look and let me know what you think.
>
> Thanks,
> Jason
>
>   include/linux/mod_devicetable.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 4bb71979a8fd..5da5d990ff58 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -211,7 +211,7 @@ struct css_device_id {
>          kernel_ulong_t driver_data;
>   };
>
> -#define ACPI_ID_LEN    9
> +#define ACPI_ID_LEN    16
>
>   struct acpi_device_id {
>          __u8 id[ACPI_ID_LEN];
> --
> 2.35.1
>



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Ard Biesheuvel Feb. 27, 2022, 11:43 a.m. UTC | #8
On Sun, 27 Feb 2022 at 12:39, Alexander Graf <graf@amazon.com> wrote:
>
>
> On 27.02.22 11:47, Ard Biesheuvel wrote:
> > On Sun, 27 Feb 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
> >> 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.
> > ...
> >> 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.
> >>
> > I typed this before reading your message on IRC, which reads:
> >
> > "Alex looked at that patch before i sent it out and did not object to
> > me keeping his authorship. I wouldn't have sent it out otherwise."
> >
> > and so I stand corrected if this is true. But please, next time,
> > please be more clear about these things.
>
>
> Yes, he did reach out to me on a separate channel and I told him to go
> for it :). Sorry if I created some confusion with that.
>

No, my bad. But in my defence, everyone on the original thread knows
that this single oneline change was suggested by Jason, not you, and
so seeing him posting it as your patch did confuse me a little.
Alexander Graf Feb. 27, 2022, 11:48 a.m. UTC | #9
On 27.02.22 12:43, Ard Biesheuvel wrote:
> On Sun, 27 Feb 2022 at 12:39, Alexander Graf <graf@amazon.com> wrote:
>>
>> On 27.02.22 11:47, Ard Biesheuvel wrote:
>>> On Sun, 27 Feb 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>> 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.
>>> ...
>>>> 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.
>>>>
>>> I typed this before reading your message on IRC, which reads:
>>>
>>> "Alex looked at that patch before i sent it out and did not object to
>>> me keeping his authorship. I wouldn't have sent it out otherwise."
>>>
>>> and so I stand corrected if this is true. But please, next time,
>>> please be more clear about these things.
>>
>> Yes, he did reach out to me on a separate channel and I told him to go
>> for it :). Sorry if I created some confusion with that.
>>
> No, my bad. But in my defence, everyone on the original thread knows
> that this single oneline change was suggested by Jason, not you, and
> so seeing him posting it as your patch did confuse me a little.


The idea came up 1y ago in conversations with Adrian when we tried to 
make _CID matching work. Unfortunately I did not file a patent for the 
mechanism to increase the array size until data fits :). It's such a 
revolutionary invention!

Back to seriousness, I'm pretty indifferent on the attribution for it. 
What I'm more interested in is a solution that allows us to match the 
correct identifier :). My take is that Jason just wanted to be nice and 
was trying to give credit.


Alex




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Ard Biesheuvel Feb. 27, 2022, 11:59 a.m. UTC | #10
On Sun, 27 Feb 2022 at 12:48, Alexander Graf <graf@amazon.com> wrote:
>
>
> On 27.02.22 12:43, Ard Biesheuvel wrote:
> > On Sun, 27 Feb 2022 at 12:39, Alexander Graf <graf@amazon.com> wrote:
> >>
> >> On 27.02.22 11:47, Ard Biesheuvel wrote:
> >>> On Sun, 27 Feb 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>> 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.
> >>> ...
> >>>> 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.
> >>>>
> >>> I typed this before reading your message on IRC, which reads:
> >>>
> >>> "Alex looked at that patch before i sent it out and did not object to
> >>> me keeping his authorship. I wouldn't have sent it out otherwise."
> >>>
> >>> and so I stand corrected if this is true. But please, next time,
> >>> please be more clear about these things.
> >>
> >> Yes, he did reach out to me on a separate channel and I told him to go
> >> for it :). Sorry if I created some confusion with that.
> >>
> > No, my bad. But in my defence, everyone on the original thread knows
> > that this single oneline change was suggested by Jason, not you, and
> > so seeing him posting it as your patch did confuse me a little.
>
>
> The idea came up 1y ago in conversations with Adrian when we tried to
> make _CID matching work. Unfortunately I did not file a patent for the
> mechanism to increase the array size until data fits :). It's such a
> revolutionary invention!
>
> Back to seriousness, I'm pretty indifferent on the attribution for it.
> What I'm more interested in is a solution that allows us to match the
> correct identifier :). My take is that Jason just wanted to be nice and
> was trying to give credit.
>

Giving credit is nice, but I do think that obfuscating authorship like
this is generally not preferred.

But in this particular case, it hardly matters, so let's not debate
this any further.
Jason A. Donenfeld Feb. 27, 2022, 12:43 p.m. UTC | #11
Hi Alex,

On Sun, Feb 27, 2022 at 12:42:03PM +0100, Alexander Graf wrote:
> > 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
> 
> 
> This is only true for 64bit systems where padding automatically bloated 
> to 9 byte array to 16. I still believe the patch is fine as it is, but 
> there will be minor .rodata overhead on 32bit targets which you may want 
> to quantify in the patch description.

Good point. So I just tried this out with a 32-bit i686 kernel and the
results are the same again for the size of vmlinux. I then ran `objdump
--headers` and looked at the size of the .rodata section, where it's
also the same. I'm not quite sure what to make of this, as it's not what
I was expecting, but I think I tested it right. So maybe we're lucky
here?

Jason
Jason A. Donenfeld Feb. 27, 2022, 11:27 p.m. UTC | #12
Hey again,

On Sun, Feb 27, 2022 at 1:43 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Alex,
>
> On Sun, Feb 27, 2022 at 12:42:03PM +0100, Alexander Graf wrote:
> > > 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
> >
> >
> > This is only true for 64bit systems where padding automatically bloated
> > to 9 byte array to 16. I still believe the patch is fine as it is, but
> > there will be minor .rodata overhead on 32bit targets which you may want
> > to quantify in the patch description.
>
> Good point. So I just tried this out with a 32-bit i686 kernel and the
> results are the same again for the size of vmlinux. I then ran `objdump
> --headers` and looked at the size of the .rodata section, where it's
> also the same. I'm not quite sure what to make of this, as it's not what
> I was expecting, but I think I tested it right. So maybe we're lucky
> here?

I tried a little harder to get _some_ difference on 32-bit, and
managed to get one by doing i386_defconfig and then switching off
modules to make all M into Y, and then compared sizes:

vmlinux: 25590780 -> 25598972, so a 0.032% increase.
bzImage: 8698944 -> 8699424, so a 0.0055% increase.

So it does increase, ever so slightly, but a) on 32-bit, and b) a
super, super tiny amount.

In other words, I still think this patch is very much a-okay. But very
eager to hear from Rafael on the approach.

Jason
Rafael J. Wysocki Feb. 28, 2022, 6:19 p.m. UTC | #13
+Mika, Andy and Hans in case they have something to add

On Mon, Feb 28, 2022 at 12:27 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hey again,
>
> On Sun, Feb 27, 2022 at 1:43 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Hi Alex,
> >
> > On Sun, Feb 27, 2022 at 12:42:03PM +0100, Alexander Graf wrote:
> > > > 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
> > >
> > >
> > > This is only true for 64bit systems where padding automatically bloated
> > > to 9 byte array to 16. I still believe the patch is fine as it is, but
> > > there will be minor .rodata overhead on 32bit targets which you may want
> > > to quantify in the patch description.
> >
> > Good point. So I just tried this out with a 32-bit i686 kernel and the
> > results are the same again for the size of vmlinux. I then ran `objdump
> > --headers` and looked at the size of the .rodata section, where it's
> > also the same. I'm not quite sure what to make of this, as it's not what
> > I was expecting, but I think I tested it right. So maybe we're lucky
> > here?
>
> I tried a little harder to get _some_ difference on 32-bit, and
> managed to get one by doing i386_defconfig and then switching off
> modules to make all M into Y, and then compared sizes:
>
> vmlinux: 25590780 -> 25598972, so a 0.032% increase.
> bzImage: 8698944 -> 8699424, so a 0.0055% increase.
>
> So it does increase, ever so slightly, but a) on 32-bit, and b) a
> super, super tiny amount.
>
> In other words, I still think this patch is very much a-okay. But very
> eager to hear from Rafael on the approach.

Increasing the ACPI_ID_LEN value is fine with me, but the patch
changelog is not entirely accurate.

The ACPI subsystem uses struct acpi_device_id mostly (if not only) for
device ID matching and it is generally used for creating lists of ACPI
device IDs in drivers (and allow/deny lists etc).  The device IDs
extracted from the ACPI tables can be longer than ACPI_ID_LEN.

This means that drivers cannot match device IDs longer than 8
characters (excluding the terminating 0), because the IDs in the lists
used by them for ID matching cannot be longer than this and not
because the ACPI subsystem is limited by that value.
Jason A. Donenfeld Feb. 28, 2022, 6:21 p.m. UTC | #14
Hi Rafael,

On Mon, Feb 28, 2022 at 7:19 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> Increasing the ACPI_ID_LEN value is fine with me, but the patch
> changelog is not entirely accurate.
>
> The ACPI subsystem uses struct acpi_device_id mostly (if not only) for
> device ID matching and it is generally used for creating lists of ACPI
> device IDs in drivers (and allow/deny lists etc).  The device IDs
> extracted from the ACPI tables can be longer than ACPI_ID_LEN.
>
> This means that drivers cannot match device IDs longer than 8
> characters (excluding the terminating 0), because the IDs in the lists
> used by them for ID matching cannot be longer than this and not
> because the ACPI subsystem is limited by that value.

Thanks for your notes there. I think Ard more or less pointed out
something similar too. I'll amend the commit message, send a v2, and
hopefully this change is okay with Mika/Andy/Hans.

Regards,
Jason
Hans de Goede March 1, 2022, 10:31 a.m. UTC | #15
Hi,

On 2/28/22 19:19, Rafael J. Wysocki wrote:
> +Mika, Andy and Hans in case they have something to add

Thanks, I don't really have anything to add to the discussion
on v6 of this patch.

Regards,

Hans



> 
> On Mon, Feb 28, 2022 at 12:27 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>
>> Hey again,
>>
>> On Sun, Feb 27, 2022 at 1:43 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>
>>> Hi Alex,
>>>
>>> On Sun, Feb 27, 2022 at 12:42:03PM +0100, Alexander Graf wrote:
>>>>> 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
>>>>
>>>>
>>>> This is only true for 64bit systems where padding automatically bloated
>>>> to 9 byte array to 16. I still believe the patch is fine as it is, but
>>>> there will be minor .rodata overhead on 32bit targets which you may want
>>>> to quantify in the patch description.
>>>
>>> Good point. So I just tried this out with a 32-bit i686 kernel and the
>>> results are the same again for the size of vmlinux. I then ran `objdump
>>> --headers` and looked at the size of the .rodata section, where it's
>>> also the same. I'm not quite sure what to make of this, as it's not what
>>> I was expecting, but I think I tested it right. So maybe we're lucky
>>> here?
>>
>> I tried a little harder to get _some_ difference on 32-bit, and
>> managed to get one by doing i386_defconfig and then switching off
>> modules to make all M into Y, and then compared sizes:
>>
>> vmlinux: 25590780 -> 25598972, so a 0.032% increase.
>> bzImage: 8698944 -> 8699424, so a 0.0055% increase.
>>
>> So it does increase, ever so slightly, but a) on 32-bit, and b) a
>> super, super tiny amount.
>>
>> In other words, I still think this patch is very much a-okay. But very
>> eager to hear from Rafael on the approach.
> 
> Increasing the ACPI_ID_LEN value is fine with me, but the patch
> changelog is not entirely accurate.
> 
> The ACPI subsystem uses struct acpi_device_id mostly (if not only) for
> device ID matching and it is generally used for creating lists of ACPI
> device IDs in drivers (and allow/deny lists etc).  The device IDs
> extracted from the ACPI tables can be longer than ACPI_ID_LEN.
> 
> This means that drivers cannot match device IDs longer than 8
> characters (excluding the terminating 0), because the IDs in the lists
> used by them for ID matching cannot be longer than this and not
> because the ACPI subsystem is limited by that value.
>
diff mbox series

Patch

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 4bb71979a8fd..5da5d990ff58 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -211,7 +211,7 @@  struct css_device_id {
 	kernel_ulong_t driver_data;
 };
 
-#define ACPI_ID_LEN	9
+#define ACPI_ID_LEN	16
 
 struct acpi_device_id {
 	__u8 id[ACPI_ID_LEN];