| Message ID | 1520377028-14818-1-git-send-email-shannon.nelson@oracle.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] macvlan: filter out xfrm feature flags | expand |
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.
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
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; }
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(-)