Re: [PATCH v4 1/2] geneve: implement support for IPv6-based tunnels
From: Jesse Gross <hidden>
Date: 2015-10-21 05:06:39
All of this looks pretty good to me, I just have a few last things that I noticed: On Tue, Oct 20, 2015 at 11:11 PM, John W. Linville [off-list ref] wrote:
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index 8f5c02eed47d..217b472ab9e7 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c /* Pseudo network device */ struct geneve_dev { struct hlist_node hlist; /* vni hash table */ struct net *net; /* netns for packet i/o */ struct net_device *dev; /* netdev for geneve tunnel */ - struct geneve_sock *sock; /* socket used for geneve tunnel */ + struct geneve_sock *sock4; /* IPv4 socket used for geneve tunnel */ + struct geneve_sock *sock6; /* IPv6 socket used for geneve tunnel */
It might be worth wrapping sock6 in #if IS_ENABLED(CONFIG_IPV6). Not because I'm trying to save space but because we make initializing it conditional on this, so it would catch if somebody tries to access it uninitialized.
+static int geneve_open(struct net_device *dev)
+{
+ struct geneve_dev *geneve = netdev_priv(dev);
+ bool ipv6 = geneve->remote.sa.sa_family == AF_INET6;
+ bool metadata = !!geneve->collect_md;A small thing but geneve->collect_md is also a bool, so I guess we probably don't need the !!.
+#if IS_ENABLED(CONFIG_IPV6)
+static int geneve6_build_skb(struct dst_entry *dst, struct sk_buff *skb,
+ __be16 tun_flags, u8 vni[3], u8 opt_len, u8 *opt,
+ bool csum, bool xnet)
+{[...]
+ skb_scrub_packet(skb, xnet);
I realized that I wasn't really all that clear with my previous comment here. I was just asking if we should also use skb_scrub_packet() in the IPv4 to keep things consistent. I agree that the rt/dst differences makes sharing code a bit difficult in the general sense.
+static netdev_tx_t geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev, + struct ip_tunnel_info *info)
[...]
+ if (geneve->collect_md) {
+ if (unlikely(info && !(info->mode & IP_TUNNEL_INFO_TX))) {
+ netdev_dbg(dev, "no tunnel metadata\n");
+ goto tx_error;It seems like this should also be checking for !info here - that's perhaps the main cause of not having any tunnel metadata. This is the same with IPv4 though too.
quoted hunk ↗ jump to hunk
@@ -870,14 +1139,29 @@ static int geneve_newlink(struct net *net, struct net_device *dev, - if (!data[IFLA_GENEVE_ID] || !data[IFLA_GENEVE_REMOTE]) + if (!data[IFLA_GENEVE_ID] || + (data[IFLA_GENEVE_REMOTE] && data[IFLA_GENEVE_REMOTE6]) || + (!data[IFLA_GENEVE_REMOTE] && !data[IFLA_GENEVE_REMOTE6])) return -EINVAL;
I think this will conflict with/revert my change in -net. Obviously, the conflict will need to be resolved at some point, but it's probably just best to remove the whole block here so the resolution is obvious.