[PATCH 0/4] tools: ynl: Fix tc filters with actions

[PATCH 0/4] tools: ynl: Fix tc filters with actions

[Zahari Doychev]

The first patch in this series introduces an example tool that
creates a flower filter with two VLAN actions. The subsequent
patches address various issues to ensure the tool operates as
intended.

Zahari Doychev (4):
  ynl: samples: add tc filter add example
  tools: ynl: zero-initialize struct ynl_sock memory
  tools: ynl: call nested attribute free function for indexed arrays
  tools: ynl: add start-index property for indexed arrays

 Documentation/netlink/netlink-raw.yaml | 13 ++++
 Documentation/netlink/specs/tc.yaml    |  7 ++
 tools/net/ynl/Makefile.deps            |  1 +
 tools/net/ynl/lib/ynl.c                |  2 +-
 tools/net/ynl/pyynl/lib/nlspec.py      |  1 +
 tools/net/ynl/pyynl/ynl_gen_c.py       | 18 ++++-
 tools/net/ynl/samples/.gitignore       |  1 +
 tools/net/ynl/samples/tc-filter-add.c  | 92 ++++++++++++++++++++++++++
 8 files changed, 133 insertions(+), 2 deletions(-)
 create mode 100644 tools/net/ynl/samples/tc-filter-add.c

-- 
2.51.0

[PATCH 1/4] ynl: samples: add tc filter add example

[Zahari Doychev]

Add a simple tool that demonstrates adding a flower filter with two
VLAN push actions. This example can be invoked as:

  # ./tools/samples/tc-filter-add p2

  # tc -j -p filter show dev p2 ingress pref 2211

	[ {
		"protocol": "802.1Q",
		"kind": "flower",
		"chain": 0
	    },{
		"protocol": "802.1Q",
		"kind": "flower",
		"chain": 0,
		"options": {
		    "handle": 1,
		    "keys": {
			"num_of_vlans": 3,
			"vlan_id": 255,
			"vlan_prio": 5
		    },
		    "not_in_hw": true,
		    "actions": [ {
			    "order": 1,
			    "kind": "vlan",
			    "vlan_action": "push",
			    "id": 255,
			    "control_action": {
				"type": "pass"
			    },
			    "index": 5,
			    "ref": 1,
			    "bind": 1
			},{
			    "order": 2,
			    "kind": "vlan",
			    "vlan_action": "push",
			    "id": 555,
			    "control_action": {
				"type": "pass"
			    },
			    "index": 6,
			    "ref": 1,
			    "bind": 1
			} ]
		}
	    } ]

This shows the filter with two VLAN push actions, verifying that tc action
attributes are handled correctly.

Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
 tools/net/ynl/Makefile.deps           |  1 +
 tools/net/ynl/samples/.gitignore      |  1 +
 tools/net/ynl/samples/tc-filter-add.c | 92 +++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
 create mode 100644 tools/net/ynl/samples/tc-filter-add.c

diff --git a/tools/net/ynl/Makefile.deps b/tools/net/ynl/Makefile.deps
index 865fd2e8519e..96c390af060e 100644
--- a/tools/net/ynl/Makefile.deps
+++ b/tools/net/ynl/Makefile.deps
@@ -47,4 +47,5 @@ CFLAGS_tc:= $(call get_hdr_inc,__LINUX_RTNETLINK_H,rtnetlink.h) \
 	$(call get_hdr_inc,_TC_MIRRED_H,tc_act/tc_mirred.h) \
 	$(call get_hdr_inc,_TC_SKBEDIT_H,tc_act/tc_skbedit.h) \
 	$(call get_hdr_inc,_TC_TUNNEL_KEY_H,tc_act/tc_tunnel_key.h)
+CFLAGS_tc-filter-add:=$(CFLAGS_tc)
 CFLAGS_tcp_metrics:=$(call get_hdr_inc,_LINUX_TCP_METRICS_H,tcp_metrics.h)
diff --git a/tools/net/ynl/samples/.gitignore b/tools/net/ynl/samples/.gitignore
index 7f5fca7682d7..05087ee323ba 100644
--- a/tools/net/ynl/samples/.gitignore
+++ b/tools/net/ynl/samples/.gitignore
@@ -7,3 +7,4 @@ rt-addr
 rt-link
 rt-route
 tc
