diff mbox series

[2/3] mptcp: remove redundant req destruct in subflow_check_req()

Message ID 6747dc58-0dbf-b4d3-e084-85816ad5caec@163.com (mailing list archive)
State Superseded, archived
Delegated to: Paolo Abeni
Headers show
Series [1/3] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join | expand

Commit Message

Jianguo Wu June 9, 2021, 10:39 a.m. UTC 
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(-)

Comments

Paolo Abeni June 9, 2021, 2:23 p.m. UTC | #1 
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;
>  			}
Jianguo Wu June 10, 2021, 3:28 a.m. UTC | #2 
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;
>>  			}
>
Paolo Abeni June 10, 2021, 10:02 a.m. UTC | #3 
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
diff mbox series

Patch

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;
 			}