In nested arrays don't require that the intermediate
attribute type should be a valid attribute type, it
might just be an index or simple 0, it is often not
even used.
See include/net/netlink.h about NLA_NESTED_ARRAY:
> The difference to NLA_NESTED is the structure:
> NLA_NESTED has the nested attributes directly inside
> while an array has the nested attributes at another
> level down and the attribute types directly in the
> nesting don't matter.
Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
tools/net/ynl/pyynl/ynl_gen_c.py | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index e6a84e13ec0a..3c0b158c4da8 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -834,11 +834,12 @@ class TypeArrayNest(Type):
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) {',
- '\tif (ynl_attr_validate(yarg, attr2))',
- '\t\treturn YNL_PARSE_CB_ERROR;',
- f'\tn_{self.c_name}++;',
- '}']
+ 'ynl_attr_for_each_nested(attr2, attr) {']
+ if self.attr['sub-type'] != 'nest':
+ get_lines.append('\tif (ynl_attr_validate(yarg, attr2))')
+ get_lines.append('\t\treturn YNL_PARSE_CB_ERROR;')
+ get_lines.append(f'\tn_{self.c_name}++;')
+ get_lines.append('}')
return get_lines, None, local_vars
def attr_put(self, ri, var):
--
2.51.0
On Thu, 4 Sep 2025 22:01:29 +0000 Asbjørn Sloth Tønnesen wrote:
> In nested arrays don't require that the intermediate
> attribute type should be a valid attribute type, it
> might just be an index or simple 0, it is often not
> even used.
>
> See include/net/netlink.h about NLA_NESTED_ARRAY:
> > The difference to NLA_NESTED is the structure:
> > NLA_NESTED has the nested attributes directly inside
> > while an array has the nested attributes at another
> > level down and the attribute types directly in the
> > nesting don't matter.
I don't understand, please provide more details.
This is an ArrayNest, right?
[ARRAY-ATTR]
[ENTRY]
[MEMBER1]
[MEMBER2]
[ENTRY]
[MEMBER1]
[MEMBER2]
Which level are you saying doesn't matter?
If entry is a nest it must be a valid nest.
What the comment you're quoting is saying is that the nla_type of ENTRY
doesn't matter.
On 9/6/25 12:23 AM, Jakub Kicinski wrote: > On Thu, 4 Sep 2025 22:01:29 +0000 Asbjørn Sloth Tønnesen wrote: >> In nested arrays don't require that the intermediate >> attribute type should be a valid attribute type, it >> might just be an index or simple 0, it is often not >> even used. >> >> See include/net/netlink.h about NLA_NESTED_ARRAY: >>> The difference to NLA_NESTED is the structure: >>> NLA_NESTED has the nested attributes directly inside >>> while an array has the nested attributes at another >>> level down and the attribute types directly in the >>> nesting don't matter. > > I don't understand, please provide more details. > This is an ArrayNest, right? > > [ARRAY-ATTR] > [ENTRY] > [MEMBER1] > [MEMBER2] > [ENTRY] > [MEMBER1] > [MEMBER2] > > Which level are you saying doesn't matter? > If entry is a nest it must be a valid nest. > What the comment you're quoting is saying is that the nla_type of ENTRY > doesn't matter. I will expand this in v2, but the gist of it is that this is part of the "split attribute counting, and later allocating an array to hold them" code. The check that I remove for nested arrays, is an early exit during the counting phase. Later in the allocation and parse phase it validates the nested payload. In include/uapi/linux/wireguard.h: > WGDEVICE_A_PEERS: NLA_NESTED > 0: NLA_NESTED > WGPEER_A_PUBLIC_KEY: NLA_EXACT_LEN, len WG_KEY_LEN > [..] > 0: NLA_NESTED > ... > ... The current check requires that the nested type is valid in the nested attribute set, which in this case resolves to WGDEVICE_A_UNSPEC, which is YNL_PT_REJECT, and it takes the early exit and returns YNL_PARSE_CB_ERROR.
On Sat, 6 Sep 2025 13:22:01 +0000 Asbjørn Sloth Tønnesen wrote:
> > I don't understand, please provide more details.
> > This is an ArrayNest, right?
> >
> > [ARRAY-ATTR]
> > [ENTRY]
> > [MEMBER1]
> > [MEMBER2]
> > [ENTRY]
> > [MEMBER1]
> > [MEMBER2]
> >
> > Which level are you saying doesn't matter?
> > If entry is a nest it must be a valid nest.
> > What the comment you're quoting is saying is that the nla_type of ENTRY
> > doesn't matter.
>
> I will expand this in v2, but the gist of it is that this is part of the
> "split attribute counting, and later allocating an array to hold them" code.
>
> The check that I remove for nested arrays, is an early exit during the
> counting phase. Later in the allocation and parse phase it validates the
> nested payload.
>
> In include/uapi/linux/wireguard.h:
> > WGDEVICE_A_PEERS: NLA_NESTED
> > 0: NLA_NESTED
> > WGPEER_A_PUBLIC_KEY: NLA_EXACT_LEN, len WG_KEY_LEN
> > [..]
> > 0: NLA_NESTED
> > ...
> > ...
>
> The current check requires that the nested type is valid in the nested
> attribute set, which in this case resolves to WGDEVICE_A_UNSPEC, which is
> YNL_PT_REJECT, and it takes the early exit and returns YNL_PARSE_CB_ERROR.
I see your point now. We're validating ENTRY as an attribute in the
parent attribute set, but it's just a meaningless id.
I think we need more fixing here. The real parsing loop will only
validate what's _inside_ the [MEMBER]. Which doesn't matter all
that much to nests, but look at what happens if subtype is a scalar.
We'll just call ynl_attr_get_u32(), type is never really validate.
I think we need this, and make the codegen feed in the ARRAY-ATTR type
to validate ENTRY?
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 2a169c3c0797..e43167398c69 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -360,15 +360,15 @@ static int ynl_cb_done(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
/* Attribute validation */
-int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
+int __ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr,
+ unsigned int type)
{
const struct ynl_policy_attr *policy;
- unsigned int type, len;
unsigned char *data;
+ unsigned int len;
data = ynl_attr_data(attr);
len = ynl_attr_data_len(attr);
- type = ynl_attr_type(attr);
if (type > yarg->rsp_policy->max_attr) {
yerr(yarg->ys, YNL_ERROR_INTERNAL,
"Internal error, validating unknown attribute");
@@ -450,6 +450,11 @@ int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
return 0;
}
+int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
+{
+ return __ynl_attr_validate(yarg, attr, ynl_attr_type(attr));
+}
+
int ynl_submsg_failed(struct ynl_parse_arg *yarg, const char *field_name,
const char *sel_name)
{
On 9/4/2025 3:01 PM, Asbjørn Sloth Tønnesen wrote:
> In nested arrays don't require that the intermediate
> attribute type should be a valid attribute type, it
> might just be an index or simple 0, it is often not
> even used.
>
> See include/net/netlink.h about NLA_NESTED_ARRAY:
>> The difference to NLA_NESTED is the structure:
>> NLA_NESTED has the nested attributes directly inside
>> while an array has the nested attributes at another
>> level down and the attribute types directly in the
>> nesting don't matter.
>
To me, it would seem like it makes more sense to define these (even if
thats defined per family?) than to just say they aren't defined at all?
Hm.
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
> tools/net/ynl/pyynl/ynl_gen_c.py | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
> index e6a84e13ec0a..3c0b158c4da8 100755
> --- a/tools/net/ynl/pyynl/ynl_gen_c.py
> +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
> @@ -834,11 +834,12 @@ class TypeArrayNest(Type):
> 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) {',
> - '\tif (ynl_attr_validate(yarg, attr2))',
> - '\t\treturn YNL_PARSE_CB_ERROR;',
> - f'\tn_{self.c_name}++;',
> - '}']
> + 'ynl_attr_for_each_nested(attr2, attr) {']
> + if self.attr['sub-type'] != 'nest':
> + get_lines.append('\tif (ynl_attr_validate(yarg, attr2))')
> + get_lines.append('\t\treturn YNL_PARSE_CB_ERROR;')
> + get_lines.append(f'\tn_{self.c_name}++;')
> + get_lines.append('}')
> return get_lines, None, local_vars
>
> def attr_put(self, ri, var):
CC: Johannes
On 9/6/25 12:24 AM, Jacob Keller wrote:
> On 9/4/2025 3:01 PM, Asbjørn Sloth Tønnesen wrote:
>> In nested arrays don't require that the intermediate
>> attribute type should be a valid attribute type, it
>> might just be an index or simple 0, it is often not
>> even used.
>>
>> See include/net/netlink.h about NLA_NESTED_ARRAY:
>>> The difference to NLA_NESTED is the structure:
>>> NLA_NESTED has the nested attributes directly inside
>>> while an array has the nested attributes at another
>>> level down and the attribute types directly in the
>>> nesting don't matter.
>>
>
> To me, it would seem like it makes more sense to define these (even if
> thats defined per family?) than to just say they aren't defined at all?
>
> Hm.
I considered adding some of that metadata too, as I am actually removing
it for wireguard (in comment form, but still).
In include/uapi/linux/wireguard.h in the comment block at the top, it is
very clear that wireguard only used type 0 for all the nested array
entries, however the truth is that it doesn't care. It therefore doesn't
matter if the generated -user.* keeps track of the index in .idx, or that
cli.py decodes a JSON array and sends it with indexes, it's not needed,
but it still works.
In practice I don't think we will break any clients if we enforced it, and
validated that wireguard only accepts type 0 entries, in it's nested arrays.
For the other families, I don't know how well defined it is, Johannes have
stated that nl80211 doesn't care which types are used, but I have no idea
how consistent clients have abused that statement to send random data,
or do they all just send zeros?
This would make a lot more sense if 'array-nest' hadn't been renamed to
'indexed-array' in ynl, because it feels wrong to add 'unindexed: true' now.
We could also call it 'all-zero-indexed: true'.
In cli.py this gives some extra issues, as seen in [1], the nested arrays
are outputted as '[{0: {..}}, {0: {..}}, ..]', but on input has the format
'[{..},{..}, ..]' because it has to be JSON-compatible on input.
If we had an attribute like 'all-zero-indexed' then cli.py, could also output
'[{..},{..}, ..]'.
[1] https://lore.kernel.org/netdev/20250904220255.1006675-3-ast@fiberby.net/
On 9/6/2025 8:10 AM, Asbjørn Sloth Tønnesen wrote:
> CC: Johannes
>
> On 9/6/25 12:24 AM, Jacob Keller wrote:
>> On 9/4/2025 3:01 PM, Asbjørn Sloth Tønnesen wrote:
>>> In nested arrays don't require that the intermediate
>>> attribute type should be a valid attribute type, it
>>> might just be an index or simple 0, it is often not
>>> even used.
>>>
>>> See include/net/netlink.h about NLA_NESTED_ARRAY:
>>>> The difference to NLA_NESTED is the structure:
>>>> NLA_NESTED has the nested attributes directly inside
>>>> while an array has the nested attributes at another
>>>> level down and the attribute types directly in the
>>>> nesting don't matter.
>>>
>>
>> To me, it would seem like it makes more sense to define these (even if
>> thats defined per family?) than to just say they aren't defined at all?
>>
>> Hm.
>
> I considered adding some of that metadata too, as I am actually removing
> it for wireguard (in comment form, but still).
>
> In include/uapi/linux/wireguard.h in the comment block at the top, it is
> very clear that wireguard only used type 0 for all the nested array
> entries, however the truth is that it doesn't care. It therefore doesn't
> matter if the generated -user.* keeps track of the index in .idx, or that
> cli.py decodes a JSON array and sends it with indexes, it's not needed,
> but it still works.
>
> In practice I don't think we will break any clients if we enforced it, and
> validated that wireguard only accepts type 0 entries, in it's nested arrays.
>
> For the other families, I don't know how well defined it is, Johannes have
> stated that nl80211 doesn't care which types are used, but I have no idea
> how consistent clients have abused that statement to send random data,
> or do they all just send zeros?
>
Changing it at this point could be a significant backwards compat break,
as some clients might somehow send data that wasn't zero-initialized,
and checking would break them. At this point I guess it makes sense to
leave it as is... It would increase code cost and complexity for no gain.
> This would make a lot more sense if 'array-nest' hadn't been renamed to
> 'indexed-array' in ynl, because it feels wrong to add 'unindexed: true' now.
> We could also call it 'all-zero-indexed: true'.
>
> In cli.py this gives some extra issues, as seen in [1], the nested arrays
> are outputted as '[{0: {..}}, {0: {..}}, ..]', but on input has the format
> '[{..},{..}, ..]' because it has to be JSON-compatible on input.
>
> If we had an attribute like 'all-zero-indexed' then cli.py, could also output
> '[{..},{..}, ..]'.
>
This part would be cool. If we know the index is "uninteresting",
eliding it so that the input and output formats match is good.
> [1] https://lore.kernel.org/netdev/20250904220255.1006675-3-ast@fiberby.net/
On Sat, 2025-09-06 at 15:10 +0000, Asbjørn Sloth Tønnesen wrote: > > For the other families, I don't know how well defined it is, Johannes have > stated that nl80211 doesn't care which types are used, but I have no idea > how consistent clients have abused that statement to send random data, > or do they all just send zeros? I think most clients probably send incrementing numbers (1, 2, 3, ...), but maybe some start at 0, some sometimes might use band numbers and thus have sparse values, etc. But as I just wrote, I'm not really sure it should be used at all. joahnnes
© 2016 - 2025 Red Hat, Inc.