+tc-filter-add
diff --git a/tools/net/ynl/samples/tc-filter-add.c b/tools/net/ynl/samples/tc-filter-add.c
new file mode 100644
index 000000000000..b9c6f30f2a30
--- /dev/null
+++ b/tools/net/ynl/samples/tc-filter-add.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <arpa/inet.h>
+#include <linux/pkt_sched.h>
+#include <linux/tc_act/tc_vlan.h>
+#include <linux/tc_act/tc_gact.h>
+#include <net/if.h>
+
+#include <ynl.h>
+
+#include "tc-user.h"
+
+int main(int argc, char **argv)
+{
+	struct tc_newtfilter_req *req;
+	struct tc_act_attrs *acts;
+	struct tc_vlan p = {
+		.v_action = TCA_VLAN_ACT_PUSH
+	};
+	__u16 flags = NLM_F_EXCL | NLM_F_CREATE;
+	struct ynl_error yerr;
+	struct ynl_sock *ys;
+	int ifi;
+
+	if (argc < 2) {
+		fprintf(stderr, "Usage: %s <interface_name>\n", argv[0]);
+		return 1;
+	}
+	ifi = if_nametoindex(argv[1]);
+	if (!ifi) {
+		perror("if_nametoindex");
+		return 1;
+	}
+
+	ys = ynl_sock_create(&ynl_tc_family, &yerr);
+	if (!ys) {
+		fprintf(stderr, "YNL: %s\n", yerr.msg);
+		return 1;
+	}
+
+	req = tc_newtfilter_req_alloc();
+	if (!req) {
+		fprintf(stderr, "tc_newtfilter_req_alloc failed\n");
+		goto err_destroy;
+	}
+	memset(req, 0, sizeof(*req));
+
+	acts = tc_act_attrs_alloc(2);
+	if (!acts) {
+		fprintf(stderr, "tc_act_attrs_alloc\n");
+		goto err_act;
+	}
+	memset(acts, 0, sizeof(*acts));
+
+	req->_hdr.tcm_ifindex = ifi;
+	req->_hdr.tcm_parent = TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS);
+	req->_hdr.tcm_info = TC_H_MAKE((2211 << 16), htons(0x8100));
+	req->chain = 0;
+
+	tc_newtfilter_req_set_nlflags(req, flags);
+	tc_newtfilter_req_set_kind(req, "flower");
+	tc_newtfilter_req_set_options_flower_key_vlan_id(req, 255);
+	tc_newtfilter_req_set_options_flower_key_vlan_prio(req, 5);
+	tc_newtfilter_req_set_options_flower_key_num_of_vlans(req, 3);
+
+	__tc_newtfilter_req_set_options_flower_act(req, acts, 2);
+
+	tc_act_attrs_set_kind(&acts[0], "vlan");
+	tc_act_attrs_set_options_vlan_parms(&acts[0], &p, sizeof(p));
+	tc_act_attrs_set_options_vlan_push_vlan_id(&acts[0], 255);
+	tc_act_attrs_set_kind(&acts[1], "vlan");
+	tc_act_attrs_set_options_vlan_parms(&acts[1], &p, sizeof(p));
+	tc_act_attrs_set_options_vlan_push_vlan_id(&acts[1], 555);
+
+	tc_newtfilter_req_set_options_flower_flags(req, 0);
+	tc_newtfilter_req_set_options_flower_key_eth_type(req, htons(0x8100));
+
+	if (tc_newtfilter(ys, req))
+		fprintf(stderr, "YNL: %s\n", ys->err.msg);
+
+	tc_newtfilter_req_free(req);
+	ynl_sock_destroy(ys);
+	return 0;
+
+err_act:
+	tc_newtfilter_req_free(req);
+err_destroy:
+	ynl_sock_destroy(ys);
+	return 2;
+}
-- 
2.51.0

[PATCH 2/4] tools: ynl: zero-initialize struct ynl_sock memory

[Zahari Doychev]

The memory belonging to tx_buf and rx_buf in ynl_sock is not
initialized after allocation. This commit ensures the entire
allocated memory is set to zero.

When asan is enabled, uninitialized bytes may contain poison values.
This can cause failures e.g. when doing ynl_attr_put_str then poisoned
bytes appear after the null terminator. As a result, tc filter addition
may fail.

Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
 tools/net/ynl/lib/ynl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 2bcd781111d7..16a4815d6a49 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -744,7 +744,7 @@ ynl_sock_create(const struct ynl_family *yf, struct ynl_error *yse)
 	ys = malloc(sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);
 	if (!ys)
 		return NULL;
