On 2020-02-27 3:48 AM, Stephen Boyd wrote:

Quoting Leonard Crestez (2020-02-25 11:52:11)

quoted

On 25.02.2020 18:52, Stephen Boyd wrote:

quoted

Quoting Leonard Crestez (2020-02-20 08:29:32)

quoted

The imx SC api strongly assumes that messages are composed out of
4-bytes words but some of our message structs have odd sizeofs.

This produces many oopses with CONFIG_KASAN=y.

Fix by marking with __aligned(4).

Fixes: fe37b4820417 ("clk: imx: add scu clock common part")
Signed-off-by: Leonard Crestez <redacted>
---
   drivers/clk/imx/clk-scu.c | 6 +++---
   1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index fbef740704d0..3c5c42d8833e 100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c

@@ -41,16 +41,16 @@ struct clk_scu {
   struct imx_sc_msg_req_set_clock_rate {
          struct imx_sc_rpc_msg hdr;
          __le32 rate;
          __le16 resource;
          u8 clk;
-} __packed;
+} __packed __aligned(4);

Sorry, this still doesn't make sense to me. Having __aligned(4) means
that the struct is placed on the stack at some alignment, great, but it
still has __packed so the sizeof this struct is some odd number like 11.
If this struct is the last element on the stack it will end at some
unaligned address and the mailbox code will read a few bytes beyond the
end of the stack.

I checked again and marking the struct with __aligned(4) makes it have
sizeof == 12 as intended. It was 11 before.

      static_assert(sizeof(struct imx_sc_msg_req_set_clock_rate) == 12);

After reading through your email and gcc docs again I'm not sure if this
portable/reliable this is but as far as I understand "sizeof" needs to
account for alignment. Or is this just an accident with my compiler?

Marking a structure both __packed and __aligned(4) means that __packed
only affects internal struct member layout but sizeof is still rounded
up to a multiple of 4:

struct test {
         u8      a;
         u16     b;
} __packed __aligned(4);

static_assert(sizeof(struct test) == 4);
static_assert(offsetof(struct test, a) == 0);
static_assert(offsetof(struct test, b) == 1);

This test is not realistic because I don't think SCU messages have any
such oddly-aligned members.

I'm not really sure as I'm not a linker expert. I'm just especially wary
of using __packed or __aligned attributes because they silently generate
code that is usually inefficient. This is why we typically do lots of
shifting and masking in the kernel, so that we can easily see how
complicated it is to pack bits into place. Maybe it makes sense to get
rid of the structs entirely and pack the bits into __le32 arrays of
varying length. Then we don't have to worry about packed or aligned or
what the compiler will do and we can easily be confident that we've put
the bits in the right place in each u32 that is eventually written to
the mailbox register space.

These message structs are not as complicated as hardware register, for 
example everything is always on a byte border.

In older versions of the imx internal tree SC messaging is done by 
packing into arrays through a layer of generated code which looks like this:

          RPC_VER(&msg) = SC_RPC_VERSION;
          RPC_SVC(&msg) = U8(SC_RPC_SVC_MISC);
          RPC_FUNC(&msg) = U8(MISC_FUNC_SET_CONTROL);
          RPC_U32(&msg, 0U) = U32(ctrl);
          RPC_U32(&msg, 4U) = U32(val);
          RPC_U16(&msg, 8U) = U16(resource);
          RPC_SIZE(&msg) = 4U;

The RPC_U32/U16 macros look like this:

#define RPC_I32(MESG, IDX)      ((MESG)->DATA.i32[(IDX) / 4U])
#define RPC_I16(MESG, IDX)      ((MESG)->DATA.i16[(IDX) / 2U])
#define RPC_I8(MESG, IDX)       ((MESG)->DATA.i8[(IDX)])
#define RPC_U32(MESG, IDX)      ((MESG)->DATA.u32[(IDX) / 4U])
#define RPC_U16(MESG, IDX)      ((MESG)->DATA.u16[(IDX) / 2U])
#define RPC_U8(MESG, IDX)       ((MESG)->DATA.u8[(IDX)])

and the message struct itself has a big union for the data:

typedef struct {
          uint8_t version;
          uint8_t size;
          uint8_t svc;
          uint8_t func;
          union {
                  int32_t i32[(SC_RPC_MAX_MSG - 1U)];
                  int16_t i16[(SC_RPC_MAX_MSG - 1U) * 2U];
                  int8_t i8[(SC_RPC_MAX_MSG - 1U) * 4U];
                  uint32_t u32[(SC_RPC_MAX_MSG - 1U)];
                  uint16_t u16[(SC_RPC_MAX_MSG - 1U) * 2U];
                  uint8_t u8[(SC_RPC_MAX_MSG - 1U) * 4U];
          } DATA;
} sc_rpc_msg_t;

This approach is very verbose to the point of being unreadable I think 
it's much to message structs instead. Compiler struct layout rules are 
not really all that complicated and casting binary data as structs is 
very common in areas such as networking. This approach is also used by 
other firmware interfaces like TI sci and nvidia bpmp.

imx8 currently has manually written message structs, it's unfortunate 
that a bug was found and fixing required a scattering patches in 
multiple subsystems. Perhaps a better solution would be to centralize 
all structs in a single header similar to drivers/firmware/ti_sci.h?

In order to ensrue that there are no issues specific to the compile 
version perhaps a bunch of static_assert statements could be added to 
check that sizeof and offset are as expected?

---------------------------------

As far as I can tell the issue KASAN warns about can be simplified to this:

struct __packed badpack {
     u32     a;
     u16     b;
     u8      c;
};

static_assert(sizeof(struct badpack) == 7);

static void func(void *x)
{
     u32* arr = (u32*)x;
     arr[0] = 0x11111111;
     arr[1] = 0x22222222;
}

static int hello(void)
{
     struct badpack s;
     u8 x = 0x33;

     printk("&s=%px &x=%px\n", &s, &x);
     func(&s);
     // x could be overwritten here, depending on stack layout.
     BUG_ON(x != 0x33);

     return 0;
}

Adding __aligned(4) bumps struct size to 8 and avoids the issue

Added KASAN maintainers to check if this is a valid fix.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel