[PATCH net-next v1 0/4] tools/net/ynl: Add support for nlctrl netlink family

[PATCH net-next v1 0/4] tools/net/ynl: Add support for nlctrl netlink family

[Donald Hunter]

This series adds a new YNL spec for the nlctrl family, plus some fixes
and enhancements for ynl.

Patch 1 fixes an extack decoding bug
Patch 2 gives cleaner netlink error reporting
Patch 3 adds multi-level array-nest for nlctrl
Patch 4 contains the nlctrl spec

Donald Hunter (4):
  tools/net/ynl: Fix extack decoding for netlink-raw
  tools/net/ynl: Report netlink errors without stacktrace
  tools/net/ynl: Extend array-nest for multi level nesting
  doc/netlink/specs: Add spec for nlctrl netlink family

 Documentation/netlink/genetlink-legacy.yaml |   3 +
 Documentation/netlink/specs/nlctrl.yaml     | 191 ++++++++++++++++++++
 tools/net/ynl/cli.py                        |  18 +-
 tools/net/ynl/lib/__init__.py               |   4 +-
 tools/net/ynl/lib/nlspec.py                 |   2 +
 tools/net/ynl/lib/ynl.py                    |  16 +-
 6 files changed, 221 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/netlink/specs/nlctrl.yaml

-- 
2.42.0

[PATCH net-next v1 1/4] tools/net/ynl: Fix extack decoding for netlink-raw

[Donald Hunter]

Extack decoding was using a hard-coded msg header size of 20 but
netlink-raw has a header size of 16.

Use a protocol specific msghdr_size() when decoding the attr offssets.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/lib/ynl.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index ac55aa5a3083..29262505a3f2 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -348,6 +348,9 @@ class NetlinkProtocol:
             raise Exception(f'Multicast group "{mcast_name}" not present in the spec')
         return mcast_groups[mcast_name].value
 
+    def msghdr_size(self):
+        return 16
+
 
 class GenlProtocol(NetlinkProtocol):
     def __init__(self, family_name):
@@ -373,6 +376,8 @@ class GenlProtocol(NetlinkProtocol):
             raise Exception(f'Multicast group "{mcast_name}" not present in the family')
         return self.genl_family['mcast'][mcast_name]
 
+    def msghdr_size(self):
+        return super().msghdr_size() + 4
 
 
 class SpaceAttrs:
@@ -693,7 +698,7 @@ class YnlFamily(SpecFamily):
             return
 
         msg = self.nlproto.decode(self, NlMsg(request, 0, op.attr_set))
-        offset = 20 + self._struct_size(op.fixed_header)
+        offset = self.nlproto.msghdr_size() + self._struct_size(op.fixed_header)
         path = self._decode_extack_path(msg.raw_attrs, op.attr_set, offset,
                                         extack['bad-attr-offs'])
         if path:
-- 
2.42.0

[PATCH net-next v1 2/4] tools/net/ynl: Report netlink errors without stacktrace

[Donald Hunter]

ynl does not handle NlError exceptions so they get reported like program
failures. Handle the NlError exceptions and report the netlink errors
more cleanly.

Example now:

Netlink error: No such file or directory
nl_len = 44 (28) nl_flags = 0x300 nl_type = 2
	error: -2	extack: {'bad-attr': '.op'}

Example before:

