[PATCH net-next v2 0/2] simplify the tcp_conn_request.

[PATCH net-next v2 0/2] simplify the tcp_conn_request.

[Tonghao Zhang]

These patches are not bugfix. But just simplify the tcp_conn_request
function.

These patches are based on Davem's net-next tree.

Tonghao Zhang (2):
  tcp: Remove unnecessary dst check in tcp_conn_request.
  tcp: Remove the unused parameter for tcp_try_fastopen.

 include/net/tcp.h       |  3 +--
 net/ipv4/tcp_fastopen.c |  6 ++----
 net/ipv4/tcp_input.c    | 11 +++++------
 3 files changed, 8 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[PATCH net-next v2 1/2] tcp: Remove unnecessary dst check in tcp_conn_request.

[Tonghao Zhang]

Because we remove the tcp_tw_recycle support in the commit
4396e46187c ('tcp: remove tcp_tw_recycle') and also delete
the code 'af_ops->route_req' for sysctl_tw_recycle in tcp_conn_request.
Now when we call the 'af_ops->route_req', the dist always is
NULL, and we remove the unnecessay check.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 net/ipv4/tcp_input.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index d73903f..7eee2c7 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6132,11 +6132,10 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 
 		isn = af_ops->init_seq(skb);
 	}
-	if (!dst) {
-		dst = af_ops->route_req(sk, &fl, req);
-		if (!dst)
-			goto drop_and_free;
-	}
+
+	dst = af_ops->route_req(sk, &fl, req);
+	if (!dst)
+		goto drop_and_free;
 
 	tcp_ecn_create_request(req, skb, sk, dst);
 
-- 
1.8.3.1

Re: [PATCH net-next v2 1/2] tcp: Remove unnecessary dst check in tcp_conn_request.

[Tonghao Zhang]

On Mon, Aug 21, 2017 at 10:56 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Please do not top post.
Got it, thanks.

> On Mon, 2017-08-21 at 21:24 +0800, Tonghao Zhang wrote:
>> Thanks,  yes  this is a bug. I found this bug exists from 3.17~ 4.13.
>> The commit is d94e0417
>>
>
> This bug was there at the beginning of git tree.
>
>
>> One question:  should I send a patch for each kernel version because
>> code conflicts ?
>>
>> a patch for v4.12
>> a patch for v4.11
>> a patch for v4.10~v4.7
>> a patch for v4.6~v3.17
>>
>> and
>> a patch for  net-next, because tcp_tw_recycle has been removed.
>>
>
> Given this bug only would matter if syncookies are disabled, I would not
> bother and only target net-next. This does not look serious enough to
> deserve backports to stable versions.
>
OK, thanks again.

>> Thanks very much.
>>
>> On Sun, Aug 20, 2017 at 12:25 PM, David Miller <davem@davemloft.net> wrote:
>> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> > Date: Wed, 16 Aug 2017 20:02:45 -0700
>> >
>> >> Because we remove the tcp_tw_recycle support in the commit
>> >> 4396e46187c ('tcp: remove tcp_tw_recycle') and also delete
>> >> the code 'af_ops->route_req' for sysctl_tw_recycle in tcp_conn_request.
>> >> Now when we call the 'af_ops->route_req', the dist always is
>> >> NULL, and we remove the unnecessay check.
>> >>
>> >> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> >
>> > This is a bug actually, rather than something to paper over
>> > by removing the check.
>> >
>> > Code earlier in this function needs a proper 'dst' in order to operate
>> > properly.
>> >
>> > There is a call to tcp_peer_is_proven() which must have a proper route
>> > to make the determination yet it will always be NULL.
>> >
>> > Please investigate what the code is doing and how a test became
>> > "unnecessary" over time before blindly removing it, thank you.
>
>

Re: [PATCH net-next v2 1/2] tcp: Remove unnecessary dst check in tcp_conn_request.

[Eric Dumazet]

Please do not top post.

On Mon, 2017-08-21 at 21:24 +0800, Tonghao Zhang wrote:
> Thanks,  yes  this is a bug. I found this bug exists from 3.17~ 4.13.
> The commit is d94e0417
> 

This bug was there at the beginning of git tree.


> One question:  should I send a patch for each kernel version because
> code conflicts ?
> 
> a patch for v4.12
> a patch for v4.11
> a patch for v4.10~v4.7
> a patch for v4.6~v3.17
> 
> and
> a patch for  net-next, because tcp_tw_recycle has been removed.
> 

