Message ID | 20201022191332.21436-1-vinay.yadav@chelsio.com |
---|---|
State | Superseded |
Headers | show |
Series | [net] chelsio/chtls: fix memory leak | expand |
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 --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);
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(-)