netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
@ 2020-11-03 10:47 Vinay Kumar Yadav
  2020-11-05  1:16 ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Vinay Kumar Yadav @ 2020-11-03 10:47 UTC (permalink / raw)
  To: netdev, davem, kuba, borisp; +Cc: secdev, Vinay Kumar Yadav

user can initialize tls ulp using setsockopt call on socket
before listen() in case of tls-toe (TLS_HW_RECORD) and same
setsockopt call on connected socket in case of kernel tls (TLS_SW).
In presence of tls-toe devices, TLS ulp is initialized, tls context
is allocated per listen socket and socket is listening at adapter
as well as kernel tcp stack. now consider the scenario, connections
are established in kernel stack.
on every connection close which is established in kernel stack,
it clears tls context which is created on listen socket causing
kernel panic.
Addressed the issue by setting child socket to base (non TLS ULP)
when tls ulp is initialized on parent socket (listen socket).

Fixes: 76f7164d02d4 ("net/tls: free ctx in sock destruct")
Signed-off-by: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
---
 .../chelsio/inline_crypto/chtls/chtls_cm.c    |  3 +++
 net/tls/tls_main.c                            | 23 ++++++++++++++++++-
 2 files changed, 25 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 63aacc184f68..c56cd9c1e40c 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
@@ -1206,6 +1206,9 @@ static struct sock *chtls_recv_sock(struct sock *lsk,
 	sk_setup_caps(newsk, dst);
 	ctx = tls_get_ctx(lsk);
 	newsk->sk_destruct = ctx->sk_destruct;
+	newsk->sk_prot = lsk->sk_prot;
+	inet_csk(newsk)->icsk_ulp_ops = inet_csk(lsk)->icsk_ulp_ops;
+	rcu_assign_pointer(inet_csk(newsk)->icsk_ulp_data, ctx);
 	csk->sk = newsk;
 	csk->passive_reap_next = oreq;
 	csk->tx_chan = cxgb4_port_chan(ndev);
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 8d93cea99f2c..9682dacae30c 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -715,7 +715,7 @@ static int tls_init(struct sock *sk)
 	tls_build_proto(sk);
 
 #ifdef CONFIG_TLS_TOE
-	if (tls_toe_bypass(sk))
+	if (sk->sk_state == TCP_CLOSE && tls_toe_bypass(sk))
 		return 0;
 #endif
 
@@ -744,6 +744,24 @@ static int tls_init(struct sock *sk)
 	return rc;
 }
 