Traceback (most recent call last):
  File "/home/donaldh/net-next/./tools/net/ynl/cli.py", line 81, in <module>
    main()
  File "/home/donaldh/net-next/./tools/net/ynl/cli.py", line 69, in main
    reply = ynl.dump(args.dump, attrs)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/donaldh/net-next/tools/net/ynl/lib/ynl.py", line 906, in dump
    return self._op(method, vals, [], dump=True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/donaldh/net-next/tools/net/ynl/lib/ynl.py", line 872, in _op
    raise NlError(nl_msg)
lib.ynl.NlError: Netlink error: No such file or directory
nl_len = 44 (28) nl_flags = 0x300 nl_type = 2
	error: -2	extack: {'bad-attr': '.op'}

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/cli.py          | 18 +++++++++++-------
 tools/net/ynl/lib/__init__.py |  4 ++--
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/tools/net/ynl/cli.py b/tools/net/ynl/cli.py
index 0f8239979670..cccf4d801b76 100755
--- a/tools/net/ynl/cli.py
+++ b/tools/net/ynl/cli.py
@@ -6,7 +6,7 @@ import json
 import pprint
 import time
 
-from lib import YnlFamily, Netlink
+from lib import YnlFamily, Netlink, NlError
 
 
 class YnlEncoder(json.JSONEncoder):
@@ -61,12 +61,16 @@ def main():
     if args.sleep:
         time.sleep(args.sleep)
 
-    if args.do:
-        reply = ynl.do(args.do, attrs, args.flags)
-        output(reply)
-    if args.dump:
-        reply = ynl.dump(args.dump, attrs)
-        output(reply)
+    try:
+        if args.do:
+            reply = ynl.do(args.do, attrs, args.flags)
+            output(reply)
+        if args.dump:
+            reply = ynl.dump(args.dump, attrs)
+            output(reply)
+    except NlError as e:
+        print(e)
+        exit(1)
 
     if args.ntf:
         ynl.check_ntf()
diff --git a/tools/net/ynl/lib/__init__.py b/tools/net/ynl/lib/__init__.py
index f7eaa07783e7..9137b83e580a 100644
--- a/tools/net/ynl/lib/__init__.py
+++ b/tools/net/ynl/lib/__init__.py
@@ -2,7 +2,7 @@
 
 from .nlspec import SpecAttr, SpecAttrSet, SpecEnumEntry, SpecEnumSet, \
     SpecFamily, SpecOperation
-from .ynl import YnlFamily, Netlink
+from .ynl import YnlFamily, Netlink, NlError
 
 __all__ = ["SpecAttr", "SpecAttrSet", "SpecEnumEntry", "SpecEnumSet",
-           "SpecFamily", "SpecOperation", "YnlFamily", "Netlink"]
+           "SpecFamily", "SpecOperation", "YnlFamily", "Netlink", "NlError"]
-- 
2.42.0

[PATCH net-next v1 3/4] tools/net/ynl: Extend array-nest for multi level nesting

[Donald Hunter]

The nlctrl family uses 2 levels of array nesting for policy attributes.
Add a 'nest-depth' property to genetlink-legacy and extend ynl to use
it.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/genetlink-legacy.yaml | 3 +++
 tools/net/ynl/lib/nlspec.py                 | 2 ++
 tools/net/ynl/lib/ynl.py                    | 9 ++++++---
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index 938703088306..872c76065f1b 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -261,6 +261,9 @@ properties:
               struct:
                 description: Name of the struct type used for the attribute.
                 type: string
+              nest-depth:
+                description: Depth of nesting for an array-nest, defaults to 1.
+                type: integer
               # End genetlink-legacy
 
       # Make sure name-prefix does not appear in subsets (subsets inherit naming)
diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index fbce52395b3b..50e8447f089a 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -161,6 +161,7 @@ class SpecAttr(SpecElement):
         sub_message   string, name of sub message type
         selector      string, name of attribute used to select
                       sub-message type
