| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0d93c6e5-a4cb-2f5c-e87d-e42a283324d6@linux-m68k.org>
Date: Wed, 17 Sep 2025 12:14:15 +1000 (AEST)
From: Finn Thain <fthain@...ux-m68k.org>
To: Arnd Bergmann <arnd@...db.de>
cc: Peter Zijlstra <peterz@...radead.org>, Will Deacon <will@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Boqun Feng <boqun.feng@...il.com>, Jonathan Corbet <corbet@....net>,
Mark Rutland <mark.rutland@....com>, linux-kernel@...r.kernel.org,
Linux-Arch <linux-arch@...r.kernel.org>,
Geert Uytterhoeven <geert@...ux-m68k.org>, linux-m68k@...r.kernel.org
Subject: Re: [RFC v2 3/3] atomic: Add alignment check to instrumented atomic
operations
On Tue, 16 Sep 2025, Arnd Bergmann wrote:
> On Tue, Sep 16, 2025, at 02:16, Finn Thain wrote:
>
> > Packing uapi structures (and adopting -malign-int) sounds easier than
> > the alternative, which might be to align certain internal kernel
> > struct members, on a case-by-case basis, where doing so could be shown
> > to improve performance on some architecture or other (while keeping
> > -mno-align-int).
> >
> > Well, it's easy to find all the structs that belong to the uapi, but
> > it's not easy to find all the internal kernel structs that describe
> > MMIO registers. For -malign-int, both kinds of structs are a problem.
>
> Right, especially since those structure definitions are more likely to
> be used on older drivers, there are probably some that do happen on
> m68k. On the other hand, any driver that is portable to non-m68k targets
> won't have this problem.
>
Yes, but only inasmuchas drivers are completely portable. Any data
structure accessed or declared using #if conditional code would defeat
that kind of reasoning.
> I tried a trivial m68k defconfig build with "-Wpadded -malign-int" and
> found 3021 instances of structure padding, compared to 2271 without
> -malign-int. That is still something one could very reasonably go
> through with 'vim -q output', as almost all of them are obviously
> kernel-internal structures and not problematic.
>
So, 3021 is the number of structs potentially needing to be checked? This
is not the kind of upper bound I consider feasible for careful manual
editing (it's worse than I thought). We need the compiler to help.
> A quick manual scan only shows a number of uapi structures but very few
> drivers. There are a few hardware structures that are internally packed
> but in a structure that has gets an extra two bytes of harmless padding
> at the end (struct CUSTOM, struct amiga_hw_present, struct
> atari_hw_present, struct TT_DMA, struct ppc_regs).
I wouldn't assume that padding at the end is inconsequential. I think the
analysis requires comparing object code. E.g. add the same padding
explicitly to see whether it alters object code under -mno-align-int.
> The only ones I could find that actually seemed suspicious are:
>
> arch/m68k/include/asm/atarihw.h:426:10: warning: padding struct to align
> 'dst_address' [-Wpadded] arch/m68k/include/asm/openprom.h:76:13:
> warning: padding struct to align 'boot_dev_ctrl' [-Wpadded]
> drivers/net/ethernet/i825xx/82596.c:250:1: warning: padding struct size
> to alignment boundary
>
> I'm sure there are a few more, but probably not a lot more. See
> https://pastebin.com/Z2bjnD0G for the full list. I've also tried
> annotating the ones that show up in defconfig, see diffstat below.
>
> Unfortunately I found no way of annotating a struct a whole to use the
> traditional padding rules: "#pragma pack(push, 2)" lowers the alignment
> of all struct members to two bytes, including those with an explicit
> alignment like __aligned_u64. A struct-wide __attribute__((packed,
> aligned(2))) seems even worse, as it makes all members inside of the
> struck tightly packed and only aligns the struct itself.
>
> This means all misaligned members have to be individually marked as
> __packed, unless someone comes up with another type of annotation.
>
Right. The trick will be to annotate the affected struct members by
automatic program transformation.
> > If better performance is to be had, my guess is that aligning atomic_t
> > will get 80% of it (just an appeal to the Pareto principle, FWIW...)
>
> arch/m68k selects CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS for anything
> other than Dragonball, so I would not expect much performance difference
> at all, unless CASL on unaligned data ends up causing alignment traps as
> it does on most architectures.
>
I think the answer to the performance question would depend on both choice
of workload and choice of architecture. I haven't done any measurements on
this patch series yet.
> Arnd
>
> arch/m68k/atari/atakeyb.c | 2 +-
> arch/m68k/include/asm/amigahw.h | 4 +--
> arch/m68k/include/asm/atarihw.h | 6 ++--
> arch/m68k/include/asm/mvme147hw.h | 2 +-
> arch/m68k/include/asm/openprom.h | 2 +-
> arch/m68k/include/uapi/asm/ptrace.h | 2 +-
> arch/m68k/include/uapi/asm/sigcontext.h | 2 +-
> arch/m68k/include/uapi/asm/stat.h | 4 +--
> drivers/net/ethernet/i825xx/82596.c | 4 +--
> include/uapi/asm-generic/termios.h | 2 +-
> include/uapi/linux/acct.h | 2 +-
> include/uapi/linux/ax25.h | 6 ++--
> include/uapi/linux/blktrace_api.h | 2 +-
> include/uapi/linux/btrfs.h | 2 +-
> include/uapi/linux/cdrom.h | 36 +++++++++++-----------
> include/uapi/linux/dlm.h | 2 +-
> include/uapi/linux/dqblk_xfs.h | 2 +-
> include/uapi/linux/ethtool.h | 14 ++++-----
> include/uapi/linux/fb.h | 6 ++--
> include/uapi/linux/fd.h | 6 ++--
> include/uapi/linux/filter.h | 2 +-
> include/uapi/linux/hdlc/ioctl.h | 6 ++--
> include/uapi/linux/hiddev.h | 2 +-
> include/uapi/linux/i2c-dev.h | 2 +-
> include/uapi/linux/i2c.h | 2 +-
> include/uapi/linux/if.h | 2 +-
> include/uapi/linux/if_arcnet.h | 4 +--
> include/uapi/linux/if_bonding.h | 2 +-
> include/uapi/linux/if_bridge.h | 14 ++++-----
> include/uapi/linux/if_link.h | 2 +-
> include/uapi/linux/if_plip.h | 2 +-
> include/uapi/linux/if_pppox.h | 2 +-
> include/uapi/linux/if_vlan.h | 2 +-
> include/uapi/linux/inet_diag.h | 2 +-
> include/uapi/linux/input.h | 4 +--
> include/uapi/linux/ip6_tunnel.h | 4 +--
> include/uapi/linux/kd.h | 2 +-
> include/uapi/linux/llc.h | 2 +-
> include/uapi/linux/loop.h | 2 +-
> include/uapi/linux/mctp.h | 4 +--
> include/uapi/linux/mptcp.h | 2 +-
> include/uapi/linux/mroute.h | 4 +--
> include/uapi/linux/mroute6.h | 6 ++--
> include/uapi/linux/msdos_fs.h | 2 +-
> include/uapi/linux/msg.h | 6 ++--
> include/uapi/linux/mtio.h | 2 +-
> include/uapi/linux/netfilter/ipset/ip_set.h | 4 +--
> include/uapi/linux/netfilter/nf_nat.h | 2 +-
> include/uapi/linux/netfilter/x_tables.h | 8 ++---
> include/uapi/linux/netfilter/xt_HMARK.h | 2 +-
> include/uapi/linux/netfilter/xt_TPROXY.h | 4 +--
> include/uapi/linux/netfilter/xt_connbytes.h | 2 +-
> include/uapi/linux/netfilter/xt_connmark.h | 6 ++--
> include/uapi/linux/netfilter/xt_conntrack.h | 4 +--
> include/uapi/linux/netfilter/xt_esp.h | 2 +-
> include/uapi/linux/netfilter/xt_hashlimit.h | 6 ++--
> include/uapi/linux/netfilter/xt_helper.h | 2 +-
> include/uapi/linux/netfilter/xt_ipcomp.h | 2 +-
> include/uapi/linux/netfilter/xt_iprange.h | 2 +-
> include/uapi/linux/netfilter/xt_l2tp.h | 2 +-
> include/uapi/linux/netfilter/xt_mac.h | 2 +-
> include/uapi/linux/netfilter/xt_mark.h | 2 +-
> include/uapi/linux/netfilter/xt_owner.h | 2 +-
> include/uapi/linux/netfilter/xt_realm.h | 2 +-
> include/uapi/linux/netfilter/xt_recent.h | 4 +--
> include/uapi/linux/netfilter/xt_set.h | 4 +--
> include/uapi/linux/netfilter/xt_statistic.h | 2 +-
> include/uapi/linux/netfilter/xt_tcpmss.h | 2 +-
> include/uapi/linux/netfilter/xt_tcpudp.h | 2 +-
> include/uapi/linux/netfilter/xt_time.h | 2 +-
> include/uapi/linux/netfilter/xt_u32.h | 6 ++--
> include/uapi/linux/netfilter_arp/arp_tables.h | 2 +-
> include/uapi/linux/netfilter_arp/arpt_mangle.h | 2 +-
> include/uapi/linux/netfilter_bridge/ebt_802_3.h | 2 +-
> include/uapi/linux/netfilter_bridge/ebt_arp.h | 2 +-
> include/uapi/linux/netfilter_bridge/ebt_arpreply.h | 2 +-
> include/uapi/linux/netfilter_bridge/ebt_mark_m.h | 2 +-
> include/uapi/linux/netfilter_bridge/ebt_nat.h | 2 +-
> include/uapi/linux/netfilter_bridge/ebt_stp.h | 4 +--
> include/uapi/linux/netfilter_bridge/ebt_vlan.h | 2 +-
> include/uapi/linux/netfilter_ipv4/ipt_ah.h | 2 +-
> include/uapi/linux/netfilter_ipv6/ip6_tables.h | 2 +-
> include/uapi/linux/netfilter_ipv6/ip6t_ah.h | 2 +-
> include/uapi/linux/netfilter_ipv6/ip6t_frag.h | 2 +-
> include/uapi/linux/netfilter_ipv6/ip6t_opts.h | 2 +-
> include/uapi/linux/netfilter_ipv6/ip6t_rt.h | 2 +-
> include/uapi/linux/netfilter_ipv6/ip6t_srh.h | 2 +-
> include/uapi/linux/netrom.h | 2 +-
> include/uapi/linux/nfs_idmap.h | 2 +-
> include/uapi/linux/nfs_mount.h | 2 +-
> include/uapi/linux/pkt_cls.h | 2 +-
> include/uapi/linux/rds.h | 4 +--
> include/uapi/linux/rose.h | 8 ++---
> include/uapi/linux/route.h | 2 +-
> include/uapi/linux/rtc.h | 2 +-
> include/uapi/linux/sctp.h | 18 +++++------
> include/uapi/linux/sed-opal.h | 2 +-
> include/uapi/linux/sem.h | 2 +-
> include/uapi/linux/serial.h | 4 +--
> include/uapi/linux/soundcard.h | 8 ++---
> include/uapi/linux/taskstats.h | 2 +-
> include/uapi/linux/virtio_net.h | 4 +--
> include/uapi/linux/wireless.h | 4 +--
> include/uapi/linux/xfrm.h | 26 ++++++++--------
> include/uapi/mtd/mtd-abi.h | 2 +-
> include/uapi/rdma/hfi/hfi1_ioctl.h | 2 +-
> include/uapi/rdma/ib_user_ioctl_verbs.h | 2 +-
> include/uapi/rdma/ib_user_mad.h | 2 +-
> 109 files changed, 205 insertions(+), 204 deletions(-)
>
Thanks for sending these results. I was looking into doing something
similar using pahole but found that difficult. When I #include'd all the
headers of interest, the thing doesn't build. I abandoned that approach
and concluded that the way forward was to get the compiler to do the
analysis (perhaps with a plug-in), during a normal kernel build, rather
than during compilation of some contrived mess containing all the headers
and all the structs.
It would be sufficient to have a plug-in to list the sites of all members
potentially needing annnotation. The list could be manually pared down and
the actual annotating could be scripted. Note that this list would be
shorter than a -Wpadded list, because there's a bunch of padding that's
not relevant.
Powered by blists - more mailing lists