netdev - Re: [PATCH net-next 1/4] vlan: Add MACsec offload operations for VLAN interface

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: <20230328182033.GA982617@electric-eye.fr.zoreil.com>
Date:   Tue, 28 Mar 2023 20:20:33 +0200
From:   Francois Romieu <romieu@...zoreil.com>
To:     Emeel Hakim <ehakim@...dia.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "sd@...asysnail.net" <sd@...asysnail.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/4] vlan: Add MACsec offload operations for
 VLAN interface

Emeel Hakim <ehakim@...dia.com> :
> > -----Original Message-----
> > From: Jakub Kicinski <kuba@...nel.org>
[...]
> > > +#if IS_ENABLED(CONFIG_MACSEC)
> > > +#define VLAN_MACSEC_MDO(mdo) \
> > > +static int vlan_macsec_ ## mdo(struct macsec_context *ctx) \ { \
> > > +     const struct macsec_ops *ops; \
> > > +     ops =  vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops; \
> > > +     return ops ? ops->mdo_ ## mdo(ctx) : -EOPNOTSUPP; \ }
> > > +
> > > +#define VLAN_MACSEC_DECLARE_MDO(mdo) vlan_macsec_ ## mdo
> > > +
> > > +VLAN_MACSEC_MDO(add_txsa);
> > > +VLAN_MACSEC_MDO(upd_txsa);
> > > +VLAN_MACSEC_MDO(del_txsa);
> > > +
> > > +VLAN_MACSEC_MDO(add_rxsa);
> > > +VLAN_MACSEC_MDO(upd_rxsa);
> > > +VLAN_MACSEC_MDO(del_rxsa);
> > > +
> > > +VLAN_MACSEC_MDO(add_rxsc);
> > > +VLAN_MACSEC_MDO(upd_rxsc);
> > > +VLAN_MACSEC_MDO(del_rxsc);
> > > +
> > > +VLAN_MACSEC_MDO(add_secy);
> > > +VLAN_MACSEC_MDO(upd_secy);
> > > +VLAN_MACSEC_MDO(del_secy);
> > 
> > -1
> > 
> > impossible to grep for the functions :( but maybe others don't care
> 
> Thank you for bringing up the issue you noticed. However, I decided to go with this approach
> because the functions are simple and look very similar, so there wasn't much to debug.
> Using a macro allowed for cleaner code instead of having to resort to ugly code duplication.

Sometime it's also nice to be able to use such modern tools as tags and
grep.

While it still implies some duplication and it doesn't automagically
prevent wrongly mixing vlan_macsec_foo() and ops->mdo_bar(), the code
below may be considered as a trade-off:

#define _B(mdo) \
	const struct macsec_ops *ops; \
	ops = vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops; \
	return ops ? ops->mdo_ ## mdo(ctx) : -EOPNOTSUPP; \


static int vlan_macsec_add_txsa(struct macsec_context *ctx) { _B(add_txsa) }
static int vlan_macsec_upd_txsa(struct macsec_context *ctx) { _B(upd_txsa) }
[...]
#undef _B

On a tangent topic, both codes expand 12 times the accessor

vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops.

It imvho deserves an helper function so that the compiler can make some
choice.

As a final remark, VLAN_MACSEC_DECLARE_MDO above does not buy much.
Compare:

static const struct macsec_ops macsec_offload_ops = {
	.mdo_add_txsa = VLAN_MACSEC_DECLARE_MDO(add_txsa),
	.mdo_upd_txsa = VLAN_MACSEC_DECLARE_MDO(upd_txsa),
[...]

vs

static const struct macsec_ops macsec_offload_ops = {
	.mdo_add_txsa = vlan_macsec_add_txsa,
	.mdo_upd_txsa = vlan_macsec_upd_txsa,
[...]

This one could probably be:

#define _I(mdo) .mdo_ ## mdo = vlan_macsec_ ## mdo

static const struct macsec_ops macsec_offload_ops = {
	_I(add_txsa),
	_I(upd_txsa),
[...]
#undef _I

-- 
Ueimor