Messages in this thread | | | Date | Fri, 20 Oct 2023 15:45:26 +0200 | From | Sabrina Dubroca <> | Subject | Re: [PATCH net-next v7 3/7] net: macsec: revert the MAC address if mdo_upd_secy fails |
| |
2023-10-19, 15:02:05 +0300, Radu Pirea (NXP OSS) wrote: > /* If h/w offloading is available, propagate to the device */ > - if (macsec_is_offloaded(macsec)) { > + if (offloaded) { > const struct macsec_ops *ops; > struct macsec_context ctx; > > ops = macsec_get_ops(macsec, &ctx); > - if (ops) { > - ctx.secy = &macsec->secy; > - macsec_offload(ops->mdo_upd_secy, &ctx); > + if (!ops) { > + err = -EINVAL;
For consistency with other places where macsec_get_ops fails, this should probably be -EOPNOTSUPP.
I'm not opposed to this change, but I'm not sure how it's related to the missing rollback issue. Can you explain that a bit?
> + goto restore_old_addr; > } > + > + ctx.secy = &macsec->secy; > + err = macsec_offload(ops->mdo_upd_secy, &ctx); > + if (err) > + goto restore_old_addr; > } > > return 0; > + > +restore_old_addr: > + if (dev->flags & IFF_UP) { > + int err;
[This bit confused me quite a bit, seeing the declaration and the "return err" out of this block]
> + err = macsec_dev_uc_update(dev, old_addr); > + if (err) > + return err;
If we can avoid it, we should try to have a rollback path without any possible failures, otherwise we'll leave things in a broken state (in this case, telling the user that the MAC address change failed, but leaving the new address set on the macsec device).
Paolo suggested to postpone the dev_uc_del until after the offload code, so we'd end up with something like this [completely untested]:
if (dev->flags & IFF_UP) { err = dev_uc_add(real_dev, addr->sa_data); if (err < 0) return err; }
ether_addr_copy(old_addr, dev->dev_addr); eth_hw_addr_set(dev, addr->sa_data);
/* If h/w offloading is available, propagate to the device */ if (macsec_is_offloaded(macsec)) { // ... }
if (dev->flags & IFF_UP) dev_uc_del(real_dev, old_addr);
return 0;
restore_old_addr: if (dev->flags & IFF_UP) dev_uc_del(real_dev, addr->sa_data);
eth_hw_addr_set(dev, old_addr);
return err;
Install both UC addresses, then remove the one we don't need.
-- Sabrina
| |