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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <167335890996.17820.293620523946399247@kwain.local>
Date:   Tue, 10 Jan 2023 14:55:09 +0100
From:   Antoine Tenart <atenart@...nel.org>
To:     Sabrina Dubroca <sd@...asysnail.net>
Cc:     ehakim@...dia.com, 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-10 11:44:13)
> 2023-01-10, 09:43:37 +0100, Antoine Tenart wrote:
> > 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.
> 
> Ouch, thanks for catching this.
> 
> > 
> > >         ... 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];
> 
> That needs to be under if (data[IFLA_MACSEC_OFFLOAD]) and then the
> rest gets a bit messy.
> 
> > 
> >   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.
> >   }
> 
> We also have a prev != new test at the start of macsec_update_offload,
> the duplication is a bit ugly. We could move it out and then only call
> macsec_update_offload when there is a change to do, both from
> macsec_changelink and macsec_upd_offload.

Agreed.

> Since we don't need to restore in the second branch, and we can only
> fetch IFLA_MACSEC_OFFLOAD when it's present, maybe:
> 
>     change = false;
>     if (data[IFLA_MACSEC_OFFLOAD]) {
>         offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]);
>         if (macsec->offload != offload) {
>             change = true;
>             macsec_update_offload ...cleanup
>         }
>     }
>     
>     if (!change && macsec_is_offloaded(macsec)) {
>         ...
>     }
> 
> Or let macsec_update_offload do the macsec->offload != offload test
> and pass &change so that changelink can know what to do next.

Either solutions work for me.

Thanks!
Antoine