Re: [PATCH 02/15] tools: ynl: Teach pyynl to handle blobs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] _blob_extend incorrectly rejects continuation messages and treats them as new objects if they contain repeated scalar attributes (e.g., object identifiers).
- [Medium] PyYNL crashes with a false length exception or type error if a blob's first chunk arrives in a continuation message instead of the initial message.
- [Low] Blob attribute coalescing is restricted exclusively to dump operations, causing 'do' operations to inconsistently return lists of chunks.
--

commit 6f54aff032c9d8543801051eb2d44c4da3645009
Author: Dan Williams <djbw@kernel.org>

tools: ynl: Teach pyynl to handle blobs

This commit updates pyynl to handle blobs by supporting the "bloblen" 
property, where multiple messages with the same attribute represent chunks 
of a byte array. It coalesces these chunks into a single byte array 
instead of treating them as separate objects.

Link: http://lore.kernel.org/20260318170014.6650d2bf@kernel.org [1]

> diff --git a/tools/net/ynl/pyynl/lib/nlspec.py b/tools/net/ynl/pyynl/lib/nlspec.py
> index 0469a0e270d05..4764263968ea6 100644
> --- a/tools/net/ynl/pyynl/lib/nlspec.py
> +++ b/tools/net/ynl/pyynl/lib/nlspec.py
[ ... ]
> @@ -388,6 +397,9 @@ class SpecOperation(SpecElement):
>              raise SpecException(f"Can't resolve attribute set for op '{self.name}'")
>          if attr_set_name:
>              self.attr_set = self.family.attr_sets[attr_set_name]
> +            reply_attrs = self.yaml.get('dump', {}).get('reply', {}).get('attributes', ())
> +            self.dump_blob_attrs = frozenset(
> +                name for name in reply_attrs if self.attr_set[name].bloblen)

[Severity: Low]
Is blob attribute coalescing meant to be restricted only to dump operations?
Since we only check the dump reply block here, a 'do' operation returning a
multi-attr blob will bypass the coalescing logic entirely and return a list
of bytes chunks instead of a single contiguous bytes object.

> diff --git a/tools/net/ynl/pyynl/lib/ynl.py b/tools/net/ynl/pyynl/lib/ynl.py
> index 092d132edec12..f21b68043bd3c 100644
> --- a/tools/net/ynl/pyynl/lib/ynl.py
> +++ b/tools/net/ynl/pyynl/lib/ynl.py
[ ... ]
> @@ -998,6 +998,47 @@ class YnlFamily(SpecFamily):
>          else:
>              rsp[name] = [decoded]
>  
> +    def _blob_fill(self, buf, off, chunks, name):
> +        for chunk in chunks:
> +            end = off + len(chunk)
> +            if end > len(buf):
> +                raise YnlException(f"Blob '{name}' data exceeds reported length")
> +            buf[off:end] = chunk
> +            off = end
> +        return off
> +
> +    def _blob_init(self, op, rsp, blob_attrs):
> +        """
> +        Start a new blob object. Preallocate each blob payload using the
> +        length carried ahead of the data and copy in the first chunk(s).
> +        Return the per-attribute write offsets for any following messages.
> +        """
> +        offsets = {}
> +        for name in blob_attrs:
> +            if name not in rsp:
> +                continue

[Severity: Medium]
Could this lead to a false length exception or type error if a blob's first
chunk arrives in a continuation message instead of the initial message?

If the initial message contains the bloblen attribute but no data chunks,
_blob_init() skips initializing the buf for that attribute here.

When the first chunk arrives in a subsequent message, _blob_extend() will
fall into its else branch and assign the raw decoded value (a list of byte
chunks) directly to rsp[name]. 

If a third message arrives with more chunks, _blob_extend() passes this list
as buf to _blob_fill(). The call to len(buf) will evaluate to the number of
chunks rather than the byte length, incorrectly triggering the "exceeds
reported length" exception.

> +            length = rsp.get(op.attr_set[name].bloblen, 0)
> +            buf = bytearray(length)
> +            offsets[name] = self._blob_fill(buf, 0, rsp[name], name)
> +            rsp[name] = buf
> +        return offsets
> +
> +    def _blob_extend(self, rsp, update, blob_attrs, offsets):
> +        """
> +        Continue filling the previous object's blob(s) when the current
> +        message carries only blob continuation attributes.
> +        """
> +        if any(name in rsp and name not in blob_attrs for name in update):
> +            return False

[Severity: Medium]
Does this logic incorrectly reject continuation messages and treat them as
new objects if they contain repeated scalar attributes?

When the kernel sends a continuation message in a dump, it typically
includes the object's identifying keys (like dev-id) alongside the blob
chunks. The above check will evaluate to True for these keys because they
are already in rsp (from the first message) and are not blob attributes.

This causes _blob_extend() to return False, treating the continuation
message as a new object and splitting the blob. Would it be better to
verify if the overlapping key values actually differ?

> +
> +        for name, value in update.items():
> +            if name in blob_attrs and name in rsp:
> +                offsets[name] = self._blob_fill(rsp[name], offsets.get(name, 0),
> +                                                value, name)
> +            else:
> +                rsp[name] = value
> +        return True
[ ... ]
> @@ -1356,7 +1397,10 @@ class YnlFamily(SpecFamily):
>          for (method, vals, flags) in ops:
>              op = self.ops[method]
>              msg = self._encode_message(op, vals, flags, req_seq)
> -            reqs_by_seq[req_seq] = (op, vals, msg, flags)
> +            blob_attrs = None
> +            if Netlink.NLM_F_DUMP in flags:
> +                blob_attrs = op.dump_blob_attrs or None

[Severity: Low]
Is there a reason blob attribute coalescing is explicitly disabled if the
request is not a dump operation? This seems to restrict do operations from
returning coalesced blobs, making them fall back to returning lists of
chunks.

> +            reqs_by_seq[req_seq] = (op, vals, msg, flags, blob_attrs)
>              payload += msg
>              req_seq += 1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705220819.2472765-1-djbw@kernel.org?part=2