+#ifdef CONFIG_TLS_TOE
+static void tls_clone(const struct request_sock *req,
+		      struct sock *newsk, const gfp_t priority)
+{
+	struct tls_context *ctx = tls_get_ctx(newsk);
+	struct inet_connection_sock *icsk = inet_csk(newsk);
+
+	/* In presence of TLS TOE devices, TLS ulp is initialized on listen
+	 * socket so lets child socket back to non tls ULP mode because tcp
+	 * connections can happen in non TLS TOE mode.
+	 */
+	newsk->sk_prot = ctx->sk_proto;
+	newsk->sk_destruct = ctx->sk_destruct;
+	icsk->icsk_ulp_ops = NULL;
+	rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
+}
+#endif
+
 static void tls_update(struct sock *sk, struct proto *p,
 		       void (*write_space)(struct sock *sk))
 {
@@ -857,6 +875,9 @@ static struct tcp_ulp_ops tcp_tls_ulp_ops __read_mostly = {
 	.update			= tls_update,
 	.get_info		= tls_get_info,
 	.get_info_size		= tls_get_info_size,
+#ifdef CONFIG_TLS_TOE
+	.clone                  = tls_clone
+#endif
 };
 
 static int __init tls_register(void)
-- 
2.18.1


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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-03 10:47 [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP Vinay Kumar Yadav
@ 2020-11-05  1:16 ` Jakub Kicinski
  2020-11-05 17:50   ` Vinay Kumar Yadav
  0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2020-11-05  1:16 UTC (permalink / raw)
  To: Vinay Kumar Yadav; +Cc: netdev, davem, borisp, secdev

On Tue,  3 Nov 2020 16:17:03 +0530 Vinay Kumar Yadav wrote:
> user can initialize tls ulp using setsockopt call on socket
> before listen() in case of tls-toe (TLS_HW_RECORD) and same
> setsockopt call on connected socket in case of kernel tls (TLS_SW).
> In presence of tls-toe devices, TLS ulp is initialized, tls context
> is allocated per listen socket and socket is listening at adapter
> as well as kernel tcp stack. now consider the scenario, connections
> are established in kernel stack.
> on every connection close which is established in kernel stack,
> it clears tls context which is created on listen socket causing
> kernel panic.
> Addressed the issue by setting child socket to base (non TLS ULP)
> when tls ulp is initialized on parent socket (listen socket).
> 
> Fixes: 76f7164d02d4 ("net/tls: free ctx in sock destruct")
> Signed-off-by: Vinay Kumar Yadav <vinay.yadav@chelsio.com>

We should prevent from the socket getting into LISTEN state in the
first place. Can we make a copy of proto_ops (like tls_sw_proto_ops) 
and set listen to sock_no_listen? 

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-05  1:16 ` Jakub Kicinski
@ 2020-11-05 17:50   ` Vinay Kumar Yadav
  2020-11-05 17:53     ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Vinay Kumar Yadav @ 2020-11-05 17:50 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, davem, borisp, secdev



On 11/5/2020 6:46 AM, Jakub Kicinski wrote:
> On Tue,  3 Nov 2020 16:17:03 +0530 Vinay Kumar Yadav wrote:
>> user can initialize tls ulp using setsockopt call on socket
>> before listen() in case of tls-toe (TLS_HW_RECORD) and same
>> setsockopt call on connected socket in case of kernel tls (TLS_SW).
>> In presence of tls-toe devices, TLS ulp is initialized, tls context
>> is allocated per listen socket and socket is listening at adapter
>> as well as kernel tcp stack. now consider the scenario, connections
>> are established in kernel stack.
>> on every connection close which is established in kernel stack,
>> it clears tls context which is created on listen socket causing
>> kernel panic.
>> Addressed the issue by setting child socket to base (non TLS ULP)
>> when tls ulp is initialized on parent socket (listen socket).
>>
>> Fixes: 76f7164d02d4 ("net/tls: free ctx in sock destruct")
>> Signed-off-by: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
> 
> We should prevent from the socket getting into LISTEN state in the
> first place. Can we make a copy of proto_ops (like tls_sw_proto_ops)
> and set listen to sock_no_listen?
> 

Once tls-toe (TLS_HW_RECORD) is configured on a socket, listen() call 
from user on same socket will create hash at two places.

tls_toe_hash() ---> ctx->sk_proto->hash(sk); dev->hash(dev, sk);

when connection establishes, same sock is cloned in case of both
(connection in adapter or kernel stack).

Please suggest if we can handle it other way?

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-05 17:50   ` Vinay Kumar Yadav
@ 2020-11-05 17:53     ` Jakub Kicinski
  2020-11-05 18:25       ` Vinay Kumar Yadav
  0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2020-11-05 17:53 UTC (permalink / raw)
  To: Vinay Kumar Yadav; +Cc: netdev, davem, borisp, secdev

On Thu, 5 Nov 2020 23:20:13 +0530 Vinay Kumar Yadav wrote:
> On 11/5/2020 6:46 AM, Jakub Kicinski wrote:
> > On Tue,  3 Nov 2020 16:17:03 +0530 Vinay Kumar Yadav wrote:  
> >> user can initialize tls ulp using setsockopt call on socket
> >> before listen() in case of tls-toe (TLS_HW_RECORD) and same
> >> setsockopt call on connected socket in case of kernel tls (TLS_SW).
> >> In presence of tls-toe devices, TLS ulp is initialized, tls context
> >> is allocated per listen socket and socket is listening at adapter
> >> as well as kernel tcp stack. now consider the scenario, connections
> >> are established in kernel stack.
> >> on every connection close which is established in kernel stack,
> >> it clears tls context which is created on listen socket causing
> >> kernel panic.
> >> Addressed the issue by setting child socket to base (non TLS ULP)
> >> when tls ulp is initialized on parent socket (listen socket).
> >>
> >> Fixes: 76f7164d02d4 ("net/tls: free ctx in sock destruct")
> >> Signed-off-by: Vinay Kumar Yadav <vinay.yadav@chelsio.com>  
> > 
> > We should prevent from the socket getting into LISTEN state in the
> > first place. Can we make a copy of proto_ops (like tls_sw_proto_ops)
> > and set listen to sock_no_listen?
> 
> Once tls-toe (TLS_HW_RECORD) is configured on a socket, listen() call 
> from user on same socket will create hash at two places.

What I'm saying is - disallow listen calls on sockets with tls-toe
installed on them. Is that not possible?

> tls_toe_hash() ---> ctx->sk_proto->hash(sk); dev->hash(dev, sk);
> 
> when connection establishes, same sock is cloned in case of both
> (connection in adapter or kernel stack).
> 
> Please suggest if we can handle it other way?


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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-05 17:53     ` Jakub Kicinski
@ 2020-11-05 18:25       ` Vinay Kumar Yadav
  2020-11-05 18:46         ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Vinay Kumar Yadav @ 2020-11-05 18:25 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, davem, borisp, secdev



On 11/5/2020 11:23 PM, Jakub Kicinski wrote:
> On Thu, 5 Nov 2020 23:20:13 +0530 Vinay Kumar Yadav wrote:
>> On 11/5/2020 6:46 AM, Jakub Kicinski wrote:
>>> On Tue,  3 Nov 2020 16:17:03 +0530 Vinay Kumar Yadav wrote:
>>>> user can initialize tls ulp using setsockopt call on socket
>>>> before listen() in case of tls-toe (TLS_HW_RECORD) and same
>>>> setsockopt call on connected socket in case of kernel tls (TLS_SW).
>>>> In presence of tls-toe devices, TLS ulp is initialized, tls context
>>>> is allocated per listen socket and socket is listening at adapter
>>>> as well as kernel tcp stack. now consider the scenario, connections
>>>> are established in kernel stack.
>>>> on every connection close which is established in kernel stack,
>>>> it clears tls context which is created on listen socket causing
>>>> kernel panic.
>>>> Addressed the issue by setting child socket to base (non TLS ULP)
>>>> when tls ulp is initialized on parent socket (listen socket).
>>>>
>>>> Fixes: 76f7164d02d4 ("net/tls: free ctx in sock destruct")
>>>> Signed-off-by: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
>>>
>>> We should prevent from the socket getting into LISTEN state in the
>>> first place. Can we make a copy of proto_ops (like tls_sw_proto_ops)
>>> and set listen to sock_no_listen?
>>
>> Once tls-toe (TLS_HW_RECORD) is configured on a socket, listen() call
>> from user on same socket will create hash at two places.
> 
> What I'm saying is - disallow listen calls on sockets with tls-toe
> installed on them. Is that not possible?
>
You mean socket with tls-toe installed shouldn't be listening at other
than adapter? basically avoid ctx->sk_proto->hash(sk) call.

>> tls_toe_hash() ---> ctx->sk_proto->hash(sk); dev->hash(dev, sk);
>>
>> when connection establishes, same sock is cloned in case of both
>> (connection in adapter or kernel stack).
>>
>> Please suggest if we can handle it other way?
> 

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-05 18:25       ` Vinay Kumar Yadav
@ 2020-11-05 18:46         ` Jakub Kicinski
  2020-11-06 20:32           ` Vinay Kumar Yadav
  0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2020-11-05 18:46 UTC (permalink / raw)
  To: Vinay Kumar Yadav; +Cc: netdev, davem, borisp, secdev

On Thu, 5 Nov 2020 23:55:15 +0530 Vinay Kumar Yadav wrote:
> >>> We should prevent from the socket getting into LISTEN state in the
> >>> first place. Can we make a copy of proto_ops (like tls_sw_proto_ops)
> >>> and set listen to sock_no_listen?  
> >>
> >> Once tls-toe (TLS_HW_RECORD) is configured on a socket, listen() call
> >> from user on same socket will create hash at two places.  
> > 
> > What I'm saying is - disallow listen calls on sockets with tls-toe
> > installed on them. Is that not possible?
> >  
> You mean socket with tls-toe installed shouldn't be listening at other
> than adapter? basically avoid ctx->sk_proto->hash(sk) call.

No, replace the listen callback, like I said. Why are you talking about
hash???

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-06 20:32           ` Vinay Kumar Yadav
@ 2020-11-06 20:28             ` Jakub Kicinski
  2020-11-09 18:51               ` Vinay Kumar Yadav
  0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2020-11-06 20:28 UTC (permalink / raw)
  To: Vinay Kumar Yadav; +Cc: netdev, davem, borisp, secdev

On Sat, 7 Nov 2020 02:02:42 +0530 Vinay Kumar Yadav wrote:
> On 11/6/2020 12:16 AM, Jakub Kicinski wrote:
> > On Thu, 5 Nov 2020 23:55:15 +0530 Vinay Kumar Yadav wrote:  
> >>>>> We should prevent from the socket getting into LISTEN state in the
> >>>>> first place. Can we make a copy of proto_ops (like tls_sw_proto_ops)
> >>>>> and set listen to sock_no_listen?  
> >>>>
> >>>> Once tls-toe (TLS_HW_RECORD) is configured on a socket, listen() call
> >>>> from user on same socket will create hash at two places.  
> >>>
> >>> What I'm saying is - disallow listen calls on sockets with tls-toe
> >>> installed on them. Is that not possible?
> >>>     
> >> You mean socket with tls-toe installed shouldn't be listening at other
> >> than adapter? basically avoid ctx->sk_proto->hash(sk) call.  
> > 
> > No, replace the listen callback, like I said. Why are you talking about
> > hash???
> >As per my understanding we can't avoid socket listen.  
> Not sure how replacing listen callback solve the issue,
> can you please elaborate ?

TLS sockets are not supposed to get into listen state. IIUC the problem
is that the user is able to set TLS TOE on a socket which then starts
to listen and the state gets cloned improperly.

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-05 18:46         ` Jakub Kicinski
@ 2020-11-06 20:32           ` Vinay Kumar Yadav
  2020-11-06 20:28             ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Vinay Kumar Yadav @ 2020-11-06 20:32 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, davem, borisp, secdev



On 11/6/2020 12:16 AM, Jakub Kicinski wrote:
> On Thu, 5 Nov 2020 23:55:15 +0530 Vinay Kumar Yadav wrote:
>>>>> We should prevent from the socket getting into LISTEN state in the
>>>>> first place. Can we make a copy of proto_ops (like tls_sw_proto_ops)
>>>>> and set listen to sock_no_listen?
>>>>
>>>> Once tls-toe (TLS_HW_RECORD) is configured on a socket, listen() call
>>>> from user on same socket will create hash at two places.
>>>
>>> What I'm saying is - disallow listen calls on sockets with tls-toe
>>> installed on them. Is that not possible?
>>>   
>> You mean socket with tls-toe installed shouldn't be listening at other
>> than adapter? basically avoid ctx->sk_proto->hash(sk) call.
> 
> No, replace the listen callback, like I said. Why are you talking about
> hash???
>As per my understanding we can't avoid socket listen.
Not sure how replacing listen callback solve the issue,
can you please elaborate ?

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-06 20:28             ` Jakub Kicinski
@ 2020-11-09 18:51               ` Vinay Kumar Yadav
  2020-11-09 18:58                 ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Vinay Kumar Yadav @ 2020-11-09 18:51 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, davem, borisp, secdev

Jakub,

On 11/7/2020 1:58 AM, Jakub Kicinski wrote:
> On Sat, 7 Nov 2020 02:02:42 +0530 Vinay Kumar Yadav wrote:
>> On 11/6/2020 12:16 AM, Jakub Kicinski wrote:
>>> On Thu, 5 Nov 2020 23:55:15 +0530 Vinay Kumar Yadav wrote:
>>>>>>> We should prevent from the socket getting into LISTEN state in the
>>>>>>> first place. Can we make a copy of proto_ops (like tls_sw_proto_ops)
>>>>>>> and set listen to sock_no_listen?
>>>>>>
>>>>>> Once tls-toe (TLS_HW_RECORD) is configured on a socket, listen() call
>>>>>> from user on same socket will create hash at two places.
>>>>>
>>>>> What I'm saying is - disallow listen calls on sockets with tls-toe
>>>>> installed on them. Is that not possible?
>>>>>      
>>>> You mean socket with tls-toe installed shouldn't be listening at other
>>>> than adapter? basically avoid ctx->sk_proto->hash(sk) call.
>>>
>>> No, replace the listen callback, like I said. Why are you talking about
>>> hash???
>>> As per my understanding we can't avoid socket listen.
>> Not sure how replacing listen callback solve the issue,
>> can you please elaborate ?
> 
> TLS sockets are not supposed to get into listen state. IIUC the problem
> is that the user is able to set TLS TOE on a socket which then starts
> to listen and the state gets cloned improperly.
> 
TLS-TOE can go to listen mode, removing listen is not an option and
TLS-TOE support only server mode so if we remove listen then we will not 
have TLS-TOE support which we don't want.

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-09 18:51               ` Vinay Kumar Yadav
@ 2020-11-09 18:58                 ` Jakub Kicinski
  2020-11-10  5:07                   ` Vinay Kumar Yadav
  0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2020-11-09 18:58 UTC (permalink / raw)
  To: Vinay Kumar Yadav; +Cc: netdev, davem, borisp, secdev

On Tue, 10 Nov 2020 00:21:13 +0530 Vinay Kumar Yadav wrote:
> On 11/7/2020 1:58 AM, Jakub Kicinski wrote:
> > On Sat, 7 Nov 2020 02:02:42 +0530 Vinay Kumar Yadav wrote:  
> >> On 11/6/2020 12:16 AM, Jakub Kicinski wrote:  
> >>> On Thu, 5 Nov 2020 23:55:15 +0530 Vinay Kumar Yadav wrote:  
> >>>>>>> We should prevent from the socket getting into LISTEN state in the
> >>>>>>> first place. Can we make a copy of proto_ops (like tls_sw_proto_ops)
> >>>>>>> and set listen to sock_no_listen?  
> >>>>>>
> >>>>>> Once tls-toe (TLS_HW_RECORD) is configured on a socket, listen() call
> >>>>>> from user on same socket will create hash at two places.  
> >>>>>
> >>>>> What I'm saying is - disallow listen calls on sockets with tls-toe
> >>>>> installed on them. Is that not possible?
> >>>>>        
> >>>> You mean socket with tls-toe installed shouldn't be listening at other
> >>>> than adapter? basically avoid ctx->sk_proto->hash(sk) call.  
> >>>
> >>> No, replace the listen callback, like I said. Why are you talking about
> >>> hash???
> >>> As per my understanding we can't avoid socket listen.  
> >> Not sure how replacing listen callback solve the issue,
> >> can you please elaborate ?  
> > 
> > TLS sockets are not supposed to get into listen state. IIUC the problem
> > is that the user is able to set TLS TOE on a socket which then starts
> > to listen and the state gets cloned improperly.
> 
> TLS-TOE can go to listen mode, removing listen is not an option and
> TLS-TOE support only server mode so if we remove listen then we will not 
> have TLS-TOE support which we don't want.

Oh, so it's completely incompatible with kernel tls. How about we
remove the support completely then? Clearly it's not an offload of
kernel tls if it supports completely different mode of operation.

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-09 18:58                 ` Jakub Kicinski
@ 2020-11-10  5:07                   ` Vinay Kumar Yadav
  2020-11-10  5:58                     ` Vinay Kumar Yadav
  2020-11-10 16:28                     ` Jakub Kicinski
  0 siblings, 2 replies; 14+ messages in thread
From: Vinay Kumar Yadav @ 2020-11-10  5:07 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, davem, borisp, secdev



On 11/10/2020 12:28 AM, Jakub Kicinski wrote:
> On Tue, 10 Nov 2020 00:21:13 +0530 Vinay Kumar Yadav wrote:
>> On 11/7/2020 1:58 AM, Jakub Kicinski wrote:
>>> On Sat, 7 Nov 2020 02:02:42 +0530 Vinay Kumar Yadav wrote:
>>>> On 11/6/2020 12:16 AM, Jakub Kicinski wrote:
>>>>> On Thu, 5 Nov 2020 23:55:15 +0530 Vinay Kumar Yadav wrote:
>>>>>>>>> We should prevent from the socket getting into LISTEN state in the
>>>>>>>>> first place. Can we make a copy of proto_ops (like tls_sw_proto_ops)
>>>>>>>>> and set listen to sock_no_listen?
>>>>>>>>
>>>>>>>> Once tls-toe (TLS_HW_RECORD) is configured on a socket, listen() call
>>>>>>>> from user on same socket will create hash at two places.
>>>>>>>
>>>>>>> What I'm saying is - disallow listen calls on sockets with tls-toe
>>>>>>> installed on them. Is that not possible?
>>>>>>>         
>>>>>> You mean socket with tls-toe installed shouldn't be listening at other
>>>>>> than adapter? basically avoid ctx->sk_proto->hash(sk) call.
>>>>>
>>>>> No, replace the listen callback, like I said. Why are you talking about
>>>>> hash???
>>>>> As per my understanding we can't avoid socket listen.
>>>> Not sure how replacing listen callback solve the issue,
>>>> can you please elaborate ?
>>>
>>> TLS sockets are not supposed to get into listen state. IIUC the problem
>>> is that the user is able to set TLS TOE on a socket which then starts
>>> to listen and the state gets cloned improperly.
>>
>> TLS-TOE can go to listen mode, removing listen is not an option and
>> TLS-TOE support only server mode so if we remove listen then we will not
>> have TLS-TOE support which we don't want.
> 
> Oh, so it's completely incompatible with kernel tls. How about we
> remove the support completely then? Clearly it's not an offload of
> kernel tls if it supports completely different mode of operation.
> 

It is not incompatible. It fits in k.org tls infrastructure (TLS-TOE 
mode). For the current issue we have proposed a fix. What is the issue 
with proposed fix, can you elaborate and we will address that?

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-10  5:07                   ` Vinay Kumar Yadav
@ 2020-11-10  5:58                     ` Vinay Kumar Yadav
  2020-11-10 16:28                     ` Jakub Kicinski
  1 sibling, 0 replies; 14+ messages in thread
From: Vinay Kumar Yadav @ 2020-11-10  5:58 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, davem, borisp, secdev



On 11/10/2020 10:37 AM, Vinay Kumar Yadav wrote:
> 
> 
> On 11/10/2020 12:28 AM, Jakub Kicinski wrote:
>> On Tue, 10 Nov 2020 00:21:13 +0530 Vinay Kumar Yadav wrote:
>>> On 11/7/2020 1:58 AM, Jakub Kicinski wrote:
>>>> On Sat, 7 Nov 2020 02:02:42 +0530 Vinay Kumar Yadav wrote:
>>>>> On 11/6/2020 12:16 AM, Jakub Kicinski wrote:
>>>>>> On Thu, 5 Nov 2020 23:55:15 +0530 Vinay Kumar Yadav wrote:
>>>>>>>>>> We should prevent from the socket getting into LISTEN state in 
>>>>>>>>>> the
>>>>>>>>>> first place. Can we make a copy of proto_ops (like 
>>>>>>>>>> tls_sw_proto_ops)
>>>>>>>>>> and set listen to sock_no_listen?
>>>>>>>>>
>>>>>>>>> Once tls-toe (TLS_HW_RECORD) is configured on a socket, 
>>>>>>>>> listen() call
>>>>>>>>> from user on same socket will create hash at two places.
>>>>>>>>
>>>>>>>> What I'm saying is - disallow listen calls on sockets with tls-toe
>>>>>>>> installed on them. Is that not possible?
>>>>>>> You mean socket with tls-toe installed shouldn't be listening at 
>>>>>>> other
>>>>>>> than adapter? basically avoid ctx->sk_proto->hash(sk) call.
>>>>>>
>>>>>> No, replace the listen callback, like I said. Why are you talking 
>>>>>> about
>>>>>> hash???
>>>>>> As per my understanding we can't avoid socket listen.
>>>>> Not sure how replacing listen callback solve the issue,
>>>>> can you please elaborate ?
>>>>
>>>> TLS sockets are not supposed to get into listen state. IIUC the problem
>>>> is that the user is able to set TLS TOE on a socket which then starts
>>>> to listen and the state gets cloned improperly.
>>>
>>> TLS-TOE can go to listen mode, removing listen is not an option and
>>> TLS-TOE support only server mode so if we remove listen then we will not
>>> have TLS-TOE support which we don't want.
>>
>> Oh, so it's completely incompatible with kernel tls. How about we
>> remove the support completely then? Clearly it's not an offload of
>> kernel tls if it supports completely different mode of operation.
>>
> 
> It is not incompatible. It fits in k.org tls infrastructure (TLS-TOE 
> mode). For the current issue we have proposed a fix. What is the issue 
> with proposed fix, can you elaborate and we will address that?
and it is valid TLS offload mode.

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-10  5:07                   ` Vinay Kumar Yadav
  2020-11-10  5:58                     ` Vinay Kumar Yadav
@ 2020-11-10 16:28                     ` Jakub Kicinski
  2020-11-10 19:49                       ` Vinay Kumar Yadav
  1 sibling, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2020-11-10 16:28 UTC (permalink / raw)
  To: Vinay Kumar Yadav; +Cc: netdev, davem, borisp, secdev

On Tue, 10 Nov 2020 10:37:11 +0530 Vinay Kumar Yadav wrote:
> It is not incompatible. It fits in k.org tls infrastructure (TLS-TOE 
> mode). For the current issue we have proposed a fix. What is the issue 
> with proposed fix, can you elaborate and we will address that?

Your lack of understanding of how netdev offloads are supposed to work
is concerning. Application is not supposed to see any difference
between offloaded and non-offloaded modes of operation.

Your offload was accepted based on the assumption that it works like
the software kernel TLS mode. Nobody had the time to look at your
thousands lines of driver code at the time.

Now you're telling us that the uAPI for the offload is completely
different - it only works on listening sockets while software tls 
only works on established sockets. Ergo there is no software fallback
for your offload.

Furthermore the severity of the bugs you just started to fix now, after
the code has been in the kernel for over a year suggests there are no
serious users and we can just remove this code.

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

* Re: [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP
  2020-11-10 16:28                     ` Jakub Kicinski
@ 2020-11-10 19:49                       ` Vinay Kumar Yadav
  0 siblings, 0 replies; 14+ messages in thread
From: Vinay Kumar Yadav @ 2020-11-10 19:49 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, davem, borisp, secdev



On 11/10/2020 9:58 PM, Jakub Kicinski wrote:
> On Tue, 10 Nov 2020 10:37:11 +0530 Vinay Kumar Yadav wrote:
>> It is not incompatible. It fits in k.org tls infrastructure (TLS-TOE
>> mode). For the current issue we have proposed a fix. What is the issue
>> with proposed fix, can you elaborate and we will address that?
> 
> Your lack of understanding of how netdev offloads are supposed to work
> is concerning. Application is not supposed to see any difference
> between offloaded and non-offloaded modes of operation.
> 
For application point of view there won't be any difference.
kernel tls in tcp offload mode works exactly similar to software
kTLS.

> Your offload was accepted based on the assumption that it works like
> the software kernel TLS mode. Nobody had the time to look at your
> thousands lines of driver code at the time.
> 
> Now you're telling us that the uAPI for the offload is completely
> different - it only works on listening sockets while software tls
> only works on established sockets. Ergo there is no software fallback
> for your offload.
>
We can consider adding the capability to working with established 
sockets.The TOE has not needed that capability to date since it can 
establish the socket itself, but it makes sense to provide uniformity 
with the kTLS approach so we will look into that.  For now, as you 
suggested replacing stack listen with toe listen makes more sense.

> Furthermore the severity of the bugs you just started to fix now, after
> the code has been in the kernel for over a year suggests there are no
> serious users and we can just remove this code.
> 
It’s been a slow process but with the new team it is picking up speed
and the quality of the code will continue to get better.

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

end of thread, other threads:[~2020-11-10 19:38 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03 10:47 [PATCH net] net/tls: Fix kernel panic when socket is in TLS ULP Vinay Kumar Yadav
2020-11-05  1:16 ` Jakub Kicinski
2020-11-05 17:50   ` Vinay Kumar Yadav
2020-11-05 17:53     ` Jakub Kicinski
2020-11-05 18:25       ` Vinay Kumar Yadav
2020-11-05 18:46         ` Jakub Kicinski
2020-11-06 20:32           ` Vinay Kumar Yadav
2020-11-06 20:28             ` Jakub Kicinski
2020-11-09 18:51               ` Vinay Kumar Yadav
2020-11-09 18:58                 ` Jakub Kicinski
2020-11-10  5:07                   ` Vinay Kumar Yadav
2020-11-10  5:58                     ` Vinay Kumar Yadav
2020-11-10 16:28                     ` Jakub Kicinski
2020-11-10 19:49                       ` Vinay Kumar Yadav

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