When we specify apply_bytes, we divide the msg into multiple segments,
each with a length of 'send', and every time we send this part of the data
using tcp_bpf_sendmsg_redir(), we use sk_msg_return_zero() to uncharge the
memory of the specified 'send' size.

However, if the first segment of data fails to send, for example, the
peer's buffer is full, we need to release all of the msg. When releasing
the msg, we haven't uncharged the memory of the subsequent segments.

This modification does not make significant logical changes, but only
fills in the missing uncharge places.

This issue has existed all along, until it was exposed after we added the
apply test in test_sockmap:
commit 3448ad23b34e ("selftests/bpf: Add apply_bytes test to test_txmsg_redir_wait_sndmem in test_sockmap")

Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
Closes: https://lore.kernel.org/bpf/aAmIi0vlycHtbXeb@pop-os.localdomain/T/#t
Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/tls/tls_sw.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index f3d7d19482da..fc88e34b7f33 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -908,6 +908,13 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
 					    &msg_redir, send, flags);
 		lock_sock(sk);
 		if (err < 0) {
+			/* Regardless of whether the data represented by
+			 * msg_redir is sent successfully, we have already
+			 * uncharged it via sk_msg_return_zero(). The
+			 * msg->sg.size represents the remaining unprocessed
+			 * data, which needs to be uncharged here.
+			 */
+			sk_mem_uncharge(sk, msg->sg.size);
 			*copied -= sk_msg_free_nocharge(sk, &msg_redir);
 			msg->sg.size = 0;
 		}
-- 
2.47.1

On Fri, Apr 25, 2025 at 01:59:57PM +0800, Jiayuan Chen wrote:
> When we specify apply_bytes, we divide the msg into multiple segments,
> each with a length of 'send', and every time we send this part of the data
> using tcp_bpf_sendmsg_redir(), we use sk_msg_return_zero() to uncharge the
> memory of the specified 'send' size.
> 
> However, if the first segment of data fails to send, for example, the
> peer's buffer is full, we need to release all of the msg. When releasing
> the msg, we haven't uncharged the memory of the subsequent segments.
> 
> This modification does not make significant logical changes, but only
> fills in the missing uncharge places.
> 
> This issue has existed all along, until it was exposed after we added the
> apply test in test_sockmap:
> commit 3448ad23b34e ("selftests/bpf: Add apply_bytes test to test_txmsg_redir_wait_sndmem in test_sockmap")
> 
> Reported-by: Cong Wang <xiyou.wangcong@gmail.com>

Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com>

Thanks!

On Fri, Apr 25, 2025 at 01:59:57PM +0800, Jiayuan Chen wrote:
>  net/tls/tls_sw.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index f3d7d19482da..fc88e34b7f33 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -908,6 +908,13 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
>  					    &msg_redir, send, flags);
>  		lock_sock(sk);
>  		if (err < 0) {
> +			/* Regardless of whether the data represented by
> +			 * msg_redir is sent successfully, we have already
> +			 * uncharged it via sk_msg_return_zero(). The
> +			 * msg->sg.size represents the remaining unprocessed
> +			 * data, which needs to be uncharged here.
> +			 */
> +			sk_mem_uncharge(sk, msg->sg.size);
>  			*copied -= sk_msg_free_nocharge(sk, &msg_redir);

Equivalent to sk_msg_free() ?

Thanks.

2025/4/29 08:14, "Cong Wang" <xiyou.wangcong@gmail.com> wrote:

> 
> On Fri, Apr 25, 2025 at 01:59:57PM +0800, Jiayuan Chen wrote:
> 
> > 
> > net/tls/tls_sw.c | 7 +++++++
> > 
> >  1 file changed, 7 insertions(+)
> > 
> >  
> > 
> >  diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > 
> >  index f3d7d19482da..fc88e34b7f33 100644
> > 
> >  --- a/net/tls/tls_sw.c
> > 
> >  +++ b/net/tls/tls_sw.c
> > 
> >  @@ -908,6 +908,13 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
> > 
> >  &msg_redir, send, flags);
> > 
> >  lock_sock(sk);
> > 
> >  if (err < 0) {
> > 
> >  + /* Regardless of whether the data represented by
> > 
> >  + * msg_redir is sent successfully, we have already
> > 
> >  + * uncharged it via sk_msg_return_zero(). The
> > 
> >  + * msg->sg.size represents the remaining unprocessed
> > 
> >  + * data, which needs to be uncharged here.
> > 
> >  + */
> > 
> >  + sk_mem_uncharge(sk, msg->sg.size);
> > 
> >  *copied -= sk_msg_free_nocharge(sk, &msg_redir);
> > 
> 
> Equivalent to sk_msg_free() ?
> 
> Thanks.
>

Before calling tcp_bpf_sendmsg_redir(), we have already uncharged some
memory using sk_msg_return_zero(). If we perform sk_msg_free(msg_redir),
it will cause the duplicate uncharge of this part of data. If we perform
sk_msg_free(msg), since tcp_bpf_sendmsg_redir() may not have sent any data
and msg->sg.start no longer points to this part of data, it will lead to
memoryleak.

So, directly calling sk_msg_free is not a good idea.