| Message ID | 20201025194935.31754-1-vinay.yadav@chelsio.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] chelsio/chtls: fix memory leaks | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 5 this patch: 5 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | fail | Link |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Mon, 26 Oct 2020 01:19:36 +0530 Vinay Kumar Yadav wrote: > 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 Sounds like two separate fixes? > 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 resources are already cleaned for this connection, > fix it by not queuing work request while socket is closing. > > v1->v2: > - fix W=1 warning. > > 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(-) > > 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); You just switched from adding two refs to adding one. Was this always leaking the skb? Also skb_get(). > } 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..62c829023da5 100644 > --- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c > +++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c > @@ -383,6 +383,9 @@ int chtls_setkey(struct chtls_sock *csk, u32 keylen, > if (ret) > goto out_notcb; > > + if (unlikely(csk_flag(sk, CSK_ABORT_SHUTDOWN))) > + goto out_notcb; > + > set_wr_txq(skb, CPL_PRIORITY_DATA, csk->tlshws.txqid); > csk->wr_credits -= DIV_ROUND_UP(len, 16); > csk->wr_unacked += DIV_ROUND_UP(len, 16);
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..62c829023da5 100644 --- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c +++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_hw.c @@ -383,6 +383,9 @@ int chtls_setkey(struct chtls_sock *csk, u32 keylen, if (ret) goto out_notcb; + if (unlikely(csk_flag(sk, CSK_ABORT_SHUTDOWN))) + goto out_notcb; + set_wr_txq(skb, CPL_PRIORITY_DATA, csk->tlshws.txqid); csk->wr_credits -= DIV_ROUND_UP(len, 16); csk->wr_unacked += DIV_ROUND_UP(len, 16);
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 resources are already cleaned for this connection, fix it by not queuing work request while socket is closing. v1->v2: - fix W=1 warning. 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(-)