-	memset(ys, 0, sizeof(*ys));
+	memset(ys, 0, sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);
 
 	ys->family = yf;
 	ys->tx_buf = &ys->raw_buf[0];
-- 
2.51.0

[PATCH 3/4] tools: ynl: call nested attribute free function for indexed arrays

[Zahari Doychev]

When freeing indexed arrays, the corresponding free function should
be called for each entry of the indexed array. For example, for
for 'struct tc_act_attrs' 'tc_act_attrs_free(...)' needs to be called
for each entry.

Previously, memory leaks were reported when enabling the ASAN
analyzer.

=================================================================
==874==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x7f221fd20cb5 in malloc ./debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:67
    #1 0x55c98db048af in tc_act_attrs_set_options_vlan_parms ../generated/tc-user.h:2813
    #2 0x55c98db048af in main  ./linux/tools/net/ynl/samples/tc-filter-add.c:71

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x7f221fd20cb5 in malloc ./debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:67
    #1 0x55c98db04a93 in tc_act_attrs_set_options_vlan_parms ../generated/tc-user.h:2813
    #2 0x55c98db04a93 in main ./linux/tools/net/ynl/samples/tc-filter-add.c:74

Direct leak of 10 byte(s) in 2 object(s) allocated from:
    #0 0x7f221fd20cb5 in malloc ./debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:67
    #1 0x55c98db0527d in tc_act_attrs_set_kind ../generated/tc-user.h:1622

SUMMARY: AddressSanitizer: 58 byte(s) leaked in 4 allocation(s).

