From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Phil Sutter <phil@nwl.cc>
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [nft PATCH 6/6] netlink: Call tunnel getters unconditionally
Date: Fri, 26 Jun 2026 13:53:27 +0200 [thread overview]
Message-ID: <aj5oN3PT7_QusykI@chamomile> (raw)
In-Reply-To: <20260603192923.1378815-7-phil@nwl.cc>
Hi Phil,
Series LGTM, only one nitpick, see below.
On Wed, Jun 03, 2026 at 09:29:23PM +0200, Phil Sutter wrote:
> All the nftnl_obj_get_u*() and nftnl_tunnel_opt_get_u*() functions
> return 0 if the attribute is not present. Since 'obj' is zeroed upon
> allocation and no unions are used within per object or tunnel type data,
> assigning that value won't change behaviour.
>
> Keep the check before calling nftnl_obj_tunnel_opts_foreach though. It
> looks like that function does not check tun->tun_opts before
> dereferencing it.
>
> Also make sure obj->tunnel.{src,dst} is not initialized twice (once for
> IPv4 and once for IPv6) which happens if merely the _is_set() check is
> removed. Let netlink_obj_tunnel_parse_addr() choose between two
> attributes to use and so assign just once to each of the fields.
>
> Fixes: 35d9c77c57452 ("src: add tunnel template support")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> src/netlink.c | 106 ++++++++++++++++++++------------------------------
> 1 file changed, 42 insertions(+), 64 deletions(-)
>
> diff --git a/src/netlink.c b/src/netlink.c
> index 5c263f39791f1..57f4b7c0edea1 100644
> --- a/src/netlink.c
> +++ b/src/netlink.c
> @@ -1842,7 +1842,7 @@ void netlink_dump_obj(struct nftnl_obj *nln, struct netlink_ctx *ctx)
> static struct in6_addr all_zeroes;
>
> static struct expr *
> -netlink_obj_tunnel_parse_addr(struct nftnl_obj *nlo, int attr)
> +netlink_obj_tunnel_parse_addr(struct nftnl_obj *nlo, int attr, int alt_attr)
Maybe I would suggest:
netlink_obj_tunnel_parse_addr(struct nftnl_obj *nlo, int ipv4_attr, int ipv6_attr)
for easier reading.
Just a nitpick, no need to send v2, just amend before pushing.
BTW, if you take my proposal, maybe this needs to be reversed:
> + netlink_obj_tunnel_parse_addr(nlo,
> + NFTNL_OBJ_TUNNEL_IPV6_SRC,
> + NFTNL_OBJ_TUNNEL_IPV4_SRC);
so it is:
> + netlink_obj_tunnel_parse_addr(nlo, NFTNL_OBJ_TUNNEL_IPV4_SRC,
> + NFTNL_OBJ_TUNNEL_IPV6_SRC);
Although you migh consider that ipv6 is more important, which also
makes sense to me in such case, alternative is:
netlink_obj_tunnel_parse_addr(struct nftnl_obj *nlo, int ipv6_attr, int ipv4_attr)
Thanks.
> {
> struct nft_data_delinearize nld;
> const struct datatype *dtype;
> @@ -1850,6 +1850,13 @@ netlink_obj_tunnel_parse_addr(struct nftnl_obj *nlo, int attr)
> struct expr *expr;
> uint32_t addr;
>
> + if (!nftnl_obj_is_set(nlo, attr)) {
> + if (!nftnl_obj_is_set(nlo, alt_attr))
> + return NULL;
> + else
> + attr = alt_attr;
> + }
> +
> memset(&nld, 0, sizeof(nld));
>
> switch (attr) {
> @@ -1912,33 +1919,23 @@ static int tunnel_parse_opt_cb(struct nftnl_tunnel_opt *opt, void *data) {
> switch (nftnl_tunnel_opt_get_type(opt)) {
> case NFTNL_TUNNEL_TYPE_ERSPAN:
> obj->tunnel.type = TUNNEL_ERSPAN;
> - if (nftnl_tunnel_opt_get_flags(opt) & (1 << NFTNL_TUNNEL_ERSPAN_VERSION)) {
> - obj->tunnel.erspan.version =
> - nftnl_tunnel_opt_get_u32(opt,
> - NFTNL_TUNNEL_ERSPAN_VERSION);
> - }
> - if (nftnl_tunnel_opt_get_flags(opt) & (1 << NFTNL_TUNNEL_ERSPAN_V1_INDEX)) {
> - obj->tunnel.erspan.v1.index =
> - nftnl_tunnel_opt_get_u32(opt,
> - NFTNL_TUNNEL_ERSPAN_V1_INDEX);
> - }
> - if (nftnl_tunnel_opt_get_flags(opt) & (1 << NFTNL_TUNNEL_ERSPAN_V2_HWID)) {
> - obj->tunnel.erspan.v2.hwid =
> - nftnl_tunnel_opt_get_u8(opt,
> - NFTNL_TUNNEL_ERSPAN_V2_HWID);
> - }
> - if (nftnl_tunnel_opt_get_flags(opt) & (1 << NFTNL_TUNNEL_ERSPAN_V2_DIR)) {
> - obj->tunnel.erspan.v2.direction =
> - nftnl_tunnel_opt_get_u8(opt,
> - NFTNL_TUNNEL_ERSPAN_V2_DIR);
> - }
> + obj->tunnel.erspan.version =
> + nftnl_tunnel_opt_get_u32(opt,
> + NFTNL_TUNNEL_ERSPAN_VERSION);
> + obj->tunnel.erspan.v1.index =
> + nftnl_tunnel_opt_get_u32(opt,
> + NFTNL_TUNNEL_ERSPAN_V1_INDEX);
> + obj->tunnel.erspan.v2.hwid =
> + nftnl_tunnel_opt_get_u8(opt,
> + NFTNL_TUNNEL_ERSPAN_V2_HWID);
> + obj->tunnel.erspan.v2.direction =
> + nftnl_tunnel_opt_get_u8(opt,
> + NFTNL_TUNNEL_ERSPAN_V2_DIR);
> break;
> case NFTNL_TUNNEL_TYPE_VXLAN:
> obj->tunnel.type = TUNNEL_VXLAN;
> - if (nftnl_tunnel_opt_get_flags(opt) & (1 << NFTNL_TUNNEL_VXLAN_GBP)) {
> - obj->tunnel.type = TUNNEL_VXLAN;
> - obj->tunnel.vxlan.gbp = nftnl_tunnel_opt_get_u32(opt, NFTNL_TUNNEL_VXLAN_GBP);
> - }
> + obj->tunnel.vxlan.gbp =
> + nftnl_tunnel_opt_get_u32(opt, NFTNL_TUNNEL_VXLAN_GBP);
> break;
> case NFTNL_TUNNEL_TYPE_GENEVE:
> if (!obj->tunnel.type) {
> @@ -1950,11 +1947,11 @@ static int tunnel_parse_opt_cb(struct nftnl_tunnel_opt *opt, void *data) {
> if (!geneve)
> memory_allocation_error();
>
> - if (nftnl_tunnel_opt_get_flags(opt) & (1 << NFTNL_TUNNEL_GENEVE_TYPE))
> - geneve->type = nftnl_tunnel_opt_get_u8(opt, NFTNL_TUNNEL_GENEVE_TYPE);
> -
> - if (nftnl_tunnel_opt_get_flags(opt) & (1 << NFTNL_TUNNEL_GENEVE_CLASS))
> - geneve->geneve_class = nftnl_tunnel_opt_get_u16(opt, NFTNL_TUNNEL_GENEVE_CLASS);
> + geneve->type =
> + nftnl_tunnel_opt_get_u8(opt, NFTNL_TUNNEL_GENEVE_TYPE);
> + geneve->geneve_class =
> + nftnl_tunnel_opt_get_u16(opt,
> + NFTNL_TUNNEL_GENEVE_CLASS);
>
> if (nftnl_tunnel_opt_get_flags(opt) & (1 << NFTNL_TUNNEL_GENEVE_DATA)) {
> gnv_data = nftnl_tunnel_opt_get_data(opt, NFTNL_TUNNEL_GENEVE_DATA,
> @@ -2069,40 +2066,21 @@ struct obj *netlink_delinearize_obj(struct netlink_ctx *ctx,
> nftnl_obj_get_u32(nlo, NFTNL_OBJ_SYNPROXY_FLAGS);
> break;
> case NFT_OBJECT_TUNNEL:
> - if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_ID))
> - obj->tunnel.id = nftnl_obj_get_u32(nlo, NFTNL_OBJ_TUNNEL_ID);
> - if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_SPORT)) {
> - obj->tunnel.sport =
> - nftnl_obj_get_u16(nlo, NFTNL_OBJ_TUNNEL_SPORT);
> - }
> - if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_DPORT)) {
> - obj->tunnel.dport =
> - nftnl_obj_get_u16(nlo, NFTNL_OBJ_TUNNEL_DPORT);
> - }
> - if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_TOS)) {
> - obj->tunnel.tos =
> - nftnl_obj_get_u8(nlo, NFTNL_OBJ_TUNNEL_TOS);
> - }
> - if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_TTL)) {
> - obj->tunnel.ttl =
> - nftnl_obj_get_u8(nlo, NFTNL_OBJ_TUNNEL_TTL);
> - }
> - if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_IPV4_SRC)) {
> - obj->tunnel.src =
> - netlink_obj_tunnel_parse_addr(nlo, NFTNL_OBJ_TUNNEL_IPV4_SRC);
> - }
> - if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_IPV4_DST)) {
> - obj->tunnel.dst =
> - netlink_obj_tunnel_parse_addr(nlo, NFTNL_OBJ_TUNNEL_IPV4_DST);
> - }
> - if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_IPV6_SRC)) {
> - obj->tunnel.src =
> - netlink_obj_tunnel_parse_addr(nlo, NFTNL_OBJ_TUNNEL_IPV6_SRC);
> - }
> - if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_IPV6_DST)) {
> - obj->tunnel.dst =
> - netlink_obj_tunnel_parse_addr(nlo, NFTNL_OBJ_TUNNEL_IPV6_DST);
> - }
> + obj->tunnel.id = nftnl_obj_get_u32(nlo, NFTNL_OBJ_TUNNEL_ID);
> + obj->tunnel.sport =
> + nftnl_obj_get_u16(nlo, NFTNL_OBJ_TUNNEL_SPORT);
> + obj->tunnel.dport =
> + nftnl_obj_get_u16(nlo, NFTNL_OBJ_TUNNEL_DPORT);
> + obj->tunnel.tos = nftnl_obj_get_u8(nlo, NFTNL_OBJ_TUNNEL_TOS);
> + obj->tunnel.ttl = nftnl_obj_get_u8(nlo, NFTNL_OBJ_TUNNEL_TTL);
> + obj->tunnel.src =
> + netlink_obj_tunnel_parse_addr(nlo,
> + NFTNL_OBJ_TUNNEL_IPV6_SRC,
> + NFTNL_OBJ_TUNNEL_IPV4_SRC);
> + obj->tunnel.dst =
> + netlink_obj_tunnel_parse_addr(nlo,
> + NFTNL_OBJ_TUNNEL_IPV6_DST,
> + NFTNL_OBJ_TUNNEL_IPV4_DST);
> if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_OPTS)) {
> nftnl_obj_tunnel_opts_foreach(nlo, tunnel_parse_opt_cb, obj);
> }
> --
> 2.54.0
>
next prev parent reply other threads:[~2026-06-26 11:53 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-03 19:29 [nft PATCH 0/6] Eliminate variable declarations in switch cases Phil Sutter
2026-06-03 19:29 ` [nft PATCH 1/6] json: Introduce tunnel_obj_print_json() Phil Sutter
2026-06-03 19:29 ` [nft PATCH 2/6] parser_json: Introduce json_parse_tunnel() Phil Sutter
2026-06-03 19:29 ` [nft PATCH 3/6] rule: Turn obj_print_comment() into obj_print_header() Phil Sutter
2026-06-03 19:29 ` [nft PATCH 4/6] rule: Introduce tunnel_obj_print_data() Phil Sutter
2026-06-03 19:29 ` [nft PATCH 5/6] src: Avoid variable declarations in switch cases Phil Sutter
2026-06-03 19:29 ` [nft PATCH 6/6] netlink: Call tunnel getters unconditionally Phil Sutter
2026-06-26 11:53 ` Pablo Neira Ayuso [this message]
2026-07-03 9:55 ` Phil Sutter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aj5oN3PT7_QusykI@chamomile \
--to=pablo@netfilter.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=phil@nwl.cc \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox