linux-kernel - Re: Convert type of 'struct dmi_system_id -> driver_data' from 'void *' to kernel_ulong

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Jan 2022 11:20:36 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Kai-Heng Feng <kai.heng.feng@...onical.com>,
        LKML <linux-kernel@...r.kernel.org>
Cc:     jkosina@...e.cz, Jason Gunthorpe <jgg@...pe.ca>,
        Bjorn Helgaas <bhelgaas@...gle.com>, mgurtovoy@...dia.com,
        linux@...ssschuh.net, Arnd Bergmann <arnd@...db.de>,
        stephan@...hold.net, "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: Convert type of 'struct dmi_system_id -> driver_data' from 'void
 *' to kernel_ulong_t?

Hi Kai-Heng,

On 1/19/22 02:22, Kai-Heng Feng wrote:
> Hi,
> 
> I wonder if there's any reason to use 'void *' instead of
> kernel_ulong_t for 'driver_data' in 'struct dmi_system_id'?
> 
> I'd like to use the driver_data for applying quirk flags, and I found
> out unlike most other struct *_id, the dmi variant is using 'void *'
> for driver_data. Is there any technical reason for this?

I don't know if there is a technical reason for this, nor
why this choice was originally made.

But I do believe that changing this now will just lead to a lot
of unnecessary churn.

You can easily use the current void * for flags by doing;

#define FLAG1	BIT(0)
#define FLAG2	BIT(1)

...
		.driver_data = (void *)(FLAG1 | FLAG20,

...

	long quirks = (long)dmi_id->driver_data;

It is already used this way in many places. Have you done a grep
to see in how many places dmi_system_id is used? DMI based quirks
are used all over the place, changing this will cause a really
large amount of churn and for what?

So I just did a quick check and dmi_system_id is used in
*204* files in the kernel.

You are asking for a technical reason why "void *" was used,
but lets turn that around, why do you believe that "unsigned long"
is inherently a better type here ?

driver_data in most places in the kernel (like data for
all sort of callback functions) actually typically is a void *
already, because often people want to pass more data then what
fits in a single long and this also applies to driver-id attached
data.

Just a random example from: drivers/mmc/host/sdhci-pci.h

#define SDHCI_PCI_DEVICE(vend, dev, cfg) { \
        .vendor = _PCI_VEND(vend), .device = _PCI_DEV(vend, dev), \
        .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, \
        .driver_data = (kernel_ulong_t)&(sdhci_##cfg) \
}

So here the unsigned long is actually not what we want and
we have to do a cast the other way around; and again the
kernel is full of these examples. So arguably if anything we
should change the other driver_data fields from
include/linux/mod_devicetable.h
to avoid the need for these kinda casts all over the kernel
(but again that would not be worth the churn IMHO).

TL;DR: lets just leave this all as it is please.

Regards,

Hans