netlink: Parse repeated nested attributes
ClosedPublic
Actions

Authored by kbowling on Mon, Aug 10, 10:39 PM.

Details

Reviewers

pouria
glebius
melifaro

Commits

rG60c0a075472c: netlink: Parse repeated nested attributes

Summary

Modern Netlink arrays encode their elements as repeated attributes of
the same type rather than as children of an additional array container.

Add an SNL callback that parses one nested element for each occurrence
and appends it to a geometrically grown parser array.  Retain the
existing parray callback for protocols that use the legacy container
form.

Store the growth capacity in struct snl_parray, appended after its
existing public count and items fields so their offsets remain stable
on LP64 and ILP32.  Require parser targets to be real snl_parray
objects, and convert bitset, generic Netlink, and route multipath arrays
accordingly.  This avoids relying on layout aliases for private growth
state.

Add regression coverage for a nested bit array that grows beyond its
initial allocation, while preserving replacement semantics when a
legacy array target is reused.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kbowling created this revision.Mon, Aug 10, 10:39 PM

Herald added a reviewer: melifaro. · View Herald TranscriptMon, Aug 10, 10:39 PM

Herald added subscribers: melifaro, imp. · View Herald Transcript

kbowling requested review of this revision.Mon, Aug 10, 10:39 PM

kbowling added a child revision: D58776: rtnetlink: Add native SR-IOV VF status.Mon, Aug 10, 10:57 PM

Overall, this is a great change. I also wanted to rename parray structure from nexthop-specific names to something more generic too, considering we really need the multi-attr parser in our library too. specially for cases like D58643.

However, this definitely breaks snl(3) compatibility for applications that depend on this library.
I have no objection, but I'd prefer to get @glebius opinion on this too.
A quick search on github shows some applications that are using these interfaces outside our code base.
https://github.com/search?q=struct+rta_mpath_nh&type=code

I'm not qualified to review this from a security perspective, so it would be helpful to get input from a security team member as well. (@markj)

Thank you, I'll review this in depth soon.

In D58775#1348901, @pouria wrote:

Overall, this is a great change. I also wanted to rename parray structure from nexthop-specific names to something more generic too, considering we really need the multi-attr parser in our library too. specially for cases like D58643.

However, this definitely breaks snl(3) compatibility for applications that depend on this library.
I have no objection, but I'd prefer to get @glebius opinion on this too.
A quick search on github shows some applications that are using these interfaces outside our code base.
https://github.com/search?q=struct+rta_mpath_nh&type=code

The cheap answer is it's not an MFC candidate :)

For completeness in case it guides later decision making, I did scan all 5 github search pages: the only case that isn't a fork of freebsd-src or spiritually the same (f-stack, rtems-libbsd) is zig's libc wrappers.

kbowling mentioned this in D58776: rtnetlink: Add native SR-IOV VF status.Thu, Aug 13, 3:52 PM

melifaro accepted this revision.Thu, Aug 13, 10:37 PM

This revision is now accepted and ready to land.Thu, Aug 13, 10:37 PM

LGTM
Thank you!
I'll rebase D58644 on top of this and replace my struct iflaf_group with snl_parray.
It *might* also need a version bump.