linux-kernel - Re: [PATCH] HID: ensure timely release of driver-allocated resources

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:   Tue, 23 May 2023 15:57:03 +0200 (CEST)
From:   Jiri Kosina <jikos@...nel.org>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
cc:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Stephen Boyd <swboyd@...omium.org>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: ensure timely release of driver-allocated
 resources

On Fri, 5 May 2023, Dmitry Torokhov wrote:

> More and more drivers rely on devres to manage their resources, however
> if bus' probe() and release() methods are not trivial and control some
> of resources as well (for example enable or disable clocks, or attach
> device to a power domain), we need to make sure that driver-allocated
> resources are released immediately after driver's remove() method
> returns, and not postponed until driver core gets around to releasing
> resources.
> 
> In case of HID we should not try to close the report and release
> associated memory until after all devres callbacks are executed. To fix
> that we open a new devres group before calling driver's probe() and
> explicitly release it when we return from driver's remove().
> 
> This is similar to what we did for I2C bus in commit 5b5475826c52 ("i2c:
> ensure timely release of driver-allocated resources"). It is tempting to
> try and move this into driver core, but actually doing so is challenging,
> we need to split bus' remove() method into pre- and post-remove methods,
> which would make the logic even less clear.
> 
> Reported-by: Stephen Boyd <swboyd@...omium.org>
> Link: https://lore.kernel.org/r/20230505232417.1377393-1-swboyd@chromium.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> ---
>  drivers/hid/hid-core.c | 55 ++++++++++++++++++++++++++++--------------
>  include/linux/hid.h    |  1 +
>  2 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index c4ac9081194c..02a43bba9091 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2602,35 +2602,29 @@ static bool hid_device_check_match(struct hid_device *hdev,
>  	return !hid_ignore_special_drivers;
>  }
>  
> -static int hid_device_probe(struct device *dev)
> +static int __hid_device_probe(struct hid_device *hdev)
>  {
> -	struct hid_driver *hdrv = to_hid_driver(dev->driver);
> -	struct hid_device *hdev = to_hid_device(dev);
> +	struct hid_driver *hdrv = to_hid_driver(hdev->dev.driver);
>  	const struct hid_device_id *id;
>  	int ret;
>  
> -	if (down_interruptible(&hdev->driver_input_lock)) {
> -		ret = -EINTR;
> -		goto end;
> -	}
>  	hdev->io_started = false;
> -
>  	clear_bit(ffs(HID_STAT_REPROBED), &hdev->status);
>  
> -	if (hdev->driver) {
> -		ret = 0;
> -		goto unlock;
> -	}
> +	if (hdev->driver)
> +		return 0;
>  
> -	if (!hid_device_check_match(hdev, hdrv, &id)) {
> -		ret = -ENODEV;
> -		goto unlock;
> -	}

Dmitry, which tree is this patch against, please? The code above looks 
different in current tree (and hasn't been touched for a while).

Thanks,

-- 
Jiri Kosina
SUSE Labs