netdev - Re: [PATCH v3 bpf-next 2/6] bpf: udp: Make sure iter->batch always contains a full bucket snapshot

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]

Message-ID: <20250417234511.39315-1-kuniyu@amazon.com>
Date: Thu, 17 Apr 2025 16:45:04 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <jordan@...fe.io>
CC: <aditi.ghag@...valent.com>, <bpf@...r.kernel.org>, <daniel@...earbox.net>,
	<kuniyu@...zon.com>, <martin.lau@...ux.dev>, <netdev@...r.kernel.org>,
	<willemdebruijn.kernel@...il.com>
Subject: Re: [PATCH v3 bpf-next 2/6] bpf: udp: Make sure iter->batch always contains a full bucket snapshot

From: Jordan Rife <jordan@...fe.io>
Date: Wed, 16 Apr 2025 16:36:17 -0700
> @@ -3454,15 +3460,26 @@ static struct sock *bpf_iter_udp_batch(struct seq_file *seq)
>  				batch_sks++;
>  			}
>  		}
> -		spin_unlock_bh(&hslot2->lock);
>  
>  		if (iter->end_sk)
>  			break;
> +next_bucket:
> +		/* Somehow the bucket was emptied or all matching sockets were
> +		 * removed while we held onto its lock. This should not happen.
> +		 */
> +		if (WARN_ON_ONCE(!resizes))
> +			/* Best effort; reset the resize budget and move on. */
> +			resizes = MAX_REALLOC_ATTEMPTS;
> +		if (lock)
> +			spin_unlock_bh(lock);
> +		lock = NULL;
>  	}
>  
>  	/* All done: no batch made. */
>  	if (!iter->end_sk)
> -		return NULL;
> +		goto done;

If we jump here when no UDP socket exists, uninitialised sk is returned.
Maybe move this condition down below the sk initialisation.


> +
> +	sk = iter->batch[0];
>  
>  	if (iter->end_sk == batch_sks) {
>  		/* Batching is done for the current bucket; return the first
> @@ -3471,16 +3488,30 @@ static struct sock *bpf_iter_udp_batch(struct seq_file *seq)
>  		iter->st_bucket_done = true;
>  		goto done;
>  	}
> -	if (!resized && !bpf_iter_udp_realloc_batch(iter, batch_sks * 3 / 2,
> -						    GFP_USER)) {
> -		resized = true;
> -		/* After allocating a larger batch, retry one more time to grab
> -		 * the whole bucket.
> -		 */
> -		goto again;
> +
> +	/* Somehow the batch size still wasn't big enough even though we held
> +	 * a lock on the bucket. This should not happen.
> +	 */
> +	if (WARN_ON_ONCE(!resizes))
> +		goto done;
> +
> +	resizes--;
> +	if (resizes) {
> +		spin_unlock_bh(lock);
> +		lock = NULL;
> +	}
> +	err = bpf_iter_udp_realloc_batch(iter, batch_sks * 3 / 2,
> +					 resizes ? GFP_USER : GFP_ATOMIC);
> +	if (err) {
> +		sk = ERR_PTR(err);
> +		goto done;
>  	}
> +
> +	goto again;
>  done:
> -	return iter->batch[0];
> +	if (lock)
> +		spin_unlock_bh(lock);
> +	return sk;
>  }
>