Hi,
On Wed, May 28, 2025 at 02:34:10AM +0200, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@xxxxxxx> wrote:
>
> Hi Fernando
>
> Thanks for working on this, I got inquiries as to nft_tunnel.c
> and how to make use of this stuff...
>
> > diff --git a/include/libnftnl/object.h b/include/libnftnl/object.h
> > index 9930355..14a42cd 100644
> > --- a/include/libnftnl/object.h
> > +++ b/include/libnftnl/object.h
> > @@ -117,15 +117,19 @@ enum {
> > NFTNL_OBJ_TUNNEL_ERSPAN_V1_INDEX,
> > NFTNL_OBJ_TUNNEL_ERSPAN_V2_HWID,
> > NFTNL_OBJ_TUNNEL_ERSPAN_V2_DIR,
> > + NFTNL_OBJ_TUNNEL_GENEVE_OPTS,
>
> If every flavour gets its own flag in the tunnel namespace we'll run
> out of u64 in no time.
>
> AFAICS these are mutually exclusive, e.g.
> NFTNL_OBJ_TUNNEL_ERSPAN_V1_INDEX and NFTNL_OBJ_TUNNEL_VXLAN_GBP cannot
> be active at the same time.
>
> Is there a way to re-use the internal flag namespace depending on the tunnel
> subtype?
>
> Or to have distinct tunnel object types?
>
> object -> tunnel -> {vxlan, erspan, ...} ?
>
> As-is, how is this API supposed to be used? The internal union seems to
> be asking for trouble later, when e.g. 'getting' NFTNL_OBJ_TUNNEL_GENEVE_OPTS
> on something that was instantiated as vxlan tunnel and fields aliasing to
> unexpected values.
>
> Perhaps the first use of any of the NFTNL_OBJ_TUNNEL_ERSPAN_V1_INDEX
> etc values in a setter should interally "lock" the object to the given
> subtype?
>
> That might allow to NOT use ->flags for those enum values and instead
> keep track of them via overlapping bits.
>
> We'd need some internal 'enum nft_obj_tunnel_type' that marks which
> part of the union is useable/instantiated so we can reject requests
> to set bits that are not available for the specific tunnel type.
>
> > switch (type) {
> > case NFTNL_OBJ_TUNNEL_ID:
> > @@ -72,6 +73,15 @@ nftnl_obj_tunnel_set(struct nftnl_obj *e, uint16_t type,
> > case NFTNL_OBJ_TUNNEL_ERSPAN_V2_DIR:
> > memcpy(&tun->u.tun_erspan.u.v2.dir, data, data_len);
> > break;
> > + case NFTNL_OBJ_TUNNEL_GENEVE_OPTS:
> > + geneve = malloc(sizeof(struct nftnl_obj_tunnel_geneve));
>
> No null check. Applies to a few other spots too.
>
> > + memcpy(geneve, data, data_len);
>
> Hmm, this looks like the API leaks internal data layout from nftables to
> libnftnl and vice versa? IMO thats a non-starter, sorry.
>
> I see that options are essentially unlimited values, so perhaps nftables
> should build the netlink blob(s) directly, similar to nftnl_udata()?
>
> Pablo, any better idea?
Maybe this API for tunnel options are proposed in this patch?
Consider this a sketch/proposal, this is compiled tested only.
struct obj_ops also needs a .free interface to release the tunnel
options object.
