* [PATCH bpf] bpf, sockmap: Fix bug that strp_done cannot be called
@ 2023-07-28 10:57 Xu Kuohai
2023-08-01 1:22 ` Martin KaFai Lau
2023-08-01 3:55 ` John Fastabend
0 siblings, 2 replies; 4+ messages in thread
From: Xu Kuohai @ 2023-07-28 10:57 UTC (permalink / raw)
To: bpf, netdev
Cc: John Fastabend, Jakub Sitnicki, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Daniel Borkmann
From: Xu Kuohai <xukuohai@huawei.com>
strp_done is only called when psock->progs.stream_parser is not NULL,
but stream_parser was set to NULL by sk_psock_stop_strp(), called
by sk_psock_drop() earlier. So, strp_done can never be called.
Introduce SK_PSOCK_RX_ENABLED to mark whether there is strp on psock.
Change the condition for calling strp_done from judging whether
stream_parser is set to judging whether this flag is set. This flag is
only set once when strp_init() succeeds, and will never be cleared later.
Fixes: c0d95d3380ee ("bpf, sockmap: Re-evaluate proto ops when psock is removed from sockmap")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
include/linux/skmsg.h | 1 +
net/core/skmsg.c | 10 ++++++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 054d7911bfc9..959c5f4c4d19 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -62,6 +62,7 @@ struct sk_psock_progs {
enum sk_psock_state_bits {
SK_PSOCK_TX_ENABLED,
+ SK_PSOCK_RX_ENABLED,
};
struct sk_psock_link {
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index a29508e1ff35..7c2764beeb04 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -1120,13 +1120,19 @@ static void sk_psock_strp_data_ready(struct sock *sk)
int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
{
+ int ret;
+
static const struct strp_callbacks cb = {
.rcv_msg = sk_psock_strp_read,
.read_sock_done = sk_psock_strp_read_done,
.parse_msg = sk_psock_strp_parse,
};
- return strp_init(&psock->strp, sk, &cb);
+ ret = strp_init(&psock->strp, sk, &cb);
+ if (!ret)
+ sk_psock_set_state(psock, SK_PSOCK_RX_ENABLED);
+
+ return ret;
}
void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
@@ -1154,7 +1160,7 @@ void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
static void sk_psock_done_strp(struct sk_psock *psock)
{
/* Parser has been stopped */
- if (psock->progs.stream_parser)
+ if (sk_psock_test_state(psock, SK_PSOCK_RX_ENABLED))
strp_done(&psock->strp);
}
#else
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH bpf] bpf, sockmap: Fix bug that strp_done cannot be called
2023-07-28 10:57 [PATCH bpf] bpf, sockmap: Fix bug that strp_done cannot be called Xu Kuohai
@ 2023-08-01 1:22 ` Martin KaFai Lau
2023-08-01 3:55 ` John Fastabend
1 sibling, 0 replies; 4+ messages in thread
From: Martin KaFai Lau @ 2023-08-01 1:22 UTC (permalink / raw)
To: Xu Kuohai, John Fastabend
Cc: netdev, bpf, Jakub Sitnicki, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Daniel Borkmann
On 7/28/23 3:57 AM, Xu Kuohai wrote:
> strp_done is only called when psock->progs.stream_parser is not NULL,
> but stream_parser was set to NULL by sk_psock_stop_strp(), called
> by sk_psock_drop() earlier. So, strp_done can never be called.
>
> Introduce SK_PSOCK_RX_ENABLED to mark whether there is strp on psock.
> Change the condition for calling strp_done from judging whether
> stream_parser is set to judging whether this flag is set. This flag is
> only set once when strp_init() succeeds, and will never be cleared later.
John, please help to review.
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH bpf] bpf, sockmap: Fix bug that strp_done cannot be called
2023-07-28 10:57 [PATCH bpf] bpf, sockmap: Fix bug that strp_done cannot be called Xu Kuohai
2023-08-01 1:22 ` Martin KaFai Lau
@ 2023-08-01 3:55 ` John Fastabend
2023-08-01 6:17 ` Xu Kuohai
1 sibling, 1 reply; 4+ messages in thread
From: John Fastabend @ 2023-08-01 3:55 UTC (permalink / raw)
To: Xu Kuohai, bpf, netdev
Cc: John Fastabend, Jakub Sitnicki, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Daniel Borkmann
Xu Kuohai wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
>
> strp_done is only called when psock->progs.stream_parser is not NULL,
> but stream_parser was set to NULL by sk_psock_stop_strp(), called
> by sk_psock_drop() earlier. So, strp_done can never be called.
>
> Introduce SK_PSOCK_RX_ENABLED to mark whether there is strp on psock.
> Change the condition for calling strp_done from judging whether
> stream_parser is set to judging whether this flag is set. This flag is
> only set once when strp_init() succeeds, and will never be cleared later.
>
> Fixes: c0d95d3380ee ("bpf, sockmap: Re-evaluate proto ops when psock is removed from sockmap")
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
> include/linux/skmsg.h | 1 +
> net/core/skmsg.c | 10 ++++++++--
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> index 054d7911bfc9..959c5f4c4d19 100644
> --- a/include/linux/skmsg.h
> +++ b/include/linux/skmsg.h
> @@ -62,6 +62,7 @@ struct sk_psock_progs {
>
> enum sk_psock_state_bits {
> SK_PSOCK_TX_ENABLED,
> + SK_PSOCK_RX_ENABLED,
small nit can be make this SK_PSOCK_RX_STRP_ENABLED? That way its
explicit what we are talking about here.
Otherwise it looks good thanks nice catch.
> };
>
> struct sk_psock_link {
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index a29508e1ff35..7c2764beeb04 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -1120,13 +1120,19 @@ static void sk_psock_strp_data_ready(struct sock *sk)
>
> int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
> {
> + int ret;
> +
> static const struct strp_callbacks cb = {
> .rcv_msg = sk_psock_strp_read,
> .read_sock_done = sk_psock_strp_read_done,
> .parse_msg = sk_psock_strp_parse,
> };
>
> - return strp_init(&psock->strp, sk, &cb);
> + ret = strp_init(&psock->strp, sk, &cb);
> + if (!ret)
> + sk_psock_set_state(psock, SK_PSOCK_RX_ENABLED);
> +
> + return ret;
> }
>
> void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
> @@ -1154,7 +1160,7 @@ void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
> static void sk_psock_done_strp(struct sk_psock *psock)
> {
> /* Parser has been stopped */
> - if (psock->progs.stream_parser)
> + if (sk_psock_test_state(psock, SK_PSOCK_RX_ENABLED))
> strp_done(&psock->strp);
> }
> #else
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf] bpf, sockmap: Fix bug that strp_done cannot be called
2023-08-01 3:55 ` John Fastabend
@ 2023-08-01 6:17 ` Xu Kuohai
0 siblings, 0 replies; 4+ messages in thread
From: Xu Kuohai @ 2023-08-01 6:17 UTC (permalink / raw)
To: John Fastabend, bpf, netdev
Cc: Jakub Sitnicki, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Daniel Borkmann
On 8/1/2023 11:55 AM, John Fastabend wrote:
> Xu Kuohai wrote:
>> From: Xu Kuohai <xukuohai@huawei.com>
>>
>> strp_done is only called when psock->progs.stream_parser is not NULL,
>> but stream_parser was set to NULL by sk_psock_stop_strp(), called
>> by sk_psock_drop() earlier. So, strp_done can never be called.
>>
>> Introduce SK_PSOCK_RX_ENABLED to mark whether there is strp on psock.
>> Change the condition for calling strp_done from judging whether
>> stream_parser is set to judging whether this flag is set. This flag is
>> only set once when strp_init() succeeds, and will never be cleared later.
>>
>> Fixes: c0d95d3380ee ("bpf, sockmap: Re-evaluate proto ops when psock is removed from sockmap")
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>> include/linux/skmsg.h | 1 +
>> net/core/skmsg.c | 10 ++++++++--
>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
>> index 054d7911bfc9..959c5f4c4d19 100644
>> --- a/include/linux/skmsg.h
>> +++ b/include/linux/skmsg.h
>> @@ -62,6 +62,7 @@ struct sk_psock_progs {
>>
>> enum sk_psock_state_bits {
>> SK_PSOCK_TX_ENABLED,
>> + SK_PSOCK_RX_ENABLED,
>
> small nit can be make this SK_PSOCK_RX_STRP_ENABLED? That way its
> explicit what we are talking about here.
>
OK, I'll rename it, thanks.
> Otherwise it looks good thanks nice catch.
>
>> };
>>
>> struct sk_psock_link {
>> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
>> index a29508e1ff35..7c2764beeb04 100644
>> --- a/net/core/skmsg.c
>> +++ b/net/core/skmsg.c
>> @@ -1120,13 +1120,19 @@ static void sk_psock_strp_data_ready(struct sock *sk)
>>
>> int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
>> {
>> + int ret;
>> +
>> static const struct strp_callbacks cb = {
>> .rcv_msg = sk_psock_strp_read,
>> .read_sock_done = sk_psock_strp_read_done,
>> .parse_msg = sk_psock_strp_parse,
>> };
>>
>> - return strp_init(&psock->strp, sk, &cb);
>> + ret = strp_init(&psock->strp, sk, &cb);
>> + if (!ret)
>> + sk_psock_set_state(psock, SK_PSOCK_RX_ENABLED);
>> +
>> + return ret;
>> }
>>
>> void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
>> @@ -1154,7 +1160,7 @@ void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
>> static void sk_psock_done_strp(struct sk_psock *psock)
>> {
>> /* Parser has been stopped */
>> - if (psock->progs.stream_parser)
>> + if (sk_psock_test_state(psock, SK_PSOCK_RX_ENABLED))
>> strp_done(&psock->strp);
>> }
>> #else
>> --
>> 2.30.2
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-08-01 6:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-28 10:57 [PATCH bpf] bpf, sockmap: Fix bug that strp_done cannot be called Xu Kuohai
2023-08-01 1:22 ` Martin KaFai Lau
2023-08-01 3:55 ` John Fastabend
2023-08-01 6:17 ` Xu Kuohai
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).