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

[John Moon <quic_johmoo@quicinc.com>]

On 4/10/2023 11:34 PM, Greg Kroah-Hartman wrote:
> On Mon, Apr 10, 2023 at 04:32:49PM -0700, John Moon wrote:
>>>> According to this tool, it looks like we broke a lot of UAPI
>>>> headers in the previous MW (between v6.2 and v6.3-rc1).
>>>
>>> That's not ok, and needs to be fixed, otherwise this is useless as no
>>> one can rely on it at all.
>>>
>>
>> Right, there are several classes of false positives that we've documented
>> and when examining thousands of commits at time, it'll flag many things.
>>
>> For some comparison, if you run checkpatch on the same changeset
>> (v6.2..v6.3-rc1), you get 995 errors and 7,313 warnings. Still, checkpatch
>> is helpful for spot-checks.
> 
> checkpatch.pl does not matter, it is a "hint", and many patches
> explicitly ignore it (think about patches in the staging tree, you could
> fix up one checkpatch issue for a line, but ignore another one as you
> are not supposed to mix them up.)
> 
> Also for some subsystems, checkpatch does not matter because their
> codebase is old and follows different rules.  And in some places,
> checkpatch is just wrong, because it's a perl script and can not really
> parse code.
> 
> So NEVER use that as a comparison to the user/kernel abi please.  It's a
> false comparison.
>

Fair enough. I was just trying to frame this tool as a "hint" as well. :)

>>
>> Others do not seem to be intentional:
>>
>>   Addition/use of flex arrays:
>>     - include/uapi/linux/rseq.h (f7b01bb0b57f)
>>     - include/uapi/scsi/scsi_bsg_mpi3mr.h (c6f2e6b6eaaf)
> 
> That is not a breakage, that's a tool problem.
> 
>>   Type change:
>>     - include/uapi/scsi/scsi_bsg_ufs.h (3f5145a615238)
> 
> Again, not a real breakage, size is still the same.
>

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?

These changes aren't _always_ safe, but if you assume the kernel 
developer is doing something reasonable, then maybe it's okay. Maybe we 
could hide these checks behind something like a "--pedantic" flag?

This logic is actually trivial to add. Filtering out issues where the 
type size stays the same brings us down to 14 failures in that same MW 
changeset. 8 of them are file removals and the 6 remaining flags are 
additions to existing structs or intentional removals of APIs.

LMK what you think and I can work this into a v6 patch.

>>   Additions into existing struct:
>>     - include/uapi/drm/amdgpu_drm.h (b299221faf9b)
>>     - include/uapi/linux/perf_event.h (09519ec3b19e)
>>     - include/uapi/linux/virtio_blk.h (95bfec41bd3d)
> 
> Adding data to the end of a structure is a well-known way to extend the
> api, in SOME instances if it is used properly.
> 
> So again, not a break.
>

I don't know of a way the tool could be smart enough to decide if the 
additions are done properly in the proper instances. Seems appropriate 
that the tool would flag these changes for closer human review.

Thanks,
John