[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <167334021775.17820.2386827809582589477@kwain.local>
Date: Tue, 10 Jan 2023 09:43:37 +0100
From: Antoine Tenart <atenart@...nel.org>
To: Sabrina Dubroca <sd@...asysnail.net>, ehakim@...dia.com
Cc: netdev@...r.kernel.org, raeds@...dia.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink
Quoting Sabrina Dubroca (2023-01-09 16:14:32)
> 2023-01-09, 10:55:56 +0200, ehakim@...dia.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:
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.
}
Thanks,
Antoine
Powered by blists - more mailing lists