[PATCH net 0/8] ynl: avoid leaks in attr override and spec fixes for C

[PATCH net 0/8] ynl: avoid leaks in attr override and spec fixes for C

[Jakub Kicinski]

The C rt-link work revealed more problems in existing codegen
and classic netlink specs.

Patches 1 - 4 fix issues with the codegen. Patches 1 and 2 are
pre-requisites for patch 3. Patch 3 fixes leaking memory if user
tries to override already set attr. Patch 4 validates attrs in case
kernel sends something we don't expect.

Remaining patches fix and align the specs. Patch 5 changes nesting,
the rest are naming adjustments.

Jakub Kicinski (8):
  tools: ynl-gen: don't declare loop iterator in place
  tools: ynl-gen: move local vars after the opening bracket
  tools: ynl-gen: individually free previous values on double set
  tools: ynl-gen: make sure we validate subtype of array-nest
  netlink: specs: rt-link: add an attr layer around alt-ifname
  netlink: specs: rtnetlink: attribute naming corrections
  netlink: specs: rt-link: adjust mctp attribute naming
  netlink: specs: rt-neigh: prefix struct nfmsg members with ndm

 Documentation/netlink/specs/rt_link.yaml  | 20 +++--
 Documentation/netlink/specs/rt_neigh.yaml | 14 ++--
 tools/net/ynl/pyynl/ynl_gen_c.py          | 96 +++++++++++++++++------
 3 files changed, 92 insertions(+), 38 deletions(-)

-- 
2.49.0

[PATCH net 1/8] tools: ynl-gen: don't declare loop iterator in place

[Jakub Kicinski]

The codegen tries to follow the "old" C style and declare loop
iterators at the start of the block / function. Only nested
request handling breaks this style, so adjust it.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index a1427c537030..305f5696bc4f 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -654,10 +654,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def attr_put(self, ri, var):
         if self.attr['type'] in scalars:
             put_type = self.type
-            ri.cw.p(f"for (unsigned int i = 0; i < {var}->n_{self.c_name}; i++)")
+            ri.cw.p(f"for (i = 0; i < {var}->n_{self.c_name}; i++)")
             ri.cw.p(f"ynl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name}[i]);")
         elif 'type' not in self.attr or self.attr['type'] == 'nest':
-            ri.cw.p(f"for (unsigned int i = 0; i < {var}->n_{self.c_name}; i++)")
+            ri.cw.p(f"for (i = 0; i < {var}->n_{self.c_name}; i++)")
             self._attr_put_line(ri, var, f"{self.nested_render_name}_put(nlh, " +
                                 f"{self.enum_name}, &{var}->{self.c_name}[i])")
         else:
