diff mbox

[RFC,net-next,1/4] net: ipv6: Introduce flowi6_init_output.

Message ID 1398487705-13430-2-git-send-email-lorenzo@google.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Lorenzo Colitti April 26, 2014, 4:48 a.m. UTC 
This is consistent with IPv4, and is a bit more compact. Also, by
forcing all common flowi6 parameters to be explicitly specified,
it makes it easier to see which parameters are being set and
which are being defaulted to zero. So, for example, no more
forgetting to do "fl6.fl6_mark = sk->sk_mark" and having to fix
it later like in net-next bf439b3.

Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
---
 include/net/flow.h               | 20 ++++++++++++++++++++
 net/ipv6/af_inet6.c              | 12 ++++--------
 net/ipv6/datagram.c              | 11 ++++-------
 net/ipv6/inet6_connection_sock.c | 23 ++++++++---------------
 net/ipv6/syncookies.c            | 12 +++++-------
 net/ipv6/tcp_ipv6.c              | 11 ++++-------
 6 files changed, 45 insertions(+), 44 deletions(-)

Comments

Julian Anastasov April 26, 2014, 5:56 a.m. UTC | #1 
Hello,

On Sat, 26 Apr 2014, Lorenzo Colitti wrote:

> +static inline void flowi6_init_output(struct flowi6 *fl6, int oif,
> +				      __u32 mark, __u8 proto, __u8 flags,
> +				      __be32 flowlabel,
> +				      struct in6_addr daddr,
> +				      struct in6_addr saddr,
> +				      __be16 dport, __be16 sport)
> +{
> +	fl6->flowi6_oif = oif;
> +	fl6->flowi6_iif = 0;

	Make sure LOOPBACK_IFINDEX is provided in
flowi6_iif instead of 0, recent commit fixed such
FIB rule lookups.

Regards

--
Julian Anastasov <ja@ssi.bg>
--
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
Lorenzo Colitti April 27, 2014, 4:03 a.m. UTC | #2 
On Sat, Apr 26, 2014 at 2:56 PM, Julian Anastasov <ja@ssi.bg> wrote:
>> +     fl6->flowi6_oif = oif;
>> +     fl6->flowi6_iif = 0;
>
>         Make sure LOOPBACK_IFINDEX is provided in
> flowi6_iif instead of 0, recent commit fixed such
> FIB rule lookups.

I'm assuming that was for IPv4? All the IPv6 code in current net-next
still seems to set flowi6_iif to zero. It's later set to
LOOPBACK_IFINDEX in ip6_route_output.

You could argue that if this patch is accepted, then setting
flowi6_iif to LOOPBACK_IFINDEX belongs there for consistency with
IPv4. That seems reasonable, but I'd prefer not doing that in this
patch because 1) this patch is semantically a no-op and I'd like to
keep it that way, 2) it would be a bigger change, because even with
this patch, there are still codepaths that initialize their own flowi6
and memset them to zero.
--
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
Julian Anastasov April 28, 2014, 7:07 a.m. UTC | #3 
Hello,

On Sun, 27 Apr 2014, Lorenzo Colitti wrote:

> On Sat, Apr 26, 2014 at 2:56 PM, Julian Anastasov <ja@ssi.bg> wrote:
> >> +     fl6->flowi6_oif = oif;
> >> +     fl6->flowi6_iif = 0;
> >
> >         Make sure LOOPBACK_IFINDEX is provided in
> > flowi6_iif instead of 0, recent commit fixed such
> > FIB rule lookups.
> 
> I'm assuming that was for IPv4? All the IPv6 code in current net-next
> still seems to set flowi6_iif to zero. It's later set to
> LOOPBACK_IFINDEX in ip6_route_output.

	I see, it seems more IPv6 places need a fix.

> You could argue that if this patch is accepted, then setting
> flowi6_iif to LOOPBACK_IFINDEX belongs there for consistency with
> IPv4. That seems reasonable, but I'd prefer not doing that in this
> patch because 1) this patch is semantically a no-op and I'd like to
> keep it that way, 2) it would be a bigger change, because even with
> this patch, there are still codepaths that initialize their own flowi6
> and memset them to zero.

	OK. I'll try a patch to fix the remaining flowi6_iif