Given this bug only would matter if syncookies are disabled, I would not
bother and only target net-next. This does not look serious enough to
deserve backports to stable versions.


> Thanks very much.
> 
> On Sun, Aug 20, 2017 at 12:25 PM, David Miller <davem@davemloft.net> wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > Date: Wed, 16 Aug 2017 20:02:45 -0700
> >
> >> Because we remove the tcp_tw_recycle support in the commit
> >> 4396e46187c ('tcp: remove tcp_tw_recycle') and also delete
> >> the code 'af_ops->route_req' for sysctl_tw_recycle in tcp_conn_request.
> >> Now when we call the 'af_ops->route_req', the dist always is
> >> NULL, and we remove the unnecessay check.
> >>
> >> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > This is a bug actually, rather than something to paper over
> > by removing the check.
> >
> > Code earlier in this function needs a proper 'dst' in order to operate
> > properly.
> >
> > There is a call to tcp_peer_is_proven() which must have a proper route
> > to make the determination yet it will always be NULL.
> >
> > Please investigate what the code is doing and how a test became
> > "unnecessary" over time before blindly removing it, thank you.

Re: [PATCH net-next v2 1/2] tcp: Remove unnecessary dst check in tcp_conn_request.

[Tonghao Zhang]

Thanks,  yes  this is a bug. I found this bug exists from 3.17~ 4.13.
The commit is d94e0417

One question:  should I send a patch for each kernel version because
code conflicts ?

a patch for v4.12
a patch for v4.11
a patch for v4.10~v4.7
a patch for v4.6~v3.17

and
a patch for  net-next, because tcp_tw_recycle has been removed.

Thanks very much.

On Sun, Aug 20, 2017 at 12:25 PM, David Miller <davem@davemloft.net> wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Date: Wed, 16 Aug 2017 20:02:45 -0700
>
>> Because we remove the tcp_tw_recycle support in the commit
>> 4396e46187c ('tcp: remove tcp_tw_recycle') and also delete
>> the code 'af_ops->route_req' for sysctl_tw_recycle in tcp_conn_request.
>> Now when we call the 'af_ops->route_req', the dist always is
>> NULL, and we remove the unnecessay check.
>>
>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> This is a bug actually, rather than something to paper over
> by removing the check.
>
> Code earlier in this function needs a proper 'dst' in order to operate
> properly.
>
> There is a call to tcp_peer_is_proven() which must have a proper route
> to make the determination yet it will always be NULL.
>
> Please investigate what the code is doing and how a test became
> "unnecessary" over time before blindly removing it, thank you.

Re: [PATCH net-next v2 1/2] tcp: Remove unnecessary dst check in tcp_conn_request.

[David Miller]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Date: Wed, 16 Aug 2017 20:02:45 -0700

> Because we remove the tcp_tw_recycle support in the commit
> 4396e46187c ('tcp: remove tcp_tw_recycle') and also delete
> the code 'af_ops->route_req' for sysctl_tw_recycle in tcp_conn_request.
> Now when we call the 'af_ops->route_req', the dist always is
> NULL, and we remove the unnecessay check.
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

This is a bug actually, rather than something to paper over
by removing the check.

Code earlier in this function needs a proper 'dst' in order to operate
properly.

There is a call to tcp_peer_is_proven() which must have a proper route
to make the determination yet it will always be NULL.

Please investigate what the code is doing and how a test became
"unnecessary" over time before blindly removing it, thank you.

[PATCH net-next v2 1/2] tcp: Remove unnecessary dst check in tcp_conn_request.

[Tonghao Zhang]

Because we remove the tcp_tw_recycle support in the commit
4396e46187c ('tcp: remove tcp_tw_recycle') and also delete
the code 'af_ops->route_req' for sysctl_tw_recycle in tcp_conn_request.
Now when we call the 'af_ops->route_req', the dist always is
NULL, and we remove the unnecessay check.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 net/ipv4/tcp_input.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index d73903f..7eee2c7 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6132,11 +6132,10 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 
 		isn = af_ops->init_seq(skb);
 	}
-	if (!dst) {
-		dst = af_ops->route_req(sk, &fl, req);
-		if (!dst)
-			goto drop_and_free;
-	}
+
+	dst = af_ops->route_req(sk, &fl, req);
+	if (!dst)
+		goto drop_and_free;
 
 	tcp_ecn_create_request(req, skb, sk, dst);
 
-- 
1.8.3.1