[systemd-devel] [PATCH] libsystemd-bus: make sure buf transfered into kenrel is 8 aligned

2013-12-16 Thread Yin Kangkai 
From: Yin Kangkai kangkai@intel.com

Otherwise, for example hello arg passed to KDBUS_CMD_HELLO might not be 8 bytes
aligned and kernel returns -EFAULT.

319 int bus_kernel_take_fd(sd_bus *b) {
320 struct kdbus_cmd_hello hello;

(gdb) p hello
$8 = (struct kdbus_cmd_hello *) 0xb354
---
 src/libsystemd-bus/bus-control.c | 4 ++--
 src/libsystemd-bus/bus-kernel.c  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/libsystemd-bus/bus-control.c b/src/libsystemd-bus/bus-control.c
index 0072c37..6b2790d 100644
--- a/src/libsystemd-bus/bus-control.c
+++ b/src/libsystemd-bus/bus-control.c
@@ -207,7 +207,7 @@ _public_ int sd_bus_release_name(sd_bus *bus, const char 
*name) {
 }
 
 static int kernel_get_list(sd_bus *bus, uint64_t flags, char ***x) {
-struct kdbus_cmd_name_list cmd = {};
+struct kdbus_cmd_name_list __attribute__ ((__aligned__(8))) cmd = {};
 struct kdbus_name_list *name_list;
 struct kdbus_cmd_name *name;
 uint64_t previous_id = 0;
@@ -1088,7 +1088,7 @@ static int bus_remove_match_internal_kernel(
 const char *match,
 uint64_t cookie) {
 
-struct kdbus_cmd_match m;
+struct kdbus_cmd_match __attribute__ ((__aligned__(8))) m;
 int r;
 
 assert(bus);
diff --git a/src/libsystemd-bus/bus-kernel.c b/src/libsystemd-bus/bus-kernel.c
index 0e47308..4947d39 100644
--- a/src/libsystemd-bus/bus-kernel.c
+++ b/src/libsystemd-bus/bus-kernel.c
@@ -317,7 +317,7 @@ fail:
 }
 
 int bus_kernel_take_fd(sd_bus *b) {
-struct kdbus_cmd_hello hello;
+struct kdbus_cmd_hello __attribute__ ((__aligned__(8))) hello;
 int r;
 
 assert(b);
-- 
1.8.2.1

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] libsystemd-bus: make sure buf transfered into kenrel is 8 aligned

2013-12-16 Thread Kay Sievers 
On Mon, Dec 16, 2013 at 4:01 PM, Lennart Poettering
lenn...@poettering.net wrote:
 On Mon, 16.12.13 15:50, Lennart Poettering (lenn...@poettering.net) wrote:

  diff --git a/src/libsystemd-bus/bus-control.c 
  b/src/libsystemd-bus/bus-control.c
  index 0072c37..6b2790d 100644
  --- a/src/libsystemd-bus/bus-control.c
  +++ b/src/libsystemd-bus/bus-control.c
  @@ -207,7 +207,7 @@ _public_ int sd_bus_release_name(sd_bus *bus, const 
  char *name) {
   }
 
   static int kernel_get_list(sd_bus *bus, uint64_t flags, char ***x) {
  -struct kdbus_cmd_name_list cmd = {};
  +struct kdbus_cmd_name_list __attribute__ ((__aligned__(8)))
  cmd = {};

 Hmm, this feels a bit like this would be better part of the type rather
 than the variable. THus, kdbus.h should add this to all is structs,
 rather then we decorate the variables...

 Kay, would this make sense to you to add to kdbus.h?

 Hmm, so thinking about this: the kdbus_cmd_name_list structure contains
 64bit values anyway, so should naturally be aligned to 64bit boundaries
 anyway... Or am I mistaken there and you are suggesting that on your
 32bit architecture (which one is it if I may ask?) 64bit values don't
 have to be aligned on an even 8byte boundary, but instead because the
 arch is 32bit anyway and thus 64bit values need to be read in two steps
 alignment on 4 is done in the abi?

We could try to define size, which is the first member in the struct, as:
  __aligned_u64
instead of the current:
  u64

That might do the trick. Could you test that if it works on your
platform/setup instead of patching the lib?

Thanks,
Kay
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] libsystemd-bus: make sure buf transfered into kenrel is 8 aligned

2013-12-16 Thread Kay Sievers 
On Mon, Dec 16, 2013 at 4:09 PM, Kay Sievers k...@vrfy.org wrote:
 On Mon, Dec 16, 2013 at 4:01 PM, Lennart Poettering
 lenn...@poettering.net wrote:
 On Mon, 16.12.13 15:50, Lennart Poettering (lenn...@poettering.net) wrote:

  diff --git a/src/libsystemd-bus/bus-control.c 
  b/src/libsystemd-bus/bus-control.c
  index 0072c37..6b2790d 100644
  --- a/src/libsystemd-bus/bus-control.c
  +++ b/src/libsystemd-bus/bus-control.c
  @@ -207,7 +207,7 @@ _public_ int sd_bus_release_name(sd_bus *bus, const 
  char *name) {
   }
 
   static int kernel_get_list(sd_bus *bus, uint64_t flags, char ***x) {
  -struct kdbus_cmd_name_list cmd = {};
  +struct kdbus_cmd_name_list __attribute__ ((__aligned__(8)))
  cmd = {};

 Hmm, this feels a bit like this would be better part of the type rather
 than the variable. THus, kdbus.h should add this to all is structs,
 rather then we decorate the variables...

 Kay, would this make sense to you to add to kdbus.h?

 Hmm, so thinking about this: the kdbus_cmd_name_list structure contains
 64bit values anyway, so should naturally be aligned to 64bit boundaries
 anyway... Or am I mistaken there and you are suggesting that on your
 32bit architecture (which one is it if I may ask?) 64bit values don't
 have to be aligned on an even 8byte boundary, but instead because the
 arch is 32bit anyway and thus 64bit values need to be read in two steps
 alignment on 4 is done in the abi?

 We could try to define size, which is the first member in the struct, as:
   __aligned_u64
 instead of the current:
   u64

 That might do the trick. Could you test that if it works on your
 platform/setup instead of patching the lib?

Just added __attribute__ ((__aligned__(8))) to kdbus.h for structures
used in ioctls.

Kay
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] libsystemd-bus: make sure buf transfered into kenrel is 8 aligned

2013-12-16 Thread Yin Kangkai 
On 2013-12-16, 16:01 +0100, Lennart Poettering wrote:
 On Mon, 16.12.13 15:50, Lennart Poettering (lenn...@poettering.net) wrote:
 
   diff --git a/src/libsystemd-bus/bus-control.c 
   b/src/libsystemd-bus/bus-control.c
   index 0072c37..6b2790d 100644
   --- a/src/libsystemd-bus/bus-control.c
   +++ b/src/libsystemd-bus/bus-control.c
   @@ -207,7 +207,7 @@ _public_ int sd_bus_release_name(sd_bus *bus, const 
   char *name) {
}

static int kernel_get_list(sd_bus *bus, uint64_t flags, char ***x) {
   -struct kdbus_cmd_name_list cmd = {};
   +struct kdbus_cmd_name_list __attribute__ ((__aligned__(8)))
   cmd = {};
  
  Hmm, this feels a bit like this would be better part of the type rather
  than the variable. THus, kdbus.h should add this to all is structs,
  rather then we decorate the variables...
  
  Kay, would this make sense to you to add to kdbus.h?
 
 Hmm, so thinking about this: the kdbus_cmd_name_list structure contains
 64bit values anyway, so should naturally be aligned to 64bit boundaries
 anyway... Or am I mistaken there and you are suggesting that on your
 32bit architecture (which one is it if I may ask?) 64bit values don't

I am doing tests in 32bit system.

 have to be aligned on an even 8byte boundary, but instead because the
 arch is 32bit anyway and thus 64bit values need to be read in two steps
 alignment on 4 is done in the abi?

I am getting a little more clearer about how gcc tries to make stack
boundary aligned..

I am using this example test code:

8
#include stdio.h
#include linux/types.h

struct kdbus_cmd_hello {
  __u64 size;
  __u64 conn_flags;
  __u64 attach_flags;
  __u64 bus_flags;
  __u64 id;
  __u64 bloom_size;
  __u64 pool_size;
  __u8 id128[16];
};

int main(int argc, char **argv)
{
struct kdbus_cmd_hello hello;
int r;

printf(hello addr: %08x\n, hello);
r = 1;

return 0;
}
8

By default, gcc will *try* to make stack boundary 16 bytes aligned
(pls refer to -mpreferred-stack-boundary=num in gcc mannual).

struct kdbus_cmd_hello itself is 64bit aligned, yes. However, if you
have a int r after the kdbus_cmd_hello, address of hello might be
16 bytes boundary + 4 (so that after pushing r, stack boundary
will be 16 bytes aligned).

$ ./test 
hello addr: bfe8d334
$ ./test 
hello addr: bf9aa9f4
$ ./test 
hello addr: bfe443c4
$ ./test 
hello addr: bfe8d334
$ ./test 
hello addr: bf9fdf24

Regards,
Kangkai

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] libsystemd-bus: make sure buf transfered into kenrel is 8 aligned

2013-12-16 Thread Yin Kangkai 
On 2013-12-16, 17:49 +0100, Kay Sievers wrote:
 On Mon, Dec 16, 2013 at 4:09 PM, Kay Sievers k...@vrfy.org wrote:
 Just added __attribute__ ((__aligned__(8))) to kdbus.h for structures
 used in ioctls.

Yes, your patch is much more cleaner, thanks Kay.

Regards,
Kangkai
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel