Re: [PATCHv2 next 3/3] ipvlan: Introduce l3s mode

Hi David, thanks for the comments.

On Tue, Sep 13, 2016 at 8:24 PM, David Ahern [off-list ref] wrote:

On 9/12/16 12:01 PM, Mahesh Bandewar wrote:

quoted

+struct sk_buff *ipvlan_l3_rcv(struct net_device *dev, struct sk_buff *skb,
+                           u16 proto)
+{
+     struct ipvl_addr *addr;
+     struct net_device *sdev;
+
+     addr = ipvlan_skb_to_addr(skb, dev);
+     if (!addr)
+             goto out;
+
+     sdev = addr->master->dev;
+     switch (proto) {
+     case AF_INET:
+     {
+             int err;
+             struct iphdr *ip4h = ip_hdr(skb);
+
+             err = ip_route_input_noref(skb, ip4h->daddr, ip4h->saddr,
+                                        ip4h->tos, sdev);
+             if (unlikely(err))
+                     goto out;
+             break;
+     }
+     case AF_INET6:
+     {
+             struct dst_entry *dst;
+             struct ipv6hdr *ip6h = ipv6_hdr(skb);
+             int flags = RT6_LOOKUP_F_HAS_SADDR;
+             struct flowi6 fl6 = {
+                     .flowi6_iif   = sdev->ifindex,
+                     .daddr        = ip6h->daddr,
+                     .saddr        = ip6h->saddr,
+                     .flowlabel    = ip6_flowinfo(ip6h),
+                     .flowi6_mark  = skb->mark,
+                     .flowi6_proto = ip6h->nexthdr,
+             };
+
+             skb_dst_drop(skb);
+             dst = ip6_route_input_lookup(dev_net(sdev), sdev, &fl6, flags);
+             skb_dst_set(skb, dst);
+             break;
+     }
+     default:
+             break;
+     }

Nit: why not put the above in separate per-version functions (ipvlan_ip_rcv and ipvlan_ip6_rcv) similar to what is done for ipvlan_process_outbound?

I can but it's small enough to have it together. Also 'proto' is a
parameter to the handler and makes it easier However do you see any
issue having just one function?

quoted

+
+out:
+     return skb;
+}
+
+unsigned int ipvlan_nf_input(void *priv, struct sk_buff *skb,
+                          const struct nf_hook_state *state)
+{
+     struct ipvl_addr *addr;
+     unsigned int len;
+
+     addr = ipvlan_skb_to_addr(skb, skb->dev);
+     if (!addr)
+             goto out;
+
+     skb->dev = addr->master->dev;
+     len = skb->len + ETH_HLEN;
+     ipvlan_count_rx(addr->master, len, true, false);
+out:
+     return NF_ACCEPT;
+}

diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 18b4e8c7f68a..d02be277e1db 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c

@@ -9,24 +9,65 @@

 #include "ipvlan.h"

+static struct nf_hook_ops ipvl_nfops[] __read_mostly = {
+     {
+             .hook     = ipvlan_nf_input,
+             .pf       = NFPROTO_IPV4,
+             .hooknum  = NF_INET_LOCAL_IN,
+             .priority = INT_MAX,
+     },
+     {
+             .hook     = ipvlan_nf_input,
+             .pf       = NFPROTO_IPV6,
+             .hooknum  = NF_INET_LOCAL_IN,
+             .priority = INT_MAX,
+     },
+};
+
+static struct l3mdev_ops ipvl_l3mdev_ops __read_mostly = {
+     .l3mdev_l3_rcv = ipvlan_l3_rcv,
+};
+
 static void ipvlan_adjust_mtu(struct ipvl_dev *ipvlan, struct net_device *dev)
 {
      ipvlan->dev->mtu = dev->mtu - ipvlan->mtu_adj;
 }

-static void ipvlan_set_port_mode(struct ipvl_port *port, u16 nval)
+static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval)
 {
      struct ipvl_dev *ipvlan;
+     int err = 0;

+     ASSERT_RTNL();
      if (port->mode != nval) {
+             if (nval == IPVLAN_MODE_L3S) {
+                     port->dev->l3mdev_ops = &ipvl_l3mdev_ops;
+                     port->dev->priv_flags |= IFF_L3MDEV_MASTER;
+                     if (!port->ipt_hook_added) {
+                             err = _nf_register_hooks(ipvl_nfops,
+                                                     ARRAY_SIZE(ipvl_nfops));

That's clever. The hooks are not device based so why do the register for each device? Alternatively, you could use a static dst like VRF does for Tx. In the ipvlan rcv function set the dst input handler to send the packet back to the ipvlan driver via dst->input. From there send the packet through the netfilter hooks and then do the real lookup, update the dst and call its input function. I have working code for VRF driver somewhere that shows how to do this.

Do you mean per slave device?  It's registering it for a port (so only
once) depending on the mode. If the mode != l3s it wont bother
registering to keep current modes as they are.
I'm sure dst idea could work as well (as you have suggested) but
l3mdev + ipt-hook is simpler and probably far less intrusive.

quoted

+                             if (!err)
+                                     port->ipt_hook_added = true;
+                             else
+                                     return err;
+                     }
+             } else {
+                     port->dev->priv_flags &= ~IFF_L3MDEV_MASTER;
+                     port->dev->l3mdev_ops = NULL;
+                     if (port->ipt_hook_added)
+                             _nf_unregister_hooks(ipvl_nfops,
+                                                  ARRAY_SIZE(ipvl_nfops));
+                     port->ipt_hook_added = false;
+             }