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
>
prev parent reply other threads:[~2026-06-26 11:53 UTC|newest]
Thread overview: 8+ 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]
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.