Re: mainline build failure due to f1e4c916f97f ("drm/edid: add EDID block count and size helpers")

[Arnd Bergmann]

On Thu, Jun 2, 2022 at 3:08 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Jun 1, 2022 at 3:28 PM Keisuke Nishimura
> <keisuke.nishimura@xxxxxxxx> wrote:
> >
> >
> > I found 13 definitions of packed structure that contains:
> >  > - spinlock_t
> >  > - atomic_t
> >  > - dma_addr_t
> >  > - phys_addr_t
> >  > - size_t
> >  > - struct mutex
> >  > - struct device
> >
> > - raw_spinlock_t
>
> Ok, so I don't think dma_addr_t/phys_addr_t/size_t are problematic,
> they are just regular integers.
>
> And 'struct device' is problematic only as it then contains any of the
> atomic types (which it does)
is
I suggested this list because they are problematic for different reasons:

- any atomics are clearly broken here

- dma_addr_t/phys_addr_t are sometimes put into hardware data
  structures in coherent DMA allocations. This is broken because these
  types are variably-sized depending on the architecture, and annotating
  structures in uncached memory as __packed is also broken on
  architectures that have neither coherent DMA nor unaligned access
  (most 32-bit mips and armv5), where this will result in a series of
  expensive one-byte uncached load/store instructions.

- having any complex kernel data structure embedded in a __packed
  struct is a red flag, because there should not be a need to mark
  it packed for compatibility with either hardware or user space.
  If the structure is actually misaligned, passing a pointer for the
  embedded struct into an interface that expects an aligned pointer
  is undefined behavior in C, and gcc may decide to do something
  bad here even on architectures that can access unaligned
  pointers.

> > security/tomoyo/common.h: atomic_t in tomoyo_shared_acl_head
> > drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls.h: spinlock_t in key_map
> > arch/s390/include/asm/kvm_host.h: atomic_t in kvm_s390_sie_block
>
> So these do look problematic.
>
> I'm not actually clear on why tomoyo_shared_acl_head would be packed.
> That makes no sense to me.
>
> Same goes for key_map, it's not clear what the reason for that
> __packed is, and it's clearly bogus. It might work, almost by mistake,
> but it's wrong to try to pack that spinlock_t.
>
> The s390 kvm use actually looks fine: the structure is packed, but
> it's also aligned, and the spin-lock is at the beginning, so the
> "packing" part is about the other members, not the first one.

Right, I think the coccinelle script should nor report structures
that are both packed and aligned.

> The two that look wrong look like they will probably work anyway
> (they'll presumably be effectively word-aligned, and that's sufficient
> for spinlocks in practice).
>
> But let's cc the tomoyo and chelsio people.

I think both of them work because the structures are always
embedded inside of larger structures that have at least word
alignment. This is the thing I was looking for, and the
__packed attribute was added in error, most likely copied
from somewhere else.

Looking at the other ones:

include/linux/ti-emif-sram.h: phys_addr_t in ti_emif_pm_data

No misalignment because of the __aligned(8), but this
might go wrong if the emif firmware relies on the structure
layout to have a 32-bit phys_addr_t.

drivers/scsi/wd719x.h: dma_addr_t in wd719x_scb

This one is correct, as the structure has 64 bytes of
hardware data and a few members that are only accessed
by the kernel. There should still be an __aligned(8)
for efficiency.

drivers/net/wireless/intel/ipw2x00/ipw2200.h: dma_addr_t in clx2_queue

Al marked the incorrect __packed annotations in 2008,
see 83f7d57c37e8 ("ipw2200 annotations and fixes").
Mostly harmless but the __packed should just get removed here.

> drivers/infiniband/hw/irdma/osdep.h: dma_addr_t in irdma_dma_mem
> drivers/infiniband/core/mad_priv.h: size_t in ib_mad_private

Same here: harmless but __packed should be removed, possibly
while reordering members by size.

> drivers/crypto/qat/qat_common/qat_asym_algs.c:
> - dma_addr_t in qat_rsa_ctx
> - dma_addr_t in qat_dh_ctx

Probably harmless because the structure is __aligned(64), but I'm
completely puzzled by what the author was actually trying to
achieve here. There are also 'bool' members in the packed struct,
which is probably something we want to look for as well.

> drivers/atm/idt77252.h: dma_addr_t in idt77252_skb_prv

This is a bug on architectures with 64-bit dma_addr_t, it should
be an __le32, and the structure should be __aligned() as a DMA
descriptor.

> drivers/net/wireless/ath/ath10k/core.h: dma_addr_t in ath10k_skb_cb
> drivers/net/wireless/ath/ath11k/core.h: dma_addr_t in ath10k_skb_cb

Should almost certainly not be __packed, fixing these will
likely improve performance on mips32 routers using ath10k

> drivers/crypto/ccp/ccp-dev.h: dma_addr_t in ccp_dma_info

This looks ok, the "__packed __aligned(4)" here can save
a bit of stack space as intended.

I think that SmPL script worked great, almost every instance is
something that ought to be changed, as long as it stops reporting
those structures that are also __aligned(). I would extend it to
also report structures with 'bool', 'enum', or any pointer, but that
could give more false-positives. Maybe have a separate script
for those instances embedding atomics or spinlocks (very broken)
vs the other members (causes more harm than good or might
need alignment).

       Arnd