| 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: Mon, 3 Apr 2023 09:29:28 +0000
From: Emeel Hakim <ehakim@...dia.com>
To: Sabrina Dubroca <sd@...asysnail.net>,
Leon Romanovsky <leon@...nel.org>
CC: "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
> -----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;
...
}
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;
}
if this is the case then I totally agree that this is a better approach.
> > > AFAIU, vlandev->hw_features should be based on realdev->vlan_features.
> >
> > Is this macsec offloaded VLAN can be called "child VLAN device"?
> >
> > 33 3. netdev->vlan_features set contains features whose state is inherited
> > 34 by child VLAN devices (limits netdev->features set). This is currently
> > 35 used for all VLAN devices whether tags are stripped or inserted in
> > 36 hardware or software.
>
> Yes. In this patch, we're talking about this situation:
>
> eth0 --------------> vlan0 --------------> macsec0
> real NIC
> (capable of (can also
> offloading MACsec) offload MACsec)
>
> And vlan0 would be a "child VLAN device" of eth0.
>
> "limits netdev->features set" is the netdev_intersect_features you quoted in your
> previous email.
>
> > > I don't see a reason to advertise a feature in the vlan device if we
> > > won't ever be able to turn it on because it's not in ->vlan_features
> > > ("grmbl why can't I enable it, ethtool says it's here?!").
> > >
> > >
> > > Emeel, I'm not a maintainer, but I don't think you should be
> > > reposting until the existing discussion has settled down.
> > >
> > > > >
> > > > > [...]
> > > > > > @@ -572,6 +573,9 @@ static int vlan_dev_init(struct net_device *dev)
> > > > > > NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
> > > > > > NETIF_F_ALL_FCOE;
> > > > > >
> > > > > > + if (real_dev->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)
> > >
> > > --
> > > Sabrina
> > >
>
> --
> Sabrina
Powered by blists - more mailing lists