+        nest_depth    integer, depth of array nesting
 
         is_auto_scalar bool, attr is a variable-size scalar
     """
@@ -178,6 +179,7 @@ class SpecAttr(SpecElement):
         self.display_hint = yaml.get('display-hint')
         self.sub_message = yaml.get('sub-message')
         self.selector = yaml.get('selector')
+        self.nest_depth = yaml.get('nest-depth', 1)
 
         self.is_auto_scalar = self.type == "sint" or self.type == "uint"
 
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 29262505a3f2..efceea9433f2 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -556,14 +556,17 @@ class YnlFamily(SpecFamily):
                 decoded = self._formatted_string(decoded, attr_spec.display_hint)
         return decoded
 
-    def _decode_array_nest(self, attr, attr_spec):
+    def _decode_array_nest(self, attr, attr_spec, depth):
         decoded = []
         offset = 0
         while offset < len(attr.raw):
             item = NlAttr(attr.raw, offset)
             offset += item.full_len
 
-            subattrs = self._decode(NlAttrs(item.raw), attr_spec['nested-attributes'])
+            if depth > 1:
+                subattrs = self._decode_array_nest(item, attr_spec, depth - 1)
+            else:
+                subattrs = self._decode(NlAttrs(item.raw), attr_spec['nested-attributes'])
             decoded.append({ item.type: subattrs })
         return decoded
 
@@ -649,7 +652,7 @@ class YnlFamily(SpecFamily):
                 if 'enum' in attr_spec:
                     decoded = self._decode_enum(decoded, attr_spec)
             elif attr_spec["type"] == 'array-nest':
-                decoded = self._decode_array_nest(attr, attr_spec)
+                decoded = self._decode_array_nest(attr, attr_spec, attr_spec.nest_depth)
             elif attr_spec["type"] == 'bitfield32':
                 value, selector = struct.unpack("II", attr.raw)
                 if 'enum' in attr_spec:
-- 
2.42.0

Re: [PATCH net-next v1 3/4] tools/net/ynl: Extend array-nest for multi level nesting

[Jakub Kicinski]

On Fri,  1 Mar 2024 17:14:30 +0000 Donald Hunter wrote:
> The nlctrl family uses 2 levels of array nesting for policy attributes.
> Add a 'nest-depth' property to genetlink-legacy and extend ynl to use
> it.

Hm, I'm 90% sure we don't need this... because nlctrl is basically what
the legacy level was written for, initially. The spec itself wasn't
sent, because the C codegen for it was quite painful. And the Python
CLI was an afterthought.

Could you describe what nesting you're trying to cover here?
Isn't it a type-value?

BTW we'll also need to deal with the C codegen situation somehow.
Try making it work, if it's not a simple matter of fixing up the 
names to match the header - we can grep nlctrl out in the Makefile.

Re: [PATCH net-next v1 3/4] tools/net/ynl: Extend array-nest for multi level nesting

[Donald Hunter]

On Sun, 3 Mar 2024 at 04:05, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri,  1 Mar 2024 17:14:30 +0000 Donald Hunter wrote:
> > The nlctrl family uses 2 levels of array nesting for policy attributes.
> > Add a 'nest-depth' property to genetlink-legacy and extend ynl to use
> > it.
>
> Hm, I'm 90% sure we don't need this... because nlctrl is basically what
> the legacy level was written for, initially. The spec itself wasn't
> sent, because the C codegen for it was quite painful. And the Python
> CLI was an afterthought.
>
> Could you describe what nesting you're trying to cover here?
> Isn't it a type-value?

I added it for getpolicy which is indexed by policy_idx and attr_idx.

./tools/net/ynl/cli.py \
    --spec Documentation/netlink/specs/nlctrl.yaml \
    --dump getpolicy --json '{"family-name": "nlctrl"}'
[{'family-id': 16, 'op-policy': [{3: {'do': 0, 'dump': 0}}]},
 {'family-id': 16, 'op-policy': [{0: {'dump': 1}}]},
 {'family-id': 16,
  'policy': [{0: [{1: {'max-value-u': 65535,
                       'min-value-u': 0,
                       'type': 'u16'}}]}]},
 {'family-id': 16,
  'policy': [{0: [{2: {'max-length': 15, 'type': 'nul-string'}}]}]},
 {'family-id': 16,
  'policy': [{1: [{1: {'max-value-u': 65535,
                       'min-value-u': 0,
                       'type': 'u16'}}]}]},
 {'family-id': 16,
  'policy': [{1: [{2: {'max-length': 15, 'type': 'nul-string'}}]}]},
 {'family-id': 16,
  'policy': [{1: [{10: {'max-value-u': 4294967295,
                        'min-value-u': 0,
                        'type': 'u32'}}]}]}]

> BTW we'll also need to deal with the C codegen situation somehow.
> Try making it work, if it's not a simple matter of fixing up the
> names to match the header - we can grep nlctrl out in the Makefile.

Yeah, I forgot to check codegen but saw the failures on patchwork. I
have fixed the names but still have a couple more things to fix.

BTW, this patchset was a step towards experimenting with removing the
hard-coded msg decoding in the Python library. Not so much for
genetlink families, more for the extack decoding so that I could add
policy attr decoding. Thinking about it some more, that might be
better done with a "core" spec that contains just extack-attrs and
policy-attrs because they don't belong to any single family - they're
kinda infrastructure for all families.

Re: [PATCH net-next v1 3/4] tools/net/ynl: Extend array-nest for multi level nesting

[Jakub Kicinski]

On Sun, 3 Mar 2024 10:50:09 +0000 Donald Hunter wrote:
> On Sun, 3 Mar 2024 at 04:05, Jakub Kicinski <kuba@kernel.org> wrote:
> > On Fri,  1 Mar 2024 17:14:30 +0000 Donald Hunter wrote:  
> > > The nlctrl family uses 2 levels of array nesting for policy attributes.
> > > Add a 'nest-depth' property to genetlink-legacy and extend ynl to use
> > > it.  
> >
> > Hm, I'm 90% sure we don't need this... because nlctrl is basically what
> > the legacy level was written for, initially. The spec itself wasn't
> > sent, because the C codegen for it was quite painful. And the Python
> > CLI was an afterthought.
> >
> > Could you describe what nesting you're trying to cover here?
> > Isn't it a type-value?  
> 
> I added it for getpolicy which is indexed by policy_idx and attr_idx.
> 
> ./tools/net/ynl/cli.py \
>     --spec Documentation/netlink/specs/nlctrl.yaml \
>     --dump getpolicy --json '{"family-name": "nlctrl"}'
> [{'family-id': 16, 'op-policy': [{3: {'do': 0, 'dump': 0}}]},
>  {'family-id': 16, 'op-policy': [{0: {'dump': 1}}]},
>  {'family-id': 16,
>   'policy': [{0: [{1: {'max-value-u': 65535,
>                        'min-value-u': 0,
>                        'type': 'u16'}}]}]},
>  {'family-id': 16,
>   'policy': [{0: [{2: {'max-length': 15, 'type': 'nul-string'}}]}]},
>  {'family-id': 16,
>   'policy': [{1: [{1: {'max-value-u': 65535,
>                        'min-value-u': 0,
>                        'type': 'u16'}}]}]},
>  {'family-id': 16,
>   'policy': [{1: [{2: {'max-length': 15, 'type': 'nul-string'}}]}]},
>  {'family-id': 16,
>   'policy': [{1: [{10: {'max-value-u': 4294967295,
>                         'min-value-u': 0,
>                         'type': 'u32'}}]}]}]

Yeah.. look at the example I used for type-value :)

https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#type-value

> > BTW we'll also need to deal with the C codegen situation somehow.
> > Try making it work, if it's not a simple matter of fixing up the
> > names to match the header - we can grep nlctrl out in the Makefile.  
> 
> Yeah, I forgot to check codegen but saw the failures on patchwork. I
> have fixed the names but still have a couple more things to fix.
> 
> BTW, this patchset was a step towards experimenting with removing the
> hard-coded msg decoding in the Python library. Not so much for
> genetlink families, more for the extack decoding so that I could add
> policy attr decoding. Thinking about it some more, that might be
> better done with a "core" spec that contains just extack-attrs and
> policy-attrs because they don't belong to any single family - they're
> kinda infrastructure for all families.

YAML specs describe information on how to parse data YNL doesn't have
to understand, just format correctly. The base level of netlink
processing, applicable to all families, is a different story.
I think hand-coding that is more than okay. The goal is not to express
everything in YAML but to avoid duplicated work per family, if that
makes sense.

Re: [PATCH net-next v1 3/4] tools/net/ynl: Extend array-nest for multi level nesting

[Donald Hunter]

On Mon, 4 Mar 2024 at 15:22, Jakub Kicinski <kuba@kernel.org> wrote:
>
> Yeah.. look at the example I used for type-value :)
>
> https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#type-value

Apologies, I totally missed this. Is the intended usage something like this:

      -
        name: policy
        type: nest-type-value
        type-value: [ policy-id, attr-id ]
        nested-attributes: policy-attrs

> YAML specs describe information on how to parse data YNL doesn't have
> to understand, just format correctly. The base level of netlink
> processing, applicable to all families, is a different story.
> I think hand-coding that is more than okay. The goal is not to express
> everything in YAML but to avoid duplicated work per family, if that
> makes sense.

Okay, I can go ahead and hard-code the policy attr decoding for extack messages.

Re: [PATCH net-next v1 3/4] tools/net/ynl: Extend array-nest for multi level nesting

[Jakub Kicinski]

On Mon, 4 Mar 2024 16:21:11 +0000 Donald Hunter wrote:
> > Yeah.. look at the example I used for type-value :)
> >
> > https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#type-value  
> 
> Apologies, I totally missed this. Is the intended usage something like this:
> 
>       -
>         name: policy
>         type: nest-type-value
>         type-value: [ policy-id, attr-id ]
>         nested-attributes: policy-attrs

Yes, I believe that'd be the right way. And then the policy-id and
attr-id are used as keys in the output / response dict.

[PATCH net-next v1 4/4] doc/netlink/specs: Add spec for nlctrl netlink family

[Donald Hunter]

Add a spec for the nlctrl family.

Example usage:

./tools/net/ynl/cli.py \
    --spec Documentation/netlink/specs/nlctrl.yaml \
    --do getfamily --json '{"family-name": "nlctrl"}'

./tools/net/ynl/cli.py \
    --spec Documentation/netlink/specs/nlctrl.yaml \
    --dump getpolicy --json '{"family-name": "nlctrl"}'

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/specs/nlctrl.yaml | 191 ++++++++++++++++++++++++
 1 file changed, 191 insertions(+)
 create mode 100644 Documentation/netlink/specs/nlctrl.yaml

diff --git a/Documentation/netlink/specs/nlctrl.yaml b/Documentation/netlink/specs/nlctrl.yaml
new file mode 100644
index 000000000000..9b35a390ae96
--- /dev/null
+++ b/Documentation/netlink/specs/nlctrl.yaml
@@ -0,0 +1,191 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: nlctrl
+
+protocol: genetlink-legacy
+
+doc: |
+  Genetlink control.
+
+definitions:
+  -
+    name: op-flags
+    type: flags
+    entries:
+      - admin-perm
+      - cmd-cap-do
+      - cmd-cap-dump
+      - cmd-cap-haspol
+      - uns-admin-perm
+  -
+    name: attr-type
+    type: enum
+    entries:
+      - flag
+      - u8
+      - u16
+      - u32
+      - u64
+      - s8
+      - s16
+      - s32
+      - s64
+      - binary
+      - string
+      - nul-string
+      - nested
+      - nested-array
+      - bitfield32
+      - sint
+      - uint
+
+attribute-sets:
+  -
+    name: ctrl-attrs
+    attributes:
+      -
+        name: family-id
+        type: u16
+      -
+        name: family-name
+        type: string
+      -
+        name: version
+        type: u32
+      -
+        name: hdrsize
+        type: u32
+      -
+        name: maxattr
+        type: u32
+      -
+        name: ops
+        type: array-nest
+        nested-attributes: op-attrs
+      -
+        name: mcast-groups
+        type: array-nest
+        nested-attributes: mcast-group-attrs
+      -
+        name: policy
+        type: array-nest
+        nest-depth: 2
+        nested-attributes: policy-attrs
+      -
+        name: op-policy
+        type: array-nest
+        nested-attributes: op-policy-attrs
+      -
+        name: op
+        type: u32
+  -
+    name: mcast-group-attrs
+    attributes:
+      -
+        name: name
+        type: string
+      -
+        name: id
+        type: u32
+  -
+    name: op-attrs
+    attributes:
+      -
+        name: id
+        type: u32
+      -
+        name: flags
+        type: u32
+        enum: op-flags
+        enum-as-flags: true
+  -
+    name: policy-attrs
+    attributes:
+      -
+        name: type
+        type: u32
+        enum: attr-type
+      -
+        name: min-value-s
+        type: s64
+      -
+        name: max-value-s
+        type: s64
+      -
+        name: min-value-u
+        type: u64
+      -
+        name: max-value-u
+        type: u64
+      -
+        name: min-length
+        type: u32
+      -
+        name: max-length
+        type: u32
+      -
+        name: policy-idx
+        type: u32
+      -
+        name: policy-maxtype
+        type: u32
+      -
+        name: bitfield32-mask
+        type: u32
+      -
+        name: mask
+        type: u64
+      -
+        name: pad
+        type: pad
+  -
+    name: op-policy-attrs
+    attributes:
+      -
+        name: do
+        type: u32
+      -
+        name: dump
+        type: u32
+
+operations:
+  enum-model: directional
+  list:
+    -
+      name: getfamily
+      doc: Get / dump genetlink families
+      attribute-set: ctrl-attrs
+      do:
+        request:
+          value: 3
+          attributes:
+            - family-name
+        reply: &all_attrs
+          value: 1
+          attributes:
+            - family-id
+            - family-name
+            - hdrsize
+            - maxattr
+            - mcast-groups
+            - ops
+            - version
+      dump:
+        reply: *all_attrs
+    -
+      name: getpolicy
+      doc: Get / dump genetlink policies
+      attribute-set: ctrl-attrs
+      dump:
+        request:
+          value: 10
+          attributes:
+            - family-name
+            - family-id
+            - op
+        reply:
+          value: 10
+          attributes:
+            - family-id
+            - op-policy
+            - policy
-- 
2.42.0