Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Sabrina Dubroca <sd@queasysnail.net>]

2023-04-03, 09:29:28 +0000, Emeel Hakim wrote:
> 
> 
> > -----Original Message-----
> > From: Sabrina Dubroca <sd@queasysnail.net>
> > Sent: Thursday, 30 March 2023 23:33
> > To: Leon Romanovsky <leon@kernel.org>
> > Cc: Emeel Hakim <ehakim@nvidia.com>; davem@davemloft.net; kuba@kernel.org;
> > pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org
> > Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN
> > interface
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > 2023-03-30, 21:56:56 +0300, Leon Romanovsky wrote:
> > > On Thu, Mar 30, 2023 at 07:19:21PM +0200, Sabrina Dubroca wrote:
> > > > 2023-03-29, 21:42:01 +0300, Leon Romanovsky wrote:
> > > > > On Wed, Mar 29, 2023 at 04:43:59PM +0200, Sabrina Dubroca wrote:
> > > > > > 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > > > > > > Add support for MACsec offload operations for VLAN driver to
> > > > > > > allow offloading MACsec when VLAN's real device supports
> > > > > > > Macsec offload by forwarding the offload request to it.
> > > > > > >
> > > > > > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > > > > > > ---
> > > > > > > V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> > > > > >
> > > > > > Uh? You're not actually doing that? You also dropped the changes
> > > > > > to vlan_dev_fix_features without explaining why.
> > > > >
> > > > > vlan_dev_fix_features() relies on real_dev->vlan_features which
> > > > > was set in mlx5 part of this patch.
> > > > >
> > > > >   643 static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
> > > > >   644         netdev_features_t features)
> > > > >   645 {
> > > > >   ...
> > > > >   649
> > > > >   650         lower_features = netdev_intersect_features((real_dev-
> > >vlan_features |
> > > > >   651                                                     NETIF_F_RXCSUM),
> > > > >   652                                                    real_dev->features);
> > > > >
> > > > > This part ensure that once real_dev->vlan_features and
> > > > > real_dev->features have NETIF_F_HW_MACSEC, the returned features will
> > include NETIF_F_HW_MACSEC too.
> > > >
> > > > Ok, thanks.
> > > >
> > > > But back to the issue of vlan_features, in vlan_dev_init: I'm not
> > > > convinced NETIF_F_HW_MACSEC should be added to hw_features based on
> > > > ->features. That would result in a new vlan device that can't
> > > > ->offload
> > > > macsec at all if it was created at the wrong time (while the lower
> > > > device's macsec offload was temporarily disabled).
> > >
> > > Sorry, I'm new to this netdev features zoo, but if I read correctly
> > > Documentation/networking/netdev-features.rst, the ->features is the
> > > list of enabled ones:
> > >
> > >    29  2. netdev->features set contains features which are currently enabled
> > >    30     for a device.  This should be changed only by network core or in
> > >    31     error paths of ndo_set_features callback.
> > >
> > > And user will have a chance to disable it for VLAN because it was
> > > added to ->hw_features:
> > >
> > >    24  1. netdev->hw_features set contains features whose state may possibly
> > >    25     be changed (enabled or disabled) for a particular device by user's
> > >    26     request.  This set should be initialized in ndo_init callback and not
> > >    27     changed later.
> > >
> > > So how can VLAN be created with NETIF_F_HW_MACSEC while real_dev
> > > mcasec offload is disabled?
> > 
> > I'm proposing that be VLAN device be created with the capability (->hw_features
> > contains NETIF_F_HW_MACSEC) but disabled (->features doesn't contain
> > NETIF_F_HW_MACSEC). That way, if NETIF_F_HW_MACSEC is re-enabled on the
> > lower device, you don't need to destroy the VLAN device to enable macsec offload
> > on it as well. You still won't be able to enable macsec offload on the VLAN device
> > unless it's active on the real NIC.
> > 
> > I think whether the lower device currently has NETIF_F_HW_MACSEC should only
> > affect whether you can enable the feature on the vlan device right now. What
> > feature is enabled at creation time should be irrelevant.
> 
> Thanks for the proposal Sabrina, I'm also new to this netdev features zone so IIUC your'e 
> proposing that we have NETIF_F_HW_MACSEC added to the dev->hw_features upon 
> vlan_dev_init, but disabled (we don’t add it to dev->features) , and upon vlan_dev_fix_features
> we check if the real_device have NETIF_F_HW_MACSEC enabled (after the intersect with the real_dev->vlan_features) 
> and if so we add it to the features.
> 
> So something like:
> 
> static int vlan_dev_init(struct net_device *dev)
> {
> ...
> 	dev->features |= dev->hw_features | NETIF_F_LLTX;
> 	dev->hw_features |= NETIF_F_HW_MACSEC;
> ...
> }

That would be adding the NETIF_F_HW_MACSEC to all VLAN devices,
whether the lower device advertises this feature or not. That's wrong.


What I had in mind was:

	if (real_dev->vlan_features & NETIF_F_HW_MACSEC)
		dev->hw_features |= NETIF_F_HW_MACSEC;


And we should enable it by default when the lower device has it
enabled, which would be the case with this:

@@ -572,6 +572,9 @@ static int vlan_dev_init(struct net_device *dev)
 			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
 			   NETIF_F_ALL_FCOE;
 
+	if (real_dev->vlan_features & NETIF_F_HW_MACSEC)
+		dev->hw_features |= NETIF_F_HW_MACSEC;
+
 	dev->features |= dev->hw_features | NETIF_F_LLTX;
 	netif_inherit_tso_max(dev, real_dev);
 	if (dev->features & NETIF_F_VLAN_FEATURES)


What I meant by "but disabled" in my previous email was that if the
lower device currently has NETIF_F_HW_MACSEC, the new vlan device
should also have it disabled, not that it should always be disabled on
creation.


> static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
>          netdev_features_t features)
> {
> ...
>  	if (lower_features &  NETIF_F_HW_MACSEC)
>  		features |= NETIF_F_HW_MACSEC;
> 
> return features;
> }

I don't think NETIF_F_HW_MACSEC is "special" enough to require hacks
in vlan_dev_fix_features. IMHO modifying vlan_dev_fix_features should
only happen if we have no other way to implement a consistent and
useful behavior. I don't think that's the case here.

-- 
Sabrina