Re: [PATCH RFC v2 01/15] ctlv: Add the schema language and generator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issues to consider:
- [Low] ctlv: compat: broken $include and shared field reconstruction in
  _foreign()
- [Low] ctlv: semantics: filled extension point accepted as normal
  record in _named()
- [Low] ctlv: semantics: order-dependent false record cycle error in
  _held()
- [Low] ctlv: spec: unresolved relative Path parent traversal in Paths
  and load()
- [Low] ctlv: semantics: single-pass _check_fills() misses nested
  extension sites
- [Low] ctlv: spec: _blob() accepts empty argument as valid backing list
- [Low] ctlv: missing composed size and fill depth checks in
  _check_composed()
--

commit c7763c8379c0de88c3274d5b9d07a09e53102368
Author: Jiri Pirko <jiri@resnulli.us>

ctlv: Add the schema language and generator

This commit adds the YAML meta-schemas for CTLV family specifications, the
family registry, and generated-file manifests, along with the ctlvgen tool
to validate specifications, check ABI compatibility, and emit UAPI and
kernel C files.

> diff --git a/tools/ctlv/ctlvgen/lib/compat.py b/tools/ctlv/ctlvgen/lib/compat.py
> new file mode 100644
> index 0000000000000..76bd14f60750b
> --- /dev/null
> +++ b/tools/ctlv/ctlvgen/lib/compat.py
[ ... ]
> +def _foreign(family: semantics.Family, record: semantics.Record,
> +             own: set[int]) -> dict[str, bool]:
> +    """What a record has that it does not declare: placed records and includes.
> +
> +    An include is one member, optional when it made a field optional that the
> +    record it splices requires.
> +    """
> +    members = {}
> +    for member in record.members:
> +        field = member.field
> +        if field.type in own:
> +            continue
> +        if field.shared:
> +            members[f'record {field.name}'] = member.required
> +            continue
> +        key = f'$include {field.record}'
> +        source = family.records[field.record]
> +        declared = any(item.required for item in source.members
> +                       if item.field.type == field.type)
> +        members.setdefault(key, True)
> +        if declared and not member.required:
> +            members[key] = False
> +    return members

[Severity: Low]
Does reconstructing foreign members from the flattened record.members
corrupt ABI snapshots and trigger false ABI breaks?

Because _include() splices target.members directly into the including
record while keeping each field's original field.record and field.shared
values, if record A places a shared field f or includes record C and
record B has "$include: A", _foreign() records "record f" or "$include C"
in B's snapshot instead of "$include A".

In addition, _foreign() infers whether "$include X" was written with "?"
by checking if any field in X is required while its spliced member is
optional.  If a request record has "$include: X?" and the last required
field in X is later relaxed to optional, declared becomes False for every
field of X, so members['$include X'] flips from False to True and
_compare_members() reports a false "$include X became required" ABI break.

> diff --git a/tools/ctlv/ctlvgen/lib/semantics.py b/tools/ctlv/ctlvgen/lib/semantics.py
> new file mode 100644
> index 0000000000000..86725e7c1d693
> --- /dev/null
> +++ b/tools/ctlv/ctlvgen/lib/semantics.py
[ ... ]
> +    def _named(self, name: str, where: str | None) -> Record:
> +        """Return the named record or field of records: called name."""
> +        if name in self.named:
> +            return self.named[name]
> +        if name not in self.declared:
> +            if name in self.extensions:
> +                raise CtlvError(where, f'{name} is an extension point, which '
> +                                       'extension names')
> +            raise CtlvError(where, f'no record is called {name}')

[Severity: Low]
Can a filled extension point be referenced as a normal named record via
"record <name>" or "$include: <name>"?

When a descendant family fills an inherited extension point in records:,
_add_records() adds that name to self.declared and self.named.  Because
_named() only checks self.extensions when name is absent from self.named
and self.declared, referencing the extension point via "record <name>" or
"$include: <name>" in the filling family or a further descendant bypasses
the self.extensions check and treats the fill as a regular reusable
record.

