[PATCH net-next v2 0/2] net: vrf: Fixup PKTINFO to return enslaved device index

[PATCH net-next v2 0/2] net: vrf: Fixup PKTINFO to return enslaved device index

[David Ahern]

Applications such as OSPF and BFD need the original ingress device not
the VRF device; the latter can be derived from the former. To that end
move the packet intercept from an rx handler that is invoked by
__netif_receive_skb_core to the ipv4 and ipv6 receive processing.

IPv6 already saves the skb_iif to the control buffer in ipv6_rcv. Since
the skb->dev has not been switched the cb has the enslaved device. Make
the same happen for IPv4 by adding the skb_iif to inet_skb_parm and set
it in ipv4 code after clearing the skb control buffer similar to IPv6.
>From there the pktinfo can just pull it from cb with the PKTINFO_SKB_CB
cast. 

David Ahern (2):
  net: l3mdev: Add hook in ip and ipv6
  net: original ingress device index in PKTINFO

 drivers/net/vrf.c         | 188 ++++++++++++++++++++++------------------------
 include/linux/ipv6.h      |  17 ++++-
 include/linux/netdevice.h |   2 +
 include/net/ip.h          |   1 +
 include/net/l3mdev.h      |  43 +++++++++++
 include/net/tcp.h         |   4 +-
 net/core/dev.c            |   3 +-
 net/ipv4/ip_input.c       |   8 ++
 net/ipv4/ip_sockglue.c    |   7 +-
 net/ipv6/ip6_input.c      |   7 ++
 10 files changed, 178 insertions(+), 102 deletions(-)

-- 
2.1.4

[PATCH net-next v2 1/2] net: l3mdev: Add hook in ip and ipv6

[David Ahern]

Currently the VRF driver uses the rx_handler to switch the skb device
to the VRF device. Switching the dev prior to the ip / ipv6 layer
means the VRF driver has to duplicate IP/IPv6 processing which adds
overhead and makes features such as retaining the ingress device index
more complicated than necessary.

This patch moves the hook to the L3 layer just after the first NF_HOOK
for PRE_ROUTING. This location makes exposing the original ingress device
trivial (next patch) and allows adding other NF_HOOKs to the VRF driver
in the future.

dev_queue_xmit_nit is exported so that the VRF driver can cycle the skb
with the switched device through the packet taps to maintain current
behavior (tcpdump can be used on either the vrf device or the enslaved
devices).

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
v2
- add skb_l3mdev_slave helper for inet6_iif and tcp_v6_iif rather
  than open coding the if case. Added benefit that the change compiles
  out if CONFIG_NET_L3_MASTER_DEV is not enabled.

 drivers/net/vrf.c         | 188 ++++++++++++++++++++++------------------------
 include/linux/ipv6.h      |  17 ++++-
 include/linux/netdevice.h |   2 +
 include/net/l3mdev.h      |  43 +++++++++++
 include/net/tcp.h         |   4 +-
 net/core/dev.c            |   3 +-
 net/ipv4/ip_input.c       |   7 ++
 net/ipv6/ip6_input.c      |   7 ++
 8 files changed, 170 insertions(+), 101 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 4b2461ae5d3b..0e2a58506a35 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -42,9 +42,6 @@
 #define DRV_NAME	"vrf"
 #define DRV_VERSION	"1.0"
 
