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

2023-01-10, 11:29:27 +0100, Antoine Tenart wrote:

Quoting Emeel Hakim (2023-01-10 10:05:36)

quoted

quoted

Quoting Sabrina Dubroca (2023-01-09 16:14:32)

quoted

2023-01-09, 10:55:56 +0200, ehakim@nvidia.com wrote:

quoted

@@ -3840,6 +3835,12 @@ static int macsec_changelink(struct net_device

*dev, struct nlattr *tb[],

quoted

quoted

      if (ret)
              goto cleanup;

+     if (data[IFLA_MACSEC_OFFLOAD]) {
+             ret = macsec_update_offload(dev,

nla_get_u8(data[IFLA_MACSEC_OFFLOAD]));

quoted

quoted

+             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.

quoted

        ... 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

quoted

  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