linux-kernel - Re: [bp:tip-x86-alternatives 1/1] error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72kXpDwRO3R+tjqYgOcbFT7rwnpH=KhVyry-+CUsJa7mRQ@mail.gmail.com>
Date:   Sat, 14 Jan 2023 12:54:18 +0100
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     kernel test robot <lkp@...el.com>, llvm@...ts.linux.dev,
        oe-kbuild-all@...ts.linux.dev, rust-for-linux@...r.kernel.org,
        lkml <linux-kernel@...r.kernel.org>,
        Yujie Liu <yujie.liu@...el.com>,
        Emilio Cobos Álvarez <emilio@...sal.io>
Subject: Re: [bp:tip-x86-alternatives 1/1] error[E0588]: packed type cannot
 transitively contain a `#[repr(align)]` type

On Thu, Jan 12, 2023 at 5:14 PM Borislav Petkov <bp@...en8.de> wrote:
>
> Right, or at least the repro instructions should state it clear.
>
> Btw, this is part of a long-running feedback process we're giving to the 0day
> bot in order to make their reports as user friendly as possible.

Sounds very useful, thanks for the effort!

> Well, I find having an --explain option too much. But there are perhaps reasons
> for it.
>
> One improvement could be, IMHO, they could turn on --explain automatically when
> it results in a build error. So that you don't have to do it yourself.
>
> What would be better, tho, is if there were no --explain option at all and the
> warnings are as human readable as possible.

Sometimes one can get quite a few errors/warnings, so printing all the
docs for all those would be probably too much.

I think `--explain` is intended to provide a way to have longer
documentation about a particular diagnostic message without filling
the terminal too much those that already know what the error/warning
is about (in this particular case, the error first line looks fine to
me -- but of course the machine-translated bindings from `bindgen` are
harder to read due to the generated identifiers).

> so the struct is:
>
> struct alt_instr {
>         s32 instr_offset;       /* original instruction */
>         s32 repl_offset;        /* offset to replacement instruction */
>
>         union {
>                 struct {
>                         u32 cpuid: 16;  /* CPUID bit set for replacement */
>                         u32 flags: 16;  /* patching control flags */
>                 };
>                 u32 ft_flags;
>         };
>
>         u8  instrlen;           /* length of original instruction */
>         u8  replacementlen;     /* length of new instruction */
> } __packed;
>
> and everything is naturally aligned.
>
> So I'm guessing this is a rust bindings glue shortcoming or so...

Yeah, this is https://github.com/rust-lang/rust-bindgen/issues/2179 (I
mentioned it in my reply to Alexander above, in case you didn't see
it, as well as the usual workarounds we use).

There are also other related issues related to GCC's `packed` and
`aligned` attributes: aligned fields in general are not supported
(including just adding the attribute), neither are structs declared
both packed and aligned. So only a subset of possibilities are handled
properly. I collected some links to related issues at
https://github.com/Rust-for-Linux/linux/issues/353.

>From what I could tell reading some old discussions the other day, the
`bindgen` maintainer (Cc'ing Emilio) is aware of the issues but nobody
has put the time to solve it within the bindings generator.

Ideally, I think, Rust could support providing alignment for
individual members in `repr(C)` structs in order to tweak its
described layout algorithm, in order for users to mimic GCC/MSVC/...
extensions as needed. That way it can be useful also for everyone
(even those not using `bindgen`), and `bindgen`'s implementation would
be easier, I imagine.

Cheers,
Miguel