linux-kernel - Re: [PATCH v2] platform/x86/amd/hsmp: switch to use device_add

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Fri, 2 Feb 2024 18:29:53 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: naveenkrishna.chatradhi@....com, linux-kernel@...r.kernel.org,
 Carlos Bilbao <carlos.bilbao@....com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v2] platform/x86/amd/hsmp: switch to use
 device_add_groups()

Hi,

On 2/2/24 16:32, Greg Kroah-Hartman wrote:
> On Fri, Feb 02, 2024 at 08:49:39AM +0100, Hans de Goede wrote:
>> Hi Greg,
>>
>> On 2/2/24 03:44, Greg Kroah-Hartman wrote:
>>> The use of devm_*() functions works properly for when the device
>>> structure itself is dynamic, but the hsmp driver is attempting to have a
>>> local, static, struct device and then calls devm_() functions attaching
>>> memory to the device that will never be freed.
>>
>> As I mentioned in my reply to v1, this is not correct.
>>
>> There is a global data struct, but that holds a struct device
>> pointer, not the device struct.
> 
> Ooops, I misread that:
> 	static struct hsmp_plat_device plat_dev;
> was not the actual device struct anymore.
> 
>> The device itself is created with platform_device_alloc() +
>> platform_device_add() from module-init and it is removed
>> on module-exit by calling platform_device_unregister()
> 
> Ok, much better.
> 
>> So AFAICT this should keep using the devm_ variant to properly
>> cleanup the sysfs attributes.
> 
> This devm_ variant is odd, and should never have been created as the
> sysfs core always cleans up the sysfs attributes when a device is
> removed, there is no need for it (i.e. they do the same thing.)
> 
> That's why I want to get rid of it, it's pointless :)
> 
>> But what this really needs is to be converted to using
>> amd_hsmp_driver.driver.dev_groups rather then manually
>> calling devm_device_add_groups() I have already asked
>> Suma Hegde (AMD) to take a look at this.
> 
> The initial issue I saw with this is that these attributes are being
> created dynamically, so using dev_groups can be a bit harder.  The code
> paths here are twisty and not obvious as it seems to want to support
> devices of multiple types in the same codebase at the same time.
> 
> But yes, using dev_groups is ideal, and if that happens, I'm happy.
> It's just that there are now only 2 in-kernel users of
> devm_device_add_groups() and I have a patch series to get rid of the
> other one, and so this would be the last, hence my attention to this.
> 
> Again, moving from devm_device_add_groups() to device_add_groups() is a
> no-op from a functional standpoint, so this should be fine.

Ok, I was not aware that the core automatically cleans up
all the attributes anyways.

In that case this fine with me and I agree with merging this
so that you can entirely remove the  devm_ variant:

Reviewed-by: Hans de Goede <hdegoede@...hat.com>

Regards,

Hans