Re: [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink

[Sabrina Dubroca <sd@queasysnail.net>]

2023-01-10, 11:29:27 +0100, Antoine Tenart wrote:
> Quoting Emeel Hakim (2023-01-10 10:05:36)
> > > Quoting Sabrina Dubroca (2023-01-09 16:14:32)
> > > > 2023-01-09, 10:55:56 +0200, ehakim@nvidia.com wrote:
> > > > > @@ -3840,6 +3835,12 @@ static int macsec_changelink(struct net_device
> > > *dev, struct nlattr *tb[],
> > > > >       if (ret)
> > > > >               goto cleanup;
> > > > >
> > > > > +     if (data[IFLA_MACSEC_OFFLOAD]) {
> > > > > +             ret = macsec_update_offload(dev,
> > > nla_get_u8(data[IFLA_MACSEC_OFFLOAD]));
> > > > > +             if (ret)
> > > > > +                     goto cleanup;
> > > > > +     }
> > > > > +
> > > > >       /* If h/w offloading is available, propagate to the device */
> > > > >       if (macsec_is_offloaded(macsec)) {
> > > > >               const struct macsec_ops *ops;
> > > >
> > > > There's a missing rollback of the offloading status in the (probably
> > > > quite unlikely) case that mdo_upd_secy fails, no? We can't fail
> > > > macsec_get_ops because macsec_update_offload would have failed
> > > > already, but I guess the driver could fail in mdo_upd_secy, and then
> > > > "goto cleanup" doesn't restore the offloading state.  Sorry I didn't
> > > > notice this earlier.
> > > >
> > > > In case the IFLA_MACSEC_OFFLOAD attribute is provided and we're
> > > > enabling offload, we also end up calling the driver's mdo_add_secy,
> > > > and then immediately afterwards mdo_upd_secy, which probably doesn't
> > > > make much sense.
> > > >
> > > > Maybe we could turn that into:
> > > >
> > > >     if (data[IFLA_MACSEC_OFFLOAD]) {
> > > 
> > > If data[IFLA_MACSEC_OFFLOAD] is provided but doesn't change the offloading
> > > state, then macsec_update_offload will return early and mdo_upd_secy won't be
> > > called.
> > > 
> > > >         ... macsec_update_offload
> > > >     } else if (macsec_is_offloaded(macsec)) {
> > > >         /* If h/w offloading is available, propagate to the device */
> > > >         ... mdo_upd_secy
> > > >     }
> > > >
> > > > Antoine, does that look reasonable to you?
> > > 
> > > But yes I agree we can improve the logic. Maybe something like:
> > 
> > Ack , I can do the change
> > 
> > >   prev_offload = macsec->offload;
> > >   offload = data[IFLA_MACSEC_OFFLOAD];
> > > 
> > >   if (prev_offload != offload) {
> > >       macsec_update_offload(...)
> > >   } else if (macsec_is_offloaded(macsec)) {
> > >       ...
> > >       prev_offload can be used to restore the offloading state on
> > >       failure here.
> > 
> > why do we need to restore offloading state here in case of failure?
> > we get to this case when prev_offload == offload.
> 
> Right, not restoring. The general question is: what to do with
> offloading on and an hw in an unknown state (upd failed).

Right, but I don't think that's introduced by this patch. I don't want
to block Emeel's patches because of an issue that was present before.

Do we need a way to distinguish
 - update failed but the HW is still offloading the old state, just
   roll back
 - update failed, this macsec device can't be offloaded anymore (or at
   least not until $unclear_condition)

and maybe some other variants (destroy and recreate the macsec device?
reload the NIC driver?)?

Would that help? Is that a useful distinction for admins and
management software?

-- 
Sabrina