Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Fri, 20 Oct 2023 15:45:26 +0200
From:   Sabrina Dubroca <sd@queasysnail.net>
To:     "Radu Pirea (NXP OSS)" <radu-nicolae.pirea@oss.nxp.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, andrew@lunn.ch, hkallweit1@gmail.com,
        linux@armlinux.org.uk, richardcochran@gmail.com,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        sebastian.tobuschat@oss.nxp.com
Subject: Re: [PATCH net-next v7 3/7] net: macsec: revert the MAC address if
 mdo_upd_secy fails
Message-ID: <ZTKEdvpNl-gwGpk6@hog>
References: <20231019120209.290480-1-radu-nicolae.pirea@oss.nxp.com>
 <20231019120209.290480-4-radu-nicolae.pirea@oss.nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20231019120209.290480-4-radu-nicolae.pirea@oss.nxp.com>
Precedence: bulk

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