linux-kernel - Re: [PATCH v3] driver core: fw_devlink: Stop trying to optimize cycle detection logic

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] [day] [month] [year] [list]

Message-ID: <922c3edf-c52c-42ef-a096-e02f74e01f85@ideasonboard.com>
Date: Mon, 6 Jan 2025 17:16:19 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Saravana Kannan <saravanak@...gle.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Francesco <francesco.dolcini@...adex.com>,
 Geert Uytterhoeven <geert@...ux-m68k.org>, kernel-team@...roid.com,
 linux-kernel@...r.kernel.org, Jai Luthra <jai.luthra@...asonboard.com>
Subject: Re: [PATCH v3] driver core: fw_devlink: Stop trying to optimize cycle
 detection logic

Hi Saravana,

On 30/10/2024 19:10, Saravana Kannan wrote:
> In attempting to optimize fw_devlink runtime, I introduced numerous cycle
> detection bugs by foregoing cycle detection logic under specific
> conditions. Each fix has further narrowed the conditions for optimization.
> 
> It's time to give up on these optimization attempts and just run the cycle
> detection logic every time fw_devlink tries to create a device link.
> 
> The specific bug report that triggered this fix involved a supplier fwnode
> that never gets a device created for it. Instead, the supplier fwnode is
> represented by the device that corresponds to an ancestor fwnode.
> 
> In this case, fw_devlink didn't do any cycle detection because the cycle
> detection logic is only run when a device link is created between the
> devices that correspond to the actual consumer and supplier fwnodes.
> 
> With this change, fw_devlink will run cycle detection logic even when
> creating SYNC_STATE_ONLY proxy device links from a device that is an
> ancestor of a consumer fwnode.
> 
> Reported-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
> Closes: https://lore.kernel.org/all/1a1ab663-d068-40fb-8c94-f0715403d276@ideasonboard.com/
> Fixes: 6442d79d880c ("driver core: fw_devlink: Improve detection of overlapping cycles")
> Tested-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
> Signed-off-by: Saravana Kannan <saravanak@...gle.com>
> ---
>   drivers/base/core.c | 55 ++++++++++++++++++++-------------------------
>   1 file changed, 24 insertions(+), 31 deletions(-)
> 
> Tomi,
> 
> Thanks for all the testing and debugging help! And do use
> post-init-providers even with this patch to improve ordering
> enforcement. I probably should change the cycle log from info to warn in
> a separate patch :)

I've had this patch in one of my WIP branches for a while. Today I 
started debugging a kmemleak in the branch, which I thought was related 
to my changes. However, it ended up being this patch.

One of the drivers touched in my WIP branch is 
drivers/media/i2c/ds90ub960.c. It does a i2c_new_client_device() and, at 
remove time, i2c_unregister_device().

Without this patch, with some debug prints, I see that the newly created 
i2c client has a kref count of 11, and when i2c_unregister_device() is 
called, I see kref dropping to 0 and i2c_client_dev_release() being called.

With this patch, after i2c_new_client_device(), I see a kref of 16, and 
after i2c_unregister_device(), a kref of 5 (and i2c_client_dev_release() 
not called).

So five new refs were added, and not released... Any thoughts?

  Tomi