The following diff illustrates the changes introduced compared to the
previous version of the code.

 void tc_flower_attrs_free(struct tc_flower_attrs *obj)
 {
+	unsigned int i;
+
 	free(obj->indev);
+	for (i = 0; i < obj->_count.act; i++)
+		tc_act_attrs_free(&obj->act[i]);
 	free(obj->act);
 	free(obj->key_eth_dst);
 	free(obj->key_eth_dst_mask);

Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 58086b101057..aadeb3abcad8 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -861,6 +861,18 @@ class TypeIndexedArray(Type):
         return [f"{member} = {self.c_name};",
                 f"{presence} = n_{self.c_name};"]
 
+    def free_needs_iter(self):
+        return self.sub_type == 'nest'
+
+    def _free_lines(self, ri, var, ref):
+        lines = []
+        if self.sub_type == 'nest':
+            lines += [
+                f"for (i = 0; i < {var}->{ref}_count.{self.c_name}; i++)",
+                f'{self.nested_render_name}_free(&{var}->{ref}{self.c_name}[i]);',
+            ]
+        lines += f"free({var}->{ref}{self.c_name});",
+        return lines
 
 class TypeNestTypeValue(Type):
     def _complex_member_type(self, ri):
-- 
2.51.0

[PATCH 4/4] tools: ynl: add start-index property for indexed arrays

[Zahari Doychev]

The Linux tc actions expect that the action order starts from index
one. To accommodate this, add a start-index property to the ynl spec
for indexed arrays. This property allows the starting index to be
specified, ensuring compatibility with consumers that require a
non-zero-based index.

For example if we have "start_index = 1" then we get the following
diff.

 		ynl_attr_put_str(nlh, TCA_FLOWER_INDEV, obj->indev);
 	array = ynl_attr_nest_start(nlh, TCA_FLOWER_ACT);
 	for (i = 0; i < obj->_count.act; i++)
-		tc_act_attrs_put(nlh, i, &obj->act[i]);
+		tc_act_attrs_put(nlh, i + 1, &obj->act[i]);
 	ynl_attr_nest_end(nlh, array);

Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
 Documentation/netlink/netlink-raw.yaml | 13 +++++++++++++
 Documentation/netlink/specs/tc.yaml    |  7 +++++++
 tools/net/ynl/pyynl/lib/nlspec.py      |  1 +
 tools/net/ynl/pyynl/ynl_gen_c.py       |  6 +++++-
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/netlink/netlink-raw.yaml b/Documentation/netlink/netlink-raw.yaml
index 246fa07bccf6..aafb7cb16beb 100644
--- a/Documentation/netlink/netlink-raw.yaml
+++ b/Documentation/netlink/netlink-raw.yaml
@@ -260,6 +260,9 @@ properties:
                   Sometimes, however, both forms are necessary, in which case header contains the enum
                   form while specific attributes may request to convert the values into a bitfield.
                 type: boolean
+              start-index:
+                description: For indexed arrays the first index value.
+                type: integer
               checks:
                 description: Kernel input validation.
                 type: object
@@ -308,6 +311,16 @@ properties:
                 type: string
               # End netlink-raw
 
+            # allow start index only for indexed arrays
+            if:
+              properties:
+                type:
+                  const: indexed-array
+            then: {}
+            else:
+              not:
+                required: [ start-index ]
+
       # Make sure name-prefix does not appear in subsets (subsets inherit naming)
       dependencies:
         name-prefix:
diff --git a/Documentation/netlink/specs/tc.yaml b/Documentation/netlink/specs/tc.yaml
index b398f7a46dae..459aa51059ec 100644
--- a/Documentation/netlink/specs/tc.yaml
+++ b/Documentation/netlink/specs/tc.yaml
@@ -2044,6 +2044,7 @@ attribute-sets:
         type: indexed-array
         sub-type: nest
         nested-attributes: act-attrs
+        start-index: 1
       -
         name: police
         type: nest
@@ -2303,6 +2304,7 @@ attribute-sets:
         type: indexed-array
         sub-type: nest
         nested-attributes: act-attrs
+        start-index: 1
       -
         name: police
         type: nest
@@ -2493,6 +2495,7 @@ attribute-sets:
         type: indexed-array
         sub-type: nest
         nested-attributes: act-attrs
+        start-index: 1
       -
         name: key-eth-dst
         type: binary
@@ -3020,6 +3023,7 @@ attribute-sets:
         type: indexed-array
         sub-type: nest
         nested-attributes: act-attrs
+        start-index: 1
       -
         name: mask
         type: u32
@@ -3180,6 +3184,7 @@ attribute-sets:
         type: indexed-array
         sub-type: nest
         nested-attributes: act-attrs
+        start-index: 1
       -
         name: flags
         type: u32
@@ -3566,6 +3571,7 @@ attribute-sets:
         type: indexed-array
         sub-type: nest
         nested-attributes: act-attrs
+        start-index: 1
   -
     name: taprio-attrs
     name-prefix: tca-taprio-attr-
@@ -3798,6 +3804,7 @@ attribute-sets:
         type: indexed-array
         sub-type: nest
         nested-attributes: act-attrs
+        start-index: 1
       -
         name: indev
         type: string
diff --git a/tools/net/ynl/pyynl/lib/nlspec.py b/tools/net/ynl/pyynl/lib/nlspec.py
index 85c17fe01e35..08660602da9d 100644
--- a/tools/net/ynl/pyynl/lib/nlspec.py
+++ b/tools/net/ynl/pyynl/lib/nlspec.py
@@ -181,6 +181,7 @@ class SpecAttr(SpecElement):
         self.display_hint = yaml.get('display-hint')
         self.sub_message = yaml.get('sub-message')
         self.selector = yaml.get('selector')
+        self.start_index = yaml.get('start-index', 0)
 
         self.is_auto_scalar = self.type == "sint" or self.type == "uint"
 
diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index aadeb3abcad8..698d6089a856 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -852,7 +852,11 @@ class TypeIndexedArray(Type):
             ri.cw.p(f"ynl_attr_put(nlh, i, {var}->{self.c_name}[i], {self.checks['exact-len']});")
         elif self.sub_type == 'nest':
             ri.cw.p(f'for (i = 0; i < {var}->_count.{self.c_name}; i++)')
-            ri.cw.p(f"{self.nested_render_name}_put(nlh, i, &{var}->{self.c_name}[i]);")
+            ri.cw.p(
+                f"{self.nested_render_name}_put(nlh, "
+                f"i{f' + {self.start_index}' if self.start_index > 0 else ''}, "
+                f"&{var}->{self.c_name}[i]);"
+            )
         else:
             raise Exception(f"Put for IndexedArray sub-type {self.attr['sub-type']} not supported, yet")
         ri.cw.p('ynl_attr_nest_end(nlh, array);')
-- 
2.51.0

Re: [PATCH 2/4] tools: ynl: zero-initialize struct ynl_sock memory

[Jakub Kicinski]

On Sat, 18 Oct 2025 17:17:35 +0200 Zahari Doychev wrote:
> The memory belonging to tx_buf and rx_buf in ynl_sock is not
> initialized after allocation. This commit ensures the entire
> allocated memory is set to zero.
> 
> When asan is enabled, uninitialized bytes may contain poison values.
> This can cause failures e.g. when doing ynl_attr_put_str then poisoned
> bytes appear after the null terminator. As a result, tc filter addition
> may fail.

We add strings with the null-terminating char, AFAICT.
Do you mean that the poison value appears in the padding?

> Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
> ---
>  tools/net/ynl/lib/ynl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> index 2bcd781111d7..16a4815d6a49 100644
> --- a/tools/net/ynl/lib/ynl.c
> +++ b/tools/net/ynl/lib/ynl.c
> @@ -744,7 +744,7 @@ ynl_sock_create(const struct ynl_family *yf, struct ynl_error *yse)
>  	ys = malloc(sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);
>  	if (!ys)
>  		return NULL;
> -	memset(ys, 0, sizeof(*ys));
> +	memset(ys, 0, sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);

This is just clearing the buffer initially, it can be used for multiple
requests. This change is no good as is.

Re: [PATCH 3/4] tools: ynl: call nested attribute free function for indexed arrays

[Jakub Kicinski]

On Sat, 18 Oct 2025 17:17:36 +0200 Zahari Doychev wrote:
> When freeing indexed arrays, the corresponding free function should
> be called for each entry of the indexed array. For example, for
> for 'struct tc_act_attrs' 'tc_act_attrs_free(...)' needs to be called
> for each entry.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH 1/4] ynl: samples: add tc filter add example

