[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.

[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.

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.

[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]

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.
>
>

[PATCH net-next v2 2/2] tcp: Remove the unused parameter for tcp_try_fastopen.

[Tonghao Zhang]

The parameter dst of tcp_try_fastopen and tcp_fastopen_create_child is
not used anymore. We remove it.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 include/net/tcp.h       | 3 +--
 net/ipv4/tcp_fastopen.c | 6 ++----
 net/ipv4/tcp_input.c    | 2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index afdab37..a995004 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1533,8 +1533,7 @@ struct tcp_fastopen_request {
 void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb);
 struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
 			      struct request_sock *req,
-			      struct tcp_fastopen_cookie *foc,
-			      struct dst_entry *dst);
+			      struct tcp_fastopen_cookie *foc);
 void tcp_fastopen_init_key_once(bool publish);
 bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
 			     struct tcp_fastopen_cookie *cookie);
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index ce9c7fe..e3c3322 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -171,7 +171,6 @@ void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb)
 
 static struct sock *tcp_fastopen_create_child(struct sock *sk,
 					      struct sk_buff *skb,
-					      struct dst_entry *dst,
 					      struct request_sock *req)
 {
 	struct tcp_sock *tp;
@@ -278,8 +277,7 @@ static bool tcp_fastopen_queue_check(struct sock *sk)
  */
 struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
 			      struct request_sock *req,
-			      struct tcp_fastopen_cookie *foc,
-			      struct dst_entry *dst)
+			      struct tcp_fastopen_cookie *foc)
 {
 	struct tcp_fastopen_cookie valid_foc = { .len = -1 };
 	bool syn_data = TCP_SKB_CB(skb)->end_seq != TCP_SKB_CB(skb)->seq + 1;
@@ -312,7 +310,7 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
 		 * data in SYN_RECV state.
 		 */
 fastopen:
-		child = tcp_fastopen_create_child(sk, skb, dst, req);
+		child = tcp_fastopen_create_child(sk, skb, req);
 		if (child) {
 			foc->len = -1;
 			NET_INC_STATS(sock_net(sk),
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7eee2c7..21df086 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6151,7 +6151,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	tcp_openreq_init_rwin(req, sk, dst);
 	if (!want_cookie) {
 		tcp_reqsk_record_syn(sk, req, skb);
-		fastopen_sk = tcp_try_fastopen(sk, skb, req, &foc, dst);
+		fastopen_sk = tcp_try_fastopen(sk, skb, req, &foc);
 	}
 	if (fastopen_sk) {
 		af_ops->send_synack(fastopen_sk, dst, &fl, req,
-- 
1.8.3.1

[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