linux-kernel - Re: [PATCH] drivers/base: fix iterator cleanup in attribute

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: <2025022606-carrousel-unstable-f2f3@gregkh>
Date: Wed, 26 Feb 2025 11:16:21 -0800
From: Greg KH <gregkh@...uxfoundation.org>
To: Ayaan Mirza Baig <ayaanmirzabaig85@...il.com>
Cc: dakr@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers/base: fix iterator cleanup in attribute_container

On Wed, Feb 26, 2025 at 11:57:52PM +0530, Ayaan Mirza Baig wrote:
> This patch addresses a long-standing FIXME in
> dirvers/base/attribute_container.c, where the function
> failed to call klist_iter_exit() before breaking
> out of the loop when a matching class device was found.
> 
> The code is now refactored to use a goto-based cleanup approach
> that ensure klist_iter_exit() is invoked exactly once regardless
> of the exit path.
> 
> In addition, checkpatch.pl warnings were fixed along with this change.
> 
> Tested on a minimal Debian System in QEMU with GDB attached.
> No regressions or errors were observed.
> 
> Signed-off-by: Ayaan Mirza Baig <ayaanmirzabaig85@...il.com>
> ---
>  drivers/base/attribute_container.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/base/attribute_container.c b/drivers/base/attribute_container.c
> index b6f941a6ab69..1e7af328dd49 100644
> --- a/drivers/base/attribute_container.c
> +++ b/drivers/base/attribute_container.c
> @@ -5,7 +5,7 @@
>   * Copyright (c) 2005 - James Bottomley <James.Bottomley@...eleye.com>
>   *
>   * The basic idea here is to enable a device to be attached to an
> - * aritrary numer of classes without having to allocate storage for them.
> + * aritrary number of classes without having to allocate storage for them.

Nit, why change this in this commit?

>   * Instead, the contained classes select the devices they need to attach
>   * to via a matching function.
>   */
> @@ -21,7 +21,8 @@
>  #include "base.h"
>  
>  /* This is a private structure used to tie the classdev and the
> - * container .. it should never be visible outside this file */
> + * container .. it should never be visible outside this file
> + */

Same for this one?
>  struct internal_container {
>  	struct klist_node node;
>  	struct attribute_container *cont;
> @@ -42,7 +43,6 @@ static void internal_container_klist_put(struct klist_node *n)
>  	put_device(&ic->classdev);
>  }
>  
> -
>  /**

And this?

I stopped reading here, please, if you are going to make logic changes,
just do that, don't mix things up in one larger commit, that's not how
we handle kernel patches at all.

Also, did you test a system that uses container devices?

thanks,

greg k-h