| Message ID | 20241029202830.3121552-1-zijianzhang@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] bpf: Add sk_is_inet check in tls_sw_has_ctx_tx/rx | expand |
On 10/29, zijianzhang@bytedance.com wrote: > From: Zijian Zhang <zijianzhang@bytedance.com> > > As the introduction of the support for vsock and unix sockets in sockmap, > tls_sw_has_ctx_tx/rx cannot presume the socket passed in must be inet. > Otherwise, tls_get_ctx may return an invalid pointer and result in page > fault in function tls_sw_ctx_rx. > > BUG: unable to handle page fault for address: 0000000000040030 > Workqueue: vsock-loopback vsock_loopback_work > RIP: 0010:sk_psock_strp_data_ready+0x23/0x60 > Call Trace: > ? __die+0x81/0xc3 > ? no_context+0x194/0x350 > ? do_page_fault+0x30/0x110 > ? async_page_fault+0x3e/0x50 > ? sk_psock_strp_data_ready+0x23/0x60 > virtio_transport_recv_pkt+0x750/0x800 > ? update_load_avg+0x7e/0x620 > vsock_loopback_work+0xd0/0x100 > process_one_work+0x1a7/0x360 > worker_thread+0x30/0x390 > ? create_worker+0x1a0/0x1a0 > kthread+0x112/0x130 > ? __kthread_cancel_work+0x40/0x40 > ret_from_fork+0x1f/0x40 > > Fixes: 0608c69c9a80 ("bpf: sk_msg, sock{map|hash} redirect through ULP") > Fixes: e91de6afa81c ("bpf: Fix running sk_skb program types with ktls") > > Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com> > --- > include/net/tls.h | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/include/net/tls.h b/include/net/tls.h > index 3a33924db2bc..a65939c7ad61 100644 > --- a/include/net/tls.h > +++ b/include/net/tls.h > @@ -390,8 +390,12 @@ tls_offload_ctx_tx(const struct tls_context *tls_ctx) > > static inline bool tls_sw_has_ctx_tx(const struct sock *sk) > { > - struct tls_context *ctx = tls_get_ctx(sk); > + struct tls_context *ctx; > + > + if (!sk_is_inet(sk)) > + return false; > > + ctx = tls_get_ctx(sk); > if (!ctx) > return false; > return !!tls_sw_ctx_tx(ctx); > @@ -399,8 +403,12 @@ static inline bool tls_sw_has_ctx_tx(const struct sock *sk) > > static inline bool tls_sw_has_ctx_rx(const struct sock *sk) > { > - struct tls_context *ctx = tls_get_ctx(sk); > + struct tls_context *ctx; > + > + if (!sk_is_inet(sk)) > + return false; > > + ctx = tls_get_ctx(sk); > if (!ctx) > return false; > return !!tls_sw_ctx_rx(ctx); This seems like a strange place to fix it. Why does tls_get_ctx return invalid pointer for non-tls/ulp sockets? Shouldn't it be NULL? Is sockmap even supposed to work with vsock?
On 10/29/24 4:07 PM, Stanislav Fomichev wrote: > On 10/29, zijianzhang@bytedance.com wrote: >> From: Zijian Zhang <zijianzhang@bytedance.com> >> >> As the introduction of the support for vsock and unix sockets in sockmap, >> tls_sw_has_ctx_tx/rx cannot presume the socket passed in must be inet. >> Otherwise, tls_get_ctx may return an invalid pointer and result in page >> fault in function tls_sw_ctx_rx. >> >> BUG: unable to handle page fault for address: 0000000000040030 >> Workqueue: vsock-loopback vsock_loopback_work >> RIP: 0010:sk_psock_strp_data_ready+0x23/0x60 >> Call Trace: >> ? __die+0x81/0xc3 >> ? no_context+0x194/0x350 >> ? do_page_fault+0x30/0x110 >> ? async_page_fault+0x3e/0x50 >> ? sk_psock_strp_data_ready+0x23/0x60 >> virtio_transport_recv_pkt+0x750/0x800 >> ? update_load_avg+0x7e/0x620 >> vsock_loopback_work+0xd0/0x100 >> process_one_work+0x1a7/0x360 >> worker_thread+0x30/0x390 >> ? create_worker+0x1a0/0x1a0 >> kthread+0x112/0x130 >> ? __kthread_cancel_work+0x40/0x40 >> ret_from_fork+0x1f/0x40 >> >> Fixes: 0608c69c9a80 ("bpf: sk_msg, sock{map|hash} redirect through ULP") >> Fixes: e91de6afa81c ("bpf: Fix running sk_skb program types with ktls") >> >> Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com> >> --- >> include/net/tls.h | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/include/net/tls.h b/include/net/tls.h >> index 3a33924db2bc..a65939c7ad61 100644 >> --- a/include/net/tls.h >> +++ b/include/net/tls.h >> @@ -390,8 +390,12 @@ tls_offload_ctx_tx(const struct tls_context *tls_ctx) >> >> static inline bool tls_sw_has_ctx_tx(const struct sock *sk) >> { >> - struct tls_context *ctx = tls_get_ctx(sk); >> + struct tls_context *ctx; >> + >> + if (!sk_is_inet(sk)) >> + return false; >> >> + ctx = tls_get_ctx(sk); >> if (!ctx) >> return false; >> return !!tls_sw_ctx_tx(ctx); >> @@ -399,8 +403,12 @@ static inline bool tls_sw_has_ctx_tx(const struct sock *sk) >> >> static inline bool tls_sw_has_ctx_rx(const struct sock *sk) >> { >> - struct tls_context *ctx = tls_get_ctx(sk); >> + struct tls_context *ctx; >> + >> + if (!sk_is_inet(sk)) >> + return false; >> >> + ctx = tls_get_ctx(sk); >> if (!ctx) >> return false; >> return !!tls_sw_ctx_rx(ctx); > > This seems like a strange place to fix it. Why does tls_get_ctx return > invalid pointer for non-tls/ulp sockets? Shouldn't it be NULL? > Is sockmap even supposed to work with vsock? Here is my understanding, please correct me if I am wrong :) ``` static inline struct tls_context *tls_get_ctx(const struct sock *sk) { const struct inet_connection_sock *icsk = inet_csk(sk); return (__force void *)icsk->icsk_ulp_data; } ``` tls_get_ctx assumes the socket passed is icsk_socket. However, unix and vsock do not have inet_connection_sock, they have unix_sock and vsock_sock. The offset of icsk_ulp_data are meaningless for them, and they might point to some other values which might not be NULL. Afaik, sockmap started to support vsock in 634f1a7110b4 ("vsock: support sockmap"), and support unix in 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap"). If the above is correct, I find that using inet_test_bit(IS_ICSK, sk) instead of sk_is_inet will be more accurate.
On 10/29, Zijian Zhang wrote: > > > On 10/29/24 4:07 PM, Stanislav Fomichev wrote: > > On 10/29, zijianzhang@bytedance.com wrote: > > > From: Zijian Zhang <zijianzhang@bytedance.com> > > > > > > As the introduction of the support for vsock and unix sockets in sockmap, > > > tls_sw_has_ctx_tx/rx cannot presume the socket passed in must be inet. > > > Otherwise, tls_get_ctx may return an invalid pointer and result in page > > > fault in function tls_sw_ctx_rx. > > > > > > BUG: unable to handle page fault for address: 0000000000040030 > > > Workqueue: vsock-loopback vsock_loopback_work > > > RIP: 0010:sk_psock_strp_data_ready+0x23/0x60 > > > Call Trace: > > > ? __die+0x81/0xc3 > > > ? no_context+0x194/0x350 > > > ? do_page_fault+0x30/0x110 > > > ? async_page_fault+0x3e/0x50 > > > ? sk_psock_strp_data_ready+0x23/0x60 > > > virtio_transport_recv_pkt+0x750/0x800 > > > ? update_load_avg+0x7e/0x620 > > > vsock_loopback_work+0xd0/0x100 > > > process_one_work+0x1a7/0x360 > > > worker_thread+0x30/0x390 > > > ? create_worker+0x1a0/0x1a0 > > > kthread+0x112/0x130 > > > ? __kthread_cancel_work+0x40/0x40 > > > ret_from_fork+0x1f/0x40 > > > > > > Fixes: 0608c69c9a80 ("bpf: sk_msg, sock{map|hash} redirect through ULP") > > > Fixes: e91de6afa81c ("bpf: Fix running sk_skb program types with ktls") > > > > > > Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com> > > > --- > > > include/net/tls.h | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/net/tls.h b/include/net/tls.h > > > index 3a33924db2bc..a65939c7ad61 100644 > > > --- a/include/net/tls.h > > > +++ b/include/net/tls.h > > > @@ -390,8 +390,12 @@ tls_offload_ctx_tx(const struct tls_context *tls_ctx) > > > static inline bool tls_sw_has_ctx_tx(const struct sock *sk) > > > { > > > - struct tls_context *ctx = tls_get_ctx(sk); > > > + struct tls_context *ctx; > > > + > > > + if (!sk_is_inet(sk)) > > > + return false; > > > + ctx = tls_get_ctx(sk); > > > if (!ctx) > > > return false; > > > return !!tls_sw_ctx_tx(ctx); > > > @@ -399,8 +403,12 @@ static inline bool tls_sw_has_ctx_tx(const struct sock *sk) > > > static inline bool tls_sw_has_ctx_rx(const struct sock *sk) > > > { > > > - struct tls_context *ctx = tls_get_ctx(sk); > > > + struct tls_context *ctx; > > > + > > > + if (!sk_is_inet(sk)) > > > + return false; > > > + ctx = tls_get_ctx(sk); > > > if (!ctx) > > > return false; > > > return !!tls_sw_ctx_rx(ctx); > > > > This seems like a strange place to fix it. Why does tls_get_ctx return > > invalid pointer for non-tls/ulp sockets? Shouldn't it be NULL? > > Is sockmap even supposed to work with vsock? > > Here is my understanding, please correct me if I am wrong :) > ``` > static inline struct tls_context *tls_get_ctx(const struct sock *sk) > { > const struct inet_connection_sock *icsk = inet_csk(sk); > return (__force void *)icsk->icsk_ulp_data; > } > ``` > tls_get_ctx assumes the socket passed is icsk_socket. However, unix > and vsock do not have inet_connection_sock, they have unix_sock and > vsock_sock. The offset of icsk_ulp_data are meaningless for them, and > they might point to some other values which might not be NULL. > > Afaik, sockmap started to support vsock in 634f1a7110b4 ("vsock: support > sockmap"), and support unix in 94531cfcbe79 ("af_unix: Add > unix_stream_proto for sockmap"). > > If the above is correct, I find that using inet_test_bit(IS_ICSK, sk) > instead of sk_is_inet will be more accurate. Thanks for the context, makes sense. And consolidating this sk_is_inet check inside tls_get_ctx is worse because it gets called outside of sockmap?
On 10/29/24 5:22 PM, Stanislav Fomichev wrote: > On 10/29, Zijian Zhang wrote: >> >> >> On 10/29/24 4:07 PM, Stanislav Fomichev wrote: >>> On 10/29, zijianzhang@bytedance.com wrote: ... >>>> diff --git a/include/net/tls.h b/include/net/tls.h >>>> index 3a33924db2bc..a65939c7ad61 100644 >>>> --- a/include/net/tls.h >>>> +++ b/include/net/tls.h >>>> @@ -390,8 +390,12 @@ tls_offload_ctx_tx(const struct tls_context *tls_ctx) >>>> static inline bool tls_sw_has_ctx_tx(const struct sock *sk) >>>> { >>>> - struct tls_context *ctx = tls_get_ctx(sk); >>>> + struct tls_context *ctx; >>>> + >>>> + if (!sk_is_inet(sk)) >>>> + return false; >>>> + ctx = tls_get_ctx(sk); >>>> if (!ctx) >>>> return false; >>>> return !!tls_sw_ctx_tx(ctx); >>>> @@ -399,8 +403,12 @@ static inline bool tls_sw_has_ctx_tx(const struct sock *sk) >>>> static inline bool tls_sw_has_ctx_rx(const struct sock *sk) >>>> { >>>> - struct tls_context *ctx = tls_get_ctx(sk); >>>> + struct tls_context *ctx; >>>> + >>>> + if (!sk_is_inet(sk)) >>>> + return false; >>>> + ctx = tls_get_ctx(sk); >>>> if (!ctx) >>>> return false; >>>> return !!tls_sw_ctx_rx(ctx); >>> >>> This seems like a strange place to fix it. Why does tls_get_ctx return >>> invalid pointer for non-tls/ulp sockets? Shouldn't it be NULL? >>> Is sockmap even supposed to work with vsock? >> >> Here is my understanding, please correct me if I am wrong :) >> ``` >> static inline struct tls_context *tls_get_ctx(const struct sock *sk) >> { >> const struct inet_connection_sock *icsk = inet_csk(sk); >> return (__force void *)icsk->icsk_ulp_data; >> } >> ``` >> tls_get_ctx assumes the socket passed is icsk_socket. However, unix >> and vsock do not have inet_connection_sock, they have unix_sock and >> vsock_sock. The offset of icsk_ulp_data are meaningless for them, and >> they might point to some other values which might not be NULL. >> >> Afaik, sockmap started to support vsock in 634f1a7110b4 ("vsock: support >> sockmap"), and support unix in 94531cfcbe79 ("af_unix: Add >> unix_stream_proto for sockmap"). >> >> If the above is correct, I find that using inet_test_bit(IS_ICSK, sk) >> instead of sk_is_inet will be more accurate. > > Thanks for the context, makes sense. And consolidating this sk_is_inet > check inside tls_get_ctx is worse because it gets called outside of > sockmap? Yes, tls_get_ctx is invoked in multiple locations, and I want to only fix sockmap related calls.
On 10/29, Zijian Zhang wrote: > > On 10/29/24 5:22 PM, Stanislav Fomichev wrote: > > On 10/29, Zijian Zhang wrote: > > > > > > > > > On 10/29/24 4:07 PM, Stanislav Fomichev wrote: > > > > On 10/29, zijianzhang@bytedance.com wrote: > ... > > > > > diff --git a/include/net/tls.h b/include/net/tls.h > > > > > index 3a33924db2bc..a65939c7ad61 100644 > > > > > --- a/include/net/tls.h > > > > > +++ b/include/net/tls.h > > > > > @@ -390,8 +390,12 @@ tls_offload_ctx_tx(const struct tls_context *tls_ctx) > > > > > static inline bool tls_sw_has_ctx_tx(const struct sock *sk) > > > > > { > > > > > - struct tls_context *ctx = tls_get_ctx(sk); > > > > > + struct tls_context *ctx; > > > > > + > > > > > + if (!sk_is_inet(sk)) > > > > > + return false; > > > > > + ctx = tls_get_ctx(sk); > > > > > if (!ctx) > > > > > return false; > > > > > return !!tls_sw_ctx_tx(ctx); > > > > > @@ -399,8 +403,12 @@ static inline bool tls_sw_has_ctx_tx(const struct sock *sk) > > > > > static inline bool tls_sw_has_ctx_rx(const struct sock *sk) > > > > > { > > > > > - struct tls_context *ctx = tls_get_ctx(sk); > > > > > + struct tls_context *ctx; > > > > > + > > > > > + if (!sk_is_inet(sk)) > > > > > + return false; > > > > > + ctx = tls_get_ctx(sk); > > > > > if (!ctx) > > > > > return false; > > > > > return !!tls_sw_ctx_rx(ctx); > > > > > > > > This seems like a strange place to fix it. Why does tls_get_ctx return > > > > invalid pointer for non-tls/ulp sockets? Shouldn't it be NULL? > > > > Is sockmap even supposed to work with vsock? > > > > > > Here is my understanding, please correct me if I am wrong :) > > > ``` > > > static inline struct tls_context *tls_get_ctx(const struct sock *sk) > > > { > > > const struct inet_connection_sock *icsk = inet_csk(sk); > > > return (__force void *)icsk->icsk_ulp_data; > > > } > > > ``` > > > tls_get_ctx assumes the socket passed is icsk_socket. However, unix > > > and vsock do not have inet_connection_sock, they have unix_sock and > > > vsock_sock. The offset of icsk_ulp_data are meaningless for them, and > > > they might point to some other values which might not be NULL. > > > > > > Afaik, sockmap started to support vsock in 634f1a7110b4 ("vsock: support > > > sockmap"), and support unix in 94531cfcbe79 ("af_unix: Add > > > unix_stream_proto for sockmap"). > > > > > > If the above is correct, I find that using inet_test_bit(IS_ICSK, sk) > > > instead of sk_is_inet will be more accurate. > > > > Thanks for the context, makes sense. And consolidating this sk_is_inet > > check inside tls_get_ctx is worse because it gets called outside of > > sockmap? > > Yes, tls_get_ctx is invoked in multiple locations, and I want to only > fix sockmap related calls. Sounds convincing. Unless John/Jakub have better suggestions: Acked-by: Stanislav Fomichev <sdf@fomichev.me>
On 10/30/24 8:38 AM, Stanislav Fomichev wrote: > On 10/29, Zijian Zhang wrote: >> >> On 10/29/24 5:22 PM, Stanislav Fomichev wrote: >>> On 10/29, Zijian Zhang wrote: >>>> >>>> >>>> On 10/29/24 4:07 PM, Stanislav Fomichev wrote: >>>>> On 10/29, zijianzhang@bytedance.com wrote: >> ... >>>>>> diff --git a/include/net/tls.h b/include/net/tls.h >>>>>> index 3a33924db2bc..a65939c7ad61 100644 >>>>>> --- a/include/net/tls.h >>>>>> +++ b/include/net/tls.h >>>>>> @@ -390,8 +390,12 @@ tls_offload_ctx_tx(const struct tls_context *tls_ctx) >>>>>> static inline bool tls_sw_has_ctx_tx(const struct sock *sk) >>>>>> { >>>>>> - struct tls_context *ctx = tls_get_ctx(sk); >>>>>> + struct tls_context *ctx; >>>>>> + >>>>>> + if (!sk_is_inet(sk)) >>>>>> + return false; >>>>>> + ctx = tls_get_ctx(sk); >>>>>> if (!ctx) >>>>>> return false; >>>>>> return !!tls_sw_ctx_tx(ctx); >>>>>> @@ -399,8 +403,12 @@ static inline bool tls_sw_has_ctx_tx(const struct sock *sk) >>>>>> static inline bool tls_sw_has_ctx_rx(const struct sock *sk) >>>>>> { >>>>>> - struct tls_context *ctx = tls_get_ctx(sk); >>>>>> + struct tls_context *ctx; >>>>>> + >>>>>> + if (!sk_is_inet(sk)) >>>>>> + return false; >>>>>> + ctx = tls_get_ctx(sk); >>>>>> if (!ctx) >>>>>> return false; >>>>>> return !!tls_sw_ctx_rx(ctx); >>>>> >>>>> This seems like a strange place to fix it. Why does tls_get_ctx return >>>>> invalid pointer for non-tls/ulp sockets? Shouldn't it be NULL? >>>>> Is sockmap even supposed to work with vsock? >>>> >>>> Here is my understanding, please correct me if I am wrong :) >>>> ``` >>>> static inline struct tls_context *tls_get_ctx(const struct sock *sk) >>>> { >>>> const struct inet_connection_sock *icsk = inet_csk(sk); >>>> return (__force void *)icsk->icsk_ulp_data; >>>> } >>>> ``` >>>> tls_get_ctx assumes the socket passed is icsk_socket. However, unix >>>> and vsock do not have inet_connection_sock, they have unix_sock and >>>> vsock_sock. The offset of icsk_ulp_data are meaningless for them, and >>>> they might point to some other values which might not be NULL. >>>> >>>> Afaik, sockmap started to support vsock in 634f1a7110b4 ("vsock: support >>>> sockmap"), and support unix in 94531cfcbe79 ("af_unix: Add >>>> unix_stream_proto for sockmap"). >>>> >>>> If the above is correct, I find that using inet_test_bit(IS_ICSK, sk) >>>> instead of sk_is_inet will be more accurate. >>> >>> Thanks for the context, makes sense. And consolidating this sk_is_inet >>> check inside tls_get_ctx is worse because it gets called outside of >>> sockmap? >> >> Yes, tls_get_ctx is invoked in multiple locations, and I want to only >> fix sockmap related calls. > > Sounds convincing. Unless John/Jakub have better suggestions: > > Acked-by: Stanislav Fomichev <sdf@fomichev.me> Thanks for the Ack and reviewing! In order to make it more accurate, I added inet_test_bit(IS_ICSK, sk) check in version2. I just found that sk_is_inet only cannot assure inet_csk is valid. For example, udp_sock does not have inet_connection_sock.
On 10/30, Zijian Zhang wrote: > On 10/30/24 8:38 AM, Stanislav Fomichev wrote: > > On 10/29, Zijian Zhang wrote: > > > > > > On 10/29/24 5:22 PM, Stanislav Fomichev wrote: > > > > On 10/29, Zijian Zhang wrote: > > > > > > > > > > > > > > > On 10/29/24 4:07 PM, Stanislav Fomichev wrote: > > > > > > On 10/29, zijianzhang@bytedance.com wrote: > > > ... > > > > > > > diff --git a/include/net/tls.h b/include/net/tls.h > > > > > > > index 3a33924db2bc..a65939c7ad61 100644 > > > > > > > --- a/include/net/tls.h > > > > > > > +++ b/include/net/tls.h > > > > > > > @@ -390,8 +390,12 @@ tls_offload_ctx_tx(const struct tls_context *tls_ctx) > > > > > > > static inline bool tls_sw_has_ctx_tx(const struct sock *sk) > > > > > > > { > > > > > > > - struct tls_context *ctx = tls_get_ctx(sk); > > > > > > > + struct tls_context *ctx; > > > > > > > + > > > > > > > + if (!sk_is_inet(sk)) > > > > > > > + return false; > > > > > > > + ctx = tls_get_ctx(sk); > > > > > > > if (!ctx) > > > > > > > return false; > > > > > > > return !!tls_sw_ctx_tx(ctx); > > > > > > > @@ -399,8 +403,12 @@ static inline bool tls_sw_has_ctx_tx(const struct sock *sk) > > > > > > > static inline bool tls_sw_has_ctx_rx(const struct sock *sk) > > > > > > > { > > > > > > > - struct tls_context *ctx = tls_get_ctx(sk); > > > > > > > + struct tls_context *ctx; > > > > > > > + > > > > > > > + if (!sk_is_inet(sk)) > > > > > > > + return false; > > > > > > > + ctx = tls_get_ctx(sk); > > > > > > > if (!ctx) > > > > > > > return false; > > > > > > > return !!tls_sw_ctx_rx(ctx); > > > > > > > > > > > > This seems like a strange place to fix it. Why does tls_get_ctx return > > > > > > invalid pointer for non-tls/ulp sockets? Shouldn't it be NULL? > > > > > > Is sockmap even supposed to work with vsock? > > > > > > > > > > Here is my understanding, please correct me if I am wrong :) > > > > > ``` > > > > > static inline struct tls_context *tls_get_ctx(const struct sock *sk) > > > > > { > > > > > const struct inet_connection_sock *icsk = inet_csk(sk); > > > > > return (__force void *)icsk->icsk_ulp_data; > > > > > } > > > > > ``` > > > > > tls_get_ctx assumes the socket passed is icsk_socket. However, unix > > > > > and vsock do not have inet_connection_sock, they have unix_sock and > > > > > vsock_sock. The offset of icsk_ulp_data are meaningless for them, and > > > > > they might point to some other values which might not be NULL. > > > > > > > > > > Afaik, sockmap started to support vsock in 634f1a7110b4 ("vsock: support > > > > > sockmap"), and support unix in 94531cfcbe79 ("af_unix: Add > > > > > unix_stream_proto for sockmap"). > > > > > > > > > > If the above is correct, I find that using inet_test_bit(IS_ICSK, sk) > > > > > instead of sk_is_inet will be more accurate. > > > > > > > > Thanks for the context, makes sense. And consolidating this sk_is_inet > > > > check inside tls_get_ctx is worse because it gets called outside of > > > > sockmap? > > > > > > Yes, tls_get_ctx is invoked in multiple locations, and I want to only > > > fix sockmap related calls. > > > > Sounds convincing. Unless John/Jakub have better suggestions: > > > > Acked-by: Stanislav Fomichev <sdf@fomichev.me> > > Thanks for the Ack and reviewing! > > In order to make it more accurate, I added inet_test_bit(IS_ICSK, sk) > check in version2. I just found that sk_is_inet only cannot assure > inet_csk is valid. For example, udp_sock does not have inet_connection_sock. Instead of testing IS_ICSK bit, will inet_csk_has_ulp helper work?
On 10/31/24 9:39 AM, Stanislav Fomichev wrote: > On 10/30, Zijian Zhang wrote: >> On 10/30/24 8:38 AM, Stanislav Fomichev wrote: >> >> Thanks for the Ack and reviewing! >> >> In order to make it more accurate, I added inet_test_bit(IS_ICSK, sk) >> check in version2. I just found that sk_is_inet only cannot assure >> inet_csk is valid. For example, udp_sock does not have inet_connection_sock. > > Instead of testing IS_ICSK bit, will inet_csk_has_ulp helper work? Nice catch, while testing IS_ICSK bit is sufficient, inet_csk_has_ulp is a stricter check. I am good with either approach.
diff --git a/include/net/tls.h b/include/net/tls.h index 3a33924db2bc..a65939c7ad61 100644 --- a/include/net/tls.h +++ b/include/net/tls.h @@ -390,8 +390,12 @@ tls_offload_ctx_tx(const struct tls_context *tls_ctx) static inline bool tls_sw_has_ctx_tx(const struct sock *sk) { - struct tls_context *ctx = tls_get_ctx(sk); + struct tls_context *ctx; + + if (!sk_is_inet(sk)) + return false; + ctx = tls_get_ctx(sk); if (!ctx) return false; return !!tls_sw_ctx_tx(ctx); @@ -399,8 +403,12 @@ static inline bool tls_sw_has_ctx_tx(const struct sock *sk) static inline bool tls_sw_has_ctx_rx(const struct sock *sk) { - struct tls_context *ctx = tls_get_ctx(sk); + struct tls_context *ctx; + + if (!sk_is_inet(sk)) + return false; + ctx = tls_get_ctx(sk); if (!ctx) return false; return !!tls_sw_ctx_rx(ctx);