linux-kernel - Re: [PATCH v6.1 05/27] af_unix: Replace BUG_ON() with WARN_ON

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: <20250523221418.6de8c601@pumpkin>
Date: Fri, 23 May 2025 22:14:18 +0100
From: David Laight <david.laight.linux@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Christian Brauner <brauner@...nel.org>, Kuniyuki
 Iwashima <kuniyu@...zon.com>, Alexander Mikhalitsyn
 <aleksandr.mikhalitsyn@...onical.com>, Jens Axboe <axboe@...nel.dk>, Sasha
 Levin <sashal@...nel.org>, Michal Luczaj <mhal@...x.co>, Rao Shoaib
 <Rao.Shoaib@...cle.com>, Simon Horman <horms@...nel.org>,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH v6.1 05/27] af_unix: Replace BUG_ON() with
 WARN_ON_ONCE().

On Wed, 21 May 2025 16:27:04 +0100
Lee Jones <lee@...nel.org> wrote:

> From: Kuniyuki Iwashima <kuniyu@...zon.com>
> 
> [ Upstream commit d0f6dc26346863e1f4a23117f5468614e54df064 ]
> 
> This is a prep patch for the last patch in this series so that
> checkpatch will not warn about BUG_ON().

Does any of this actually make any sense?
Either the BUG_ON() should be just deleted because it can't happen
(or doesn't matter) or there should be an error path.
Blindly replacing with WARN_ON_ONCE() can't be right.

The last change (repeated here)
>  	if (u) {
> -		BUG_ON(!u->inflight);
> -		BUG_ON(list_empty(&u->link));
> +		WARN_ON_ONCE(!u->inflight);
> +		WARN_ON_ONCE(list_empty(&u->link));
>  
>  		u->inflight--;
>  		if (!u->inflight)
is clearly just plain wrong.
If 'inflight' is zero then 'decrementing' it to ~0 is just going
to 'crash and burn' very badly not much later on.

	David

> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> Acked-by: Jens Axboe <axboe@...nel.dk>
> Link: https://lore.kernel.org/r/20240129190435.57228-2-kuniyu@amazon.com
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> (cherry picked from commit d0f6dc26346863e1f4a23117f5468614e54df064)
> Signed-off-by: Lee Jones <lee@...nel.org>
> ---
>  net/unix/garbage.c | 8 ++++----
>  net/unix/scm.c     | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> index 2934d7b68036..7eeaac165e85 100644
> --- a/net/unix/garbage.c
> +++ b/net/unix/garbage.c
> @@ -145,7 +145,7 @@ static void scan_children(struct sock *x, void (*func)(struct unix_sock *),
>  			/* An embryo cannot be in-flight, so it's safe
>  			 * to use the list link.
>  			 */
> -			BUG_ON(!list_empty(&u->link));
> +			WARN_ON_ONCE(!list_empty(&u->link));
>  			list_add_tail(&u->link, &embryos);
>  		}
>  		spin_unlock(&x->sk_receive_queue.lock);
> @@ -224,8 +224,8 @@ static void __unix_gc(struct work_struct *work)
>  
>  		total_refs = file_count(sk->sk_socket->file);
>  
> -		BUG_ON(!u->inflight);
> -		BUG_ON(total_refs < u->inflight);
> +		WARN_ON_ONCE(!u->inflight);
> +		WARN_ON_ONCE(total_refs < u->inflight);
>  		if (total_refs == u->inflight) {
>  			list_move_tail(&u->link, &gc_candidates);
>  			__set_bit(UNIX_GC_CANDIDATE, &u->gc_flags);
> @@ -318,7 +318,7 @@ static void __unix_gc(struct work_struct *work)
>  		list_move_tail(&u->link, &gc_inflight_list);
>  
>  	/* All candidates should have been detached by now. */
> -	BUG_ON(!list_empty(&gc_candidates));
> +	WARN_ON_ONCE(!list_empty(&gc_candidates));
>  
>  	/* Paired with READ_ONCE() in wait_for_unix_gc(). */
>  	WRITE_ONCE(gc_in_progress, false);
> diff --git a/net/unix/scm.c b/net/unix/scm.c
> index 693817a31ad8..6f446dd2deed 100644
> --- a/net/unix/scm.c
> +++ b/net/unix/scm.c
> @@ -50,10 +50,10 @@ void unix_inflight(struct user_struct *user, struct file *fp)
>  
>  	if (u) {
>  		if (!u->inflight) {
> -			BUG_ON(!list_empty(&u->link));
> +			WARN_ON_ONCE(!list_empty(&u->link));
>  			list_add_tail(&u->link, &gc_inflight_list);
>  		} else {
> -			BUG_ON(list_empty(&u->link));
> +			WARN_ON_ONCE(list_empty(&u->link));
>  		}
>  		u->inflight++;
>  		/* Paired with READ_ONCE() in wait_for_unix_gc() */
> @@ -70,8 +70,8 @@ void unix_notinflight(struct user_struct *user, struct file *fp)
>  	spin_lock(&unix_gc_lock);
>