[PATCH net 0/7] Bug fixes for chtls driver
STALE2042d
LANDED
Landed in mainline as 85bd6055e352 on 2021-01-08.
9 messages,
2 authors,
2021-01-08 · open the first message on its own page
patch 1: Fix hardware tid leak.
patch 2: Remove invalid set_tcb call.
patch 3: Fix panic when route to peer not configured.
patch 4: Avoid unnecessary freeing of oreq pointer.
patch 5: Replace skb_dequeue with skb_peek.
patch 6: Added a check to avoid NULL pointer dereference patch.
patch 7: Fix chtls resources release sequence.
Ayush Sawal (7):
chtls: Fix hardware tid leak
chtls: Remove invalid set_tcb call
chtls: Fix panic when route to peer not configured
chtls: Avoid unnecessary freeing of oreq pointer
chtls: Replace skb_dequeue with skb_peek
chtls: Added a check to avoid NULL pointer dereference
chtls: Fix chtls resources release sequence
.../chelsio/inline_crypto/chtls/chtls_cm.c | 71 +++++++------------
1 file changed, 24 insertions(+), 47 deletions(-)
--
2.28.0.rc1.6.gae46588
send_abort_rpl() is not calculating cpl_abort_req_rss offset and
ends up sending wrong TID with abort_rpl WR causng tid leaks.
Replaced send_abort_rpl() with chtls_send_abort_rpl() as it is
redundant.
Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
Signed-off-by: Rohit Maheshwari <redacted>
Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com>
---
.../chelsio/inline_crypto/chtls/chtls_cm.c | 39 ++-----------------
1 file changed, 3 insertions(+), 36 deletions(-)
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 a0e0d8a83681..561b5f2273af 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c @@ -1997,39 +1997,6 @@ static void t4_defer_reply(struct sk_buff *skb, struct chtls_dev *cdev,
spin_unlock_bh ( & cdev -> deferq . lock );
}
- static void send_abort_rpl ( struct sock * sk , struct sk_buff * skb ,
- struct chtls_dev * cdev , int status , int queue )
- {
- struct cpl_abort_req_rss * req = cplhdr ( skb );
- struct sk_buff * reply_skb ;
- struct chtls_sock * csk ;
-
- csk = rcu_dereference_sk_user_data ( sk );
-
- reply_skb = alloc_skb ( sizeof ( struct cpl_abort_rpl ),
- GFP_KERNEL );
-
- if ( ! reply_skb ) {
- req -> status = ( queue << 1 );
- t4_defer_reply ( skb , cdev , send_defer_abort_rpl );
- return ;
- }
-
- set_abort_rpl_wr ( reply_skb , GET_TID ( req ), status );
- kfree_skb ( skb );
-
- set_wr_txq ( reply_skb , CPL_PRIORITY_DATA , queue );
- if ( csk_conn_inline ( csk )) {
- struct l2t_entry * e = csk -> l2t_entry ;
-
- if ( e && sk -> sk_state != TCP_SYN_RECV ) {
- cxgb4_l2t_send ( csk -> egress_dev , reply_skb , e );
- return ;
- }
- }
- cxgb4_ofld_send ( cdev -> lldi -> ports [ 0 ], reply_skb );
- }
-
static void chtls_send_abort_rpl ( struct sock * sk , struct sk_buff * skb ,
struct chtls_dev * cdev ,
int status , int queue ) @@ -2079,8 +2046,8 @@ static void bl_abort_syn_rcv(struct sock *lsk, struct sk_buff *skb)
skb -> sk = NULL ;
do_abort_syn_rcv ( child , lsk );
- send_abort_rpl ( child , skb , BLOG_SKB_CB ( skb ) -> cdev ,
- CPL_ABORT_NO_RST , queue );
+ chtls_send_abort_rpl ( child , skb , BLOG_SKB_CB ( skb ) -> cdev ,
+ CPL_ABORT_NO_RST , queue );
}
static int abort_syn_rcv ( struct sock * sk , struct sk_buff * skb ) @@ -2111,7 +2078,7 @@ static int abort_syn_rcv(struct sock *sk, struct sk_buff *skb)
int queue = csk -> txq_idx ;
do_abort_syn_rcv ( sk , psk );
- send_abort_rpl ( sk , skb , cdev , CPL_ABORT_NO_RST , queue );
+ chtls_send_abort_rpl ( sk , skb , cdev , CPL_ABORT_NO_RST , queue );
} else {
skb -> sk = sk ;
BLOG_SKB_CB ( skb ) -> backlog_rcv = bl_abort_syn_rcv ; --
2.28.0.rc1.6.gae46588
At the time of SYN_RECV, connection information is not
initialized at FW, updating tcb flag over uninitialized
connection causes adapter crash. We don't need to
update the flag during SYN_RECV state, so avoid this.
Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
Signed-off-by: Rohit Maheshwari <redacted>
Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com>
---
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c | 3 ---
1 file changed, 3 deletions(-)
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 561b5f2273af..431d1e3844ab 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c @@ -2096,9 +2096,6 @@ static void chtls_abort_req_rss(struct sock *sk, struct sk_buff *skb)
int queue = csk -> txq_idx ;
if ( is_neg_adv ( req -> status )) {
- if ( sk -> sk_state == TCP_SYN_RECV )
- chtls_set_tcb_tflag ( sk , 0 , 0 );
-
kfree_skb ( skb );
return ;
} --
2.28.0.rc1.6.gae46588
If route to peer is not configured, we might get non tls
devices from dst_neigh_lookup() which is invalid, adding a
check to avoid it.
Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
Signed-off-by: Rohit Maheshwari <redacted>
Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com>
---
.../chelsio/inline_crypto/chtls/chtls_cm.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
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 431d1e3844ab..04a8bd5af3b9 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c @@ -1109,6 +1109,7 @@ static struct sock *chtls_recv_sock(struct sock *lsk,
const struct cpl_pass_accept_req * req ,
struct chtls_dev * cdev )
{
+ struct adapter * adap = pci_get_drvdata ( cdev -> pdev );
struct neighbour * n = NULL ;
struct inet_sock * newinet ;
const struct iphdr * iph ; @@ -1118,9 +1119,10 @@ static struct sock *chtls_recv_sock(struct sock *lsk,
struct dst_entry * dst ;
struct tcp_sock * tp ;
struct sock * newsk ;
+ bool found = false ;
u16 port_id ;
int rxq_idx ;
- int step ;
+ int step , i ;
iph = ( const struct iphdr * ) network_hdr ;
newsk = tcp_create_openreq_child ( lsk , oreq , cdev -> askb ); @@ -1152,7 +1154,7 @@ static struct sock *chtls_recv_sock(struct sock *lsk,
n = dst_neigh_lookup ( dst , & ip6h -> saddr );
#endif
}
- if ( ! n )
+ if ( ! n || ! n -> dev )
goto free_sk ;
ndev = n -> dev ; @@ -1161,6 +1163,13 @@ static struct sock *chtls_recv_sock(struct sock *lsk,
if ( is_vlan_dev ( ndev ))
ndev = vlan_dev_real_dev ( ndev );
+ for_each_port ( adap , i )
+ if ( cdev -> ports [ i ] == ndev )
+ found = true ;
+
+ if ( ! found )
+ goto free_dst ;
+
port_id = cxgb4_port_idx ( ndev );
csk = chtls_sock_create ( cdev ); @@ -1238,6 +1247,7 @@ static struct sock *chtls_recv_sock(struct sock *lsk,
free_csk :
chtls_sock_release ( & csk -> kref );
free_dst :
+ neigh_release ( n );
dst_release ( dst );
free_sk :
inet_csk_prepare_forced_close ( newsk ); --
2.28.0.rc1.6.gae46588
In chtls_pass_accept_request(), removing the chtls_reqsk_free()
call to avoid oreq freeing twice. Here oreq is the pointer to
struct request_sock.
Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
Signed-off-by: Rohit Maheshwari <redacted>
Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com>
---
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
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 04a8bd5af3b9..3022c802d09a 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c @@ -1397,7 +1397,7 @@ static void chtls_pass_accept_request(struct sock *sk,
newsk = chtls_recv_sock ( sk , oreq , network_hdr , req , cdev );
if ( ! newsk )
- goto free_oreq ;
+ goto reject ;
if ( chtls_get_module ( newsk ))
goto reject ; @@ -1413,8 +1413,6 @@ static void chtls_pass_accept_request(struct sock *sk,
kfree_skb ( skb );
return ;
- free_oreq :
- chtls_reqsk_free ( oreq );
reject :
mk_tid_release ( reply_skb , 0 , tid );
cxgb4_ofld_send ( cdev -> lldi -> ports [ 0 ], reply_skb ); --
2.28.0.rc1.6.gae46588
The skb is unlinked twice, one in __skb_dequeue in function
chtls_reset_synq() and another in cleanup_syn_rcv_conn().
So in this patch using skb_peek() instead of __skb_dequeue(),
so that unlink will be handled only in cleanup_syn_rcv_conn().
Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
Signed-off-by: Vinay Kumar Yadav <redacted>
Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com>
---
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c | 2 +-
1 file changed, 1 insertion(+), 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 3022c802d09a..ff3969a24d74 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c @@ -621,7 +621,7 @@ static void chtls_reset_synq(struct listen_ctx *listen_ctx)
while ( ! skb_queue_empty ( & listen_ctx -> synq )) {
struct chtls_sock * csk =
- container_of (( struct synq * ) __skb_dequeue
+ container_of (( struct synq * ) skb_peek
( & listen_ctx -> synq ), struct chtls_sock , synq );
struct sock * child = csk -> sk ;
--
2.28.0.rc1.6.gae46588
In case of server removal lookup_stid() may return NULL pointer, which
is used as listen_ctx. So added a check before accessing this pointer.
Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
Signed-off-by: Vinay Kumar Yadav <redacted>
Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com>
---
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c | 5 +++++
1 file changed, 5 insertions(+)
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 ff3969a24d74..1c6d3c93a0c8 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c @@ -1597,6 +1597,11 @@ static int chtls_pass_establish(struct chtls_dev *cdev, struct sk_buff *skb)
sk_wake_async ( sk , 0 , POLL_OUT );
data = lookup_stid ( cdev -> tids , stid );
+ if ( ! data ) {
+ /* listening server close */
+ kfree_skb ( skb );
+ goto unlock ;
+ }
lsk = (( struct listen_ctx * ) data ) -> lsk ;
bh_lock_sock ( lsk ); --
2.28.0.rc1.6.gae46588
CPL_ABORT_RPL is sent after releasing the resources by calling
chtls_release_resources(sk); and chtls_conn_done(sk);
eventually causing kernel panic. Fixing it by calling release
in appropriate order.
Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
Signed-off-by: Vinay Kumar Yadav <redacted>
Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com>
---
.../net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
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 1c6d3c93a0c8..51dd030b3b36 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c @@ -2058,9 +2058,9 @@ static void bl_abort_syn_rcv(struct sock *lsk, struct sk_buff *skb)
queue = csk -> txq_idx ;
skb -> sk = NULL ;
- do_abort_syn_rcv ( child , lsk );
chtls_send_abort_rpl ( child , skb , BLOG_SKB_CB ( skb ) -> cdev ,
CPL_ABORT_NO_RST , queue );
+ do_abort_syn_rcv ( child , lsk );
}
static int abort_syn_rcv ( struct sock * sk , struct sk_buff * skb ) @@ -2090,8 +2090,8 @@ static int abort_syn_rcv(struct sock *sk, struct sk_buff *skb)
if ( ! sock_owned_by_user ( psk )) {
int queue = csk -> txq_idx ;
- do_abort_syn_rcv ( sk , psk );
chtls_send_abort_rpl ( sk , skb , cdev , CPL_ABORT_NO_RST , queue );
+ do_abort_syn_rcv ( sk , psk );
} else {
skb -> sk = sk ;
BLOG_SKB_CB ( skb ) -> backlog_rcv = bl_abort_syn_rcv ; @@ -2135,12 +2135,12 @@ static void chtls_abort_req_rss(struct sock *sk, struct sk_buff *skb)
if ( sk -> sk_state == TCP_SYN_RECV && ! abort_syn_rcv ( sk , skb ))
return ;
- chtls_release_resources ( sk );
- chtls_conn_done ( sk );
}
chtls_send_abort_rpl ( sk , skb , BLOG_SKB_CB ( skb ) -> cdev ,
rst_status , queue );
+ chtls_release_resources ( sk );
+ chtls_conn_done ( sk );
}
static void chtls_abort_rpl_rss ( struct sock * sk , struct sk_buff * skb ) --
2.28.0.rc1.6.gae46588
Hello:
This series was applied to netdev/net.git (refs/heads/master):
On Wed, 6 Jan 2021 09:59:05 +0530 you wrote: patch 1: Fix hardware tid leak.
patch 2: Remove invalid set_tcb call.
patch 3: Fix panic when route to peer not configured.
patch 4: Avoid unnecessary freeing of oreq pointer.
patch 5: Replace skb_dequeue with skb_peek.
patch 6: Added a check to avoid NULL pointer dereference patch.
patch 7: Fix chtls resources release sequence.
[...]
Here is the summary with links:
- [net,1/7] chtls: Fix hardware tid leak
https://git.kernel.org/netdev/net/c/717df0f4cdc9
- [net,2/7] chtls: Remove invalid set_tcb call
https://git.kernel.org/netdev/net/c/827d329105bf
- [net,3/7] chtls: Fix panic when route to peer not configured
https://git.kernel.org/netdev/net/c/5a5fac9966bb
- [net,4/7] chtls: Avoid unnecessary freeing of oreq pointer
https://git.kernel.org/netdev/net/c/f8d15d29d6e6
- [net,5/7] chtls: Replace skb_dequeue with skb_peek
https://git.kernel.org/netdev/net/c/a84b2c0d5fa2
- [net,6/7] chtls: Added a check to avoid NULL pointer dereference
https://git.kernel.org/netdev/net/c/eade1e0a4fb3
- [net,7/7] chtls: Fix chtls resources release sequence
https://git.kernel.org/netdev/net/c/15ef6b0e30b3
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html