-#define vrf_master_get_rcu(dev) \
-	((struct net_device *)rcu_dereference(dev->rx_handler_data))
-
 struct net_vrf {
 	struct rtable           *rth;
 	struct rt6_info		*rt6;
@@ -60,90 +57,12 @@ struct pcpu_dstats {
 	struct u64_stats_sync	syncp;
 };
 
-/* neighbor handling is done with actual device; do not want
- * to flip skb->dev for those ndisc packets. This really fails
- * for multiple next protocols (e.g., NEXTHDR_HOP). But it is
- * a start.
- */
-#if IS_ENABLED(CONFIG_IPV6)
-static bool check_ipv6_frame(const struct sk_buff *skb)
-{
-	const struct ipv6hdr *ipv6h;
-	struct ipv6hdr _ipv6h;
-	bool rc = true;
-
-	ipv6h = skb_header_pointer(skb, 0, sizeof(_ipv6h), &_ipv6h);
-	if (!ipv6h)
-		goto out;
-
-	if (ipv6h->nexthdr == NEXTHDR_ICMP) {
-		const struct icmp6hdr *icmph;
-		struct icmp6hdr _icmph;
-
-		icmph = skb_header_pointer(skb, sizeof(_ipv6h),
-					   sizeof(_icmph), &_icmph);
-		if (!icmph)
-			goto out;
-
-		switch (icmph->icmp6_type) {
-		case NDISC_ROUTER_SOLICITATION:
-		case NDISC_ROUTER_ADVERTISEMENT:
-		case NDISC_NEIGHBOUR_SOLICITATION:
-		case NDISC_NEIGHBOUR_ADVERTISEMENT:
-		case NDISC_REDIRECT:
-			rc = false;
-			break;
-		}
-	}
-
-out:
-	return rc;
-}
-#else
-static bool check_ipv6_frame(const struct sk_buff *skb)
-{
-	return false;
-}
-#endif
-
-static bool is_ip_rx_frame(struct sk_buff *skb)
-{
-	switch (skb->protocol) {
-	case htons(ETH_P_IP):
-		return true;
-	case htons(ETH_P_IPV6):
-		return check_ipv6_frame(skb);
-	}
-	return false;
-}
-
 static void vrf_tx_error(struct net_device *vrf_dev, struct sk_buff *skb)
 {
 	vrf_dev->stats.tx_errors++;
 	kfree_skb(skb);
 }
 
-/* note: already called with rcu_read_lock */
-static rx_handler_result_t vrf_handle_frame(struct sk_buff **pskb)
-{
-	struct sk_buff *skb = *pskb;
-
-	if (is_ip_rx_frame(skb)) {
-		struct net_device *dev = vrf_master_get_rcu(skb->dev);
-		struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
-
-		u64_stats_update_begin(&dstats->syncp);
-		dstats->rx_pkts++;
-		dstats->rx_bytes += skb->len;
-		u64_stats_update_end(&dstats->syncp);
-
-		skb->dev = dev;
-
-		return RX_HANDLER_ANOTHER;
-	}
-	return RX_HANDLER_PASS;
-}
-
 static struct rtnl_link_stats64 *vrf_get_stats64(struct net_device *dev,
 						 struct rtnl_link_stats64 *stats)
 {
@@ -506,28 +425,14 @@ static int do_vrf_add_slave(struct net_device *dev, struct net_device *port_dev)
 {
 	int ret;
 
-	/* register the packet handler for slave ports */
-	ret = netdev_rx_handler_register(port_dev, vrf_handle_frame, dev);
-	if (ret) {
-		netdev_err(port_dev,
-			   "Device %s failed to register rx_handler\n",
-			   port_dev->name);
-		goto out_fail;
-	}
-
 	ret = netdev_master_upper_dev_link(port_dev, dev, NULL, NULL);
 	if (ret < 0)
-		goto out_unregister;
+		return ret;
 
 	port_dev->priv_flags |= IFF_L3MDEV_SLAVE;
 	cycle_netdev(port_dev);
 
 	return 0;
-
-out_unregister:
-	netdev_rx_handler_unregister(port_dev);
-out_fail:
-	return ret;
 }
 
 static int vrf_add_slave(struct net_device *dev, struct net_device *port_dev)
@@ -544,8 +449,6 @@ static int do_vrf_del_slave(struct net_device *dev, struct net_device *port_dev)
 	netdev_upper_dev_unlink(port_dev, dev);
 	port_dev->priv_flags &= ~IFF_L3MDEV_SLAVE;
 
-	netdev_rx_handler_unregister(port_dev);
-
 	cycle_netdev(port_dev);
 
 	return 0;
@@ -668,6 +571,94 @@ static int vrf_get_saddr(struct net_device *dev, struct flowi4 *fl4)
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
+/* neighbor handling is done with actual device; do not want
+ * to flip skb->dev for those ndisc packets. This really fails
+ * for multiple next protocols (e.g., NEXTHDR_HOP). But it is
+ * a start.
+ */
+static bool ipv6_ndisc_frame(const struct sk_buff *skb)
+{
+	const struct ipv6hdr *ipv6h = (struct ipv6hdr *)skb->data;
+	size_t hlen = sizeof(*ipv6h);
+	bool rc = false;
+
+	if (ipv6h->nexthdr == NEXTHDR_ICMP) {
+		const struct icmp6hdr *icmph;
+
+		if (skb->len < hlen + sizeof(*icmph))
+			goto out;
+
+		icmph = (struct icmp6hdr *)(skb->data + sizeof(*ipv6h));
+		switch (icmph->icmp6_type) {
+		case NDISC_ROUTER_SOLICITATION:
+		case NDISC_ROUTER_ADVERTISEMENT:
+		case NDISC_NEIGHBOUR_SOLICITATION:
+		case NDISC_NEIGHBOUR_ADVERTISEMENT:
+		case NDISC_REDIRECT:
+			rc = true;
+			break;
+		}
+	}
+
+out:
+	return rc;
+}
+
+static struct sk_buff *vrf_ip6_rcv(struct net_device *vrf_dev,
+				   struct sk_buff *skb)
+{
+	/* if packet is NDISC keep the ingress interface */
+	if (!ipv6_ndisc_frame(skb)) {
+		skb->dev = vrf_dev;
+		skb->skb_iif = vrf_dev->ifindex;
+
+		skb_push(skb, skb->mac_len);
+		dev_queue_xmit_nit(skb, vrf_dev);
+		skb_pull(skb, skb->mac_len);
+
+		IP6CB(skb)->flags |= IP6SKB_L3SLAVE;
+	}
+
+	return skb;
+}
+
+#else
+static struct sk_buff *vrf_ip6_rcv(struct net_device *vrf_dev,
+				   struct sk_buff *skb)
+{
+	return skb;
+}
+#endif
+
+static struct sk_buff *vrf_ip_rcv(struct net_device *vrf_dev,
+				  struct sk_buff *skb)
+{
+	skb->dev = vrf_dev;
+	skb->skb_iif = vrf_dev->ifindex;
+
+	skb_push(skb, skb->mac_len);
+	dev_queue_xmit_nit(skb, vrf_dev);
+	skb_pull(skb, skb->mac_len);
+
+	return skb;
+}
+
+/* called with rcu lock held */
+static struct sk_buff *vrf_l3_rcv(struct net_device *vrf_dev,
+				  struct sk_buff *skb,
+				  u16 proto)
+{
+	switch (proto) {
+	case AF_INET:
+		return vrf_ip_rcv(vrf_dev, skb);
+	case AF_INET6:
+		return vrf_ip6_rcv(vrf_dev, skb);
+	}
+
+	return skb;
+}
+
+#if IS_ENABLED(CONFIG_IPV6)
 static struct dst_entry *vrf_get_rt6_dst(const struct net_device *dev,
 					 const struct flowi6 *fl6)
 {
@@ -688,6 +679,7 @@ static const struct l3mdev_ops vrf_l3mdev_ops = {
 	.l3mdev_fib_table	= vrf_fib_table,
 	.l3mdev_get_rtable	= vrf_get_rtable,
 	.l3mdev_get_saddr	= vrf_get_saddr,
+	.l3mdev_l3_rcv		= vrf_l3_rcv,
 #if IS_ENABLED(CONFIG_IPV6)
 	.l3mdev_get_rt6_dst	= vrf_get_rt6_dst,
 #endif
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 58d6e158755f..5c91b0b055d4 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -118,14 +118,29 @@ struct inet6_skb_parm {
 #define IP6SKB_ROUTERALERT	8
 #define IP6SKB_FRAGMENTED      16
 #define IP6SKB_HOPBYHOP        32
+#define IP6SKB_L3SLAVE         64
 };
 
+#if defined(CONFIG_NET_L3_MASTER_DEV)
+static inline bool skb_l3mdev_slave(__u16 flags)
+{
+	return flags & IP6SKB_L3SLAVE;
+}
+#else
+static inline bool skb_l3mdev_slave(__u16 flags)
+{
+	return false;
+}
+#endif
+
 #define IP6CB(skb)	((struct inet6_skb_parm*)((skb)->cb))
 #define IP6CBMTU(skb)	((struct ip6_mtuinfo *)((skb)->cb))
 
 static inline int inet6_iif(const struct sk_buff *skb)
 {
-	return IP6CB(skb)->iif;
+	bool l3_slave = skb_l3mdev_slave(IP6CB(skb)->flags);
+
+	return l3_slave ? skb->skb_iif : IP6CB(skb)->iif;
 }
 
 struct tcp6_request_sock {
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 63580e6d0df4..c2f5112f08f7 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3258,6 +3258,8 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
 bool is_skb_forwardable(const struct net_device *dev,
 			const struct sk_buff *skb);
 
+void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev);
+
 extern int		netdev_budget;
 
 /* Called by rtnetlink.c:rtnl_unlock() */
diff --git a/include/net/l3mdev.h b/include/net/l3mdev.h
index c43a9c73de5e..19d8171d7cbb 100644
--- a/include/net/l3mdev.h
+++ b/include/net/l3mdev.h
@@ -25,6 +25,8 @@
 
 struct l3mdev_ops {
 	u32		(*l3mdev_fib_table)(const struct net_device *dev);
+	struct sk_buff * (*l3mdev_l3_rcv)(struct net_device *dev,
+					  struct sk_buff *skb, u16 proto);
 
 	/* IPv4 ops */
 	struct rtable *	(*l3mdev_get_rtable)(const struct net_device *dev,
@@ -177,6 +179,35 @@ struct dst_entry *l3mdev_rt6_dst_by_oif(struct net *net,
 	return dst;
 }
 
+static inline
+struct sk_buff *l3mdev_l3_rcv(struct sk_buff *skb, u16 proto)
+{
+	struct net_device *master = NULL;
+
+	if (netif_is_l3_slave(skb->dev))
+		master = netdev_master_upper_dev_get_rcu(skb->dev);
+
+	else if (netif_is_l3_master(skb->dev))
+		master = skb->dev;
+
+	if (master && master->l3mdev_ops->l3mdev_l3_rcv)
+		skb = master->l3mdev_ops->l3mdev_l3_rcv(master, skb, proto);
+
+	return skb;
+}
+
+static inline
+struct sk_buff *l3mdev_ip_rcv(struct sk_buff *skb)
+{
+	return l3mdev_l3_rcv(skb, AF_INET);
+}
+
+static inline
+struct sk_buff *l3mdev_ip6_rcv(struct sk_buff *skb)
+{
+	return l3mdev_l3_rcv(skb, AF_INET6);
+}
+
 #else
 
 static inline int l3mdev_master_ifindex_rcu(const struct net_device *dev)
@@ -244,6 +275,18 @@ struct dst_entry *l3mdev_rt6_dst_by_oif(struct net *net,
 {
 	return NULL;
 }
+
+static inline
+struct sk_buff *l3mdev_ip_rcv(struct sk_buff *skb)
+{
+	return skb;
+}
+
+static inline
+struct sk_buff *l3mdev_ip6_rcv(struct sk_buff *skb)
+{
+	return skb;
+}
 #endif
 
 #endif /* _NET_L3MDEV_H_ */
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 24ec80483805..264e8bb3d1ea 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -781,7 +781,9 @@ struct tcp_skb_cb {
  */
 static inline int tcp_v6_iif(const struct sk_buff *skb)
 {
-	return TCP_SKB_CB(skb)->header.h6.iif;
+	bool l3_slave = skb_l3mdev_slave(TCP_SKB_CB(skb)->header.h6.flags);
+
+	return l3_slave ? skb->skb_iif : TCP_SKB_CB(skb)->header.h6.iif;
 }
 #endif
 
diff --git a/net/core/dev.c b/net/core/dev.c
index e98ba63fe280..51a8bf28a3e0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1850,7 +1850,7 @@ static inline bool skb_loop_sk(struct packet_type *ptype, struct sk_buff *skb)
  *	taps currently in use.
  */
 
-static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
+void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct packet_type *ptype;
 	struct sk_buff *skb2 = NULL;
@@ -1907,6 +1907,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 		pt_prev->func(skb2, skb->dev, pt_prev, skb->dev);
 	rcu_read_unlock();
 }
+EXPORT_SYMBOL_GPL(dev_queue_xmit_nit);
 
 /**
  * netif_setup_tc - Handle tc mappings on real_num_tx_queues change
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 751c0658e194..37375eedeef9 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -313,6 +313,13 @@ static int ip_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
 	const struct iphdr *iph = ip_hdr(skb);
 	struct rtable *rt;
 
+	/* if ingress device is enslaved to an L3 master device pass the
+	 * skb to its handler for processing
+	 */
+	skb = l3mdev_ip_rcv(skb);
+	if (!skb)
+		return NET_RX_SUCCESS;
+
 	if (net->ipv4.sysctl_ip_early_demux &&
 	    !skb_dst(skb) &&
 	    !skb->sk &&
diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index 6ed56012005d..f185cbcda114 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -49,6 +49,13 @@
 
 int ip6_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
+	/* if ingress device is enslaved to an L3 master device pass the
+	 * skb to its handler for processing
+	 */
+	skb = l3mdev_ip6_rcv(skb);
+	if (!skb)
+		return NET_RX_SUCCESS;
+
 	if (net->ipv4.sysctl_ip_early_demux && !skb_dst(skb) && skb->sk == NULL) {
 		const struct inet6_protocol *ipprot;
 
-- 
2.1.4

Re: [PATCH net-next v2 1/2] net: l3mdev: Add hook in ip and ipv6

[Shmulik Ladkani]

Hi David,

On Fri,  6 May 2016 18:49:40 -0700 David Ahern <dsa@cumulusnetworks.com> wrote:
> +static bool ipv6_ndisc_frame(const struct sk_buff *skb)
> +{
> +	const struct ipv6hdr *ipv6h = (struct ipv6hdr *)skb->data;
> +	size_t hlen = sizeof(*ipv6h);
> +	bool rc = false;
> +
> +	if (ipv6h->nexthdr == NEXTHDR_ICMP) {
> +		const struct icmp6hdr *icmph;
> +
> +		if (skb->len < hlen + sizeof(*icmph))
> +			goto out;
> +
> +		icmph = (struct icmp6hdr *)(skb->data + sizeof(*ipv6h));
> +		switch (icmph->icmp6_type) {

Don't we need an additional pskb_may_pull here?

If I get it right, 'ipv6_rcv' only assures sizeof(ipv6hdr) to be in the
linear header (unless it's a NEXTHDR_HOP, which is not the case here).

> +static inline
> +struct sk_buff *l3mdev_l3_rcv(struct sk_buff *skb, u16 proto)
> +{
> +	struct net_device *master = NULL;
> +
> +	if (netif_is_l3_slave(skb->dev))
> +		master = netdev_master_upper_dev_get_rcu(skb->dev);
> +
> +	else if (netif_is_l3_master(skb->dev))
> +		master = skb->dev;
> +
> +	if (master && master->l3mdev_ops->l3mdev_l3_rcv)
> +		skb = master->l3mdev_ops->l3mdev_l3_rcv(master, skb, proto);

In the case where netif_is_l3_master(skb->dev) is true, can you explain
why we need to pass it through the l3mdev_l3_rcv callback again?

Regards,
Shmulik

Re: [PATCH net-next v2 1/2] net: l3mdev: Add hook in ip and ipv6

[David Ahern]

On 5/7/16 2:30 AM, Shmulik Ladkani wrote:
> Hi David,
>
> On Fri,  6 May 2016 18:49:40 -0700 David Ahern <dsa@cumulusnetworks.com> wrote:
>> +static bool ipv6_ndisc_frame(const struct sk_buff *skb)
>> +{
>> +	const struct ipv6hdr *ipv6h = (struct ipv6hdr *)skb->data;
>> +	size_t hlen = sizeof(*ipv6h);
>> +	bool rc = false;
>> +
>> +	if (ipv6h->nexthdr == NEXTHDR_ICMP) {
>> +		const struct icmp6hdr *icmph;
>> +
>> +		if (skb->len < hlen + sizeof(*icmph))
>> +			goto out;
>> +
>> +		icmph = (struct icmp6hdr *)(skb->data + sizeof(*ipv6h));
>> +		switch (icmph->icmp6_type) {
>
> Don't we need an additional pskb_may_pull here?
>
> If I get it right, 'ipv6_rcv' only assures sizeof(ipv6hdr) to be in the
> linear header (unless it's a NEXTHDR_HOP, which is not the case here).

yes, I inadvertently dropped this:

commit 65c38aa653c14df49e19faad74bd375f36e61c57
Author: David Ahern <dsa@cumulusnetworks.com>
Date:   Tue Feb 23 10:10:26 2016 -0800

     net: vrf: Remove direct access to skb->data

when I forward ported this patch. Will fix and re-send.

>
>> +static inline
>> +struct sk_buff *l3mdev_l3_rcv(struct sk_buff *skb, u16 proto)
>> +{
>> +	struct net_device *master = NULL;
>> +
>> +	if (netif_is_l3_slave(skb->dev))
>> +		master = netdev_master_upper_dev_get_rcu(skb->dev);
>> +
>> +	else if (netif_is_l3_master(skb->dev))
>> +		master = skb->dev;
>> +
>> +	if (master && master->l3mdev_ops->l3mdev_l3_rcv)
>> +		skb = master->l3mdev_ops->l3mdev_l3_rcv(master, skb, proto);
>
> In the case where netif_is_l3_master(skb->dev) is true, can you explain
> why we need to pass it through the l3mdev_l3_rcv callback again?

what do you mean again? This is only time the l3mdev_l3_rcv method is 
called on a packet.

Re: [PATCH net-next v2 1/2] net: l3mdev: Add hook in ip and ipv6

[Shmulik Ladkani]

Hi David,

On Sat, 7 May 2016 08:50:49 -0600 David Ahern <dsa@cumulusnetworks.com> wrote:
> >> +static inline
> >> +struct sk_buff *l3mdev_l3_rcv(struct sk_buff *skb, u16 proto)
> >> +{
> >> +	struct net_device *master = NULL;
> >> +
> >> +	if (netif_is_l3_slave(skb->dev))
> >> +		master = netdev_master_upper_dev_get_rcu(skb->dev);
> >> +
> >> +	else if (netif_is_l3_master(skb->dev))
> >> +		master = skb->dev;
> >> +
> >> +	if (master && master->l3mdev_ops->l3mdev_l3_rcv)
> >> +		skb = master->l3mdev_ops->l3mdev_l3_rcv(master, skb, proto);  
> >
> > In the case where netif_is_l3_master(skb->dev) is true, can you explain
> > why we need to pass it through the l3mdev_l3_rcv callback again?  
> 
> what do you mean again? This is only time the l3mdev_l3_rcv method is 
> called on a packet.

You have the following:

	if (netif_is_l3_slave(skb->dev))
		master = netdev_master_upper_dev_get_rcu(skb->dev);
	else if (netif_is_l3_master(skb->dev))
		master = skb->dev;
	if (master && master->l3mdev_ops->l3mdev_l3_rcv)
		skb = master->l3mdev_ops->l3mdev_l3_rcv(master, skb, proto);

So in both conditions (skb->dev being the slave or the master) the skb
is passed to master's l3mdev_l3_rcv callback.

Appreciate if you can elaborate:
 - Why callback needs to be invoked when skb->dev is the L3 master?
 - On which circumstances we end up entering
   l3mdev_ip_rcv/l3mdev_ip6_rcv where skb->dev is the master?
   If I got it right, we enter 'ip_rcv_finish' on a slave device,
   the callback is invoked and eventually sets skb->dev and skb->skb_iif
   to the VRF device; then ip_rcv_finish continues processing the
   altered skb (with the changed skb->dev).
   So on which cicumstances do we enter 'ip_rcv_finish' where the
   skb->dev is ALREADY a master device?

Thanks,
Shmulik

Re: [PATCH net-next v2 1/2] net: l3mdev: Add hook in ip and ipv6

[David Ahern]

On 5/7/16 12:32 PM, Shmulik Ladkani wrote:
> Hi David,
>
> On Sat, 7 May 2016 08:50:49 -0600 David Ahern <dsa@cumulusnetworks.com> wrote:
>>>> +static inline
>>>> +struct sk_buff *l3mdev_l3_rcv(struct sk_buff *skb, u16 proto)
>>>> +{
>>>> +	struct net_device *master = NULL;
>>>> +
>>>> +	if (netif_is_l3_slave(skb->dev))
>>>> +		master = netdev_master_upper_dev_get_rcu(skb->dev);
>>>> +
>>>> +	else if (netif_is_l3_master(skb->dev))
>>>> +		master = skb->dev;
>>>> +
>>>> +	if (master && master->l3mdev_ops->l3mdev_l3_rcv)
>>>> +		skb = master->l3mdev_ops->l3mdev_l3_rcv(master, skb, proto);
>>>
>>> In the case where netif_is_l3_master(skb->dev) is true, can you explain
>>> why we need to pass it through the l3mdev_l3_rcv callback again?
>>
>> what do you mean again? This is only time the l3mdev_l3_rcv method is
>> called on a packet.
>
> You have the following:
>
> 	if (netif_is_l3_slave(skb->dev))
> 		master = netdev_master_upper_dev_get_rcu(skb->dev);
> 	else if (netif_is_l3_master(skb->dev))
> 		master = skb->dev;
> 	if (master && master->l3mdev_ops->l3mdev_l3_rcv)
> 		skb = master->l3mdev_ops->l3mdev_l3_rcv(master, skb, proto);
>
> So in both conditions (skb->dev being the slave or the master) the skb
> is passed to master's l3mdev_l3_rcv callback.
>
> Appreciate if you can elaborate:
>   - Why callback needs to be invoked when skb->dev is the L3 master?

Every l3mdev_ops has converged on that ordering -- if l3 slave else if 
l3 master.

>   - On which circumstances we end up entering
>     l3mdev_ip_rcv/l3mdev_ip6_rcv where skb->dev is the master?
>     If I got it right, we enter 'ip_rcv_finish' on a slave device,
>     the callback is invoked and eventually sets skb->dev and skb->skb_iif
>     to the VRF device; then ip_rcv_finish continues processing the
>     altered skb (with the changed skb->dev).
>     So on which cicumstances do we enter 'ip_rcv_finish' where the
>     skb->dev is ALREADY a master device?

If you look at the full patchset I posted on 5/4 the patch after PKTINFO 
allows local traffic. That change needs the netif_is_l3_master().

Re: [PATCH net-next v2 1/2] net: l3mdev: Add hook in ip and ipv6

[Shmulik Ladkani]

Hi,

On Sat, 7 May 2016 20:25:40 -0600 David Ahern <dsa@cumulusnetworks.com> wrote:
> >   - On which circumstances we end up entering
> >     l3mdev_ip_rcv/l3mdev_ip6_rcv where skb->dev is the master?
> >     If I got it right, we enter 'ip_rcv_finish' on a slave device,
> >     the callback is invoked and eventually sets skb->dev and skb->skb_iif
> >     to the VRF device; then ip_rcv_finish continues processing the
> >     altered skb (with the changed skb->dev).
> >     So on which cicumstances do we enter 'ip_rcv_finish' where the
> >     skb->dev is ALREADY a master device?  
> 
> If you look at the full patchset I posted on 5/4 the patch after PKTINFO 
> allows local traffic. That change needs the netif_is_l3_master().

I see. Thanks David.

[PATCH net-next 2/2] net: original ingress device index in PKTINFO

[David Ahern]

Applications such as OSPF and BFD need the original ingress device not
the VRF device; the latter can be derived from the former. To that end
add the skb_iif to inet_skb_parm and set it in ipv4 code after clearing
the skb control buffer similar to IPv6. From there the pktinfo can just
pull it from cb with the PKTINFO_SKB_CB cast.

The previous patch moving the skb->dev change to L3 means nothing else
is needed for IPv6; it just works.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 include/net/ip.h       | 1 +
 net/ipv4/ip_input.c    | 1 +
 net/ipv4/ip_sockglue.c | 7 ++++++-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index 247ac82e9cf2..37165fba3741 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -36,6 +36,7 @@
 struct sock;
 
 struct inet_skb_parm {
+	int			iif;
 	struct ip_options	opt;		/* Compiled IP options		*/
 	unsigned char		flags;
 
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 37375eedeef9..4b351af3e67b 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -478,6 +478,7 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt,
 
 	/* Remove any debris in the socket control block */
 	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
+	IPCB(skb)->iif = skb->skb_iif;
 
 	/* Must drop socket now because of tproxy. */
 	skb_orphan(skb);
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index bdb222c0c6a2..5805762d7fc7 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -1193,7 +1193,12 @@ void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb)
 		       ipv6_sk_rxinfo(sk);
 
 	if (prepare && skb_rtable(skb)) {
-		pktinfo->ipi_ifindex = inet_iif(skb);
+		/* skb->cb is overloaded: prior to this point it is IP{6}CB
+		 * which has interface index (iif) as the first member of the
+		 * underlying inet{6}_skb_parm struct. This code then overlays
+		 * PKTINFO_SKB_CB and in_pktinfo also has iif as the first
+		 * element so the iif is picked up from the prior IPCB
+		 */
 		pktinfo->ipi_spec_dst.s_addr = fib_compute_spec_dst(skb);
 	} else {
 		pktinfo->ipi_ifindex = 0;
-- 
2.1.4

Re: [PATCH net-next 2/2] net: original ingress device index in PKTINFO

[Shmulik Ladkani]

Hi David,

On Fri,  6 May 2016 18:49:41 -0700 David Ahern <dsa@cumulusnetworks.com> wrote:
> Applications such as OSPF and BFD need the original ingress device not
> the VRF device;

Would you consider this true for any IP_PKTINFO users in VRF setups?

> @@ -1193,7 +1193,12 @@ void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb)
>  		       ipv6_sk_rxinfo(sk);
>  
>  	if (prepare && skb_rtable(skb)) {
> -		pktinfo->ipi_ifindex = inet_iif(skb);
> +		/* skb->cb is overloaded: prior to this point it is IP{6}CB
> +		 * which has interface index (iif) as the first member of the
> +		 * underlying inet{6}_skb_parm struct. This code then overlays
> +		 * PKTINFO_SKB_CB and in_pktinfo also has iif as the first
> +		 * element so the iif is picked up from the prior IPCB
> +		 */

Better if there was a guarantee in the code that inet_skb_parm layout stays
that way. Or instead just explicitly assign the iif.

Regards,
Shmulik

Re: [PATCH net-next 2/2] net: original ingress device index in PKTINFO

[David Ahern]

On 5/7/16 2:41 AM, Shmulik Ladkani wrote:
> Hi David,
>
> On Fri,  6 May 2016 18:49:41 -0700 David Ahern <dsa@cumulusnetworks.com> wrote:
>> Applications such as OSPF and BFD need the original ingress device not
>> the VRF device;
>
> Would you consider this true for any IP_PKTINFO users in VRF setups?

yes. I was just giving specific examples, but certainly every app I am 
aware of wants the enslaved index. If an app pops up that wants the vrf 
index it can be derived from the enslaved index.

>
>> @@ -1193,7 +1193,12 @@ void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb)
>>   		       ipv6_sk_rxinfo(sk);
>>
>>   	if (prepare && skb_rtable(skb)) {
>> -		pktinfo->ipi_ifindex = inet_iif(skb);
>> +		/* skb->cb is overloaded: prior to this point it is IP{6}CB
>> +		 * which has interface index (iif) as the first member of the
>> +		 * underlying inet{6}_skb_parm struct. This code then overlays
>> +		 * PKTINFO_SKB_CB and in_pktinfo also has iif as the first
>> +		 * element so the iif is picked up from the prior IPCB
>> +		 */
>
> Better if there was a guarantee in the code that inet_skb_parm layout stays
> that way. Or instead just explicitly assign the iif.

At this point inet_iif points to the vrf device so can't use it.

Re: [PATCH net-next 2/2] net: original ingress device index in PKTINFO

[Shmulik Ladkani]

Hi,

On Sat, 7 May 2016 08:53:44 -0600 David Ahern <dsa@cumulusnetworks.com> wrote:
> >> @@ -1193,7 +1193,12 @@ void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb)
> >>   		       ipv6_sk_rxinfo(sk);
> >>
> >>   	if (prepare && skb_rtable(skb)) {
> >> -		pktinfo->ipi_ifindex = inet_iif(skb);
> >> +		/* skb->cb is overloaded: prior to this point it is IP{6}CB
> >> +		 * which has interface index (iif) as the first member of the
> >> +		 * underlying inet{6}_skb_parm struct. This code then overlays
> >> +		 * PKTINFO_SKB_CB and in_pktinfo also has iif as the first
> >> +		 * element so the iif is picked up from the prior IPCB
> >> +		 */  
> >
> > Better if there was a guarantee in the code that inet_skb_parm layout stays
> > that way. Or instead just explicitly assign the iif.  
> 
> At this point inet_iif points to the vrf device so can't use it.

Initially I was thinking about explicitly getting the iif out of the
IPCB first, then assign to ipi_ifindex. Seems more readable, and less
fragile. However this depends on the IPCB/IP6CB layout relationship as
well (iif being first on both).

I assume documenting the IP{6}CB/PKTINFO_SKB_CB layout relationship at the
struct definitions would be beneficial.

Regards,
Shmulik