Re: [PATCH v5 1/2] check-uapi: Introduce check-uapi.sh

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/12/2023 9:43 AM, Greg Kroah-Hartman wrote:
On Wed, Apr 12, 2023 at 09:37:16AM -0700, John Moon wrote:
On 4/11/2023 11:14 PM, Greg Kroah-Hartman wrote:
Would you find the tool more useful if it simply filtered out all instances
where the size of the type did not change? This would filter out the
following which the tool currently flags:

- enum expansions
- reserved field expansions
- expansions of a struct with a flex array at the end
- type changes
- re-ordering of existing members
- ...others?

Obviously not, as some of those are real breakages, and some are not at
all.

Please understand what is an abi breakage.  Adding new enums is not.
Using a reserved field is not.  Reording existing members IS.


Yes, understood that method would miss certain classes of breakages. I was
suggesting it as a way to improve the signal-to-noise ratio of the tool
since we don't currently have an algorithm for determining breakages with
100% accuracy.

Why not?  You know the different types of things here based on the
differences between the dwarf data, and they fall into different
categories, and those different categories mean different things.

If you have questions as to which type of change is allowed and which is
not, just ask us, the rules are not complex, nor impossible to describe,
otherwise we wouldn't have a stable api at all, right?


Right, it's currently a limitation of parsing the abidiff output.

Even in trivial situations like an enum expansion, the tool knows that a variant was added and another variant had its offset changed. There's not a good way to say for sure that the variant whose offset changed is a "*_MAX" variant. So if we simply ignored enum expansion, we'd miss breakages like this:

enum foo {
	FLAG_A,
+       FLAG_B,
	FLAG_C,
	FLAG_MAX
}

Maybe we can ignore an enum expansion if only the last variant's offset changed, but then we'd miss cases where enums don't have a MAX variant. Maybe we could limit the check to last variant's offset whose name contains string "MAX", but what if someone calls it "LAST" instead? It gets fragile.

Or situations like expanding into reserved fields. How can we detect the difference between this:

struct foo {
	__u32 x;
	__u32 y;
+       __u32 z;
+       __u8  padding[12];
-	__u8  padding[16];
}

And this:

struct foo {
	__u32 x;
	__u32 y;
+       __u32 z;
+       char  codename[4]; /* Takes "NAME" */
-	char  codename[8]; /* Takes "CODENAME" */
}

Maybe we grep for "pad" or "reserved", but again... fragile.

Another idea is to add some sort of in-line comment to give the checker a hint that the field is intentionally unstable. It could be implicit for "*_MAX" enum variants or "*padding" at the end of structs, but if you wanted to have something like "end[]" (like in the rseq change), you could add /* ABI-unstable */ next to it and the script would ignore it.

Beyond those issues, we have non-trivial situations like when it's safe to add members to a struct. We know the kernel will zero-extend mismatches between kernel and userspace, but how do we know the driver properly handles the case of an old userspace passing an old struct?

So far, we've erred on the side of flagging it if it _could_ be a break and relied on the reviewer to make the final determination.








[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux