Re: abidiff improvements for kernel UAPI checker

[Dodji Seketeli <dodji@seketeli.org>]

Hello, John & Satya,

John Moon <quic_johmoo@quicinc.com> a écrit:

[...]

> As you may be aware from the Linux kernel mailing list threads I've
> been CC'ing you on, we're in the process of upstreaming a shell script
> that uses abidiff to compare Linux kernel userspace APIs between
> revisions. Linux UAPIs are supposed to be stable forever, so we're
> trying to detect when a patch breaks a UAPI.
>
> If you haven't been following, the latest thread is here:
> https://lore.kernel.org/lkml/20230407203456.27141-1-quic_johmoo@quicinc.com/
>
> At a high level, we're taking the header file before/after the patch,
> building it in a dummy C file, then taking advantage of abidiff's 
> "--non-reachable-types" argument to compare all the symbols.
>
> This method works great! However, there are a couple of classes of ABI
> differences that are detected but don't qualify as UAPI-breaking.
>
> By far the most common class of differences occur with enum expansions.
>
> For example, this change triggers finding:
> https://github.com/torvalds/linux/commit/622f1b2fae2eea28a80b04f130e3bb54227699f8#diff-a7f82b68b3459e13934c123bda4c3a9be20eadebe938a376e39a395e5ffa825a 
>
>
> Specifically this change to include/uapi/linux/dbcnl.h:
> diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
> index 99047223ab26..7e15214aa5dd 100644
> --- a/include/uapi/linux/dcbnl.h
> +++ b/include/uapi/linux/dcbnl.h
> @@ -411,6 +411,7 @@ enum dcbnl_attrs {
>   * @DCB_ATTR_IEEE_PEER_PFC: peer PFC configuration - get only
>   * @DCB_ATTR_IEEE_PEER_APP: peer APP tlv - get only
>   * @DCB_ATTR_DCB_APP_TRUST_TABLE: selector trust table
> + * @DCB_ATTR_DCB_REWR_TABLE: rewrite configuration
>   */
>  enum ieee_attrs {
>         DCB_ATTR_IEEE_UNSPEC,
> @@ -425,6 +426,7 @@ enum ieee_attrs {
>         DCB_ATTR_IEEE_QCN_STATS,
>         DCB_ATTR_DCB_BUFFER,
>         DCB_ATTR_DCB_APP_TRUST_TABLE,
> +       DCB_ATTR_DCB_REWR_TABLE,
>         __DCB_ATTR_IEEE_MAX
>  };
>
>
> abidiff correctly reports there's an ABI difference:
>
>     [C] 'enum ieee_attrs' changed:
>       type size hasn't changed
>       1 enumerator insertion:
>         'ieee_attrs::DCB_ATTR_DCB_REWR_TABLE' value '12'
>       1 enumerator change:
>         'ieee_attrs::__DCB_ATTR_IEEE_MAX' from value '12' to '13' at
>         dcbnl.h:416:1
>
> It's quite common in the kernel to have the last variant in your enum
> be *_MAX, then define the max value of the enum based on that variant.
>
> However, just adding a variant to this enum doesn't break userspace
> (unless you have a program that does something silly like this):
>
> if (DCB_ATTR_IEEE_MAX > 12) {
>     // crash the program
> }
>
> Assuming that programs will use the MAX value in a reasonable way, we
> can consider this change safe for userspace backwards-compatibility if 
> it meets the following criteria:
>
>   1. No variants are removed/renamed
>   2. No variants have a changed value (unless it's the last one)

I think feature this would not be difficult to add and would indeed be a
useful feature for others, I believe.

Would it be okay if you'd have to carry a suppression specification
file that would have:

    $ cat filter-enums.suppr

    [suppress_type]
      type_kind = enum
      has_last_enumerator_change_only = true


Then you'd invoke the tool by doing:

    $ abidiff --suppr filter-enum.suppr binary-version1 binary-version2

To get an idea of what suppression specifications do, you can read more
at https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppression-specifications.

Please note that the "has_last_enumerator_change_only" property is not
yet implemented.  I'd like to know first if this "user interface" would
fit your use case.  If it does then we can go ahead and add it.

[...]

> Technically, we could parse the stdout of abidiff to see if these
> conditions are met, but that'd be quite complicated and fragile. It 
> seems a better choice to detect them when the data structures are
> available.

Indeed, that is why I am proposing to extend the suppression
specifications we already have to handle this kind of cases.

> Do you have any ideas on how these condition could be detected from
> abidiff? We could spend some time to implement the logic, but I'm not 
> sure if this kind of specific use case is within the scope of your project.

Suppression specifications are precisely designed to handle this kind of
needs, and yes, that is in the scope of the project.  They are added on
a case by case basis as projects like yours raise the need for it.

> If so, could something be added like "--ignore-enum-expansion" (or
> some other flag)?

I think the suppression specification route would be better as the
combination of cases to "filter out" depending on each use case is
probably too big to handle via command line switches.

> There are other similar "false positives" discussed in the LKML
> thread, but this first one is the largest and probably the easiest to
> detect algorithmically, so I figure it's a good starting point.

Right, I'd be glad to be made aware about those and help tackle them.

> Thank you in advance for any help!

Thank you for considering abidiff and I hope we'll be useful.

Cheers,

-- 
		Dodji