linux-kernel - RE: [PATCH] platform: release resource itself instead of resource tree

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, 26 Jul 2019 10:24:22 +0000
From:   "Chang, Junxiao" <junxiao.chang@...el.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "rafael@...nel.org" <rafael@...nel.org>,
        "Li, Lili" <lili.li@...el.com>
Subject: RE: [PATCH] platform: release resource itself instead of resource
 tree

Hi Greg,

Thank you for looking at it.

Below example is simplified description. Our detail problem is:
1. ACPI driver registers a MEM resource during bootup;
2. Our PUNIT(Intel CPU power management module) platform device reads ACPI driver's resource, and registers same MEM resource;
3. According to current resource management logic, if two resources are same, later registered resource will be parent. That is, PUNIT platform device's resource will be ACPI driver resource's parent.
4. PUNIT kernel module is removed, its resource will be removed. If we use original API "release_resource", ACPI driver's resource will be released as well because it is PUNIT device's child;
5. Next time PUNIT platform device kernel module is inserted, it might read wrong ACPI MEM resource because it has been released in step 4.

How should we handle this case? :) 
We should not register same MEM resource in step 2? Or, make change in resource management logic, if two resources are same, later registered resource should be child instead of parent?

Thanks,
Junxiao

-----Original Message-----	
From: Greg KH [mailto:gregkh@...uxfoundation.org] 
Sent: Thursday, July 25, 2019 9:39 PM
To: Chang, Junxiao <junxiao.chang@...el.com>
Cc: linux-kernel@...r.kernel.org; rafael@...nel.org; Li, Lili <lili.li@...el.com>
Subject: Re: [PATCH] platform: release resource itself instead of resource tree

On Thu, Apr 25, 2019 at 02:24:18PM +0800, junxiao.chang@...el.com wrote:
> From: Junxiao Chang <junxiao.chang@...el.com>
> 
> When platform device is deleted or there is error in adding device, 
> platform device resources should be released. Currently API 
> release_resource is used to release platform device resources.
> However, this API releases not only platform resource itself but also 
> its child resources. It might release resources which are still in 
> use. Calling remove_resource only releases current resource itself, 
> not resource tree, it moves its child resources to up level.

But shouldn't the parent device not get removed until all of the children are removed?  What is causing this "inversion" to happen?

> 
> For example, platform device 1 and device 2 are registered, then only 
> device 1 is unregistered in below code:
> 
>   ...
>   // Register platform test device 1, resource 0xfed1a000 ~ 0xfed1afff
>   pdev1 = platform_device_register_full(&pdevinfo1);
> 
>   // Register platform test device 2, resource 0xfed1a200 ~ 0xfed1a2ff
>   pdev2 = platform_device_register_full(&pdevinfo2);
> 
>   // Now platform device 2 resource should be device 1 resource's 
> child
> 
>   // Unregister device 1 only
>   platform_device_unregister(pdev1);
>   ...

Don't do that.  :)

You created this mess of platform devices, so you need to keep track of them.

> Platform device 2 resource will be released as well because its parent 
> resource(device 1's resource) is released, this is not expected.
> If using API remove_resource, device 2 resource will not be released.
> 
> This change fixed an intel pmc platform device resource issue when 
> intel pmc ipc kernel module is inserted/removed for twice.

Why not fix that kernel module instead?  It seems like that is the real problem here, not a driver core issue.

thanks,

greg k-h