diff mbox series

[net] chelsio/chtls: fix memory leak

Message ID 20201022191332.21436-1-vinay.yadav@chelsio.com
State Changes Requested
Delegated to: David Miller
Headers show
Series [net] chelsio/chtls: fix memory leak | expand

Checks

Context Check Description
jkicinski/stable success Stable not CCed
jkicinski/header_inline success Link
jkicinski/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
jkicinski/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
jkicinski/verify_fixes success Link
jkicinski/kdoc success Errors and warnings before: 0 this patch: 0
jkicinski/build_32bit success Errors and warnings before: 1 this patch: 1
jkicinski/module_param success Was 0 now: 0
jkicinski/verify_signedoff success Link
jkicinski/source_inline success Was 0 now: 0
jkicinski/subject_prefix success Link
jkicinski/tree_selection success Clearly marked for net
jkicinski/patch_count success Link
jkicinski/fixes_present success Link
jkicinski/cover_letter success Link

Commit Message

Vinay Kumar Yadav Oct. 22, 2020, 7:13 p.m. UTC
Correct skb refcount in alloc_ctrl_skb(), causing skb memleak
when chtls_send_abort() called with NULL skb.
Also race between user context and softirq causing memleak,
consider the call sequence scenario

chtls_setkey()         //user context
chtls_peer_close()
chtls_abort_req_rss()
chtls_setkey()         //user context

work request skb queued in chtls_setkey() won't be freed
because resource is already cleaned for this connection,
fix it by not queuing work request while socket is closing

Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
Signed-off-by: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
---
 drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c | 2 +-
 drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

Comments

kernel test robot Oct. 22, 2020, 9:57 p.m. UTC | #1
Hi Vinay,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Vinay-Kumar-Yadav/chelsio-chtls-fix-memory-leak/20201023-031509
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 287d35405989cfe0090e3059f7788dc531879a8d
config: arm64-randconfig-r001-20201022 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ee6abef5323d59b983129bf3514ef6775d1d6cd5)
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/9e4d31e51d5f847c2ea311f865f1f1464d0c37ad
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vinay-Kumar-Yadav/chelsio-chtls-fix-memory-leak/20201023-031509
        git checkout 9e4d31e51d5f847c2ea311f865f1f1464d0c37ad
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

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

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c:380:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (unlikely(csk_flag(sk, CSK_ABORT_SHUTDOWN)))
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:78:22: note: expanded from macro 'unlikely'
   # define unlikely(x)    __builtin_expect(!!(x), 0)
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c:428:9: note: uninitialized use occurs here
           return ret;
                  ^~~
   drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c:380:2: note: remove the 'if' if its condition is always false
           if (unlikely(csk_flag(sk, CSK_ABORT_SHUTDOWN)))
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c:327:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.

vim +380 drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c

   315	
   316	int chtls_setkey(struct chtls_sock *csk, u32 keylen,
   317			 u32 optname, int cipher_type)
   318	{
   319		struct tls_key_req *kwr;
   320		struct chtls_dev *cdev;
   321		struct _key_ctx *kctx;
   322		int wrlen, klen, len;
   323		struct sk_buff *skb;
   324		struct sock *sk;
   325		int keyid;
   326		int kaddr;
   327		int ret;
   328	
   329		cdev = csk->cdev;
   330		sk = csk->sk;
   331	
   332		klen = roundup((keylen + AEAD_H_SIZE) + sizeof(*kctx), 32);
   333		wrlen = roundup(sizeof(*kwr), 16);
   334		len = klen + wrlen;
   335	
   336		/* Flush out-standing data before new key takes effect */
   337		if (optname == TLS_TX) {
   338			lock_sock(sk);
   339			if (skb_queue_len(&csk->txq))
   340				chtls_push_frames(csk, 0);
   341			release_sock(sk);
   342		}
   343	
   344		skb = alloc_skb(len, GFP_KERNEL);
   345		if (!skb)
   346			return -ENOMEM;
   347	
   348		keyid = get_new_keyid(csk, optname);
   349		if (keyid < 0) {
   350			ret = -ENOSPC;
   351			goto out_nokey;
   352		}
   353	
   354		kaddr = keyid_to_addr(cdev->kmap.start, keyid);
   355		kwr = (struct tls_key_req *)__skb_put_zero(skb, len);
   356		kwr->wr.op_to_compl =
   357			cpu_to_be32(FW_WR_OP_V(FW_ULPTX_WR) | FW_WR_COMPL_F |
   358			      FW_WR_ATOMIC_V(1U));
   359		kwr->wr.flowid_len16 =
   360			cpu_to_be32(FW_WR_LEN16_V(DIV_ROUND_UP(len, 16) |
   361				    FW_WR_FLOWID_V(csk->tid)));
   362		kwr->wr.protocol = 0;
   363		kwr->wr.mfs = htons(TLS_MFS);
   364		kwr->wr.reneg_to_write_rx = optname;
   365	
   366		/* ulptx command */
   367		kwr->req.cmd = cpu_to_be32(ULPTX_CMD_V(ULP_TX_MEM_WRITE) |
   368				    T5_ULP_MEMIO_ORDER_V(1) |
   369				    T5_ULP_MEMIO_IMM_V(1));
   370		kwr->req.len16 = cpu_to_be32((csk->tid << 8) |
   371				      DIV_ROUND_UP(len - sizeof(kwr->wr), 16));
   372		kwr->req.dlen = cpu_to_be32(ULP_MEMIO_DATA_LEN_V(klen >> 5));
   373		kwr->req.lock_addr = cpu_to_be32(ULP_MEMIO_ADDR_V(kaddr));
   374	
   375		/* sub command */
   376		kwr->sc_imm.cmd_more = cpu_to_be32(ULPTX_CMD_V(ULP_TX_SC_IMM));
   377		kwr->sc_imm.len = cpu_to_be32(klen);
   378	
   379		lock_sock(sk);
 > 380		if (unlikely(csk_flag(sk, CSK_ABORT_SHUTDOWN)))

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

Patch

diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
index 24154816d1d1..63aacc184f68 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
@@ -212,7 +212,7 @@  static struct sk_buff *alloc_ctrl_skb(struct sk_buff *skb, int len)
 {
 	if (likely(skb && !skb_shared(skb) && !skb_cloned(skb))) {
 		__skb_trim(skb, 0);
-		refcount_add(2, &skb->users);
+		refcount_inc(&skb->users);
 	} else {
 		skb = alloc_skb(len, GFP_KERNEL | __GFP_NOFAIL);
 	}
diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c
index f1820aca0d33..e37cbfc34dd3 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c
@@ -377,6 +377,9 @@  int chtls_setkey(struct chtls_sock *csk, u32 keylen,
 	kwr->sc_imm.len = cpu_to_be32(klen);
 
 	lock_sock(sk);
+	if (unlikely(csk_flag(sk, CSK_ABORT_SHUTDOWN)))
+		goto out_notcb;
+
 	/* key info */
 	kctx = (struct _key_ctx *)(kwr + 1);
 	ret = chtls_key_info(csk, kctx, keylen, optname, cipher_type);