[Jakub Kicinski]

On Sat, 18 Oct 2025 17:17:34 +0200 Zahari Doychev wrote:
> Add a simple tool that demonstrates adding a flower filter with two
> VLAN push actions. This example can be invoked as:

Could you also do a dump and then delete? Make the sample work as a
quasi-selftest for YNL? Take a look at the rt-link sample for instance.

Re: [PATCH 4/4] tools: ynl: add start-index property for indexed arrays

[Jakub Kicinski]

On Sat, 18 Oct 2025 17:17:37 +0200 Zahari Doychev wrote:
> The Linux tc actions expect that the action order starts from index
> one. To accommodate this, add a start-index property to the ynl spec
> for indexed arrays. This property allows the starting index to be
> specified, ensuring compatibility with consumers that require a
> non-zero-based index.
> 
> For example if we have "start_index = 1" then we get the following
> diff.
> 
>  		ynl_attr_put_str(nlh, TCA_FLOWER_INDEV, obj->indev);
>  	array = ynl_attr_nest_start(nlh, TCA_FLOWER_ACT);
>  	for (i = 0; i < obj->_count.act; i++)
> -		tc_act_attrs_put(nlh, i, &obj->act[i]);
> +		tc_act_attrs_put(nlh, i + 1, &obj->act[i]);
>  	ynl_attr_nest_end(nlh, array);

The first one is just silently skipped by the kernel right?

We need to be selective about what API stupidity we try to
cover up in YNL. Otherwise the specs will be unmanageably complex.
IMO this one should be a comment in the spec explaining that action
0 is ignore and that's it.

Re: [PATCH 1/4] ynl: samples: add tc filter add example

[Zahari Doychev]

On Mon, Oct 20, 2025 at 04:20:20PM -0700, Jakub Kicinski wrote:
> On Sat, 18 Oct 2025 17:17:34 +0200 Zahari Doychev wrote:
> > Add a simple tool that demonstrates adding a flower filter with two
> > VLAN push actions. This example can be invoked as:
> 
> Could you also do a dump and then delete? Make the sample work as a
> quasi-selftest for YNL? Take a look at the rt-link sample for instance.
> 

sure, I will do that and send an update.

Re: [PATCH 2/4] tools: ynl: zero-initialize struct ynl_sock memory

[Zahari Doychev]

On Mon, Oct 20, 2025 at 04:16:39PM -0700, Jakub Kicinski wrote:
> On Sat, 18 Oct 2025 17:17:35 +0200 Zahari Doychev wrote:
> > The memory belonging to tx_buf and rx_buf in ynl_sock is not
> > initialized after allocation. This commit ensures the entire
> > allocated memory is set to zero.
> > 
> > When asan is enabled, uninitialized bytes may contain poison values.
> > This can cause failures e.g. when doing ynl_attr_put_str then poisoned
> > bytes appear after the null terminator. As a result, tc filter addition
> > may fail.
> 
> We add strings with the null-terminating char, AFAICT.
> Do you mean that the poison value appears in the padding?
> 

Yes, correct. The function nla_strcmp(...) does not match in this case as
the poison value appears in the padding after the null byte.

> > Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
> > ---
> >  tools/net/ynl/lib/ynl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> > index 2bcd781111d7..16a4815d6a49 100644
> > --- a/tools/net/ynl/lib/ynl.c
> > +++ b/tools/net/ynl/lib/ynl.c
> > @@ -744,7 +744,7 @@ ynl_sock_create(const struct ynl_family *yf, struct ynl_error *yse)
> >  	ys = malloc(sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);
> >  	if (!ys)
> >  		return NULL;
> > -	memset(ys, 0, sizeof(*ys));
> > +	memset(ys, 0, sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);
> 
> This is just clearing the buffer initially, it can be used for multiple
> requests. This change is no good as is.

I see. Should then the ynl_attr_put_str be changed to zero the padding
bytes or it is better to make sure the buffers are cleared for each
request?

Thanks

Re: [PATCH 4/4] tools: ynl: add start-index property for indexed arrays

[Zahari Doychev]

On Mon, Oct 20, 2025 at 04:32:21PM -0700, Jakub Kicinski wrote:
> On Sat, 18 Oct 2025 17:17:37 +0200 Zahari Doychev wrote:
> > The Linux tc actions expect that the action order starts from index
> > one. To accommodate this, add a start-index property to the ynl spec
> > for indexed arrays. This property allows the starting index to be
> > specified, ensuring compatibility with consumers that require a
> > non-zero-based index.
> > 
> > For example if we have "start_index = 1" then we get the following
> > diff.
> > 
> >  		ynl_attr_put_str(nlh, TCA_FLOWER_INDEV, obj->indev);
> >  	array = ynl_attr_nest_start(nlh, TCA_FLOWER_ACT);
> >  	for (i = 0; i < obj->_count.act; i++)
> > -		tc_act_attrs_put(nlh, i, &obj->act[i]);
> > +		tc_act_attrs_put(nlh, i + 1, &obj->act[i]);
> >  	ynl_attr_nest_end(nlh, array);
> 
> The first one is just silently skipped by the kernel right?

yes, and then only the second action is being confiugred. The
index defines the action order and the expectation is that they
start from order 1.

> 
> We need to be selective about what API stupidity we try to
> cover up in YNL. Otherwise the specs will be unmanageably complex.
> IMO this one should be a comment in the spec explaining that action
> 0 is ignore and that's it.
> 

I am not sure if this applies for all cases of indexed arrays. For sure
it applies for the tc_act_attrs case but I need to check the rest again.

Do you think it would be fine to start from 1 for all indexed arrays?

Re: [PATCH 4/4] tools: ynl: add start-index property for indexed arrays

[Asbjørn Sloth Tønnesen]

On 10/21/25 5:50 PM, Zahari Doychev wrote:
> On Mon, Oct 20, 2025 at 04:32:21PM -0700, Jakub Kicinski wrote:
>> We need to be selective about what API stupidity we try to
>> cover up in YNL. Otherwise the specs will be unmanageably complex.
>> IMO this one should be a comment in the spec explaining that action
>> 0 is ignore and that's it.
>>
> 
> I am not sure if this applies for all cases of indexed arrays. For sure
> it applies for the tc_act_attrs case but I need to check the rest again.
> 
> Do you think it would be fine to start from 1 for all indexed arrays?

I have a series, that I will try to get posted tomorrow, where I add a new
attribute `ignore-index` which can be used to mark indexed arrays where the
index is just an incremental value. This is a follow-up to an earlier
discussion[1].

In that series, in order to add the new attribute to the existing specs,
in the commit messages I walk through all the existing indexed arrays,
and also include things like if they start their indexes from 0 or 1.

[1] https://lore.kernel.org/netdev/7fff6b2f-f17e-4179-8507-397b76ea24bb@intel.com/

Re: [PATCH 2/4] tools: ynl: zero-initialize struct ynl_sock memory

[Jakub Kicinski]

