On Sun, Dec 03, 2023 at 05:59:32PM +0100, Lukas Wunner wrote:
> Hi Dan,
>
> > > Please advise whether these sparse warnings are false positives which
> > > can be ignored and if they aren't, how to resolve them. If you happen
> > > to know the rationale for the cast, I'd be grateful if you could shed
> > > some light on it. Thanks a lot!
> >
> > The question is more why is pci_channel_state_t declared as __bit_wise.
> > __bit_wise data can only be used through accessor functions, like user
> > pointers have to go through copy_from_user() and endian data has to go
> > through le32_to_cpu() etc.
>
> __bitwise is not only to ensure endian correctness. It can be used to
> define data types which are integer-based, but treated as a distinct
> data type by sparse. A pci_channel_state_t value cannot be assigned
> to an integer variable of a different type, for example.
Correct. It was done on purpose to make a sort of 'strong' enum type,
and thus warn if pci_channel_state_t values are mixed with some other types.
> A few arches define arch_xchg() and arch_cmpxchg() as pure macros.
> The sparse warning for pci_channel_state_t does not appear on those
> arches. (These are: arc csky riscv x86)
>
> All other arches use a mix of macros and static inlines to define
> arch_xchg() and arch_cmpxchg(). The static inlines use unsigned long
> as argument and return types and the macros cast the argument type to
> unsigned long.
>
> Why are the casts necessary? Because there are callers of xchg() and
> cmpxchg() which pass pointers instead of integers as arguments.
Hmmm, I'm missing the precise context but it make me wonder what's happening
on 64-bit archs where enums are usually only 32-bit ...
> Examples include llist_del_all(), __wake_q_add(), mark_oom_victim(),
> fsnotify_attach_connector_to_object(). (Note that NULL is defined as
> "(void *)0".)
>
> When using xchg() or cmpxchg() with __bitwise arguments (as is done
> by commit 74ff8864cc84 in pci_dev_set_io_state()), the arches which
> define arch_xchg() and arch_cmpxchg() with static inlines see sparse
> warnings because the __bitwise arguments are cast to unsigned long.
> Those arches are: alpha arm arm64 hexagon loongarch m68k mips openrisc
> parisc powerpc s390 sh sparc xtensa
OK, if the underlying macros do as:
switch (size) {
case 1: ...
then things are ok there (but only if the size is given by a sizeof() of
the correct type (not casted to long or something).
> Indeed adding __force to the cast, as you suggest, should avoid the
> issue. sparse cannot parse the inline assembler, so it does not
> understand that arch_xchg() and arch_cmpxchg() internally perform a
> comparison and/or assignment. By adding __force, we therefore do not
> lose any validation coverage.
Yes, indeed, using __force inside specific accessors or any low-level macros,
like here these arch_xchg(), is OK. It's done in plenty of similar cases.
> A better approach might be to teach sparse that arch_xchg() and
> arch_cmpxchg() internally perform a comparison and/or assignment.
> Then sparse could validate the argument and return value types.
>
> builtin.c in the sparse source code already contains functions
> to handle __atomic_exchange() and __atomic_compare_exchange(),
> so I think xchg() and cmpxchg() should be handled there as well.
I don't agree. Sparse shouldn't know about the semantics of functions
in the kernel. Sparse offers some tools (annotations like the __bitwise,
noderef or address_space attributes for __user, __iomem, ... and type
checking stricter than standard/GCC C). By using these annotations, the
kernel can define a stricter semantic, that better suits its needs,
Sparse should know nothing about this, it's not its job.
Best regards,
-- Luc
