Re: [PATCH] bpf, sockmap: keep sk_msg copy state in sync

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf_msg_pull_data() leaves stale copy bits for collapsed scatterlist elements when they are not overwritten by trailing elements.
--

commit a47d507f1f74e146090ec75c8b70147c2a08cf57
Author: Zhang Cen <rollkingzzc@gmail.com>

bpf, sockmap: keep sk_msg copy state in sync

This patch updates BPF sk_msg helpers to keep the parallel msg->sg.copy
bitmap in sync when scatterlist entries are shifted, split, or compacted.
It preserves the copy bit for split tail entries and clears it whenever a
helper replaces an entry with a freshly allocated private page.

> diff --git a/net/core/filter.c b/net/core/filter.c
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -2738,6 +2751,8 @@ BPF_CALL_4(bpf_msg_pull_data, struct sk_msg *, msg, u32, start,
>  	} while (i != last_sge);
>  
>  	sg_set_page(&msg->sg.data[first_sge], page, copy, 0);
> +	sk_msg_set_elem_copy(msg, first_sge, false);
> +	sk_msg_set_elem_copy(msg, msg->sg.end, false);
>  
>  	/* To repair sg ring we need to shift entries. If we only
>  	 * had a single entry though we can just replace it and
> @@ -2754,6 +2769,7 @@ BPF_CALL_4(bpf_msg_pull_data, struct sk_msg *, msg, u32, start,
>  	sk_msg_iter_var_next(i);
>  	do {
>  		u32 move_from;
> +		bool move_copy;
>  
>  		if (i + shift >= NR_MSG_FRAG_IDS)
>  			move_from = i + shift - NR_MSG_FRAG_IDS;
> @@ -2762,10 +2778,13 @@ BPF_CALL_4(bpf_msg_pull_data, struct sk_msg *, msg, u32, start,
>  		if (move_from == msg->sg.end)
>  			break;

If the collapsed region is at the end of the message (where last_sge ==
msg->sg.end), will this loop break immediately on the first iteration before
clearing the copy bits for the interior slots (from first_sge + 1 to
last_sge - 1)?

If these vacated slots retain their stale copy bits and are later reused
for newly allocated private memory, could they inherit copy == true and
cause sk_msg_compute_data_pointers() to set msg->data = NULL, incorrectly
denying the BPF program direct write access?

> +		move_copy = sk_msg_elem_is_copy(msg, move_from);
>  		msg->sg.data[i] = msg->sg.data[move_from];
> +		sk_msg_set_elem_copy(msg, i, move_copy);
>  		msg->sg.data[move_from].length = 0;
>  		msg->sg.data[move_from].page_link = 0;
>  		msg->sg.data[move_from].offset = 0;
> +		sk_msg_set_elem_copy(msg, move_from, false);
>  		sk_msg_iter_var_next(i);
>  	} while (1);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516164319.1519418-1-rollkingzzc@gmail.com?part=1