| Message ID | 20201103104702.798-1-vinay.yadav@chelsio.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net] net/tls: Fix kernel panic when socket is in TLS ULP | expand |
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?
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?
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?
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? >
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???
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.
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 ?
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.
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.
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?
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.
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.
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.
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)
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(-)