Re: [PATCH net-next v7 3/7] net: macsec: revert the MAC address if mdo_upd_secy fails

[Sabrina Dubroca <sd@queasysnail.net>]

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