[PATCH net-next v3 1/2] net: tcp_ipv6 do some cleanup

[PATCH net-next v3 1/2] net: tcp_ipv6 do some cleanup

[Wangyufen]

From: Wang Yufen <wangyufen@huawei.com>


Signed-off-by: Wang Yufen <wangyufen@huawei.com>
---
 net/ipv6/tcp_ipv6.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 3277680..f6f38fe 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -39,7 +39,7 @@
 #include <linux/ipsec.h>
 #include <linux/times.h>
 #include <linux/slab.h>
-
+#include <linux/uaccess.h>
 #include <linux/ipv6.h>
 #include <linux/icmpv6.h>
 #include <linux/random.h>
@@ -65,8 +65,6 @@
 #include <net/tcp_memcontrol.h>
 #include <net/busy_poll.h>
 
-#include <asm/uaccess.h>
-
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 
@@ -532,7 +530,7 @@ static struct tcp_md5sig_key *tcp_v6_reqsk_md5_lookup(struct sock *sk,
 	return tcp_v6_md5_do_lookup(sk, &inet_rsk(req)->ir_v6_rmt_addr);
 }
 
-static int tcp_v6_parse_md5_keys (struct sock *sk, char __user *optval,
+static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
 				  int optlen)
 {
 	struct tcp_md5sig cmd;
@@ -717,7 +715,7 @@ struct request_sock_ops tcp6_request_sock_ops __read_mostly = {
 	.send_ack	=	tcp_v6_reqsk_send_ack,
 	.destructor	=	tcp_v6_reqsk_destructor,
 	.send_reset	=	tcp_v6_send_reset,
-	.syn_ack_timeout = 	tcp_syn_ack_timeout,
+	.syn_ack_timeout =	tcp_syn_ack_timeout,
 };
 
 #ifdef CONFIG_TCP_MD5SIG
@@ -1261,7 +1259,8 @@ static struct sock *tcp_v6_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
 
 #ifdef CONFIG_TCP_MD5SIG
 	/* Copy over the MD5 key from the original socket */
-	if ((key = tcp_v6_md5_do_lookup(sk, &newsk->sk_v6_daddr)) != NULL) {
+	key = tcp_v6_md5_do_lookup(sk, &newsk->sk_v6_daddr);
+	if (key != NULL) {
 		/* We're using one, so create a matching key
 		 * on the newsk structure. If we fail to get
 		 * memory, then we end up not copying the key
@@ -1305,9 +1304,8 @@ static __sum16 tcp_v6_checksum_init(struct sk_buff *skb)
 					      &ipv6_hdr(skb)->saddr,
 					      &ipv6_hdr(skb)->daddr, 0));
 
-	if (skb->len <= 76) {
+	if (skb->len <= 76)
 		return __skb_checksum_complete(skb);
-	}
 	return 0;
 }
 
@@ -1337,7 +1335,7 @@ static int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 		return tcp_v4_do_rcv(sk, skb);
 
 #ifdef CONFIG_TCP_MD5SIG
-	if (tcp_v6_inbound_md5_hash (sk, skb))
+	if (tcp_v6_inbound_md5_hash(sk, skb))
 		goto discard;
 #endif
 
@@ -1604,7 +1602,8 @@ do_time_wait:
 		break;
 	case TCP_TW_RST:
 		goto no_tcp_socket;
-	case TCP_TW_SUCCESS:;
+	case TCP_TW_SUCCESS:
+		;
 	}
 	goto discard_it;
 }
@@ -1649,7 +1648,7 @@ static void tcp_v6_early_demux(struct sk_buff *skb)
 static struct timewait_sock_ops tcp6_timewait_sock_ops = {
 	.twsk_obj_size	= sizeof(struct tcp6_timewait_sock),
 	.twsk_unique	= tcp_twsk_unique,
-	.twsk_destructor= tcp_twsk_destructor,
+	.twsk_destructor = tcp_twsk_destructor,
 };
 
 static const struct inet_connection_sock_af_ops ipv6_specific = {
@@ -1683,7 +1682,6 @@ static const struct tcp_sock_af_ops tcp_sock_ipv6_specific = {
 /*
  *	TCP over IPv4 via INET6 API
  */
-
 static const struct inet_connection_sock_af_ops ipv6_mapped = {
 	.queue_xmit	   = ip_queue_xmit,
 	.send_check	   = tcp_v4_send_check,
-- 
1.7.12

[PATCH net-next v3 2/2] net: tcp_ipv6 policy route issue

[Wangyufen]

From: Wang Yufen <wangyufen@huawei.com>

The issue raises when adding policy route, specify a particular
NIC as oif, the policy route did not take effect. The reason is
that fl6.oif is not set and route map failed. From the 
tcp_v6_send_response function, if the binding address is linklocal,
fl6.oif is set, but not for global address.

Signed-off-by: Wang Yufen <wangyufen@huawei.com>
---
 net/ipv6/tcp_ipv6.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index f6f38fe..066f36f 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -726,7 +726,7 @@ static const struct tcp_request_sock_ops tcp_request_sock_ipv6_ops = {
 #endif
 
 static void tcp_v6_send_response(struct sk_buff *skb, u32 seq, u32 ack, u32 win,
-				 u32 tsval, u32 tsecr,
+				 u32 tsval, u32 tsecr, int oif,
 				 struct tcp_md5sig_key *key, int rst, u8 tclass,
 				 u32 label)
 {
@@ -802,6 +802,8 @@ static void tcp_v6_send_response(struct sk_buff *skb, u32 seq, u32 ack, u32 win,
 		fl6.flowi6_oif = inet6_iif(skb);
 	fl6.fl6_dport = t1->dest;
 	fl6.fl6_sport = t1->source;
+	if (fl6.flowi6_oif == 0)
+		fl6.flowi6_oif = oif;
 	security_skb_classify_flow(skb, flowi6_to_flowi(&fl6));
 
 	/* Pass a socket to ip6_dst_lookup either it is for RST
@@ -833,6 +835,7 @@ static void tcp_v6_send_reset(struct sock *sk, struct sk_buff *skb)
 	int genhash;
 	struct sock *sk1 = NULL;
 #endif
+	int oif;
 
 	if (th->rst)
 		return;
@@ -876,7 +879,8 @@ static void tcp_v6_send_reset(struct sock *sk, struct sk_buff *skb)
 		ack_seq = ntohl(th->seq) + th->syn + th->fin + skb->len -
 			  (th->doff << 2);
 
-	tcp_v6_send_response(skb, seq, ack_seq, 0, 0, 0, key, 1, 0, 0);
+	oif = sk ? sk->sk_bound_dev_if : 0;
+	tcp_v6_send_response(skb, seq, ack_seq, 0, 0, 0, oif, key, 1, 0, 0);
 
 #ifdef CONFIG_TCP_MD5SIG
 release_sk1:
@@ -888,11 +892,11 @@ release_sk1:
 }
 
 static void tcp_v6_send_ack(struct sk_buff *skb, u32 seq, u32 ack,
-			    u32 win, u32 tsval, u32 tsecr,
+			    u32 win, u32 tsval, u32 tsecr, int oif,
 			    struct tcp_md5sig_key *key, u8 tclass,
 			    u32 label)
 {
-	tcp_v6_send_response(skb, seq, ack, win, tsval, tsecr, key, 0, tclass,
+	tcp_v6_send_response(skb, seq, ack, win, tsval, tsecr, oif, key, 0, tclass,
 			     label);
 }
 
@@ -904,7 +908,7 @@ static void tcp_v6_timewait_ack(struct sock *sk, struct sk_buff *skb)
 	tcp_v6_send_ack(skb, tcptw->tw_snd_nxt, tcptw->tw_rcv_nxt,
 			tcptw->tw_rcv_wnd >> tw->tw_rcv_wscale,
 			tcp_time_stamp + tcptw->tw_ts_offset,
-			tcptw->tw_ts_recent, tcp_twsk_md5_key(tcptw),
+			tcptw->tw_ts_recent, tw->tw_bound_dev_if, tcp_twsk_md5_key(tcptw),
 			tw->tw_tclass, (tw->tw_flowlabel << 12));
 
 	inet_twsk_put(tw);
@@ -914,7 +918,7 @@ static void tcp_v6_reqsk_send_ack(struct sock *sk, struct sk_buff *skb,
 				  struct request_sock *req)
 {
 	tcp_v6_send_ack(skb, tcp_rsk(req)->snt_isn + 1, tcp_rsk(req)->rcv_isn + 1,
-			req->rcv_wnd, tcp_time_stamp, req->ts_recent,
+			req->rcv_wnd, tcp_time_stamp, req->ts_recent, sk->sk_bound_dev_if,
 			tcp_v6_md5_do_lookup(sk, &ipv6_hdr(skb)->daddr),
 			0, 0);
 }
-- 
1.7.12

Re: [PATCH net-next v3 2/2] net: tcp_ipv6 policy route issue

[Hannes Frederic Sowa]

On Mon, Mar 24, 2014 at 02:25:22PM +0800, Wangyufen wrote:
> From: Wang Yufen <wangyufen@huawei.com>
> 
> The issue raises when adding policy route, specify a particular
> NIC as oif, the policy route did not take effect. The reason is
> that fl6.oif is not set and route map failed. From the 
> tcp_v6_send_response function, if the binding address is linklocal,
> fl6.oif is set, but not for global address.
> 
> Signed-off-by: Wang Yufen <wangyufen@huawei.com>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

But this looks like a bug to me, so maybe this is something for net
inclusion.

Thanks,

  Hannes

Re: [PATCH net-next v3 1/2] net: tcp_ipv6 do some cleanup

[David Miller]

From: Wangyufen <wangyufen@huawei.com>
Date: Mon, 24 Mar 2014 14:25:21 +0800

> -static int tcp_v6_parse_md5_keys (struct sock *sk, char __user *optval,
> +static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
>  				  int optlen)

If you change the first line, you must adjust the indentation of the second
line so that "int optlen)" begins precisely at the first column after the
openning parenthesis on the first line.

Re: [PATCH net-next v3 1/2] net: tcp_ipv6 do some cleanup

[Joe Perches]

On Wed, 2014-03-26 at 15:42 -0400, David Miller wrote:
> From: Wangyufen <wangyufen@huawei.com>
> Date: Mon, 24 Mar 2014 14:25:21 +0800
> 
> > -static int tcp_v6_parse_md5_keys (struct sock *sk, char __user *optval,
> > +static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
> >  				  int optlen)
> 
> If you change the first line, you must adjust the indentation of the second
> line so that "int optlen)" begins precisely at the first column after the
> openning parenthesis on the first line.

checkpatch's --strict option emits a message about alignment
issues like this.

David, do you want the --strict option to be auto-enabled for
patches of files in drivers/net and net/?

Re: [PATCH net-next v3 1/2] net: tcp_ipv6 do some cleanup

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Wed, 26 Mar 2014 13:40:43 -0700

> On Wed, 2014-03-26 at 15:42 -0400, David Miller wrote:
>> From: Wangyufen <wangyufen@huawei.com>
>> Date: Mon, 24 Mar 2014 14:25:21 +0800
>> 
>> > -static int tcp_v6_parse_md5_keys (struct sock *sk, char __user *optval,
>> > +static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
>> >  				  int optlen)
>> 
>> If you change the first line, you must adjust the indentation of the second
>> line so that "int optlen)" begins precisely at the first column after the
>> openning parenthesis on the first line.
> 
> checkpatch's --strict option emits a message about alignment
> issues like this.
> 
> David, do you want the --strict option to be auto-enabled for
> patches of files in drivers/net and net/?

I know this doesn't help, but I'm ambivalent.

Re: [PATCH net-next v3 1/2] net: tcp_ipv6 do some cleanup

[wangyufen]

On 2014/3/27 3:42, David Miller wrote:
> From: Wangyufen <wangyufen@huawei.com>
> Date: Mon, 24 Mar 2014 14:25:21 +0800
> 
>> -static int tcp_v6_parse_md5_keys (struct sock *sk, char __user *optval,
>> +static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
>>  				  int optlen)
> 
> If you change the first line, you must adjust the indentation of the second
> line so that "int optlen)" begins precisely at the first column after the
> openning parenthesis on the first line.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
ok, thanks
I'll send v4 later

Re: [PATCH net-next v3 2/2] net: tcp_ipv6 policy route issue

[wangyufen]

On 2014/3/26 15:05, Hannes Frederic Sowa wrote:
> On Mon, Mar 24, 2014 at 02:25:22PM +0800, Wangyufen wrote:
>> From: Wang Yufen <wangyufen@huawei.com>
>>
>> The issue raises when adding policy route, specify a particular
>> NIC as oif, the policy route did not take effect. The reason is
>> that fl6.oif is not set and route map failed. From the 
>> tcp_v6_send_response function, if the binding address is linklocal,
>> fl6.oif is set, but not for global address.
>>
>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
> 
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> 
> But this looks like a bug to me, so maybe this is something for net
> inclusion.
I checked the commit log, I think, changes to ipv6 was incompleted in 
commit 4c67525849e0b7f4bd4fab leded to this issue . 
In tcp_v6_send_response, fl6.oif can't be directly set to iif for global
address, but it should not be 0.

> 
> Thanks,
> 
>   Hannes
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH net-next v3 2/2] net: tcp_ipv6 policy route issue

[Hannes Frederic Sowa]

On Thu, Mar 27, 2014 at 10:00:34AM +0800, wangyufen wrote:
> On 2014/3/26 15:05, Hannes Frederic Sowa wrote:
> > On Mon, Mar 24, 2014 at 02:25:22PM +0800, Wangyufen wrote:
> >> From: Wang Yufen <wangyufen@huawei.com>
> >>
> >> The issue raises when adding policy route, specify a particular
> >> NIC as oif, the policy route did not take effect. The reason is
> >> that fl6.oif is not set and route map failed. From the 
> >> tcp_v6_send_response function, if the binding address is linklocal,
> >> fl6.oif is set, but not for global address.
> >>
> >> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
> > 
> > Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > 
> > But this looks like a bug to me, so maybe this is something for net
> > inclusion.
> I checked the commit log, I think, changes to ipv6 was incompleted in 
> commit 4c67525849e0b7f4bd4fab leded to this issue . 
> In tcp_v6_send_response, fl6.oif can't be directly set to iif for global
> address, but it should not be 0.

Actually, I wonder if

if (rt6_need_strict(&fl6.daddr) || !oif)
	fl6.flowi6_oif = inet6_iif(skb);
else
	fl6.flowi6_oif = oif;

would be ok, too, and would ensure that errors with no sockets would
reach their target with higher probability in case of policy routes.

In routing code we don't do strict lookup unless either we have the
indication by the socket (if available) or destination address is
multicast, linklocal or loopback. Otherwise we only favour flowi6_oif
interfaces.

Bye,

  Hannes

Re: [PATCH net-next v3 2/2] net: tcp_ipv6 policy route issue

[wangyufen]

On 2014/3/27 13:32, Hannes Frederic Sowa wrote:
> On Thu, Mar 27, 2014 at 10:00:34AM +0800, wangyufen wrote:
>> On 2014/3/26 15:05, Hannes Frederic Sowa wrote:
>>> On Mon, Mar 24, 2014 at 02:25:22PM +0800, Wangyufen wrote:
>>>> From: Wang Yufen <wangyufen@huawei.com>
>>>>
>>>> The issue raises when adding policy route, specify a particular
>>>> NIC as oif, the policy route did not take effect. The reason is
>>>> that fl6.oif is not set and route map failed. From the 
>>>> tcp_v6_send_response function, if the binding address is linklocal,
>>>> fl6.oif is set, but not for global address.
>>>>
>>>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>>>
>>> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>>
>>> But this looks like a bug to me, so maybe this is something for net
>>> inclusion.
>> I checked the commit log, I think, changes to ipv6 was incompleted in 
>> commit 4c67525849e0b7f4bd4fab leded to this issue . 
>> In tcp_v6_send_response, fl6.oif can't be directly set to iif for global
>> address, but it should not be 0.
> 
> Actually, I wonder if
> 
> if (rt6_need_strict(&fl6.daddr) || !oif)
> 	fl6.flowi6_oif = inet6_iif(skb);
> else
> 	fl6.flowi6_oif = oif;
> 
> would be ok, too, and would ensure that errors with no sockets would
> reach their target with higher probability in case of policy routes.
> 
That would be better, I'll send v4 later
> In routing code we don't do strict lookup unless either we have the
> indication by the socket (if available) or destination address is
> multicast, linklocal or loopback. Otherwise we only favour flowi6_oif
> interfaces.
> 
> Bye,
> 
>   Hannes
> 
> 
>