places...

Regards

--
Julian Anastasov <ja@ssi.bg>
--
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
diff mbox

Patch

diff --git a/include/net/flow.h b/include/net/flow.h
index 8109a15..84044af 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -150,6 +150,26 @@  struct flowidn {
 #define fld_dport		uli.ports.dport
 } __attribute__((__aligned__(BITS_PER_LONG/8)));
 
+static inline void flowi6_init_output(struct flowi6 *fl6, int oif,
+				      __u32 mark, __u8 proto, __u8 flags,
+				      __be32 flowlabel,
+				      struct in6_addr daddr,
+				      struct in6_addr saddr,
+				      __be16 dport, __be16 sport)
+{
+	fl6->flowi6_oif = oif;
+	fl6->flowi6_iif = 0;
+	fl6->flowi6_mark = mark;
+	fl6->flowi6_proto = proto;
+	fl6->flowi6_flags = flags;
+	fl6->flowi6_secid = 0;
+	fl6->daddr = daddr;
+	fl6->saddr = saddr;
+	fl6->flowlabel = flowlabel;
+	fl6->fl6_dport = dport;
+	fl6->fl6_sport = sport;
+}
+
 struct flowi {
 	union {
 		struct flowi_common	__fl_common;
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index d935889..f8c11d2 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -649,14 +649,10 @@  int inet6_sk_rebuild_header(struct sock *sk)
 		struct flowi6 fl6;
 
 		memset(&fl6, 0, sizeof(fl6));
-		fl6.flowi6_proto = sk->sk_protocol;
-		fl6.daddr = sk->sk_v6_daddr;
-		fl6.saddr = np->saddr;
-		fl6.flowlabel = np->flow_label;
-		fl6.flowi6_oif = sk->sk_bound_dev_if;
-		fl6.flowi6_mark = sk->sk_mark;
-		fl6.fl6_dport = inet->inet_dport;
-		fl6.fl6_sport = inet->inet_sport;
+		flowi6_init_output(&fl6, sk->sk_bound_dev_if, sk->sk_mark,
+				   sk->sk_protocol, 0, np->flow_label,
+				   sk->sk_v6_daddr, np->saddr,
+				   inet->inet_dport, inet->inet_sport);
 		security_sk_classify_flow(sk, flowi6_to_flowi(&fl6));
 
 		final_p = fl6_update_dst(&fl6, np->opt, &final);
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index c3bf2d2..f15c165 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -154,13 +154,10 @@  ipv4_connected:
 	 *	destination cache for it.
 	 */
 
-	fl6.flowi6_proto = sk->sk_protocol;
-	fl6.daddr = sk->sk_v6_daddr;
-	fl6.saddr = np->saddr;
-	fl6.flowi6_oif = sk->sk_bound_dev_if;
-	fl6.flowi6_mark = sk->sk_mark;
-	fl6.fl6_dport = inet->inet_dport;
-	fl6.fl6_sport = inet->inet_sport;
+	flowi6_init_output(&fl6, sk->sk_bound_dev_if, sk->sk_mark,
+			   sk->sk_protocol, 0, fl6.flowlabel,
+			   sk->sk_v6_daddr, np->saddr,
+			   inet->inet_dport, inet->inet_sport);
 
 	if (!fl6.flowi6_oif && (addr_type&IPV6_ADDR_MULTICAST))
 		fl6.flowi6_oif = np->mcast_oif;
diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index d4ade34..47f2272 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -76,14 +76,11 @@  struct dst_entry *inet6_csk_route_req(struct sock *sk,
 	struct dst_entry *dst;
 
 	memset(fl6, 0, sizeof(*fl6));
-	fl6->flowi6_proto = IPPROTO_TCP;
-	fl6->daddr = ireq->ir_v6_rmt_addr;
+	flowi6_init_output(fl6, ireq->ir_iif, sk->sk_mark,
+			   IPPROTO_TCP, 0, 0,
+			   ireq->ir_v6_rmt_addr, ireq->ir_v6_loc_addr,
+			   ireq->ir_rmt_port, htons(ireq->ir_num));
 	final_p = fl6_update_dst(fl6, np->opt, &final);
-	fl6->saddr = ireq->ir_v6_loc_addr;
-	fl6->flowi6_oif = ireq->ir_iif;
-	fl6->flowi6_mark = sk->sk_mark;
-	fl6->fl6_dport = ireq->ir_rmt_port;
-	fl6->fl6_sport = htons(ireq->ir_num);
 	security_req_classify_flow(req, flowi6_to_flowi(fl6));
 
 	dst = ip6_dst_lookup_flow(sk, fl6, final_p);
@@ -201,15 +198,11 @@  static struct dst_entry *inet6_csk_route_socket(struct sock *sk,
 	struct dst_entry *dst;
 
 	memset(fl6, 0, sizeof(*fl6));
-	fl6->flowi6_proto = sk->sk_protocol;
-	fl6->daddr = sk->sk_v6_daddr;
-	fl6->saddr = np->saddr;
-	fl6->flowlabel = np->flow_label;
+	flowi6_init_output(fl6, sk->sk_bound_dev_if, sk->sk_mark,
+			   sk->sk_protocol, 0, np->flow_label,
+			   sk->sk_v6_daddr, np->saddr,
+			   inet->inet_dport, inet->inet_sport);
 	IP6_ECN_flow_xmit(sk, fl6->flowlabel);
-	fl6->flowi6_oif = sk->sk_bound_dev_if;
-	fl6->flowi6_mark = sk->sk_mark;
-	fl6->fl6_sport = inet->inet_sport;
-	fl6->fl6_dport = inet->inet_dport;
 	security_sk_classify_flow(sk, flowi6_to_flowi(fl6));
 
 	final_p = fl6_update_dst(fl6, np->opt, &final);
diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index bb53a5e7..09bb685 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -237,14 +237,12 @@  struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 		struct in6_addr *final_p, final;
 		struct flowi6 fl6;
 		memset(&fl6, 0, sizeof(fl6));
-		fl6.flowi6_proto = IPPROTO_TCP;
-		fl6.daddr = ireq->ir_v6_rmt_addr;
+		flowi6_init_output(&fl6, sk->sk_bound_dev_if, sk->sk_mark,
+				   IPPROTO_TCP, 0, 0,
+				   ireq->ir_v6_rmt_addr, ireq->ir_v6_loc_addr,
+				   ireq->ir_rmt_port, inet_sk(sk)->inet_sport);
+
 		final_p = fl6_update_dst(&fl6, np->opt, &final);
-		fl6.saddr = ireq->ir_v6_loc_addr;
-		fl6.flowi6_oif = sk->sk_bound_dev_if;
-		fl6.flowi6_mark = sk->sk_mark;
-		fl6.fl6_dport = ireq->ir_rmt_port;
-		fl6.fl6_sport = inet_sk(sk)->inet_sport;
 		security_req_classify_flow(req, flowi6_to_flowi(&fl6));
 
 		dst = ip6_dst_lookup_flow(sk, &fl6, final_p);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index e289830..8f4f68a 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -243,13 +243,10 @@  static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
 	if (!ipv6_addr_any(&sk->sk_v6_rcv_saddr))
 		saddr = &sk->sk_v6_rcv_saddr;
 
-	fl6.flowi6_proto = IPPROTO_TCP;
-	fl6.daddr = sk->sk_v6_daddr;
-	fl6.saddr = saddr ? *saddr : np->saddr;
-	fl6.flowi6_oif = sk->sk_bound_dev_if;
-	fl6.flowi6_mark = sk->sk_mark;
-	fl6.fl6_dport = usin->sin6_port;
-	fl6.fl6_sport = inet->inet_sport;
+	flowi6_init_output(&fl6, sk->sk_bound_dev_if, sk->sk_mark,
+			   IPPROTO_TCP, 0, fl6.flowlabel,
+			   sk->sk_v6_daddr, saddr ? *saddr : np->saddr,
+			   usin->sin6_port, inet->inet_sport);
 
 	final_p = fl6_update_dst(&fl6, np->opt, &final);