Re: [PATCH net v2 1/2] tcp: diag: bound bucket lock hold in tcp_diag_dump()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing fallback to resume by `s_num` when the dump cursor is invalidated, causing dump restarts and duplicate socket messages.
--

commit 027317402fdfe4d3580f395cdc040206720287ef
Author: Zihan Xi <zihanx@nebusec.ai>

tcp: diag: bound bucket lock hold in tcp_diag_dump()

This commit limits the time spent under bucket locks in tcp_diag_dump() by
collecting only a batch of referenced sockets while holding the lock. It
introduces a cursor mechanism to resume dumps instead of restarting from
the bucket head.

Link: https://lore.kernel.org/all/cover.1785307984.git.zihanx@nebusec.ai/

> diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
> index ba1fdbe9807fd..842e13ee79e5b 100644
> --- a/net/ipv4/tcp_diag.c
> +++ b/net/ipv4/tcp_diag.c
[ ... ]
> @@ -344,52 +418,81 @@ static void tcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
>  				continue;
>  			}
>  			spin_lock(&ilb->lock);
> -			sk_nulls_for_each(sk, node, &ilb->nulls_head) {
> -				struct inet_sock *inet = inet_sk(sk);
[ ... ]
> +			node = use_cursor ? cursor->sk_nulls_node.next :
> +					    ilb->nulls_head.first;
> +			if (!use_cursor)
> +				s_num = 0;
> +			hlist_nulls_for_each_entry_from(sk, node, sk_nulls_node) {

[Severity: High]
If the cursor is invalidated (use_cursor evaluates to false), the iteration
restarts from the bucket head, but the fallback skip logic:

    if (num < s_num)
        continue;

appears to have been completely removed. By explicitly setting s_num to 0,
does this cause the dump to restart from the beginning of the bucket without
skipping previously processed sockets? 

If a large bucket has high churn, could this prevent the dump from progressing
past the first few batches and lead to an endless stream of duplicate sockets?

[ ... ]
> @@ -412,34 +517,46 @@ static void tcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
[ ... ]
> +			if (!use_cursor)
> +				s_num = 0;
> +			if (use_cursor) {
[ ... ]
> +			} else {
> +				inet_bind_bucket_for_each(tb2, &ibb->chain) {
> +					if (tcp_diag_bind_collect_owners(&tb2->owners,
> +									 sk_arr,
> +									 num_arr,
> +									 &accum,
> +									 &num, s_num))
> +						goto pause_bind_walk;
>  				}
>  			}

[Severity: High]
Similar to the listen walk, s_num is explicitly zeroed before being passed
into tcp_diag_bind_collect_owners() when use_cursor is false. 

Does this break the intended fallback skip mechanism and result in duplicate
messages or infinite loops if the cursor becomes invalid?

[ ... ]
> @@ -483,43 +620,35 @@ static void tcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
[ ... ]
> -		sk_nulls_for_each(sk, node, &head->chain) {
> -			int state;
> -
> -			if (!net_eq(sock_net(sk), net))
> -				continue;
> -			if (num < s_num)
> -				goto next_normal;
[ ... ]
> +		node = use_cursor ? cursor->sk_nulls_node.next : head->chain.first;
> +		if (!use_cursor)
> +			s_num = 0;
> +		hlist_nulls_for_each_entry_from(sk, node, sk_nulls_node) {

[Severity: High]
Here in the ehash walk, the original check for skipping elements
(goto next_normal) was removed, and s_num is zeroed if the cursor is missing
or invalid. 

Could this lead to the same duplicate dump issue if a socket pointed to by the
cursor is closed and unhashed between recvmsg() calls?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788248601.git.zihanx@nebusec.ai?part=1