[v1,bpf-next,3/5] bpf: udp: Propagate ENOMEM up from bpf_iter_udp_batch

Commit Message

Jordan Rife April 9, 2025, 6:22 p.m. UTC

Stop iteration if the current bucket can't be contained in a single
batch to avoid choosing between skipping or repeating sockets. In cases
where none of the saved cookies can be found in the current bucket and
the batch isn't big enough to contain all the sockets in the bucket,
there are really only two choices, neither of which is desirable:

1. Start from the beginning, assuming we haven't seen any sockets in the
   current bucket, in which case we might repeat a socket we've already
   seen.
2. Go to the next bucket to avoid repeating a socket we may have already
   seen, in which case we may accidentally skip a socket that we didn't
   yet visit.

To avoid this tradeoff, enforce the invariant that the batch always
contains a full snapshot of the bucket from last time by returning
-ENOMEM if bpf_iter_udp_realloc_batch() can't grab enough memory to fit
all sockets in the current bucket.

To test this code path, I forced bpf_iter_udp_realloc_batch() to return
-ENOMEM when called from within bpf_iter_udp_batch() and observed that
read() fails in userspace with errno set to ENOMEM. Otherwise, it's a
bit hard to test this scenario.

Link: https://lore.kernel.org/bpf/CABi4-ogUtMrH8-NVB6W8Xg_F_KDLq=[e-mail]/

Signed-off-by: Jordan Rife <[e-mail]>
---
 net/ipv4/udp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Kuniyuki Iwashima April 10, 2025, 8:27 p.m. UTC | #1

From: Jordan Rife <[e-mail]>
Date: Wed,  9 Apr 2025 11:22:32 -0700
> Stop iteration if the current bucket can't be contained in a single
> batch to avoid choosing between skipping or repeating sockets. In cases
> where none of the saved cookies can be found in the current bucket and
> the batch isn't big enough to contain all the sockets in the bucket,
> there are really only two choices, neither of which is desirable:
> 
> 1. Start from the beginning, assuming we haven't seen any sockets in the
>    current bucket, in which case we might repeat a socket we've already
>    seen.
> 2. Go to the next bucket to avoid repeating a socket we may have already
>    seen, in which case we may accidentally skip a socket that we didn't
>    yet visit.
> 
> To avoid this tradeoff, enforce the invariant that the batch always
> contains a full snapshot of the bucket from last time by returning
> -ENOMEM if bpf_iter_udp_realloc_batch() can't grab enough memory to fit
> all sockets in the current bucket.
> 
> To test this code path, I forced bpf_iter_udp_realloc_batch() to return
> -ENOMEM when called from within bpf_iter_udp_batch() and observed that
> read() fails in userspace with errno set to ENOMEM. Otherwise, it's a
> bit hard to test this scenario.
> 
> Link: https://lore.kernel.org/bpf/CABi4-ogUtMrH8-NVB6W8Xg_F_KDLq=[e-mail]/
> 
> Signed-off-by: Jordan Rife <[e-mail]>

I felt this patch is patch 2 but have no strong preference.

Reviewed-by: Kuniyuki Iwashima <[e-mail]>

Jordan Rife April 10, 2025, 8:44 p.m. UTC | #2

> I felt this patch is patch 2 but have no strong preference.

Funnily enough, I originally had this as patch 2 but moved it after
much indecision :). I will reorder this in v2.

-Jordan

Patch

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index f6a579d61717..de58dae6ff3c 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -3429,6 +3429,7 @@  static struct sock *bpf_iter_udp_batch(struct seq_file *seq)
 	bool resized = false;
 	int resume_bucket;
 	struct sock *sk;
+	int err = 0;
 
 	resume_bucket = state->bucket;
 
@@ -3503,7 +3504,11 @@  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)) {
+	if (!resized) {
+		err = bpf_iter_udp_realloc_batch(iter, batch_sks * 3 / 2);
+		if (err)
+			return ERR_PTR(err);
+
 		resized = true;
 		/* After allocating a larger batch, retry one more time to grab
 		 * the whole bucket.