[note, to avoid confusion: the problem mentioned below is independent of an ynl problem I ran into with -next today: https://lore.kernel.org/all/59ba7a94-17b9-485f-aa6d-14e4f01a7a39@leemhuis.in... ]
On 23.04.25 16:42, Greg Kroah-Hartman wrote:
6.14-stable review patch. If anyone has any objections, please let me know.
From: Jakub Kicinski kuba@kernel.org
[ Upstream commit ce6cb8113c842b94e77364b247c4f85c7b34e0c2 ]
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.
This change caused the following build error for me on Fedora while using a slightly modified version of the distro's kernel.spec file to build kernel rpms for Fedora. Reverting the change fixed the problem.
""" [....] CC net_shaper-user.o CC netdev-user.o CC nfsd-user.o In file included from net_shaper-user.c:8: net_shaper-user.h: In function ‘__net_shaper_group_req_set_leaves’: net_shaper-user.h:420:1: error: old-style parameter declarations in prototyped function definition 420 | __net_shaper_group_req_set_leaves(struct net_shaper_group_req *req, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from dpll-user.c:8: dpll-user.h: In function ‘__dpll_pin_set_req_set_parent_device’: dpll-user.h:522:1: error: old-style parameter declarations in prototyped function definition 522 | __dpll_pin_set_req_set_parent_device(struct dpll_pin_set_req *req, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ net_shaper-user.h:426:14: error: ‘i’ undeclared (first use in this function) 426 | for (i = 0; i < req->n_leaves; i++) | ^ net_shaper-user.h:426:14: note: each undeclared identifier is reported only once for each function it appears in dpll-user.h:528:14: error: ‘i’ undeclared (first use in this function) 528 | for (i = 0; i < req->n_parent_device; i++) | ^ dpll-user.h:528:14: note: each undeclared identifier is reported only once for each function it appears in dpll-user.h: In function ‘__dpll_pin_set_req_set_parent_pin’: dpll-user.h:535:1: error: old-style parameter declarations in prototyped function definition 535 | __dpll_pin_set_req_set_parent_pin(struct dpll_pin_set_req *req, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ dpll-user.h:541:14: error: ‘i’ undeclared (first use in this function) 541 | for (i = 0; i < req->n_parent_pin; i++) | ^ CC nlctrl-user.o make[1]: *** [Makefile:53: net_shaper-user.o] Error 1 make[1]: *** Waiting for unfinished jobs.... CC ovs_datapath-user.o make[1]: *** [Makefile:53: dpll-user.o] Error 1 In file included from netdev-user.c:8: netdev-user.h: In function ‘__netdev_bind_rx_req_set_queues’: In file included from nfsd-user.c:8: nfsd-user.h: In function ‘__nfsd_version_set_req_set_version’: netdev-user.h:563:1: error: old-style parameter declarations in prototyped function definition 563 | __netdev_bind_rx_req_set_queues(struct netdev_bind_rx_req *req, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ netdev-user.h:569:14: error: ‘i’ undeclared (first use in this function) 569 | for (i = 0; i < req->n_queues; i++) | ^ nfsd-user.h:190:1: error: old-style parameter declarations in prototyped function definition 190 | __nfsd_version_set_req_set_version(struct nfsd_version_set_req *req, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ netdev-user.h:569:14: note: each undeclared identifier is reported only once for each function it appears in nfsd-user.h:196:14: error: ‘i’ undeclared (first use in this function) 196 | for (i = 0; i < req->n_version; i++) | ^ nfsd-user.h:196:14: note: each undeclared identifier is reported only once for each function it appears in nfsd-user.h: In function ‘__nfsd_listener_set_req_set_addr’: nfsd-user.h:237:1: error: old-style parameter declarations in prototyped function definition 237 | __nfsd_listener_set_req_set_addr(struct nfsd_listener_set_req *req, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ nfsd-user.h:242:14: error: ‘i’ undeclared (first use in this function) 242 | for (i = 0; i < req->n_addr; i++) | ^ make[1]: *** [Makefile:53: nfsd-user.o] Error 1 make[1]: *** [Makefile:53: netdev-user.o] Error 1 AR ynl.a make: *** [Makefile:25: generated] Error 2 """
Full log: https://download.copr.fedorainfracloud.org/results/@kernel-vanilla/fedora-rc...
Ciao, Thorsten
Fixes: be5bea1cc0bf ("net: add basic C code generators for Netlink") Reviewed-by: Donald Hunter donald.hunter@gmail.com Reviewed-by: Jacob Keller jacob.e.keller@intel.com Link: https://patch.msgid.link/20250414211851.602096-4-kuba@kernel.org Signed-off-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Sasha Levin sashal@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 c2eabc90dce8c..9c9a62a93afe7 100755 --- a/tools/net/ynl/pyynl/ynl_gen_c.py +++ b/tools/net/ynl/pyynl/ynl_gen_c.py @@ -157,9 +157,15 @@ class Type(SpecAttr): 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) @@ -258,6 +264,10 @@ class Type(SpecAttr): 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)):@@ -267,6 +277,10 @@ class Type(SpecAttr): 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)}" @@ -274,7 +288,8 @@ class Type(SpecAttr): 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))@@ -477,8 +492,7 @@ class TypeString(Type): ['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;']@@ -531,8 +545,7 @@ class TypeBinary(Type): ['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);']@@ -569,12 +582,14 @@ class TypeNest(Type): 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 linesdef _attr_typol(self): return f'.type = YNL_PT_NEST, .nest = &{self.nested_render_name}_nest, ' @@ -627,15 +642,19 @@ class TypeMultiAttr(Type): 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 linesdef _attr_policy(self, policy): return self.base_type._attr_policy(policy) @@ -661,8 +680,7 @@ class TypeMultiAttr(Type): 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};"]@@ -747,6 +765,7 @@ class Struct: self.request = False self.reply = False self.recursive = False
self.in_multi_val = False # used by a MultiAttr or and legacy arraysself.attr_list = [] self.attrs = dict() @@ -1114,6 +1133,10 @@ class Family(SpecFamily): 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 = Trueself._sort_pure_types()# Propagate the request / reply / recursive @@ -1128,6 +1151,8 @@ class Family(SpecFamily): 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@@ -2921,6 +2946,9 @@ def main(): 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} ============== */")