| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 5 Apr 2023 11:35:00 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Emeel Hakim <ehakim@...dia.com>
Cc: Leon Romanovsky <leon@...nel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for
VLAN interface
2023-04-04, 14:37:58 +0000, Emeel Hakim wrote:
>
>
> > -----Original Message-----
> > From: Sabrina Dubroca <sd@...asysnail.net>
> > Sent: Tuesday, 4 April 2023 15:54
> > To: Emeel Hakim <ehakim@...dia.com>
> > Cc: Leon Romanovsky <leon@...nel.org>; davem@...emloft.net;
> > kuba@...nel.org; pabeni@...hat.com; edumazet@...gle.com;
> > netdev@...r.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-04-03, 09:29:28 +0000, Emeel Hakim wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Sabrina Dubroca <sd@...asysnail.net>
> > > > Sent: Thursday, 30 March 2023 23:33
> > > > To: Leon Romanovsky <leon@...nel.org>
> > > > Cc: Emeel Hakim <ehakim@...dia.com>; davem@...emloft.net;
> > > > kuba@...nel.org; pabeni@...hat.com; edumazet@...gle.com;
> > > > netdev@...r.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@...dia.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.
> >
>
> Thanks for the explanation its clear for me now, I tested it and its working for me.
> I agree with this approach.
> I can prepare a new version if we are closed on everything.
> Should I send a v3 (since previous v3 got discarded) or I send it as a v4 ?
I'd call that v4 so that nobody gets confused. It's going to be
different from the v3 you already posted. I'm going through the
patches again, I have a couple of other comments.
--
Sabrina
Powered by blists - more mailing lists