> +        if name in self.building:
> +            raise CtlvError(where, 'record cycle: ' +
> +                                   ' -> '.join(self.building + (name,)))
[ ... ]
> +    def _held(self, name: str, where: str) -> str:
> +        """Return the path of the named record a field holds."""
> +        self.used.add(name)
> +        if name in self.building:
> +            # A record holding itself, which the tree re-enters.
> +            is_field = self.declared[name][3].body is None
> +        else:
> +            is_field = self._named(name, where).role == 'field'
> +        if is_field:
> +            raise CtlvError(where, f'record {name} holds a named record, and '
> +                                   f'{name} is a field of records:, which '
> +                                   'bare record places')
> +        return name

[Severity: Low]
Does _held() raise an order-dependent false "record cycle" error when a
held record includes the enclosing record?

If record A has a field "b?: record B" and record B has "$include: A",
declaring B before A succeeds because _held('B') sees 'B' in
self.building and inspects self.declared['B'][3].body directly.  If A is
declared before B, _named('A') calls _held('B') while 'A' is in
self.building, and _held('B') calls _named('B'), which resolves
"$include: A" and fails with "record cycle: A -> B -> A".  Checking
self.declared[name][3].body whenever name is in self.declared avoids
resolving B while A is still being built.

[ ... ]

> +    def _check_fills(self, fills: tuple[Fill, ...]) -> None:
> +        """Hold each fill to every place its extension point sits.
> +
> +        A fill nested in another is met after it, so its places are known.
> +        """
> +        for fill in fills:
> +            where = self.declared[fill.name][1]
> +            for site, context, level in self.sites.get(fill.name, []):
> +                self._check_subtree(f'{where} in {site}', fill.root, context,
> +                                    level)

[Severity: Low]
Can a single pass in _check_fills() miss extension sites when a later fill
reuses a record containing an earlier-filled extension point?

 If an ancestor family declares extension point ext-b inside a named record
rec-b and extension point ext-a in an op, and a descendant fills ext-b
before ext-a (either earlier in records: or in an intermediate parent
family) while filling ext-a using rec-b, ext-b precedes ext-a in fills.
_check_fills() then checks ext-b before visiting ext-a, so the site for
ext-b inside ext-a is appended to self.sites['ext-b'] after ext-b's loop
has already finished, skipping context and depth validation for ext-b at
that site.

[ ... ]

> +def _check_composed(family: Family) -> None:
> +    def walk_composed(where: str, node: Node) -> int:
> +        members = composed_members(family, node)
> +        required = sum(not optional for _child, optional in members)
> +        optional = len(members) - required
> +
> +        _check_count(where, 'composed required fields', required,
> +                     spec.U32_MAX)
> +        _check_count(where, 'composed optional fields', optional,
> +                     spec.U32_MAX)
> +        count = 1
> +        for child, _optional in members:
> +            count += walk_composed(where, child)
> +        return count
> +
> +    for op in family.ops:
> +        total = 0
> +        for which, root in (('request', op.request), ('reply', op.reply)):
> +            if root is None:
> +                continue
> +            where = f'{family.path}: op {op.name} {which}'
> +            count = walk_composed(where, root)
> +            _check_count(where, 'composed schema nodes', count,
> +                         spec.U32_MAX)
> +            total += count
> +        _check_count(f'{family.path}: op {op.name}',
> +                     'composed request and reply nodes', total,
> +                     spec.U32_MAX)

[Severity: Low]
Does _check_composed() miss checking composed op message sizes against
MAX_INLINE_MESSAGE_LEN?

_add_ops() runs sizes.compute() on the uncomposed op tree where extension
points are empty, while sizes.composed() in tools/ctlv/ctlvgen/lib/sizes.py
is never called here.  Even after commit "tools/ctlv: Add the userspace
library" calls sizes.composed() in gen_user.py, running ctlvgen with
--validate or --mode kernel (or reconciling manifests without optional
user-source outputs) will not reject extension fills that push an
inherited op's composed request, action reply, or minimum query reply
length past MAX_INLINE_MESSAGE_LEN.

Similarly, in tools/ctlv/ctlvgen/lib/gen_kernel.py:_limits(), returning
early when family.local_ops is empty skips emitting the
CTLV_MAX_DECLARED_RECORD_DEPTH static_assert for family.fills on
descendant families that only fill inherited extension points.

