xsm/flask: Fix build with Clang 13

Commit Message

Andrew Cooper April 25, 2022, 6:07 p.m. UTC

Clang 13 chokes with:

  In file included from xsm/flask/flask_op.c:780:
  xsm/flask/flask_op.c:698:33: error: passing 4-byte aligned argument to
  8-byte aligned parameter 1 of 'flask_ocontext_add' may result in an
  unaligned pointer access [-Werror,-Walign-mismatch]
          rv = flask_ocontext_add(&op.u.ocontext);
                                  ^

and the same for flask_ocontext_del().  It isn't a problem in practice,
because the union always starts 8 bytes into {xen,compat}_flask_op_t, but the
diagnostic is based on type alignment alone.

struct xen_flask_ocontext has the same layout between native and compat, but
does change alignment because of uint64_t, and there is only a native
implementation of flask_ocontext_add().

Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Daniel De Graaf <[e-mail]>
CC: Daniel Smith <[e-mail]>
CC: George Dunlap <[e-mail]>
CC: Jan Beulich <[e-mail]>
CC: Stefano Stabellini <[e-mail]>
CC: Wei Liu <[e-mail]>
CC: Julien Grall <[e-mail]>
CC: Roger Pau Monné <[e-mail]>

Slightly RFC because there don't appear to be any good options here.
---
 xen/include/public/xsm/flask_op.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jan Beulich April 26, 2022, 7:20 a.m. UTC | #1

On 25.04.2022 20:07, Andrew Cooper wrote:
> Clang 13 chokes with:
> 
>   In file included from xsm/flask/flask_op.c:780:
>   xsm/flask/flask_op.c:698:33: error: passing 4-byte aligned argument to
>   8-byte aligned parameter 1 of 'flask_ocontext_add' may result in an
>   unaligned pointer access [-Werror,-Walign-mismatch]
>           rv = flask_ocontext_add(&op.u.ocontext);
>                                   ^
> 
> and the same for flask_ocontext_del().  It isn't a problem in practice,
> because the union always starts 8 bytes into {xen,compat}_flask_op_t, but the
> diagnostic is based on type alignment alone.
> 
> struct xen_flask_ocontext has the same layout between native and compat, but
> does change alignment because of uint64_t, and there is only a native
> implementation of flask_ocontext_add().
> 
> Signed-off-by: Andrew Cooper <[e-mail]>
> ---
> CC: Daniel De Graaf <[e-mail]>
> CC: Daniel Smith <[e-mail]>
> CC: George Dunlap <[e-mail]>
> CC: Jan Beulich <[e-mail]>
> CC: Stefano Stabellini <[e-mail]>
> CC: Wei Liu <[e-mail]>
> CC: Julien Grall <[e-mail]>
> CC: Roger Pau Monné <[e-mail]>
> 
> Slightly RFC because there don't appear to be any good options here.

We cannot address this by altering the public header. Besides us having
previously agreed to avoid the use of extensions outside of tools-only
parts of these headers (or else you could simply use uint64_aligned_t),
you're also altering the ABI for compat guests by changing the alignment.

On irc yesterday we had

18:12:06 - jbeulich: With alignof() == 4 the compiler could put the variable on the stack, but not 8-byte aligned (if something else occupies another 32-bit slot).
18:13:34 - andyhhp: well - it can't in this case
18:13:37 - andyhhp: I agree it can in principle

which I don't understand, but which I'd like to understand in this
context: Why would the compiler not be allowed to place
compat_flask_op()'s op variable 4-but-not-8-byte aligned on the stack?
Then, if forcing 8-byte alignment of op, the compiler still issuing a
diagnostic would be a (minor) bug imo, as taking into account variable
alignment and offset into the structure would be enough to know that
_this instance_ of the struct cannot be misaligned. (Of course this
wouldn't help us, as we'd still need to work around the deficiency.)

One possible way to deal with the problem that I can see (without
having actually tried it) is to make the two functions take a parameter
of compat_flask_ocontext_t *. That's type-compatible with struct
xen_flask_ocontext *, but has reduced alignment. Of course this will
require ugly #ifdef-ary because the type won't be available without
CONFIG_COMPAT. Otoh any approach avoiding #ifdef-ary (like introducing
yet another typedef matching that of compat_flask_ocontext_t, just
without reference to struct compat_flask_ocontext) would needlessly
impact !COMPAT builds.

Jan

Patch

diff --git a/xen/include/public/xsm/flask_op.h b/xen/include/public/xsm/flask_op.h
index b41dd6dac894..80dc35122320 100644
--- a/xen/include/public/xsm/flask_op.h
+++ b/xen/include/public/xsm/flask_op.h
@@ -146,7 +146,7 @@  struct xen_flask_ocontext {
     uint32_t ocon;
     uint32_t sid;
     uint64_t low, high;
-};
+} __attribute__((__aligned__(8)));
 typedef struct xen_flask_ocontext xen_flask_ocontext_t;
 
 struct xen_flask_peersid {