Re: [PATCH] kcm: fix socket memory accounting leak on TX completion, error, and close

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The commit message's stated bug does not exist, and roughly half
  of the patch is dead code.

--

kcm: fix socket memory accounting leak on TX completion, error, and close

The patch replaces the direct sk->sk_wmem_queued arithmetic in
kcm_write_msgs() with sk_wmem_queued_add(), and adds sk_wmem_queued_add()
plus sk_mem_uncharge() calls wherever TX skbs are freed in the
kcm_sendmsg() out_error path and in kcm_release().  It also adds NULL
checks on head before freeing it in kcm_sendmsg().

> diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> index 71af69d442f2..accf0d427e11 100644
> --- a/net/kcm/kcmsock.c
> +++ b/net/kcm/kcmsock.c
> @@ -693,7 +693,8 @@ static int kcm_write_msgs(struct kcm_sock *kcm)
>  		}
>  
>  		/* Successfully sent the whole packet, account for it. */
> -		sk->sk_wmem_queued -= txm->sent;
> +		sk_wmem_queued_add(sk, -txm->sent);
> +		sk_mem_uncharge(sk, txm->sent);
>  		total_sent += txm->sent;
>  		skb_dequeue(&sk->sk_write_queue);
>  		kfree_skb(head);

[Severity: High]
Are the sk_mem_uncharge() calls added by this patch reachable at all on a
KCM socket?  kcm_proto declares no .memory_allocated:

net/kcm/kcmsock.c:
static struct proto kcm_proto = {
	.name	= "KCM",
	.owner	= THIS_MODULE,
	.obj_size = sizeof(struct kcm_sock),
};

and the accounting helpers are gated on that field:

include/net/sock.h:
static inline bool sk_has_account(struct sock *sk)
{
	return !!sk->sk_prot->memory_allocated;
}

static inline void sk_mem_uncharge(struct sock *sk, int size)
{
	if (!sk_has_account(sk))
		return;
	sk_forward_alloc_add(sk, size);
	sk_mem_reclaim(sk);
}

If sk_has_account() is always false for KCM, then the new
sk_mem_uncharge() in kcm_write_msgs(), the two in the kcm_sendmsg()
out_error path and the two in kcm_release() are all no-ops, and only the
sk_wmem_queued_add() half of the patch has any effect.  Would it make
sense to drop the sk_mem_uncharge() calls (or wire up real proto
accounting for KCM as a separate change)?

The same reasoning applies to the premise in the changelog:

  "every TX fragment is charged to both `sk->sk_wmem_queued` and
   `sk->sk_forward_alloc` (`sk_mem_charge()`)"

The sk_mem_charge() inside skb_copy_to_page_nocache() and the explicit one
in the MSG_SPLICE_PAGES branch of kcm_sendmsg() are gated by the same
sk_has_account() test, so is sk->sk_forward_alloc ever charged here?

And for claim 1:

  "When `kcm_write_msgs()` finishes transmitting `head`, it subtracts
   `txm->sent` from `sk->sk_wmem_queued` but never calls
   `sk_mem_uncharge(sk, txm->sent)`."

The line this hunk removes already did sk->sk_wmem_queued -= txm->sent, so
is anything leaked on the TX completion path, or is this hunk just a
cosmetic conversion to the WRITE_ONCE() wrapper?

> @@ -960,10 +961,17 @@ static int kcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>  		 */
>  		if (copied)
>  			goto partial_message;
> -		if (head != kcm->seq_skb)
> +		if (head && head != kcm->seq_skb) {
> +			sk_wmem_queued_add(sk, -head->len);
> +			sk_mem_uncharge(sk, head->len);
>  			kfree_skb(head);
> +		}
>  	} else {
> -		kfree_skb(head);
> +		if (head) {
> +			sk_wmem_queued_add(sk, -head->len);
> +			sk_mem_uncharge(sk, head->len);
> +			kfree_skb(head);
> +		}
>  		kcm->seq_skb = NULL;
>  	}
>  

These two hunks in kcm_sendmsg() look like the one place where a real
imbalance existed: head carries data already added to sk->sk_wmem_queued,
and the pre-patch code dropped it with a bare kfree_skb().

Since the patch carries a Fixes: tag and is a stable candidate, could the
changelog describe that symptom instead?  A leaked sk_wmem_queued count
permanently shrinks the send space reported to sk_stream_memory_free(),
sk_stream_wait_memory() and SIOCOUTQ, so a socket that repeatedly hits
out_error can end up blocking or returning -EAGAIN forever.  As written,
the described symptom is not reproducible.

> @@ -1695,13 +1704,21 @@ static int kcm_release(struct socket *sock)
>  
>  	lock_sock(sk);
>  	sock_orphan(sk);
> -	kfree_skb(kcm->seq_skb);
> +	if (kcm->seq_skb) {
> +		sk_wmem_queued_add(sk, -kcm->seq_skb->len);
> +		sk_mem_uncharge(sk, kcm->seq_skb->len);
> +		kfree_skb(kcm->seq_skb);
> +	}
>  
>  	/* Purge queue under lock to avoid race condition with tx_work trying
>  	 * to act when queue is nonempty. If tx_work runs after this point
>  	 * it will just return.
>  	 */
> -	__skb_queue_purge(&sk->sk_write_queue);
> +	while ((skb = __skb_dequeue(&sk->sk_write_queue)) != NULL) {
> +		sk_wmem_queued_add(sk, -skb->len);
> +		sk_mem_uncharge(sk, skb->len);
> +		kfree_skb(skb);
> +	}
>  
>  	release_sock(sk);
>  

Can the warning named in claim 2 of the changelog actually fire here?

  "triggering a `WARN_ON` in `inet_sock_destruct()` and `__sk_destruct()`
   when the KCM socket is closed"

The WARN_ON_ONCE(sk->sk_wmem_queued) / WARN_ON_ONCE(sk->sk_forward_alloc)
pair lives only in inet_sock_destruct() in net/ipv4/af_inet.c, and
kcm_create()/kcm_clone() install the destructor via sock_init_data(), which
leaves the empty sock_def_destruct() in net/core/sock.c as sk->sk_destruct
for PF_KCM.  __sk_destruct() itself has no wmem or forward_alloc warning.

Given that the socket is being torn down at this point, are the
adjustments in kcm_release() observable by anything?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919212853.3240951-1-benquike%40gmail.com