[net-next-2.6 PATCH] macvlan: add VLAN filters to lowerdev

[net-next-2.6 PATCH] macvlan: add VLAN filters to lowerdev

[John Fastabend]

Stacking VLANs on top of the macvlan device does not
work if the lowerdev device is using vlan filters set
by NETIF_F_HW_VLAN_FILTER. Add ndo ops to pass vlan
calls to lowerdev.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 drivers/net/macvlan.c |   39 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index d6aeaa5..cc67cbe 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -414,7 +414,8 @@ static struct lock_class_key macvlan_netdev_addr_lock_key;
 #define MACVLAN_FEATURES \
 	(NETIF_F_SG | NETIF_F_ALL_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \
 	 NETIF_F_GSO | NETIF_F_TSO | NETIF_F_UFO | NETIF_F_GSO_ROBUST | \
-	 NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM)
+	 NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \
+	 NETIF_F_HW_VLAN_FILTER)
 
 #define MACVLAN_STATE_MASK \
 	((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT))
@@ -509,6 +510,39 @@ static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev,
 	return stats;
 }
 
+static void macvlan_vlan_rx_register(struct net_device *dev,
+				     struct vlan_group *grp)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	struct net_device *lowerdev = vlan->lowerdev;
+	const struct net_device_ops *ops = lowerdev->netdev_ops;
+
+	if (ops->ndo_vlan_rx_register)
+		ops->ndo_vlan_rx_register(lowerdev, grp);
+}
+
+static void macvlan_vlan_rx_add_vid(struct net_device *dev,
+				    unsigned short vid)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	struct net_device *lowerdev = vlan->lowerdev;
+	const struct net_device_ops *ops = lowerdev->netdev_ops;
+
+	if (ops->ndo_vlan_rx_add_vid)
+		ops->ndo_vlan_rx_add_vid(lowerdev, vid);
+}
+
+static void macvlan_vlan_rx_kill_vid(struct net_device *dev,
+				     unsigned short vid)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	struct net_device *lowerdev = vlan->lowerdev;
+	const struct net_device_ops *ops = lowerdev->netdev_ops;
+
+	if (ops->ndo_vlan_rx_kill_vid)
+		ops->ndo_vlan_rx_kill_vid(lowerdev, vid);
+}
+
 static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
 					struct ethtool_drvinfo *drvinfo)
 {
@@ -541,6 +575,9 @@ static const struct net_device_ops macvlan_netdev_ops = {
 	.ndo_set_multicast_list	= macvlan_set_multicast_list,
 	.ndo_get_stats64	= macvlan_dev_get_stats64,
 	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_vlan_rx_register	= macvlan_vlan_rx_register,
+	.ndo_vlan_rx_add_vid	= macvlan_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid	= macvlan_vlan_rx_kill_vid,
 };
 
 void macvlan_common_setup(struct net_device *dev)

Re: [net-next-2.6 PATCH] macvlan: add VLAN filters to lowerdev

[David Miller]

From: John Fastabend <john.r.fastabend@intel.com>
Date: Mon, 06 Jun 2011 07:27:16 -0700

> Stacking VLANs on top of the macvlan device does not
> work if the lowerdev device is using vlan filters set
> by NETIF_F_HW_VLAN_FILTER. Add ndo ops to pass vlan
> calls to lowerdev.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

I think this might have unintended side-effects.

Much of the VLAN code makes decisions based upon whether these
ops are NULL or not.

Now, no matter what is implemented in the lower device, the VLAN
code will see them non-NULL in the macvlan device.

Re: [net-next-2.6 PATCH] macvlan: add VLAN filters to lowerdev

[John Fastabend]

On 6/6/2011 3:03 PM, David Miller wrote:
> From: John Fastabend <john.r.fastabend@intel.com>
> Date: Mon, 06 Jun 2011 07:27:16 -0700
> 
>> Stacking VLANs on top of the macvlan device does not
>> work if the lowerdev device is using vlan filters set
>> by NETIF_F_HW_VLAN_FILTER. Add ndo ops to pass vlan
>> calls to lowerdev.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> 
> I think this might have unintended side-effects.
> 
> Much of the VLAN code makes decisions based upon whether these
> ops are NULL or not.
> 
> Now, no matter what is implemented in the lower device, the VLAN
> code will see them non-NULL in the macvlan device.

I would expect these decisions to be wrapped in the feature flag
like this,

        if (vlan_id && (real_dev->features & NETIF_F_HW_VLAN_FILTER))
                ops->ndo_vlan_rx_kill_vid(real_dev, vlan_id);

Although grep found two call sites not wrapped,

int register_vlan_dev(struct net_device *dev)
	[...]
        if (ngrp) {
                if (ops->ndo_vlan_rx_register)
                        ops->ndo_vlan_rx_register(real_dev, ngrp);
                rcu_assign_pointer(real_dev->vlgrp, ngrp);
        }


And,

void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
	[...]

        /* If the group is now empty, kill off the group. */
        if (grp->nr_vlans == 0) {
                vlan_gvrp_uninit_applicant(real_dev);

                rcu_assign_pointer(real_dev->vlgrp, NULL);
                if (ops->ndo_vlan_rx_register)
                        ops->ndo_vlan_rx_register(real_dev, NULL);

                /* Free the group, after all cpu's are done. */
                call_rcu(&grp->rcu, vlan_rcu_free);
        }


I could wrap these in feature flag checks as well but I see no harm
in letting these fall through to the macvlan driver and failing.

Thanks,
John.

Re: [net-next-2.6 PATCH] macvlan: add VLAN filters to lowerdev

[David Miller]

From: John Fastabend <john.r.fastabend@intel.com>
Date: Mon, 06 Jun 2011 15:44:39 -0700

> On 6/6/2011 3:03 PM, David Miller wrote:
>> From: John Fastabend <john.r.fastabend@intel.com>
>> Date: Mon, 06 Jun 2011 07:27:16 -0700
>> 
>>> Stacking VLANs on top of the macvlan device does not
>>> work if the lowerdev device is using vlan filters set
>>> by NETIF_F_HW_VLAN_FILTER. Add ndo ops to pass vlan
>>> calls to lowerdev.
>>>
>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> 
>> I think this might have unintended side-effects.
>> 
>> Much of the VLAN code makes decisions based upon whether these
>> ops are NULL or not.
>> 
>> Now, no matter what is implemented in the lower device, the VLAN
>> code will see them non-NULL in the macvlan device.
> 
> I would expect these decisions to be wrapped in the feature flag
> like this,
> 
>         if (vlan_id && (real_dev->features & NETIF_F_HW_VLAN_FILTER))
>                 ops->ndo_vlan_rx_kill_vid(real_dev, vlan_id);
> 
> Although grep found two call sites not wrapped,
 ...
> I could wrap these in feature flag checks as well but I see no harm
> in letting these fall through to the macvlan driver and failing.

Fair enough, patch applied.