From 69072db934dfc7a566d4eb1fac04146e97ab365f Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Mon, 6 Jan 2025 18:28:18 -0800 Subject: [PATCH 1/3] tools: ynl: correctly handle overrides of fields in subset We stated in documentation [1] and previous discussions [2] that the need for overriding fields in members of subsets is anticipated. Implement it. Since each attr is now a new object we need to make sure that the modifications are propagated. Specifically C codegen wants to annotate which attrs are used in requests and replies to generate the right validation artifacts. [1] https://docs.kernel.org/next/userspace-api/netlink/specs.html#subset-of [2] https://lore.kernel.org/netdev/20231004171350.1f59cd1d@kernel.org/ Reviewed-by: Donald Hunter Link: https://patch.msgid.link/20250107022820.2087101-2-kuba@kernel.org Signed-off-by: Jakub Kicinski --- tools/net/ynl/lib/nlspec.py | 5 ++++- tools/net/ynl/ynl-gen-c.py | 26 ++++++++++++++++++++++---- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py index a745739655ad..314ec8007496 100644 --- a/tools/net/ynl/lib/nlspec.py +++ b/tools/net/ynl/lib/nlspec.py @@ -219,7 +219,10 @@ class SpecAttrSet(SpecElement): else: real_set = family.attr_sets[self.subset_of] for elem in self.yaml['attributes']: - attr = real_set[elem['name']] + real_attr = real_set[elem['name']] + combined_elem = real_attr.yaml | elem + attr = self.new_attr(combined_elem, real_attr.value) + self.attrs[attr.name] = attr self.attrs_by_val[attr.value] = attr diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py index ec2288948795..58657dd7dedb 100755 --- a/tools/net/ynl/ynl-gen-c.py +++ b/tools/net/ynl/ynl-gen-c.py @@ -79,6 +79,20 @@ class Type(SpecAttr): self.enum_name = None delattr(self, "enum_name") + def _get_real_attr(self): + # if the attr is for a subset return the "real" attr (just one down, does not recurse) + return self.family.attr_sets[self.attr_set.subset_of][self.name] + + def set_request(self): + self.request = True + if self.attr_set.subset_of: + self._get_real_attr().set_request() + + def set_reply(self): + self.reply = True + if self.attr_set.subset_of: + self._get_real_attr().set_reply() + def get_limit(self, limit, default=None): value = self.checks.get(limit, default) if value is None: @@ -106,6 +120,10 @@ class Type(SpecAttr): enum_name = f"{self.attr_set.name_prefix}{self.name}" self.enum_name = c_upper(enum_name) + if self.attr_set.subset_of: + if self.checks != self._get_real_attr().checks: + raise Exception("Overriding checks not supported by codegen, yet") + def is_multi_val(self): return None @@ -1119,17 +1137,17 @@ class Family(SpecFamily): for _, struct in self.pure_nested_structs.items(): if struct.request: for _, arg in struct.member_list(): - arg.request = True + arg.set_request() if struct.reply: for _, arg in struct.member_list(): - arg.reply = True + arg.set_reply() for root_set, rs_members in self.root_sets.items(): for attr, spec in self.attr_sets[root_set].items(): if attr in rs_members['request']: - spec.request = True + spec.set_request() if attr in rs_members['reply']: - spec.reply = True + spec.set_reply() def _load_global_policy(self): global_set = set() From 7aae6505351e4c9fc2f3108d16fd06ce76f4b475 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Mon, 6 Jan 2025 18:28:19 -0800 Subject: [PATCH 2/3] tools: ynl: print some information about attribute we can't parse When parsing throws an exception one often has to figure out which attribute couldn't be parsed from first principles. For families with large message parsing trees like rtnetlink guessing the attribute can be hard. Print a bit of information as the exception travels out, e.g.: # when dumping rt links Error decoding 'flags' from 'linkinfo-ip6tnl-attrs' Error decoding 'data' from 'linkinfo-attrs' Error decoding 'linkinfo' from 'link-attrs' Traceback (most recent call last): File "/home/kicinski/linux/./tools/net/ynl/cli.py", line 119, in main() File "/home/kicinski/linux/./tools/net/ynl/cli.py", line 100, in main reply = ynl.dump(args.dump, attrs) File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 1064, in dump return self._op(method, vals, dump=True) File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 1058, in _op return self._ops(ops)[0] File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 1045, in _ops rsp_msg = self._decode(decoded.raw_attrs, op.attr_set.name) File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 738, in _decode subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'], search_attrs) File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 763, in _decode decoded = self._decode_sub_msg(attr, attr_spec, search_attrs) File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 714, in _decode_sub_msg subdict = self._decode(NlAttrs(attr.raw, offset), msg_format.attr_set) File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 749, in _decode decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order) File "/home/kicinski/linux/tools/net/ynl/lib/ynl.py", line 147, in as_scalar return format.unpack(self.raw)[0] struct.error: unpack requires a buffer of 2 bytes The Traceback is what we would previously see, the "Error..." messages are new. We print a message per level (in the stack order). Printing single combined message gets tricky quickly given sub-messages etc. Reviewed-by: Donald Hunter Acked-by: Stanislav Fomichev Link: https://patch.msgid.link/20250107022820.2087101-3-kuba@kernel.org Signed-off-by: Jakub Kicinski --- tools/net/ynl/lib/ynl.py | 72 +++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 34 deletions(-) diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index eea29359a899..08f8bf89cfc2 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -733,41 +733,45 @@ class YnlFamily(SpecFamily): self._rsp_add(rsp, attr_name, None, self._decode_unknown(attr)) continue - if attr_spec["type"] == 'nest': - subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'], search_attrs) - decoded = subdict - elif attr_spec["type"] == 'string': - decoded = attr.as_strz() - elif attr_spec["type"] == 'binary': - decoded = self._decode_binary(attr, attr_spec) - elif attr_spec["type"] == 'flag': - decoded = True - elif attr_spec.is_auto_scalar: - decoded = attr.as_auto_scalar(attr_spec['type'], attr_spec.byte_order) - elif attr_spec["type"] in NlAttr.type_formats: - decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order) - if 'enum' in attr_spec: - decoded = self._decode_enum(decoded, attr_spec) - elif attr_spec.display_hint: - decoded = self._formatted_string(decoded, attr_spec.display_hint) - elif attr_spec["type"] == 'indexed-array': - decoded = self._decode_array_attr(attr, attr_spec) - elif attr_spec["type"] == 'bitfield32': - value, selector = struct.unpack("II", attr.raw) - if 'enum' in attr_spec: - value = self._decode_enum(value, attr_spec) - selector = self._decode_enum(selector, attr_spec) - decoded = {"value": value, "selector": selector} - elif attr_spec["type"] == 'sub-message': - decoded = self._decode_sub_msg(attr, attr_spec, search_attrs) - elif attr_spec["type"] == 'nest-type-value': - decoded = self._decode_nest_type_value(attr, attr_spec) - else: - if not self.process_unknown: - raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}') - decoded = self._decode_unknown(attr) + try: + if attr_spec["type"] == 'nest': + subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'], search_attrs) + decoded = subdict + elif attr_spec["type"] == 'string': + decoded = attr.as_strz() + elif attr_spec["type"] == 'binary': + decoded = self._decode_binary(attr, attr_spec) + elif attr_spec["type"] == 'flag': + decoded = True + elif attr_spec.is_auto_scalar: + decoded = attr.as_auto_scalar(attr_spec['type'], attr_spec.byte_order) + elif attr_spec["type"] in NlAttr.type_formats: + decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order) + if 'enum' in attr_spec: + decoded = self._decode_enum(decoded, attr_spec) + elif attr_spec.display_hint: + decoded = self._formatted_string(decoded, attr_spec.display_hint) + elif attr_spec["type"] == 'indexed-array': + decoded = self._decode_array_attr(attr, attr_spec) + elif attr_spec["type"] == 'bitfield32': + value, selector = struct.unpack("II", attr.raw) + if 'enum' in attr_spec: + value = self._decode_enum(value, attr_spec) + selector = self._decode_enum(selector, attr_spec) + decoded = {"value": value, "selector": selector} + elif attr_spec["type"] == 'sub-message': + decoded = self._decode_sub_msg(attr, attr_spec, search_attrs) + elif attr_spec["type"] == 'nest-type-value': + decoded = self._decode_nest_type_value(attr, attr_spec) + else: + if not self.process_unknown: + raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}') + decoded = self._decode_unknown(attr) - self._rsp_add(rsp, attr_spec["name"], attr_spec.is_multi, decoded) + self._rsp_add(rsp, attr_spec["name"], attr_spec.is_multi, decoded) + except: + print(f"Error decoding '{attr_spec.name}' from '{space}'") + raise return rsp From 6ffdbb93a59c60ee18bebd9acdbbca91e6bf0e64 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Mon, 6 Jan 2025 18:28:20 -0800 Subject: [PATCH 3/3] netlink: specs: rt_link: decode ip6tnl, vti and vti6 link attrs Some of our tests load vti and ip6tnl so not being able to decode the link attrs gets in the way of using Python YNL for testing. Decode link attributes for ip6tnl, vti and vti6. ip6tnl uses IFLA_IPTUN_FLAGS as u32, while ipv4 and sit expect a u16 attribute, so we have a (first?) subset type override... Reviewed-by: Donald Hunter Acked-by: Stanislav Fomichev Link: https://patch.msgid.link/20250107022820.2087101-4-kuba@kernel.org Signed-off-by: Jakub Kicinski --- Documentation/netlink/specs/rt_link.yaml | 87 ++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/Documentation/netlink/specs/rt_link.yaml b/Documentation/netlink/specs/rt_link.yaml index 2f62d51c26cd..0d492500c7e5 100644 --- a/Documentation/netlink/specs/rt_link.yaml +++ b/Documentation/netlink/specs/rt_link.yaml @@ -1825,6 +1825,48 @@ attribute-sets: - name: erspan-hwid type: u16 + - + name: linkinfo-vti-attrs + name-prefix: ifla-vti- + attributes: + - + name: link + type: u32 + - + name: ikey + type: u32 + - + name: okey + type: u32 + - + name: local + type: binary + display-hint: ipv4 + - + name: remote + type: binary + display-hint: ipv4 + - + name: fwmark + type: u32 + - + name: linkinfo-vti6-attrs + subset-of: linkinfo-vti-attrs + attributes: + - + name: link + - + name: ikey + - + name: okey + - + name: local + display-hint: ipv6 + - + name: remote + display-hint: ipv6 + - + name: fwmark - name: linkinfo-geneve-attrs name-prefix: ifla-geneve- @@ -1941,6 +1983,42 @@ attribute-sets: - name: fwmark type: u32 + - + name: linkinfo-ip6tnl-attrs + subset-of: linkinfo-iptun-attrs + attributes: + - + name: link + - + name: local + display-hint: ipv6 + - + name: remote + display-hint: ipv6 + - + name: ttl + - + name: encap-limit + - + name: flowinfo + - + name: flags + # ip6tnl unlike ipip and sit has 32b flags + type: u32 + - + name: proto + - + name: encap-type + - + name: encap-flags + - + name: encap-sport + - + name: encap-dport + - + name: collect-metadata + - + name: fwmark - name: linkinfo-tun-attrs name-prefix: ifla-tun- @@ -2201,6 +2279,9 @@ sub-messages: - value: ipip attribute-set: linkinfo-iptun-attrs + - + value: ip6tnl + attribute-set: linkinfo-ip6tnl-attrs - value: sit attribute-set: linkinfo-iptun-attrs @@ -2213,6 +2294,12 @@ sub-messages: - value: vrf attribute-set: linkinfo-vrf-attrs + - + value: vti + attribute-set: linkinfo-vti-attrs + - + value: vti6 + attribute-set: linkinfo-vti6-attrs - value: netkit attribute-set: linkinfo-netkit-attrs