@@ -1644,11 +1644,23 @@ _C_KW = {
 
 
 def put_req_nested(ri, struct):
+    local_vars = []
+    init_lines = []
+
+    local_vars.append('struct nlattr *nest;')
+    init_lines.append("nest = ynl_attr_nest_start(nlh, attr_type);")
+
+    for _, arg in struct.member_list():
+        if arg.presence_type() == 'count':
+            local_vars.append('unsigned int i;')
+            break
+
     put_req_nested_prototype(ri, struct, suffix='')
     ri.cw.block_start()
-    ri.cw.write_func_lvar('struct nlattr *nest;')
+    ri.cw.write_func_lvar(local_vars)
 
-    ri.cw.p("nest = ynl_attr_nest_start(nlh, attr_type);")
+    for line in init_lines:
+        ri.cw.p(line)
 
     for _, arg in struct.member_list():
         arg.attr_put(ri, "obj")
@@ -1850,6 +1862,11 @@ _C_KW = {
         local_vars += ['size_t hdr_len;',
                        'void *hdr;']
 
+    for _, attr in ri.struct["request"].member_list():
+        if attr.presence_type() == 'count':
+            local_vars += ['unsigned int i;']
+            break
+
     print_prototype(ri, direction, terminate=False)
     ri.cw.block_start()
     ri.cw.write_func_lvar(local_vars)
-- 
2.49.0

[PATCH net 2/8] tools: ynl-gen: move local vars after the opening bracket

[Jakub Kicinski]

The "function writing helper" tries to put local variables
between prototype and the opening bracket. Clearly wrong,
but up until now nothing actually uses it to write local
vars so it wasn't noticed.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 305f5696bc4f..662a925bd9e1 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -1399,9 +1399,9 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
 
     def write_func(self, qual_ret, name, body, args=None, local_vars=None):
         self.write_func_prot(qual_ret=qual_ret, name=name, args=args)
+        self.block_start()
         self.write_func_lvar(local_vars=local_vars)
 
-        self.block_start()
         for line in body:
             self.p(line)
         self.block_end()
-- 
2.49.0

[PATCH net 3/8] tools: ynl-gen: individually free previous values on double set

[Jakub Kicinski]

When user calls request_attrA_set() multiple times (for the same
attribute), and attrA is of type which allocates memory -
we try to free the previously associated values. For array
types (including multi-attr) we have only freed the array,
but the array may have contained pointers.

Refactor the code generation for free attr and reuse the generated
lines in setters to flush out the previous state. Since setters
are static inlines in the header we need to add forward declarations
for the free helpers of pure nested structs. Track which types get
used by arrays and include the right forwad declarations.

At least ethtool string set and bit set would not be freed without
this. Tho, admittedly, overriding already set attribute twice is likely
a very very rare thing to do.

Fixes: be5bea1cc0bf ("net: add basic C code generators for Netlink")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 62 +++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 662a925bd9e1..2d856ccc88f4 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -162,9 +162,15 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def free_needs_iter(self):
         return False
 
-    def free(self, ri, var, ref):
+    def _free_lines(self, ri, var, ref):
         if self.is_multi_val() or self.presence_type() == 'len':
-            ri.cw.p(f'free({var}->{ref}{self.c_name});')
+            return [f'free({var}->{ref}{self.c_name});']
+        return []
+
+    def free(self, ri, var, ref):
+        lines = self._free_lines(ri, var, ref)
+        for line in lines:
+            ri.cw.p(line)
 
     def arg_member(self, ri):
         member = self._complex_member_type(ri)
@@ -263,6 +269,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         var = "req"
         member = f"{var}->{'.'.join(ref)}"
 
+        local_vars = []
+        if self.free_needs_iter():
+            local_vars += ['unsigned int i;']
+
         code = []
         presence = ''
         for i in range(0, len(ref)):
@@ -272,6 +282,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
             if i == len(ref) - 1 and self.presence_type() != 'bit':
                 continue
             code.append(presence + ' = 1;')
+        ref_path = '.'.join(ref[:-1])
+        if ref_path:
+            ref_path += '.'
+        code += self._free_lines(ri, var, ref_path)
         code += self._setter_lines(ri, member, presence)
 
         func_name = f"{op_prefix(ri, direction, deref=deref)}_set_{'_'.join(ref)}"
@@ -279,7 +293,8 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         alloc = bool([x for x in code if 'alloc(' in x])
         if free and not alloc:
             func_name = '__' + func_name
-        ri.cw.write_func('static inline void', func_name, body=code,
+        ri.cw.write_func('static inline void', func_name, local_vars=local_vars,
+                         body=code,
                          args=[f'{type_name(ri, direction, deref=deref)} *{var}'] + self.arg_member(ri))
 
 
@@ -482,8 +497,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
                ['unsigned int len;']
 
     def _setter_lines(self, ri, member, presence):
-        return [f"free({member});",
-                f"{presence}_len = strlen({self.c_name});",
+        return [f"{presence}_len = strlen({self.c_name});",
                 f"{member} = malloc({presence}_len + 1);",
                 f'memcpy({member}, {self.c_name}, {presence}_len);',
                 f'{member}[{presence}_len] = 0;']
@@ -536,8 +550,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
                ['unsigned int len;']
 
     def _setter_lines(self, ri, member, presence):
-        return [f"free({member});",
-                f"{presence}_len = len;",
+        return [f"{presence}_len = len;",
                 f"{member} = malloc({presence}_len);",
                 f'memcpy({member}, {self.c_name}, {presence}_len);']
 
@@ -574,12 +587,14 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def _complex_member_type(self, ri):
         return self.nested_struct_type
 
-    def free(self, ri, var, ref):
+    def _free_lines(self, ri, var, ref):
+        lines = []
         at = '&'
         if self.is_recursive_for_op(ri):
             at = ''
-            ri.cw.p(f'if ({var}->{ref}{self.c_name})')
-        ri.cw.p(f'{self.nested_render_name}_free({at}{var}->{ref}{self.c_name});')
+            lines += [f'if ({var}->{ref}{self.c_name})']
+        lines += [f'{self.nested_render_name}_free({at}{var}->{ref}{self.c_name});']
+        return lines
 
     def _attr_typol(self):
         return f'.type = YNL_PT_NEST, .nest = &{self.nested_render_name}_nest, '
@@ -632,15 +647,19 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def free_needs_iter(self):
         return 'type' not in self.attr or self.attr['type'] == 'nest'
 
-    def free(self, ri, var, ref):
+    def _free_lines(self, ri, var, ref):
+        lines = []
         if self.attr['type'] in scalars:
-            ri.cw.p(f"free({var}->{ref}{self.c_name});")
+            lines += [f"free({var}->{ref}{self.c_name});"]
         elif 'type' not in self.attr or self.attr['type'] == 'nest':
-            ri.cw.p(f"for (i = 0; i < {var}->{ref}n_{self.c_name}; i++)")
-            ri.cw.p(f'{self.nested_render_name}_free(&{var}->{ref}{self.c_name}[i]);')
-            ri.cw.p(f"free({var}->{ref}{self.c_name});")
+            lines += [
+                f"for (i = 0; i < {var}->{ref}n_{self.c_name}; i++)",
+                f'{self.nested_render_name}_free(&{var}->{ref}{self.c_name}[i]);',
+                f"free({var}->{ref}{self.c_name});",
+            ]
         else:
             raise Exception(f"Free of MultiAttr sub-type {self.attr['type']} not supported yet")
+        return lines
 
     def _attr_policy(self, policy):
         return self.base_type._attr_policy(policy)
@@ -666,8 +685,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def _setter_lines(self, ri, member, presence):
         # For multi-attr we have a count, not presence, hack up the presence
         presence = presence[:-(len('_present.') + len(self.c_name))] + "n_" + self.c_name
-        return [f"free({member});",
-                f"{member} = {self.c_name};",
+        return [f"{member} = {self.c_name};",
                 f"{presence} = n_{self.c_name};"]
 
 
@@ -755,6 +773,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         self.request = False
         self.reply = False
         self.recursive = False
+        self.in_multi_val = False  # used by a MultiAttr or and legacy arrays
 
         self.attr_list = []
         self.attrs = dict()
@@ -1122,6 +1141,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
                     if attr in rs_members['reply']:
                         self.pure_nested_structs[nested].reply = True
 
+                if spec.is_multi_val():
+                    child = self.pure_nested_structs.get(nested)
+                    child.in_multi_val = True
+
         self._sort_pure_types()
 
         # Propagate the request / reply / recursive
@@ -1136,6 +1159,8 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
                             struct.child_nests.update(child.child_nests)
                         child.request |= struct.request
                         child.reply |= struct.reply
+                        if spec.is_multi_val():
+                            child.in_multi_val = True
                 if attr_set in struct.child_nests:
                     struct.recursive = True
 
@@ -2958,6 +2983,9 @@ _C_KW = {
             for attr_set, struct in parsed.pure_nested_structs.items():
                 ri = RenderInfo(cw, parsed, args.mode, "", "", attr_set)
                 print_type_full(ri, struct)
+                if struct.request and struct.in_multi_val:
+                    free_rsp_nested_prototype(ri)
+                    cw.nl()
 
             for op_name, op in parsed.ops.items():
                 cw.p(f"/* ============== {op.enum_name} ============== */")
-- 
2.49.0

[PATCH net 4/8] tools: ynl-gen: make sure we validate subtype of array-nest

[Jakub Kicinski]

ArrayNest AKA indexed-array support currently skips inner type
validation. We count the attributes and then we parse them,
make sure we call validate, too. Otherwise buggy / unexpected
kernel response may lead to crashes.

Fixes: be5bea1cc0bf ("net: add basic C code generators for Netlink")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 2d856ccc88f4..30c0a34b2784 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -714,8 +714,11 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def _attr_get(self, ri, var):
         local_vars = ['const struct nlattr *attr2;']
         get_lines = [f'attr_{self.c_name} = attr;',
-                     'ynl_attr_for_each_nested(attr2, attr)',
-                     f'\t{var}->n_{self.c_name}++;']
+                     'ynl_attr_for_each_nested(attr2, attr) {',
+                     '\tif (ynl_attr_validate(yarg, attr2))',
+                     '\t\treturn YNL_PARSE_CB_ERROR;',
+                     f'\t{var}->n_{self.c_name}++;',
+                     '}']
         return get_lines, None, local_vars
 
 
-- 
2.49.0

[PATCH net 5/8] netlink: specs: rt-link: add an attr layer around alt-ifname

[Jakub Kicinski]

alt-ifname attr is directly placed in requests (as an alternative
to ifname) but in responses its wrapped up in IFLA_PROP_LIST
and only there is may be multi-attr. See rtnl_fill_prop_list().

Fixes: b2f63d904e72 ("doc/netlink: Add spec for rt link messages")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/rt_link.yaml | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/netlink/specs/rt_link.yaml b/Documentation/netlink/specs/rt_link.yaml
index 31238455f8e9..200e9a7e5b11 100644
--- a/Documentation/netlink/specs/rt_link.yaml
+++ b/Documentation/netlink/specs/rt_link.yaml
@@ -1113,11 +1113,10 @@ protonum: 0
       -
         name: prop-list
         type: nest
-        nested-attributes: link-attrs
+        nested-attributes: prop-list-link-attrs
       -
         name: alt-ifname
         type: string
-        multi-attr: true
       -
         name: perm-address
         type: binary
@@ -1163,6 +1162,13 @@ protonum: 0
       -
         name: netns-immutable
         type: u8
+  -
+    name: prop-list-link-attrs
+    subset-of: link-attrs
+    attributes:
+      -
+        name: alt-ifname
+        multi-attr: true
   -
     name: af-spec-attrs
     attributes:
@@ -2453,7 +2459,6 @@ protonum: 0
             - min-mtu
             - max-mtu
             - prop-list
-            - alt-ifname
             - perm-address
             - proto-down-reason
             - parent-dev-name
-- 
2.49.0

[PATCH net 6/8] netlink: specs: rtnetlink: attribute naming corrections

[Jakub Kicinski]

Some attribute names diverge in very minor ways from the C names.
These are most likely typos, and they prevent the C codegen from
working.

Fixes: bc515ed06652 ("netlink: specs: Add a spec for neighbor tables in rtnetlink")
Fixes: b2f63d904e72 ("doc/netlink: Add spec for rt link messages")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/rt_link.yaml  | 6 +++---
 Documentation/netlink/specs/rt_neigh.yaml | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/netlink/specs/rt_link.yaml b/Documentation/netlink/specs/rt_link.yaml
index 200e9a7e5b11..03323d7f58dc 100644
--- a/Documentation/netlink/specs/rt_link.yaml
+++ b/Documentation/netlink/specs/rt_link.yaml
@@ -1591,7 +1591,7 @@ protonum: 0
         name: nf-call-iptables
         type: u8
       -
-        name: nf-call-ip6-tables
+        name: nf-call-ip6tables
         type: u8
       -
         name: nf-call-arptables
@@ -2083,7 +2083,7 @@ protonum: 0
         name: id
         type: u16
       -
-        name: flag
+        name: flags
         type: binary
         struct: ifla-vlan-flags
       -
@@ -2171,7 +2171,7 @@ protonum: 0
         type: binary
         struct: ifla-cacheinfo
       -
-        name: icmp6-stats
+        name: icmp6stats
         type: binary
         struct: ifla-icmp6-stats
       -
diff --git a/Documentation/netlink/specs/rt_neigh.yaml b/Documentation/netlink/specs/rt_neigh.yaml
index e670b6dc07be..a1e137a16abd 100644
--- a/Documentation/netlink/specs/rt_neigh.yaml
+++ b/Documentation/netlink/specs/rt_neigh.yaml
@@ -189,7 +189,7 @@ protonum: 0
         type: binary
         display-hint: ipv4
       -
-        name: lladr
+        name: lladdr
         type: binary
         display-hint: mac
       -
-- 
2.49.0

[PATCH net 7/8] netlink: specs: rt-link: adjust mctp attribute naming

[Jakub Kicinski]

MCTP attribute naming is inconsistent. In C we have:
    IFLA_MCTP_NET,
    IFLA_MCTP_PHYS_BINDING,
         ^^^^

but in YAML:
    - mctp-net
    - phys-binding
      ^
       no "mctp"

It's unclear whether the "mctp" part of the name is supposed
to be a prefix or part of attribute name. Make it a prefix,
seems cleaner, even tho technically phys-binding was added later.

Fixes: b2f63d904e72 ("doc/netlink: Add spec for rt link messages")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/rt_link.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/netlink/specs/rt_link.yaml b/Documentation/netlink/specs/rt_link.yaml
index 03323d7f58dc..6b9d5ee87d93 100644
--- a/Documentation/netlink/specs/rt_link.yaml
+++ b/Documentation/netlink/specs/rt_link.yaml
@@ -2185,9 +2185,10 @@ protonum: 0
         type: u32
   -
     name: mctp-attrs
+    name-prefix: ifla-mctp-
     attributes:
       -
-        name: mctp-net
+        name: net
         type: u32
       -
         name: phys-binding
-- 
2.49.0

[PATCH net 8/8] netlink: specs: rt-neigh: prefix struct nfmsg members with ndm

[Jakub Kicinski]

Attach ndm- to all members of struct nfmsg. We could possibly
use name-prefix just for C, but I don't think we have any precedent
for using name-prefix on structs, and other rtnetlink sub-specs
give full names for fixed header struct members.

Fixes: bc515ed06652 ("netlink: specs: Add a spec for neighbor tables in rtnetlink")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/rt_neigh.yaml | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/netlink/specs/rt_neigh.yaml b/Documentation/netlink/specs/rt_neigh.yaml
index a1e137a16abd..a843caa72259 100644
--- a/Documentation/netlink/specs/rt_neigh.yaml
+++ b/Documentation/netlink/specs/rt_neigh.yaml
@@ -13,25 +13,25 @@ protonum: 0
     type: struct
     members:
       -
-        name: family
+        name: ndm-family
         type: u8
       -
-        name: pad
+        name: ndm-pad
         type: pad
         len: 3
       -
-        name: ifindex
+        name: ndm-ifindex
         type: s32
       -
-        name: state
+        name: ndm-state
         type: u16
         enum: nud-state
       -
-        name: flags
+        name: ndm-flags
         type: u8
         enum: ntf-flags
       -
-        name: type
+        name: ndm-type
         type: u8
         enum: rtm-type
   -
-- 
2.49.0

Re: [PATCH net 0/8] ynl: avoid leaks in attr override and spec fixes for C

[Jacob Keller]

On 4/14/2025 2:18 PM, Jakub Kicinski wrote:
> The C rt-link work revealed more problems in existing codegen
> and classic netlink specs.
> 
> Patches 1 - 4 fix issues with the codegen. Patches 1 and 2 are
> pre-requisites for patch 3. Patch 3 fixes leaking memory if user
> tries to override already set attr. Patch 4 validates attrs in case
> kernel sends something we don't expect.
> 
> Remaining patches fix and align the specs. Patch 5 changes nesting,
> the rest are naming adjustments.
> 
> Jakub Kicinski (8):
>   tools: ynl-gen: don't declare loop iterator in place
>   tools: ynl-gen: move local vars after the opening bracket
>   tools: ynl-gen: individually free previous values on double set
>   tools: ynl-gen: make sure we validate subtype of array-nest
>   netlink: specs: rt-link: add an attr layer around alt-ifname
>   netlink: specs: rtnetlink: attribute naming corrections
>   netlink: specs: rt-link: adjust mctp attribute naming
>   netlink: specs: rt-neigh: prefix struct nfmsg members with ndm
> 

For the series:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  Documentation/netlink/specs/rt_link.yaml  | 20 +++--
>  Documentation/netlink/specs/rt_neigh.yaml | 14 ++--
>  tools/net/ynl/pyynl/ynl_gen_c.py          | 96 +++++++++++++++++------
>  3 files changed, 92 insertions(+), 38 deletions(-)
> 

Re: [PATCH net 2/8] tools: ynl-gen: move local vars after the opening bracket

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> The "function writing helper" tries to put local variables
> between prototype and the opening bracket. Clearly wrong,
> but up until now nothing actually uses it to write local
> vars so it wasn't noticed.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH net 1/8] tools: ynl-gen: don't declare loop iterator in place

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> The codegen tries to follow the "old" C style and declare loop
> iterators at the start of the block / function. Only nested
> request handling breaks this style, so adjust it.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/net/ynl/pyynl/ynl_gen_c.py | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
> index a1427c537030..305f5696bc4f 100755
> --- a/tools/net/ynl/pyynl/ynl_gen_c.py
> +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
> @@ -654,10 +654,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>      def attr_put(self, ri, var):
>          if self.attr['type'] in scalars:
>              put_type = self.type
> -            ri.cw.p(f"for (unsigned int i = 0; i < {var}->n_{self.c_name}; i++)")
> +            ri.cw.p(f"for (i = 0; i < {var}->n_{self.c_name}; i++)")
>              ri.cw.p(f"ynl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name}[i]);")
>          elif 'type' not in self.attr or self.attr['type'] == 'nest':
> -            ri.cw.p(f"for (unsigned int i = 0; i < {var}->n_{self.c_name}; i++)")
> +            ri.cw.p(f"for (i = 0; i < {var}->n_{self.c_name}; i++)")
>              self._attr_put_line(ri, var, f"{self.nested_render_name}_put(nlh, " +
>                                  f"{self.enum_name}, &{var}->{self.c_name}[i])")
>          else:
> @@ -1644,11 +1644,23 @@ _C_KW = {
>  
>  
>  def put_req_nested(ri, struct):
> +    local_vars = []
> +    init_lines = []
> +
> +    local_vars.append('struct nlattr *nest;')
> +    init_lines.append("nest = ynl_attr_nest_start(nlh, attr_type);")

Minor nit: the series uses += ['...'] elsewhere, is it worth being
consistent?

Either way,
Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH net 3/8] tools: ynl-gen: individually free previous values on double set

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> When user calls request_attrA_set() multiple times (for the same
> attribute), and attrA is of type which allocates memory -
> we try to free the previously associated values. For array
> types (including multi-attr) we have only freed the array,
> but the array may have contained pointers.
>
> Refactor the code generation for free attr and reuse the generated
> lines in setters to flush out the previous state. Since setters
> are static inlines in the header we need to add forward declarations
> for the free helpers of pure nested structs. Track which types get
> used by arrays and include the right forwad declarations.
>
> At least ethtool string set and bit set would not be freed without
> this. Tho, admittedly, overriding already set attribute twice is likely
> a very very rare thing to do.
>
> Fixes: be5bea1cc0bf ("net: add basic C code generators for Netlink")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH net 4/8] tools: ynl-gen: make sure we validate subtype of array-nest

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> ArrayNest AKA indexed-array support currently skips inner type
> validation. We count the attributes and then we parse them,
> make sure we call validate, too. Otherwise buggy / unexpected
> kernel response may lead to crashes.
>
> Fixes: be5bea1cc0bf ("net: add basic C code generators for Netlink")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH net 5/8] netlink: specs: rt-link: add an attr layer around alt-ifname

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> alt-ifname attr is directly placed in requests (as an alternative
> to ifname) but in responses its wrapped up in IFLA_PROP_LIST
> and only there is may be multi-attr. See rtnl_fill_prop_list().
>
> Fixes: b2f63d904e72 ("doc/netlink: Add spec for rt link messages")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH net 6/8] netlink: specs: rtnetlink: attribute naming corrections

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> Some attribute names diverge in very minor ways from the C names.
> These are most likely typos, and they prevent the C codegen from
> working.
>
> Fixes: bc515ed06652 ("netlink: specs: Add a spec for neighbor tables in rtnetlink")
> Fixes: b2f63d904e72 ("doc/netlink: Add spec for rt link messages")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH net 7/8] netlink: specs: rt-link: adjust mctp attribute naming

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> MCTP attribute naming is inconsistent. In C we have:
>     IFLA_MCTP_NET,
>     IFLA_MCTP_PHYS_BINDING,
>          ^^^^
>
> but in YAML:
>     - mctp-net
>     - phys-binding
>       ^
>        no "mctp"
>
> It's unclear whether the "mctp" part of the name is supposed
> to be a prefix or part of attribute name. Make it a prefix,
> seems cleaner, even tho technically phys-binding was added later.
>
> Fixes: b2f63d904e72 ("doc/netlink: Add spec for rt link messages")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH net 8/8] netlink: specs: rt-neigh: prefix struct nfmsg members with ndm

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> Attach ndm- to all members of struct nfmsg. We could possibly
> use name-prefix just for C, but I don't think we have any precedent
> for using name-prefix on structs, and other rtnetlink sub-specs
> give full names for fixed header struct members.
>
> Fixes: bc515ed06652 ("netlink: specs: Add a spec for neighbor tables in rtnetlink")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH net 1/8] tools: ynl-gen: don't declare loop iterator in place