On Tue, 21 Oct 2025 20:36:38 +0300 Zahari Doychev wrote:
> On Mon, Oct 20, 2025 at 04:16:39PM -0700, Jakub Kicinski wrote:
> > On Sat, 18 Oct 2025 17:17:35 +0200 Zahari Doychev wrote:  
> > > The memory belonging to tx_buf and rx_buf in ynl_sock is not
> > > initialized after allocation. This commit ensures the entire
> > > allocated memory is set to zero.
> > > 
> > > When asan is enabled, uninitialized bytes may contain poison values.
> > > This can cause failures e.g. when doing ynl_attr_put_str then poisoned
> > > bytes appear after the null terminator. As a result, tc filter addition
> > > may fail.  
> > 
> > We add strings with the null-terminating char, AFAICT.
> > Do you mean that the poison value appears in the padding?
> >   
> 
> Yes, correct. The function nla_strcmp(...) does not match in this case as
> the poison value appears in the padding after the null byte.
> 
> > > Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
> > > ---
> > >  tools/net/ynl/lib/ynl.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> > > index 2bcd781111d7..16a4815d6a49 100644
> > > --- a/tools/net/ynl/lib/ynl.c
> > > +++ b/tools/net/ynl/lib/ynl.c
> > > @@ -744,7 +744,7 @@ ynl_sock_create(const struct ynl_family *yf, struct ynl_error *yse)
> > >  	ys = malloc(sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);
> > >  	if (!ys)
> > >  		return NULL;
> > > -	memset(ys, 0, sizeof(*ys));
> > > +	memset(ys, 0, sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);  
> > 
> > This is just clearing the buffer initially, it can be used for multiple
> > requests. This change is no good as is.  
> 
> I see. Should then the ynl_attr_put_str be changed to zero the padding
> bytes or it is better to make sure the buffers are cleared for each
> request?

Eek, I think the bug is in how ynl_attr_put_str() computes len.
len is attr len, it should not include padding.
At the same time we should probably zero-terminate the strings
in case kernel wants NLA_NUL_STRING.

Just for illustration -- I think we should do something like 
the following, please turn this into a real patch if it makes sense:

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 29481989ea76..515c6d12f68a 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -314,14 +314,14 @@ ynl_attr_put_str(struct nlmsghdr *nlh, unsigned int attr_type, const char *str)
        size_t len;
 
        len = strlen(str);
-       if (__ynl_attr_put_overflow(nlh, len))
+       if (__ynl_attr_put_overflow(nlh, len + 1))
                return;
 
        attr = (struct nlattr *)ynl_nlmsg_end_addr(nlh);
        attr->nla_type = attr_type;
 
        strcpy((char *)ynl_attr_data(attr), str);
-       attr->nla_len = NLA_HDRLEN + NLA_ALIGN(len);
+       attr->nla_len = NLA_HDRLEN + len + 1;
 
        nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);

Re: [PATCH 4/4] tools: ynl: add start-index property for indexed arrays

[Jakub Kicinski]

On Tue, 21 Oct 2025 20:50:03 +0300 Zahari Doychev wrote:
> > We need to be selective about what API stupidity we try to
> > cover up in YNL. Otherwise the specs will be unmanageably complex.
> > IMO this one should be a comment in the spec explaining that action
> > 0 is ignore and that's it.
> 
> I am not sure if this applies for all cases of indexed arrays. For sure
> it applies for the tc_act_attrs case but I need to check the rest again.
> 
> Do you think it would be fine to start from 1 for all indexed arrays?

Not sure off the top of my head

Re: [PATCH 4/4] tools: ynl: add start-index property for indexed arrays

[Asbjørn Sloth Tønnesen]

On 10/21/25 5:50 PM, Zahari Doychev wrote:
> On Mon, Oct 20, 2025 at 04:32:21PM -0700, Jakub Kicinski wrote:
>> We need to be selective about what API stupidity we try to
>> cover up in YNL. Otherwise the specs will be unmanageably complex.
>> IMO this one should be a comment in the spec explaining that action
>> 0 is ignore and that's it.
>>
> 
> I am not sure if this applies for all cases of indexed arrays. For sure
> it applies for the tc_act_attrs case but I need to check the rest again.
> 
> Do you think it would be fine to start from 1 for all indexed arrays?
Yes, AFAICT it would. Most of indexed-array attributes that are parsed by
the kernel uses nla_for_each_nested(), and don't use the index. The TC
actions are the only ones I found, that are parsed into a nlattr array.

Disclaimer: I have only mapped out the indexed-arrays that are declared in
the current specs.

See patch 4-7 in this series for the full analysis:
https://lore.kernel.org/netdev/20251022182701.250897-1-ast@fiberby.net/

Re: [PATCH 2/4] tools: ynl: zero-initialize struct ynl_sock memory

[Zahari Doychev]

