bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf, sockmap 0/2] potential memleak and proc stats fix
@ 2021-06-30 21:53 John Fastabend
  2021-06-30 21:53 ` [PATCH bpf, sockmap 1/2] bpf, sockmap: fix potential memory leak on unlikely error case John Fastabend
  2021-06-30 21:53 ` [PATCH bpf, sockmap 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats John Fastabend
  0 siblings, 2 replies; 5+ messages in thread
From: John Fastabend @ 2021-06-30 21:53 UTC (permalink / raw)
  To: ast, daniel, andriin; +Cc: john.fastabend, bpf, netdev

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.

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    | 4 +++-
 net/core/sock_map.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.25.1


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

* [PATCH bpf, sockmap 1/2] bpf, sockmap: fix potential memory leak on unlikely error case
  2021-06-30 21:53 [PATCH bpf, sockmap 0/2] potential memleak and proc stats fix John Fastabend
@ 2021-06-30 21:53 ` John Fastabend
  2021-06-30 21:53 ` [PATCH bpf, sockmap 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats John Fastabend
  1 sibling, 0 replies; 5+ messages in thread
From: John Fastabend @ 2021-06-30 21:53 UTC (permalink / raw)
  To: ast, daniel, andriin; +Cc: john.fastabend, bpf, netdev

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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 9b6160a191f8..22603289c2b2 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -505,8 +505,10 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
 	 * drop the skb. We need to linearize the skb so that the mapping
 	 * in skb_to_sgvec can not error.
 	 */
-	if (skb_linearize(skb))
+	if (skb_linearize(skb)) {
+		kfree(msg);
 		return -EAGAIN;
+	}
 	num_sge = skb_to_sgvec(skb, msg->sg.data, 0, skb->len);
 	if (unlikely(num_sge < 0)) {
 		kfree(msg);
-- 
2.25.1


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

* [PATCH bpf, sockmap 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats
  2021-06-30 21:53 [PATCH bpf, sockmap 0/2] potential memleak and proc stats fix John Fastabend
  2021-06-30 21:53 ` [PATCH bpf, sockmap 1/2] bpf, sockmap: fix potential memory leak on unlikely error case John Fastabend
@ 2021-06-30 21:53 ` John Fastabend
  2021-07-01  6:58   ` kernel test robot
  2021-07-01  8:21   ` kernel test robot
  1 sibling, 2 replies; 5+ messages in thread
From: John Fastabend @ 2021-06-30 21:53 UTC (permalink / raw)
  To: ast, daniel, andriin; +Cc: john.fastabend, bpf, netdev

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.

Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 net/core/sock_map.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 60decd6420ca..29e7bae65db5 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -221,7 +221,7 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
 	struct bpf_prog *skb_verdict = NULL;
 	struct bpf_prog *msg_parser = NULL;
 	struct sk_psock *psock;
-	int ret;
+	int ret, idx;
 
 	/* Only sockets we can redirect into/from in BPF need to hold
 	 * refs to parser/verdict progs and have their sk_data_ready
@@ -293,9 +293,11 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
 	if (msg_parser)
 		psock_set_prog(&psock->progs.msg_parser, msg_parser);

+	idx = sk->sk_prot->inuse_idx;
 	ret = sock_map_init_proto(sk, psock);
 	if (ret < 0)
 		goto out_drop;
+	sk->sk_prot->inuse_idx = idx;
 
 	write_lock_bh(&sk->sk_callback_lock);
 	if (stream_parser && stream_verdict && !psock->saved_data_ready) {
-- 
2.25.1


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

* Re: [PATCH bpf, sockmap 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats
  2021-06-30 21:53 ` [PATCH bpf, sockmap 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats John Fastabend
@ 2021-07-01  6:58   ` kernel test robot
  2021-07-01  8:21   ` kernel test robot
  1 sibling, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-07-01  6:58 UTC (permalink / raw)
  To: John Fastabend, ast, daniel, andriin
  Cc: kbuild-all, john.fastabend, bpf, netdev

[-- Attachment #1: Type: text/plain, Size: 6355 bytes --]

Hi John,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.13 next-20210630]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/John-Fastabend/potential-memleak-and-proc-stats-fix/20210701-055546
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 440462198d9c45e48f2d8d9b18c5702d92282f46
config: sparc-buildonly-randconfig-r006-20210701 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/82ee893f50c6899cb557f22d0ae9a657b14d183f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review John-Fastabend/potential-memleak-and-proc-stats-fix/20210701-055546
        git checkout 82ee893f50c6899cb557f22d0ae9a657b14d183f
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross O=build_dir ARCH=sparc SHELL=/bin/bash net/core/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/core/sock_map.c: In function 'sock_map_link':
>> net/core/sock_map.c:296:19: error: 'struct proto' has no member named 'inuse_idx'
     296 |  idx = sk->sk_prot->inuse_idx;
         |                   ^~
   net/core/sock_map.c:300:13: error: 'struct proto' has no member named 'inuse_idx'
     300 |  sk->sk_prot->inuse_idx = idx;
         |             ^~


vim +296 net/core/sock_map.c

   215	
   216	static int sock_map_link(struct bpf_map *map, struct sock *sk)
   217	{
   218		struct sk_psock_progs *progs = sock_map_progs(map);
   219		struct bpf_prog *stream_verdict = NULL;
   220		struct bpf_prog *stream_parser = NULL;
   221		struct bpf_prog *skb_verdict = NULL;
   222		struct bpf_prog *msg_parser = NULL;
   223		struct sk_psock *psock;
   224		int ret, idx;
   225	
   226		/* Only sockets we can redirect into/from in BPF need to hold
   227		 * refs to parser/verdict progs and have their sk_data_ready
   228		 * and sk_write_space callbacks overridden.
   229		 */
   230		if (!sock_map_redirect_allowed(sk))
   231			goto no_progs;
   232	
   233		stream_verdict = READ_ONCE(progs->stream_verdict);
   234		if (stream_verdict) {
   235			stream_verdict = bpf_prog_inc_not_zero(stream_verdict);
   236			if (IS_ERR(stream_verdict))
   237				return PTR_ERR(stream_verdict);
   238		}
   239	
   240		stream_parser = READ_ONCE(progs->stream_parser);
   241		if (stream_parser) {
   242			stream_parser = bpf_prog_inc_not_zero(stream_parser);
   243			if (IS_ERR(stream_parser)) {
   244				ret = PTR_ERR(stream_parser);
   245				goto out_put_stream_verdict;
   246			}
   247		}
   248	
   249		msg_parser = READ_ONCE(progs->msg_parser);
   250		if (msg_parser) {
   251			msg_parser = bpf_prog_inc_not_zero(msg_parser);
   252			if (IS_ERR(msg_parser)) {
   253				ret = PTR_ERR(msg_parser);
   254				goto out_put_stream_parser;
   255			}
   256		}
   257	
   258		skb_verdict = READ_ONCE(progs->skb_verdict);
   259		if (skb_verdict) {
   260			skb_verdict = bpf_prog_inc_not_zero(skb_verdict);
   261			if (IS_ERR(skb_verdict)) {
   262				ret = PTR_ERR(skb_verdict);
   263				goto out_put_msg_parser;
   264			}
   265		}
   266	
   267	no_progs:
   268		psock = sock_map_psock_get_checked(sk);
   269		if (IS_ERR(psock)) {
   270			ret = PTR_ERR(psock);
   271			goto out_progs;
   272		}
   273	
   274		if (psock) {
   275			if ((msg_parser && READ_ONCE(psock->progs.msg_parser)) ||
   276			    (stream_parser  && READ_ONCE(psock->progs.stream_parser)) ||
   277			    (skb_verdict && READ_ONCE(psock->progs.skb_verdict)) ||
   278			    (skb_verdict && READ_ONCE(psock->progs.stream_verdict)) ||
   279			    (stream_verdict && READ_ONCE(psock->progs.skb_verdict)) ||
   280			    (stream_verdict && READ_ONCE(psock->progs.stream_verdict))) {
   281				sk_psock_put(sk, psock);
   282				ret = -EBUSY;
   283				goto out_progs;
   284			}
   285		} else {
   286			psock = sk_psock_init(sk, map->numa_node);
   287			if (IS_ERR(psock)) {
   288				ret = PTR_ERR(psock);
   289				goto out_progs;
   290			}
   291		}
   292	
   293		if (msg_parser)
   294			psock_set_prog(&psock->progs.msg_parser, msg_parser);
   295	
 > 296		idx = sk->sk_prot->inuse_idx;
   297		ret = sock_map_init_proto(sk, psock);
   298		if (ret < 0)
   299			goto out_drop;
   300		sk->sk_prot->inuse_idx = idx;
   301	
   302		write_lock_bh(&sk->sk_callback_lock);
   303		if (stream_parser && stream_verdict && !psock->saved_data_ready) {
   304			ret = sk_psock_init_strp(sk, psock);
   305			if (ret)
   306				goto out_unlock_drop;
   307			psock_set_prog(&psock->progs.stream_verdict, stream_verdict);
   308			psock_set_prog(&psock->progs.stream_parser, stream_parser);
   309			sk_psock_start_strp(sk, psock);
   310		} else if (!stream_parser && stream_verdict && !psock->saved_data_ready) {
   311			psock_set_prog(&psock->progs.stream_verdict, stream_verdict);
   312			sk_psock_start_verdict(sk,psock);
   313		} else if (!stream_verdict && skb_verdict && !psock->saved_data_ready) {
   314			psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
   315			sk_psock_start_verdict(sk, psock);
   316		}
   317		write_unlock_bh(&sk->sk_callback_lock);
   318		return 0;
   319	out_unlock_drop:
   320		write_unlock_bh(&sk->sk_callback_lock);
   321	out_drop:
   322		sk_psock_put(sk, psock);
   323	out_progs:
   324		if (skb_verdict)
   325			bpf_prog_put(skb_verdict);
   326	out_put_msg_parser:
   327		if (msg_parser)
   328			bpf_prog_put(msg_parser);
   329	out_put_stream_parser:
   330		if (stream_parser)
   331			bpf_prog_put(stream_parser);
   332	out_put_stream_verdict:
   333		if (stream_verdict)
   334			bpf_prog_put(stream_verdict);
   335		return ret;
   336	}
   337	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39363 bytes --]

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

* Re: [PATCH bpf, sockmap 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats
  2021-06-30 21:53 ` [PATCH bpf, sockmap 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats John Fastabend
  2021-07-01  6:58   ` kernel test robot
@ 2021-07-01  8:21   ` kernel test robot
  1 sibling, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-07-01  8:21 UTC (permalink / raw)
  To: John Fastabend, ast, daniel, andriin
  Cc: clang-built-linux, kbuild-all, john.fastabend, bpf, netdev

[-- Attachment #1: Type: text/plain, Size: 6499 bytes --]

Hi John,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.13 next-20210630]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/John-Fastabend/potential-memleak-and-proc-stats-fix/20210701-055546
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 440462198d9c45e48f2d8d9b18c5702d92282f46
config: arm64-randconfig-r026-20210630 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project e7e71e9454ed76c1b3d8140170b5333c28bef1be)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/82ee893f50c6899cb557f22d0ae9a657b14d183f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review John-Fastabend/potential-memleak-and-proc-stats-fix/20210701-055546
        git checkout 82ee893f50c6899cb557f22d0ae9a657b14d183f
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash arch/arm64/ net/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/core/sock_map.c:296:21: error: no member named 'inuse_idx' in 'struct proto'
           idx = sk->sk_prot->inuse_idx;
                 ~~~~~~~~~~~  ^
   net/core/sock_map.c:300:15: error: no member named 'inuse_idx' in 'struct proto'
           sk->sk_prot->inuse_idx = idx;
           ~~~~~~~~~~~  ^
   2 errors generated.


vim +296 net/core/sock_map.c

   215	
   216	static int sock_map_link(struct bpf_map *map, struct sock *sk)
   217	{
   218		struct sk_psock_progs *progs = sock_map_progs(map);
   219		struct bpf_prog *stream_verdict = NULL;
   220		struct bpf_prog *stream_parser = NULL;
   221		struct bpf_prog *skb_verdict = NULL;
   222		struct bpf_prog *msg_parser = NULL;
   223		struct sk_psock *psock;
   224		int ret, idx;
   225	
   226		/* Only sockets we can redirect into/from in BPF need to hold
   227		 * refs to parser/verdict progs and have their sk_data_ready
   228		 * and sk_write_space callbacks overridden.
   229		 */
   230		if (!sock_map_redirect_allowed(sk))
   231			goto no_progs;
   232	
   233		stream_verdict = READ_ONCE(progs->stream_verdict);
   234		if (stream_verdict) {
   235			stream_verdict = bpf_prog_inc_not_zero(stream_verdict);
   236			if (IS_ERR(stream_verdict))
   237				return PTR_ERR(stream_verdict);
   238		}
   239	
   240		stream_parser = READ_ONCE(progs->stream_parser);
   241		if (stream_parser) {
   242			stream_parser = bpf_prog_inc_not_zero(stream_parser);
   243			if (IS_ERR(stream_parser)) {
   244				ret = PTR_ERR(stream_parser);
   245				goto out_put_stream_verdict;
   246			}
   247		}
   248	
   249		msg_parser = READ_ONCE(progs->msg_parser);
   250		if (msg_parser) {
   251			msg_parser = bpf_prog_inc_not_zero(msg_parser);
   252			if (IS_ERR(msg_parser)) {
   253				ret = PTR_ERR(msg_parser);
   254				goto out_put_stream_parser;
   255			}
   256		}
   257	
   258		skb_verdict = READ_ONCE(progs->skb_verdict);
   259		if (skb_verdict) {
   260			skb_verdict = bpf_prog_inc_not_zero(skb_verdict);
   261			if (IS_ERR(skb_verdict)) {
   262				ret = PTR_ERR(skb_verdict);
   263				goto out_put_msg_parser;
   264			}
   265		}
   266	
   267	no_progs:
   268		psock = sock_map_psock_get_checked(sk);
   269		if (IS_ERR(psock)) {
   270			ret = PTR_ERR(psock);
   271			goto out_progs;
   272		}
   273	
   274		if (psock) {
   275			if ((msg_parser && READ_ONCE(psock->progs.msg_parser)) ||
   276			    (stream_parser  && READ_ONCE(psock->progs.stream_parser)) ||
   277			    (skb_verdict && READ_ONCE(psock->progs.skb_verdict)) ||
   278			    (skb_verdict && READ_ONCE(psock->progs.stream_verdict)) ||
   279			    (stream_verdict && READ_ONCE(psock->progs.skb_verdict)) ||
   280			    (stream_verdict && READ_ONCE(psock->progs.stream_verdict))) {
   281				sk_psock_put(sk, psock);
   282				ret = -EBUSY;
   283				goto out_progs;
   284			}
   285		} else {
   286			psock = sk_psock_init(sk, map->numa_node);
   287			if (IS_ERR(psock)) {
   288				ret = PTR_ERR(psock);
   289				goto out_progs;
   290			}
   291		}
   292	
   293		if (msg_parser)
   294			psock_set_prog(&psock->progs.msg_parser, msg_parser);
   295	
 > 296		idx = sk->sk_prot->inuse_idx;
   297		ret = sock_map_init_proto(sk, psock);
   298		if (ret < 0)
   299			goto out_drop;
   300		sk->sk_prot->inuse_idx = idx;
   301	
   302		write_lock_bh(&sk->sk_callback_lock);
   303		if (stream_parser && stream_verdict && !psock->saved_data_ready) {
   304			ret = sk_psock_init_strp(sk, psock);
   305			if (ret)
   306				goto out_unlock_drop;
   307			psock_set_prog(&psock->progs.stream_verdict, stream_verdict);
   308			psock_set_prog(&psock->progs.stream_parser, stream_parser);
   309			sk_psock_start_strp(sk, psock);
   310		} else if (!stream_parser && stream_verdict && !psock->saved_data_ready) {
   311			psock_set_prog(&psock->progs.stream_verdict, stream_verdict);
   312			sk_psock_start_verdict(sk,psock);
   313		} else if (!stream_verdict && skb_verdict && !psock->saved_data_ready) {
   314			psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
   315			sk_psock_start_verdict(sk, psock);
   316		}
   317		write_unlock_bh(&sk->sk_callback_lock);
   318		return 0;
   319	out_unlock_drop:
   320		write_unlock_bh(&sk->sk_callback_lock);
   321	out_drop:
   322		sk_psock_put(sk, psock);
   323	out_progs:
   324		if (skb_verdict)
   325			bpf_prog_put(skb_verdict);
   326	out_put_msg_parser:
   327		if (msg_parser)
   328			bpf_prog_put(msg_parser);
   329	out_put_stream_parser:
   330		if (stream_parser)
   331			bpf_prog_put(stream_parser);
   332	out_put_stream_verdict:
   333		if (stream_verdict)
   334			bpf_prog_put(stream_verdict);
   335		return ret;
   336	}
   337	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 46463 bytes --]

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

end of thread, other threads:[~2021-07-01  8:21 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-30 21:53 [PATCH bpf, sockmap 0/2] potential memleak and proc stats fix John Fastabend
2021-06-30 21:53 ` [PATCH bpf, sockmap 1/2] bpf, sockmap: fix potential memory leak on unlikely error case John Fastabend
2021-06-30 21:53 ` [PATCH bpf, sockmap 2/2] bpf, sockmap: sk_prot needs inuse_idx set for proc stats John Fastabend
2021-07-01  6:58   ` kernel test robot
2021-07-01  8:21   ` kernel test robot

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).