2023-09-01, 09:09:06 +0000, Radu Pirea wrote:
> On Wed, 2023-08-30 at 13:35 +0200, Sabrina Dubroca wrote:
> ...
>
> > And it's not restored when the link goes back up? That's inconvenient
> > :/
> > Do we end up with inconsistent state? ie driver and core believe
> > everything is still offloaded, but HW lost all state? do we leak
> > some resources allocated by the driver?
>
> Yes. We end up with inconsistent state. The HW will lost all state when
> the phy is reseted. No resource is leaked, everything is there, but the
> configuration needs to be reapplied.
>
> >
> > We could add a flush/restore in macsec_notify when the lower device
> > goes down/up, maybe limited to devices that request this (I don't
> > know
> > if all devices would need it, or maybe all devices offloading to the
> > PHY but not to the MAC).
>
> Agreed.
> We can do a flush very simple, but to restore the configuration maybe
> we should to save the key in the macsec_key structure. I am not sure if
> the key can be extracted from crypto_aead structure.
Either that or in the driver. I have a small preference for driver,
because then cases that don't need this restore won't have to keep the
key in memory, reducing the likelihood of accidentally sharing it.
OTOH, if we centralize that code, it's easier to make sure everything
is cleared from kernel memory when we delete the SA.
> > And what happens in this case?
> > ip link add link eth0 type macsec offload phy
> > ip link set eth0 down
> > ip macsec add macsec0 rx sci ...
> > ip macsec add macsec0 tx sa 0 ...
> > # etc
> > ip link set eth0 up
> >
> > Will offload work with the current code?
>
> (the interface was up before)
> [root@alarm ~]# ip link add link end0 macsec0 type macsec encrypt on
> offload phy
> [root@alarm ~]# ip link set end0 down
> [root@alarm ~]# ip macsec add macsec0 rx port 1 address
> 00:01:be:be:ef:33
> RTNETLINK answers: Operation not supported
Where does that EOPNOTSUPP come from? nxp_c45_mdo_add_rxsc from this
version of the code can't return that, and macsec_add_rxsc also
shouldn't at this point.
Ideally all implementations (HW or SW) should behave the same, but at
least that saves us from having to restore state in the HW, if we
couldn't create it at all.
> But let's consider the next case:
> ip link add link eth0 type macsec offload phy
> ip link set eth0 down
> ip link set eth0 up
> ip macsec add macsec0 rx sci ...
> ip macsec add macsec0 tx sa 0 ...
> # etc
>
> In this case, any HW configuration written by .mdo_add_secy will be
> lost.
So we need a way to restore the config in HW, whether that's done
entirely in the driver or initiated by macsec itself.
Antoine, is any of this relevant to the mscc driver?
--
Sabrina
On Fri, Sep 01, 2023 at 12:07:32PM +0200, Sabrina Dubroca wrote:
> 2023-09-01, 09:09:06 +0000, Radu Pirea wrote:
> > On Wed, 2023-08-30 at 13:35 +0200, Sabrina Dubroca wrote:
> > ...
> >
> > > And it's not restored when the link goes back up? That's inconvenient
> > > :/
> > > Do we end up with inconsistent state? ie driver and core believe
> > > everything is still offloaded, but HW lost all state? do we leak
> > > some resources allocated by the driver?
> >
> > Yes. We end up with inconsistent state. The HW will lost all state when
> > the phy is reseted. No resource is leaked, everything is there, but the
> > configuration needs to be reapplied.
> >
> > >
> > > We could add a flush/restore in macsec_notify when the lower device
> > > goes down/up, maybe limited to devices that request this (I don't
> > > know
> > > if all devices would need it, or maybe all devices offloading to the
> > > PHY but not to the MAC).
> >
> > Agreed.
> > We can do a flush very simple, but to restore the configuration maybe
> > we should to save the key in the macsec_key structure. I am not sure if
> > the key can be extracted from crypto_aead structure.
>
> Either that or in the driver. I have a small preference for driver,
> because then cases that don't need this restore won't have to keep the
> key in memory, reducing the likelihood of accidentally sharing it.
> OTOH, if we centralize that code, it's easier to make sure everything
> is cleared from kernel memory when we delete the SA.
Maybe consider about doing it as a library function, so drivers that
need this don't have to reimplement the functionality in randomly
buggy ways?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!