Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak
@ 2021-07-12 19:55 John Fastabend
  2021-07-12 19:55 ` [PATCH bpf v4 1/2] bpf, sockmap: fix potential memory leak on unlikely error case John Fastabend
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: John Fastabend @ 2021-07-12 19:55 UTC (permalink / raw)
  To: ast, jakub, daniel, andriin, xiyou.wangcong; +Cc: bpf, netdev, john.fastabend

While investigating a memleak in sockmap I found these two issues. Patch
1 found doing code review, I wasn't able to get KASAN to trigger a
memleak here, but should be necessary. Patch 2 fixes proc stats so when
we use sockstats for debugging we get correct values.

The fix for observered memleak will come after these, but requires some
more discussion and potentially patch revert so I'll try to get the set
here going now.

v4: fix both users of sk_psock_skb_ingress_enqueue and then fix the
    inuse idx by moving init hook later after tcp/udp init calls.
v3: move kfree into same function as kalloc

John Fastabend (2):
  bpf, sockmap: fix potential memory leak on unlikely error case
  bpf, sockmap: sk_prot needs inuse_idx set for proc stats

 net/core/skmsg.c    | 16 +++++++++++-----
 net/core/sock_map.c | 11 ++++++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH bpf v4 1/2] bpf, sockmap: fix potential memory leak on unlikely error case
  2021-07-12 19:55 [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak John Fastabend
@ 2021-07-12 19:55 ` John Fastabend
  2021-07-14  0:35   ` Cong Wang
  2021-07-12 19:55 ` [PATCH bpf v4 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats John Fastabend
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: John Fastabend @ 2021-07-12 19:55 UTC (permalink / raw)
  To: ast, jakub, daniel, andriin, xiyou.wangcong; +Cc: bpf, netdev, john.fastabend

If skb_linearize is needed and fails we could leak a msg on the error
handling. To fix ensure we kfree the msg block before returning error.
Found during code review.

Fixes: 4363023d2668e ("bpf, sockmap: Avoid failures from skb_to_sgvec when skb has frag_list")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 net/core/skmsg.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 9b6160a191f8..15d71288e741 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -508,10 +508,8 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
 	if (skb_linearize(skb))
 		return -EAGAIN;
 	num_sge = skb_to_sgvec(skb, msg->sg.data, 0, skb->len);
-	if (unlikely(num_sge < 0)) {
-		kfree(msg);
+	if (unlikely(num_sge < 0))
 		return num_sge;
-	}
 
 	copied = skb->len;
 	msg->sg.start = 0;
@@ -530,6 +528,7 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb)
 {
 	struct sock *sk = psock->sk;
 	struct sk_msg *msg;
+	int err;
 
 	/* If we are receiving on the same sock skb->sk is already assigned,
 	 * skip memory accounting and owner transition seeing it already set
@@ -548,7 +547,10 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb)
 	 * into user buffers.
 	 */
 	skb_set_owner_r(skb, sk);
-	return sk_psock_skb_ingress_enqueue(skb, psock, sk, msg);
+	err = sk_psock_skb_ingress_enqueue(skb, psock, sk, msg);
+	if (err < 0)
+		kfree(msg);
+	return err;
 }
 
 /* Puts an skb on the ingress queue of the socket already assigned to the
@@ -559,12 +561,16 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
 {
 	struct sk_msg *msg = kzalloc(sizeof(*msg), __GFP_NOWARN | GFP_ATOMIC);
 	struct sock *sk = psock->sk;
+	int err;
 
 	if (unlikely(!msg))
 		return -EAGAIN;
 	sk_msg_init(msg);
 	skb_set_owner_r(skb, sk);
-	return sk_psock_skb_ingress_enqueue(skb, psock, sk, msg);
+	err = sk_psock_skb_ingress_enqueue(skb, psock, sk, msg);
+	if (err < 0)
+		kfree(msg);
+	return err;
 }
 
 static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH bpf v4 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats
  2021-07-12 19:55 [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak John Fastabend
  2021-07-12 19:55 ` [PATCH bpf v4 1/2] bpf, sockmap: fix potential memory leak on unlikely error case John Fastabend
@ 2021-07-12 19:55 ` John Fastabend
  2021-07-13  7:47   ` Jakub Sitnicki
  2021-07-14  0:56   ` Cong Wang
  2021-07-13  7:47 ` [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak Jakub Sitnicki
  2021-07-15 18:00 ` patchwork-bot+netdevbpf
  3 siblings, 2 replies; 8+ messages in thread
From: John Fastabend @ 2021-07-12 19:55 UTC (permalink / raw)
  To: ast, jakub, daniel, andriin, xiyou.wangcong; +Cc: bpf, netdev, john.fastabend

Proc socket stats use sk_prot->inuse_idx value to record inuse sock stats.
We currently do not set this correctly from sockmap side. The result is
reading sock stats '/proc/net/sockstat' gives incorrect values. The
socket counter is incremented correctly, but because we don't set the
counter correctly when we replace sk_prot we may omit the decrement.

To get the correct inuse_idx value move the core_initcall that initializes
the tcp/udp proto handlers to late_initcall. This way it is initialized
after TCP/UDP has the chance to assign the inuse_idx value from the
register protocol handler.

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 net/ipv4/tcp_bpf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index f26916a62f25..d3e9386b493e 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -503,7 +503,7 @@ static int __init tcp_bpf_v4_build_proto(void)
 	tcp_bpf_rebuild_protos(tcp_bpf_prots[TCP_BPF_IPV4], &tcp_prot);
 	return 0;
 }
-core_initcall(tcp_bpf_v4_build_proto);
+late_initcall(tcp_bpf_v4_build_proto);
 
 static int tcp_bpf_assert_proto_ops(struct proto *ops)
 {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v4 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats
  2021-07-12 19:55 ` [PATCH bpf v4 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats John Fastabend
@ 2021-07-13  7:47   ` Jakub Sitnicki
  2021-07-14  0:56   ` Cong Wang
  1 sibling, 0 replies; 8+ messages in thread
From: Jakub Sitnicki @ 2021-07-13  7:47 UTC (permalink / raw)
  To: John Fastabend; +Cc: ast, daniel, andriin, xiyou.wangcong, bpf, netdev

On Mon, Jul 12, 2021 at 09:55 PM CEST, John Fastabend wrote:
> Proc socket stats use sk_prot->inuse_idx value to record inuse sock stats.
> We currently do not set this correctly from sockmap side. The result is
> reading sock stats '/proc/net/sockstat' gives incorrect values. The
> socket counter is incremented correctly, but because we don't set the
> counter correctly when we replace sk_prot we may omit the decrement.
>
> To get the correct inuse_idx value move the core_initcall that initializes
> the tcp/udp proto handlers to late_initcall. This way it is initialized
> after TCP/UDP has the chance to assign the inuse_idx value from the
> register protocol handler.
>
> Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
> Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
>  net/ipv4/tcp_bpf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index f26916a62f25..d3e9386b493e 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -503,7 +503,7 @@ static int __init tcp_bpf_v4_build_proto(void)
>  	tcp_bpf_rebuild_protos(tcp_bpf_prots[TCP_BPF_IPV4], &tcp_prot);
>  	return 0;
>  }
> -core_initcall(tcp_bpf_v4_build_proto);
> +late_initcall(tcp_bpf_v4_build_proto);
>
>  static int tcp_bpf_assert_proto_ops(struct proto *ops)
>  {

Respective change for udp_bpf is missing. I've posted it separately [1]
to save us an iteration. Hope you don't mind.

[1] https://lore.kernel.org/bpf/20210713074401.475209-1-jakub@cloudflare.com/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak
  2021-07-12 19:55 [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak John Fastabend
  2021-07-12 19:55 ` [PATCH bpf v4 1/2] bpf, sockmap: fix potential memory leak on unlikely error case John Fastabend
  2021-07-12 19:55 ` [PATCH bpf v4 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats John Fastabend
@ 2021-07-13  7:47 ` Jakub Sitnicki
  2021-07-15 18:00 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 8+ messages in thread
From: Jakub Sitnicki @ 2021-07-13  7:47 UTC (permalink / raw)
  To: John Fastabend; +Cc: ast, daniel, andriin, xiyou.wangcong, bpf, netdev

On Mon, Jul 12, 2021 at 09:55 PM CEST, John Fastabend wrote:
> While investigating a memleak in sockmap I found these two issues. Patch
> 1 found doing code review, I wasn't able to get KASAN to trigger a
> memleak here, but should be necessary. Patch 2 fixes proc stats so when
> we use sockstats for debugging we get correct values.
>
> The fix for observered memleak will come after these, but requires some
> more discussion and potentially patch revert so I'll try to get the set
> here going now.
>
> v4: fix both users of sk_psock_skb_ingress_enqueue and then fix the
>     inuse idx by moving init hook later after tcp/udp init calls.
> v3: move kfree into same function as kalloc
>
> John Fastabend (2):
>   bpf, sockmap: fix potential memory leak on unlikely error case
>   bpf, sockmap: sk_prot needs inuse_idx set for proc stats
>
>  net/core/skmsg.c    | 16 +++++++++++-----
>  net/core/sock_map.c | 11 ++++++++++-
>  2 files changed, 21 insertions(+), 6 deletions(-)

For the series:

Acked-by: Jakub Sitnicki <jakub@cloudflare.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v4 1/2] bpf, sockmap: fix potential memory leak on unlikely error case
  2021-07-12 19:55 ` [PATCH bpf v4 1/2] bpf, sockmap: fix potential memory leak on unlikely error case John Fastabend
@ 2021-07-14  0:35   ` Cong Wang
  0 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2021-07-14  0:35 UTC (permalink / raw)
  To: John Fastabend
  Cc: Alexei Starovoitov, Jakub Sitnicki, Daniel Borkmann,
	Andrii Nakryiko, bpf, Linux Kernel Network Developers

On Mon, Jul 12, 2021 at 12:56 PM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> If skb_linearize is needed and fails we could leak a msg on the error
> handling. To fix ensure we kfree the msg block before returning error.
> Found during code review.
>
> Fixes: 4363023d2668e ("bpf, sockmap: Avoid failures from skb_to_sgvec when skb has frag_list")
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>

Reviewed-by: Cong Wang <cong.wang@bytedance.com>

Thanks for the update.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v4 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats
  2021-07-12 19:55 ` [PATCH bpf v4 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats John Fastabend
  2021-07-13  7:47   ` Jakub Sitnicki
@ 2021-07-14  0:56   ` Cong Wang
  1 sibling, 0 replies; 8+ messages in thread
From: Cong Wang @ 2021-07-14  0:56 UTC (permalink / raw)
  To: John Fastabend
  Cc: Alexei Starovoitov, Jakub Sitnicki, Daniel Borkmann,
	Andrii Nakryiko, bpf, Linux Kernel Network Developers

On Mon, Jul 12, 2021 at 12:56 PM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> Proc socket stats use sk_prot->inuse_idx value to record inuse sock stats.
> We currently do not set this correctly from sockmap side. The result is
> reading sock stats '/proc/net/sockstat' gives incorrect values. The
> socket counter is incremented correctly, but because we don't set the
> counter correctly when we replace sk_prot we may omit the decrement.
>
> To get the correct inuse_idx value move the core_initcall that initializes
> the tcp/udp proto handlers to late_initcall. This way it is initialized
> after TCP/UDP has the chance to assign the inuse_idx value from the
> register protocol handler.
>
> Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
> Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>

For IPv6, I think the module is always loaded before we can
trigger tcp_bpf_check_v6_needs_rebuild(). So,

Reviewed-by: Cong Wang <cong.wang@bytedance.com>

Thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak
  2021-07-12 19:55 [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak John Fastabend
                   ` (2 preceding siblings ...)
  2021-07-13  7:47 ` [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak Jakub Sitnicki
@ 2021-07-15 18:00 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-07-15 18:00 UTC (permalink / raw)
  To: John Fastabend; +Cc: ast, jakub, daniel, andriin, xiyou.wangcong, bpf, netdev

Hello:

This series was applied to bpf/bpf.git (refs/heads/master):

On Mon, 12 Jul 2021 12:55:44 -0700 you wrote:
> While investigating a memleak in sockmap I found these two issues. Patch
> 1 found doing code review, I wasn't able to get KASAN to trigger a
> memleak here, but should be necessary. Patch 2 fixes proc stats so when
> we use sockstats for debugging we get correct values.
> 
> The fix for observered memleak will come after these, but requires some
> more discussion and potentially patch revert so I'll try to get the set
> here going now.
> 
> [...]

Here is the summary with links:
  - [bpf,v4,1/2] bpf, sockmap: fix potential memory leak on unlikely error case
    https://git.kernel.org/bpf/bpf/c/7e6b27a69167
  - [bpf,v4,2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats
    https://git.kernel.org/bpf/bpf/c/228a4a7ba8e9

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-07-15 18:00 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-12 19:55 [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak John Fastabend
2021-07-12 19:55 ` [PATCH bpf v4 1/2] bpf, sockmap: fix potential memory leak on unlikely error case John Fastabend
2021-07-14  0:35   ` Cong Wang
2021-07-12 19:55 ` [PATCH bpf v4 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats John Fastabend
2021-07-13  7:47   ` Jakub Sitnicki
2021-07-14  0:56   ` Cong Wang
2021-07-13  7:47 ` [PATCH bpf v4 0/2] bpf, sockmap: fix potential memory leak Jakub Sitnicki
2021-07-15 18:00 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).