[PATCH net] macvlan: filter out xfrm feature flags

[PATCH net] macvlan: filter out xfrm feature flags

[Shannon Nelson]

Adding a macvlan device on top of a lowerdev that supports
the xfrm offloads fails.
    # ip link add link ens1f0 mv0 type macvlan
    RTNETLINK answers: Operation not permitted

Tracing down the failure shows that the macvlan device inherits
the NETIF_F_HW_ESP and NETIF_F_HW_ESP_TX_CSUM feature flags from
the lowerdev, but doesn't actually support xfrm so doesn't have
the dev->xfrmdev_ops API filled in.  When the request is made
to add the new macvlan device, the various feature flags are
checked by the feature subsystems, and the xfrm_api_check()
fails the check since the dev->xfrmdev_ops are not set up.
The macvlan creation succeeds when we filter out those flags
in macvlan_fix_features().

This isn't broken for vlans because they use a separate features
connection (vlan_features) for inheriting features.  This is
fine, but I don't think trying to add something like this to
every driver for every new upperdev is a good idea - I think
the upperdev should try to protect itself.

Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
---
 drivers/net/macvlan.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 8fc02d9..76b8fb5 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -844,6 +844,10 @@ static struct lock_class_key macvlan_netdev_addr_lock_key;
 	 NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \
 	 NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER)
 
+#define MACVLAN_NON_FEATURES \
+	(NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | NETIF_F_GSO_ESP | \
+	 NETIF_F_NETNS_LOCAL)
+
 #define MACVLAN_STATE_MASK \
 	((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT))
 
@@ -1036,7 +1040,7 @@ static netdev_features_t macvlan_fix_features(struct net_device *dev,
 	lowerdev_features &= (features | ~NETIF_F_LRO);
 	features = netdev_increment_features(lowerdev_features, features, mask);
 	features |= ALWAYS_ON_FEATURES;
-	features &= ~NETIF_F_NETNS_LOCAL;
+	features &= ~MACVLAN_NON_FEATURES;
 
 	return features;
 }
-- 
2.7.4

Re: [PATCH net] macvlan: filter out xfrm feature flags

[David Miller]

From: Shannon Nelson <shannon.nelson@oracle.com>
Date: Tue,  6 Mar 2018 14:57:08 -0800

> This isn't broken for vlans because they use a separate features
> connection (vlan_features) for inheriting features.  This is
> fine, but I don't think trying to add something like this to
> every driver for every new upperdev is a good idea - I think
> the upperdev should try to protect itself.

I think this fix is correct.

But for how many upperdevs are we going to duplicate code like this,
and how many subtle differences and in fact bugs will result from all
of that duplication?

I think we really need something common for these upperdev drivers
to use.  Maybe just a macro defining feature bits to trim in this
situation.

Thanks.

Re: [PATCH net] macvlan: filter out xfrm feature flags

[Shannon Nelson]

On 3/8/2018 9:33 AM, David Miller wrote:
> From: Shannon Nelson <shannon.nelson@oracle.com>
> Date: Tue,  6 Mar 2018 14:57:08 -0800
> 
>> This isn't broken for vlans because they use a separate features
>> connection (vlan_features) for inheriting features.  This is
>> fine, but I don't think trying to add something like this to
>> every driver for every new upperdev is a good idea - I think
>> the upperdev should try to protect itself.
> 
> I think this fix is correct.
> 
> But for how many upperdevs are we going to duplicate code like this,
> and how many subtle differences and in fact bugs will result from all
> of that duplication?
> 
> I think we really need something common for these upperdev drivers
> to use.  Maybe just a macro defining feature bits to trim in this
> situation.
> 
> Thanks.

Thanks, Dave.  Yes, this could use something a little more generic, 
something that would catch any future "dangerous" bits.

I'm not sure we can come up with a generic mask to be used by everyone, 
as each upper and lower dev has their own feature support levels.  But 
we might come up with a better example for others to follow.

Rather than calling out specific non-supported bits, we can probably 
just build a mask from bits that we already know are supported, 
something like this:
	features &= (ALWAYS_ON_FEATURES | MACVLAN_FEATURES);

which would take care of NETIF_F_NETNS_LOCAL, the ESP flags, and 
anything else that wasn't already specifically called for.  I'll repost 
with this and see what folks think.

sln