* [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 related [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
* [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 related [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 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
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 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