linux-kernel - RE: [PATCH net-next v3 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <IA1PR12MB635345D00CDE8F81721EEC89AB1A9@IA1PR12MB6353.namprd12.prod.outlook.com>
Date:   Wed, 7 Dec 2022 15:52:15 +0000
From:   Emeel Hakim <ehakim@...dia.com>
To:     Sabrina Dubroca <sd@...asysnail.net>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Raed Salem <raeds@...dia.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "atenart@...nel.org" <atenart@...nel.org>,
        "jiri@...nulli.us" <jiri@...nulli.us>
Subject: RE: [PATCH net-next v3 1/2] macsec: add support for
 IFLA_MACSEC_OFFLOAD in macsec_changelink



> -----Original Message-----
> From: Sabrina Dubroca <sd@...asysnail.net>
> Sent: Wednesday, 7 December 2022 17:46
> To: Emeel Hakim <ehakim@...dia.com>
> Cc: linux-kernel@...r.kernel.org; Raed Salem <raeds@...dia.com>;
> davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com; netdev@...r.kernel.org; atenart@...nel.org; jiri@...nulli.us
> Subject: Re: [PATCH net-next v3 1/2] macsec: add support for
> IFLA_MACSEC_OFFLOAD in macsec_changelink
> 
> External email: Use caution opening links or attachments
> 
> 
> 2022-12-07, 12:10:16 +0200, ehakim@...dia.com wrote:
> > From: Emeel Hakim <ehakim@...dia.com>
> >
> > Add support for changing Macsec offload selection through the netlink
> > layer by implementing the relevant changes in macsec_change link.
> 
> nit: macsec_changelink

Ack

> [...]
> > +static int macsec_update_offload(struct macsec_dev *macsec, enum
> > +macsec_offload offload) {
> > +     enum macsec_offload prev_offload;
> > +     const struct macsec_ops *ops;
> > +     struct macsec_context ctx;
> > +     int ret = 0;
> > +
> > +     prev_offload = macsec->offload;
> > +
> > +     /* Check if the device already has rules configured: we do not support
> > +      * rules migration.
> > +      */
> > +     if (macsec_is_configured(macsec))
> > +             return -EBUSY;
> > +
> > +     ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload :
> offload,
> > +                            macsec, &ctx);
> > +     if (!ops)
> > +             return -EOPNOTSUPP;
> > +
> > +     macsec->offload = offload;
> > +
> > +     ctx.secy = &macsec->secy;
> > +     ret = (offload == MACSEC_OFFLOAD_OFF) ? macsec_offload(ops-
> >mdo_del_secy, &ctx) :
> > +                   macsec_offload(ops->mdo_add_secy, &ctx);
> 
> I think aligning the two macsec_offload(...) calls would make this a bit easier to
> read:
> 
>         ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops-
> >mdo_del_secy, &ctx)
>                                             : macsec_offload(ops->mdo_add_secy, &ctx);
> 
> (and remove the unnecessary ())

Ack

> > +
> > +     if (ret)
> > +             macsec->offload = prev_offload;
> > +
> > +     return ret;
> > +}
> > +
> 
> [...]
> > +static int macsec_changelink_upd_offload(struct net_device *dev,
> > +struct nlattr *data[]) {
> > +     enum macsec_offload offload;
> > +     struct macsec_dev *macsec;
> > +
> > +     macsec = macsec_priv(dev);
> > +     offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]);
> 
> All those checks are also present in macsec_upd_offload, why not move them into
> macsec_update_offload as well? (and then you don't really need
> macsec_changelink_upd_offload anymore)
> 

Right, I thought about it , but I realized that those checks are done before holding the lock in macsec_upd_offload
and if I move them to macsec_update_offload I will hold the lock for a longer time , I want to minimize the time
of holding the lock.

> > +     if (macsec->offload == offload)
> > +             return 0;
> > +
> > +     /* Check if the offloading mode is supported by the underlying layers */
> > +     if (offload != MACSEC_OFFLOAD_OFF &&
> > +         !macsec_check_offload(offload, macsec))
> > +             return -EOPNOTSUPP;
> > +
> > +     /* Check if the net device is busy. */
> > +     if (netif_running(dev))
> > +             return -EBUSY;
> > +
> > +     return macsec_update_offload(macsec, offload); }
> > +
> 
> --
> Sabrina