> diff --git a/tools/ctlv/ctlvgen/lib/spec.py b/tools/ctlv/ctlvgen/lib/spec.py
> new file mode 100644
> index 0000000000000..7d1f2e14f70fd
> --- /dev/null
> +++ b/tools/ctlv/ctlvgen/lib/spec.py
[ ... ]
> +class Paths:
> +    """Where the specs, the registry and the meta-schemas are."""
> +
> +    def __init__(self, spec: pathlib.Path | str,
> +                 schema: pathlib.Path | str | None = None,
> +                 registry: pathlib.Path | str | None = None,
> +                 abi: pathlib.Path | str | None = None) -> None:
> +        self.spec = pathlib.Path(spec)
> +        self.spec_dir = self.spec.parent
> +        self.doc_dir = self.spec.parent.parent
> +        self.schema = pathlib.Path(schema or self.doc_dir / 'ctlv-schema.yaml')
> +        self.registry = pathlib.Path(registry or
> +                                     self.doc_dir / 'families.yaml')
> +        # The two meta-schemas ship together, so overriding one locates both.
> +        self.registry_schema = (self.schema.parent /
> +                                'ctlv-registry-schema.yaml')
> +        self.abi_dir = pathlib.Path(abi or self.doc_dir / 'abi')
> +        self.registry_abi = self.abi_dir / 'families.registry.yaml'
> +        self.generated = self.doc_dir / 'generated-files.yaml'
> +        self.generated_schema = self.doc_dir / 'generated-files-schema.yaml'
> +        self.kernel_dir = self.schema.parent.parent.parent
[ ... ]
> +def load(path: pathlib.Path | str, schema: pathlib.Path) -> FamilySpec:
> +    """Load, syntax-check and normalize one family specification."""
> +    path = pathlib.Path(path)
> +    data, lines = yamlload.load_with_lines(path)
> +    _validate_syntax(path, data, schema)
> +
> +    reader = _Reader(path, lines,
> +                     pathlib.Path(schema).parent.parent.parent / UAPI_DIR)

[Severity: Low]
Does traversing .parent on unresolved relative Path objects in
Paths.__init__() and load() misresolve kernel_dir and the UAPI directory?

In pathlib, taking .parent on a relative path strips components until it
reaches Path('.'), where .parent stays Path('.').  If ctlvgen is invoked
from Documentation/ctlv/ with "--spec specs/ctlv.yaml --validate",
self.schema is Path('ctlv-schema.yaml') and self.schema.parent.parent.parent
collapses to Path('.') instead of the kernel tree root, mislocating both
self.kernel_dir and include/uapi.  Resolving the paths first (or using
os.path.abspath) avoids this.

[ ... ]

> +    def _blob(self, where: str, args: str) -> BlobDecl:
> +        parts = _arguments(args) if args is not None else ['']
> +        direction = DIRECTIONS.get(parts[0])
> +        if direction is None:
> +            raise CtlvError(where, 'a blob starts with its direction, in, '
> +                                   'out or inout')
> +        maximum, alignment, backings = None, None, None
> +        for arg in parts[1:]:
> +            aligned = _ALIGN.fullmatch(arg)
> +            if arg.startswith('..') and maximum is None:
> +                maximum = self._size(where, arg[2:].strip(), 'blob length',
> +                                     U64_MAX)
> +            elif aligned is not None and alignment is None:
> +                alignment = self._size(where, aligned['value'],
> +                                       'blob alignment')
> +            elif backings is None and all(word in BACKINGS
> +                                          for word in arg.split()):
> +                backings = tuple(arg.split())

[Severity: Low]
Does _blob() silently accept an empty comma-separated argument such as
"blob(in, )"?

When arg is empty, arg.split() is [], so all(word in BACKINGS for word in
arg.split()) evaluates to True and sets backings = (), which is then
replaced by DEFAULT_BACKINGS via "backings or DEFAULT_BACKINGS".  Checking
that arg.split() is non-empty would reject malformed empty arguments.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113155515.2510194-1-jiri@resnulli.us?part=1