[Jakub Kicinski]

On Wed, 16 Apr 2025 11:06:39 +0100 Donald Hunter wrote:
> >  def put_req_nested(ri, struct):
> > +    local_vars = []
> > +    init_lines = []
> > +
> > +    local_vars.append('struct nlattr *nest;')
> > +    init_lines.append("nest = ynl_attr_nest_start(nlh, attr_type);")  
> 
> Minor nit: the series uses += ['...'] elsewhere, is it worth being
> consistent?

Agreed, it was annoying me too.. but in _multi_parse() I seem to have
used .append() in the exact same scenario. Hard to define "consistent"
in this code base :(

RE: [PATCH net 1/8] tools: ynl-gen: don't declare loop iterator in place

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, April 16, 2025 6:45 AM
> To: Donald Hunter <donald.hunter@gmail.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Dumazet, Eric
> <edumazet@google.com>; pabeni@redhat.com; andrew+netdev@lunn.ch;
> horms@kernel.org; daniel@iogearbox.net; sdf@fomichev.me; Keller, Jacob E
> <jacob.e.keller@intel.com>
> Subject: Re: [PATCH net 1/8] tools: ynl-gen: don't declare loop iterator in place
> 
> On Wed, 16 Apr 2025 11:06:39 +0100 Donald Hunter wrote:
> > >  def put_req_nested(ri, struct):
> > > +    local_vars = []
> > > +    init_lines = []
> > > +
> > > +    local_vars.append('struct nlattr *nest;')
> > > +    init_lines.append("nest = ynl_attr_nest_start(nlh, attr_type);")
> >
> > Minor nit: the series uses += ['...'] elsewhere, is it worth being
> > consistent?
> 
> Agreed, it was annoying me too.. but in _multi_parse() I seem to have
> used .append() in the exact same scenario. Hard to define "consistent"
> in this code base :(

+= [ ... ] is more akin to joining two lists, which may technically be less efficient if you're just adding a single element. Of course that probably doesn't matter for something like this.

I'd say its fairly clear either way so not sure how much we gain from consistency vs re-rolling.

Thanks,
Jake

Re: [PATCH net 0/8] ynl: avoid leaks in attr override and spec fixes for C

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 14 Apr 2025 14:18:43 -0700 you wrote:
> The C rt-link work revealed more problems in existing codegen
> and classic netlink specs.
> 
> Patches 1 - 4 fix issues with the codegen. Patches 1 and 2 are
> pre-requisites for patch 3. Patch 3 fixes leaking memory if user
> tries to override already set attr. Patch 4 validates attrs in case
> kernel sends something we don't expect.
> 
> [...]

Here is the summary with links:
  - [net,1/8] tools: ynl-gen: don't declare loop iterator in place
    https://git.kernel.org/netdev/net/c/4d07bbf2d456
  - [net,2/8] tools: ynl-gen: move local vars after the opening bracket
    https://git.kernel.org/netdev/net/c/dfa464b4a603
  - [net,3/8] tools: ynl-gen: individually free previous values on double set
    https://git.kernel.org/netdev/net/c/ce6cb8113c84
  - [net,4/8] tools: ynl-gen: make sure we validate subtype of array-nest
    https://git.kernel.org/netdev/net/c/57e7dedf2b8c
  - [net,5/8] netlink: specs: rt-link: add an attr layer around alt-ifname
    https://git.kernel.org/netdev/net/c/acf4da17dead
  - [net,6/8] netlink: specs: rtnetlink: attribute naming corrections
    https://git.kernel.org/netdev/net/c/540201c0ef7e
  - [net,7/8] netlink: specs: rt-link: adjust mctp attribute naming
    https://git.kernel.org/netdev/net/c/beb3c5ad8829
  - [net,8/8] netlink: specs: rt-neigh: prefix struct nfmsg members with ndm
    https://git.kernel.org/netdev/net/c/e31f86ee4b9c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html