On Tue, Oct 21, 2025 at 04:22:09PM -0700, Jakub Kicinski wrote:
> On Tue, 21 Oct 2025 20:36:38 +0300 Zahari Doychev wrote:
> > On Mon, Oct 20, 2025 at 04:16:39PM -0700, Jakub Kicinski wrote:
> > > On Sat, 18 Oct 2025 17:17:35 +0200 Zahari Doychev wrote:  
> > > > The memory belonging to tx_buf and rx_buf in ynl_sock is not
> > > > initialized after allocation. This commit ensures the entire
> > > > allocated memory is set to zero.
> > > > 
> > > > When asan is enabled, uninitialized bytes may contain poison values.
> > > > This can cause failures e.g. when doing ynl_attr_put_str then poisoned
> > > > bytes appear after the null terminator. As a result, tc filter addition
> > > > may fail.  
> > > 
> > > We add strings with the null-terminating char, AFAICT.
> > > Do you mean that the poison value appears in the padding?
> > >   
> > 
> > Yes, correct. The function nla_strcmp(...) does not match in this case as
> > the poison value appears in the padding after the null byte.
> > 
> > > > Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
> > > > ---
> > > >  tools/net/ynl/lib/ynl.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> > > > index 2bcd781111d7..16a4815d6a49 100644
> > > > --- a/tools/net/ynl/lib/ynl.c
> > > > +++ b/tools/net/ynl/lib/ynl.c
> > > > @@ -744,7 +744,7 @@ ynl_sock_create(const struct ynl_family *yf, struct ynl_error *yse)
> > > >  	ys = malloc(sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);
> > > >  	if (!ys)
> > > >  		return NULL;
> > > > -	memset(ys, 0, sizeof(*ys));
> > > > +	memset(ys, 0, sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);  
> > > 
> > > This is just clearing the buffer initially, it can be used for multiple
> > > requests. This change is no good as is.  
> > 
> > I see. Should then the ynl_attr_put_str be changed to zero the padding
> > bytes or it is better to make sure the buffers are cleared for each
> > request?
> 
> Eek, I think the bug is in how ynl_attr_put_str() computes len.
> len is attr len, it should not include padding.
> At the same time we should probably zero-terminate the strings
> in case kernel wants NLA_NUL_STRING.
> 
> Just for illustration -- I think we should do something like 
> the following, please turn this into a real patch if it makes sense:
> 
> diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
> index 29481989ea76..515c6d12f68a 100644
> --- a/tools/net/ynl/lib/ynl-priv.h
> +++ b/tools/net/ynl/lib/ynl-priv.h
> @@ -314,14 +314,14 @@ ynl_attr_put_str(struct nlmsghdr *nlh, unsigned int attr_type, const char *str)
>         size_t len;
>  
>         len = strlen(str);
> -       if (__ynl_attr_put_overflow(nlh, len))
> +       if (__ynl_attr_put_overflow(nlh, len + 1))
>                 return;
>  
>         attr = (struct nlattr *)ynl_nlmsg_end_addr(nlh);
>         attr->nla_type = attr_type;
>  
>         strcpy((char *)ynl_attr_data(attr), str);
> -       attr->nla_len = NLA_HDRLEN + NLA_ALIGN(len);
> +       attr->nla_len = NLA_HDRLEN + len + 1;
>  
>         nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);
>

thanks, we take a look at it.

Re: [PATCH 4/4] tools: ynl: add start-index property for indexed arrays

[Zahari Doychev]

On Wed, Oct 22, 2025 at 07:37:10PM +0000, Asbjørn Sloth Tønnesen wrote:
> On 10/21/25 5:50 PM, Zahari Doychev wrote:
> > On Mon, Oct 20, 2025 at 04:32:21PM -0700, Jakub Kicinski wrote:
> > > We need to be selective about what API stupidity we try to
> > > cover up in YNL. Otherwise the specs will be unmanageably complex.
> > > IMO this one should be a comment in the spec explaining that action
> > > 0 is ignore and that's it.
> > > 
> > 
> > I am not sure if this applies for all cases of indexed arrays. For sure
> > it applies for the tc_act_attrs case but I need to check the rest again.
> > 
> > Do you think it would be fine to start from 1 for all indexed arrays?
> Yes, AFAICT it would. Most of indexed-array attributes that are parsed by
> the kernel uses nla_for_each_nested(), and don't use the index. The TC
> actions are the only ones I found, that are parsed into a nlattr array.
> 
> Disclaimer: I have only mapped out the indexed-arrays that are declared in
> the current specs.
> 
> See patch 4-7 in this series for the full analysis:
> https://lore.kernel.org/netdev/20251022182701.250897-1-ast@fiberby.net/
>

thanks, will try it out.