* [PATCH 2/3] mptcp: remove redundant req destruct in subflow_check_req()
@ 2021-06-09 10:39 Jianguo Wu
2021-06-09 14:23 ` Paolo Abeni
0 siblings, 1 reply; 4+ messages in thread
From: Jianguo Wu @ 2021-06-09 10:39 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
From: Jianguo Wu <wujianguo@chinatelecom.cn>
In subflow_check_req(), if subflow sport is mismatch, will put msk,
destroy token, and destruct req, then return -EPERM, which can be
done by subflow_req_destructor() via:
tcp_conn_request()
|--__reqsk_free()
|--subflow_req_destructor()
So we should remove these redundant code, otherwise will call tcp_v4_reqsk_destructor()
twice, and may double free inet_rsk(req)->ireq_opt.
Fixes: 5bc56388c74f ("mptcp: add port number check for MP_JOIN")
Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
---
net/mptcp/subflow.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 6b1cd42..75ed530 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -213,11 +213,6 @@ static int subflow_check_req(struct request_sock *req,
ntohs(inet_sk(sk_listener)->inet_sport),
ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) {
- sock_put((struct sock *)subflow_req->msk);
- mptcp_token_destroy_request(req);
- tcp_request_sock_ops.destructor(req);
- subflow_req->msk = NULL;
- subflow_req->mp_join = 0;
SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX);
return -EPERM;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] mptcp: remove redundant req destruct in subflow_check_req()
2021-06-09 10:39 [PATCH 2/3] mptcp: remove redundant req destruct in subflow_check_req() Jianguo Wu
@ 2021-06-09 14:23 ` Paolo Abeni
2021-06-10 3:28 ` Jianguo Wu
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Abeni @ 2021-06-09 14:23 UTC (permalink / raw)
To: Jianguo Wu, mptcp; +Cc: Geliang Tang
On Wed, 2021-06-09 at 18:39 +0800, Jianguo Wu wrote:
> From: Jianguo Wu <wujianguo@chinatelecom.cn>
>
> In subflow_check_req(), if subflow sport is mismatch, will put msk,
> destroy token, and destruct req, then return -EPERM, which can be
> done by subflow_req_destructor() via:
> tcp_conn_request()
> |--__reqsk_free()
> |--subflow_req_destructor()
> So we should remove these redundant code, otherwise will call tcp_v4_reqsk_destructor()
> twice, and may double free inet_rsk(req)->ireq_opt.
_if_ I read the above correctly, there is no duplicated test, we should
just drop the destructor invocation otherwise we could not enforce the
sport check?!? am I missing something?!?
> Fixes: 5bc56388c74f ("mptcp: add port number check for MP_JOIN")
> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
> ---
> net/mptcp/subflow.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 6b1cd42..75ed530 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -213,11 +213,6 @@ static int subflow_check_req(struct request_sock *req,
> ntohs(inet_sk(sk_listener)->inet_sport),
> ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
> if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) {
> - sock_put((struct sock *)subflow_req->msk);
> - mptcp_token_destroy_request(req);
> - tcp_request_sock_ops.destructor(req);
I mean: we just need to drop the above line.
> - subflow_req->msk = NULL;
> - subflow_req->mp_join = 0;
> SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX);
> return -EPERM;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] mptcp: remove redundant req destruct in subflow_check_req()
2021-06-09 14:23 ` Paolo Abeni
@ 2021-06-10 3:28 ` Jianguo Wu
2021-06-10 10:02 ` Paolo Abeni
0 siblings, 1 reply; 4+ messages in thread
From: Jianguo Wu @ 2021-06-10 3:28 UTC (permalink / raw)
To: Paolo Abeni, mptcp; +Cc: Geliang Tang
Hi Paolo,
On 2021/6/9 22:23, Paolo Abeni wrote:
> On Wed, 2021-06-09 at 18:39 +0800, Jianguo Wu wrote:
>> From: Jianguo Wu <wujianguo@chinatelecom.cn>
>>
>> In subflow_check_req(), if subflow sport is mismatch, will put msk,
>> destroy token, and destruct req, then return -EPERM, which can be
>> done by subflow_req_destructor() via:
>> tcp_conn_request()
>> |--__reqsk_free()
>> |--subflow_req_destructor()
>> So we should remove these redundant code, otherwise will call tcp_v4_reqsk_destructor()
>> twice, and may double free inet_rsk(req)->ireq_opt.
>
> _if_ I read the above correctly, there is no duplicated test, we should
> just drop the destructor invocation otherwise we could not enforce the
> sport check?!? am I missing something?!?
Sorry, I don't quite understand, I think the sport check is untouched by this patch.
>
>> Fixes: 5bc56388c74f ("mptcp: add port number check for MP_JOIN")
>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
>> ---
>> net/mptcp/subflow.c | 5 -----
>> 1 file changed, 5 deletions(-)
>>
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 6b1cd42..75ed530 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -213,11 +213,6 @@ static int subflow_check_req(struct request_sock *req,
>> ntohs(inet_sk(sk_listener)->inet_sport),
>> ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
>> if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) {
>
>
>> - sock_put((struct sock *)subflow_req->msk);
>> - mptcp_token_destroy_request(req);
>> - tcp_request_sock_ops.destructor(req);
>
> I mean: we just need to drop the above line.
>
subflow_req_destructor() will do sock_put((struct sock *)subflow_req->msk)(if subflow_req->msk not NULL),
mptcp_token_destroy_request(req) and tcp_request_sock_ops.destructor(req).
>> - subflow_req->msk = NULL;
>> - subflow_req->mp_join = 0;
and subflow_req->mp_join won't be used again, so can drop all this five lines?
let tcp_conn_request()->__reqsk_free()->subflow_req_destructor() do this?
Thanks,
Jianguo
>> SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX);
>> return -EPERM;
>> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] mptcp: remove redundant req destruct in subflow_check_req()
2021-06-10 3:28 ` Jianguo Wu
@ 2021-06-10 10:02 ` Paolo Abeni
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Abeni @ 2021-06-10 10:02 UTC (permalink / raw)
To: Jianguo Wu, mptcp; +Cc: Geliang Tang
On Thu, 2021-06-10 at 11:28 +0800, Jianguo Wu wrote:
> Hi Paolo,
>
> On 2021/6/9 22:23, Paolo Abeni wrote:
> > On Wed, 2021-06-09 at 18:39 +0800, Jianguo Wu wrote:
> > > From: Jianguo Wu <wujianguo@chinatelecom.cn>
> > >
> > > In subflow_check_req(), if subflow sport is mismatch, will put msk,
> > > destroy token, and destruct req, then return -EPERM, which can be
> > > done by subflow_req_destructor() via:
> > > tcp_conn_request()
> > > |--__reqsk_free()
> > > |--subflow_req_destructor()
> > > So we should remove these redundant code, otherwise will call tcp_v4_reqsk_destructor()
> > > twice, and may double free inet_rsk(req)->ireq_opt.
> >
> > _if_ I read the above correctly, there is no duplicated test, we should
> > just drop the destructor invocation otherwise we could not enforce the
> > sport check?!? am I missing something?!?
>
> Sorry, I don't quite understand, I think the sport check is untouched by this patch.
Whoops, sorry, I misread the patch - I should fetch more coffee ;)
>
> > > Fixes: 5bc56388c74f ("mptcp: add port number check for MP_JOIN")
> > > Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
> > > ---
> > > net/mptcp/subflow.c | 5 -----
> > > 1 file changed, 5 deletions(-)
> > >
> > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > > index 6b1cd42..75ed530 100644
> > > --- a/net/mptcp/subflow.c
> > > +++ b/net/mptcp/subflow.c
> > > @@ -213,11 +213,6 @@ static int subflow_check_req(struct request_sock *req,
> > > ntohs(inet_sk(sk_listener)->inet_sport),
> > > ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
> > > if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) {
> > > - sock_put((struct sock *)subflow_req->msk);
> > > - mptcp_token_destroy_request(req);
> > > - tcp_request_sock_ops.destructor(req);
> >
> > I mean: we just need to drop the above line.
> >
>
> subflow_req_destructor() will do sock_put((struct sock *)subflow_req->msk)(if subflow_req->msk not NULL),
> mptcp_token_destroy_request(req) and tcp_request_sock_ops.destructor(req).
>
> > > - subflow_req->msk = NULL;
> > > - subflow_req->mp_join = 0;
>
> and subflow_req->mp_join won't be used again, so can drop all this five lines?
> let tcp_conn_request()->__reqsk_free()->subflow_req_destructor() do this?
Yep, looks safe as-is. No changes needed to this patch, AFAICS.
Thanks!
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-10 10:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-09 10:39 [PATCH 2/3] mptcp: remove redundant req destruct in subflow_check_req() Jianguo Wu
2021-06-09 14:23 ` Paolo Abeni
2021-06-10 3:28 ` Jianguo Wu
2021-06-10 10:02 ` Paolo Abeni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.