[systemd-devel] [PATCH 1/2] connection: fix cpu stall by checking kdbus item alignment
Djalal Harouni
tixxdz at opendz.org
Wed Jun 11 09:36:25 PDT 2014
Hi,
The tests are not clean... just a copy/past if you want to confirm this
one! they are attached.
patch with policy-holder.patch
then run: test-kdbus-policy-holder
You should hit it!
On Wed, Jun 11, 2014 at 05:27:58PM +0100, Djalal Harouni wrote:
> Fix the following stall triggered by a policy holder hello:
> call test/kdbus-util.c:kdbus_hello_registrar() and pass
> KDBUS_HELLO_POLICY_HOLDER as a flag.
>
> While we are it make sure that kdbus_cmd_conn_update() also checks for
> proper 8 byte alignment.
>
> [ 142.731011] INFO: rcu_sched self-detected stall on CPU { 3} (t=65000 jiffies g=3085 c=3084 q=4316)
> [ 142.731011] sending NMI to all CPUs:
> [ 142.731011] NMI backtrace for cpu 3
> [ 142.731011] CPU: 3 PID: 1352 Comm: test-kdbus-poli Tainted: G OE 3.15.0-0.rc8.git4.1.fc21.x86_64 #1
> [ 142.731011] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [ 142.731011] task: ffff88005a2719d0 ti: ffff88004587c000 task.ti: ffff88004587c000
> [ 142.731011] RIP: 0010:[<ffffffff81055a12>] [<ffffffff81055a12>] flat_send_IPI_all+0x92/0xd0
> [ 142.731011] RSP: 0018:ffff88005dc03dc0 EFLAGS: 00010006
> [ 142.731011] RAX: 0000000000000000 RBX: 0000000000000c00 RCX: 0000000000000000
> [ 142.731011] RDX: 0000000000000c00 RSI: 0000000000000000 RDI: 0000000000000300
> [ 142.731011] RBP: ffff88005dc03dd8 R08: 0000000000000001 R09: 0000000000000001
> [ 142.731011] R10: ffffffff81e5d720 R11: 0000000000000001 R12: 0000000000000046
> [ 142.731011] R13: 000000000000000f R14: ffffffff81e86880 R15: 0000000000000003
> [ 142.731011] FS: 00007f4b9d727740(0000) GS:ffff88005dc00000(0000) knlGS:0000000000000000
> [ 142.731011] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [ 142.731011] CR2: 0000000000401312 CR3: 000000005b66f000 CR4: 00000000000006e0
> [ 142.731011] Stack:
> [ 142.731011] 0000000000002710 ffffffff81e86880 ffffffff81fa3e40 ffff88005dc03df0
> [ 142.731011] ffffffff81050654 ffff88005ddcf000 ffff88005dc03e48 ffffffff8111a194
> [ 142.731011] 0000000000000000 7fffffffffffffff 0000000000000046 00000000000010dc
> [ 142.731011] Call Trace:
> [ 142.731011] <IRQ>
>
> [ 142.731011] [<ffffffff81050654>] arch_trigger_all_cpu_backtrace+0x64/0xa0
> [ 142.731011] [<ffffffff8111a194>] rcu_check_callbacks+0x584/0x850
> [ 142.731011] [<ffffffff810a74a7>] update_process_times+0x47/0x70
> [ 142.731011] [<ffffffff81126765>] tick_sched_handle.isra.19+0x25/0x60
> [ 142.731011] [<ffffffff81127061>] tick_sched_timer+0x41/0x60
> [ 142.731011] [<ffffffff810c7446>] __run_hrtimer+0x86/0x460
> [ 142.731011] [<ffffffff81127020>] ? tick_sched_do_timer+0x40/0x40
> [ 142.731011] [<ffffffff810c809f>] hrtimer_interrupt+0x10f/0x260
> [ 142.731011] [<ffffffff8104e55a>] local_apic_timer_interrupt+0x3a/0x60
> [ 142.731011] [<ffffffff8180089f>] smp_apic_timer_interrupt+0x3f/0x50
> [ 142.731011] [<ffffffff817ff1f2>] apic_timer_interrupt+0x72/0x80
> [ 142.731011] <EOI>
>
> [ 142.731011] [<ffffffff817f42b3>] ? retint_restore_args+0x13/0x13
> [ 142.731011] [<ffffffffa02a666b>] ? kdbus_conn_new+0x25b/0xf20 [kdbus]
> [ 142.731011] [<ffffffffa02a6741>] ? kdbus_conn_new+0x331/0xf20 [kdbus]
> [ 142.731011] [<ffffffffa02a8161>] kdbus_handle_ioctl+0x221/0xad0 [kdbus]
> [ 142.731011] [<ffffffff81361a31>] ? inode_has_perm.isra.47+0x51/0x90
> [ 142.731011] [<ffffffff81251f60>] do_vfs_ioctl+0x2f0/0x520
> [ 142.731011] [<ffffffff81252211>] SyS_ioctl+0x81/0xa0
> [ 142.731011] [<ffffffff817fe4e9>] system_call_fastpath+0x16/0x1b
>
> Signed-off-by: Djalal Harouni <tixxdz at opendz.org>
> ---
> I've checked all the other calls, the remaining one is:
> connection.c:kdbus_conn_payload_add() it seems that it fakes the size
> and hanldes the alignment, if I've more time I'll try to check it. This
> one is tricky!
>
> connection.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/connection.c b/connection.c
> index 3f27889..3e8c5de 100644
> --- a/connection.c
> +++ b/connection.c
> @@ -1784,12 +1784,18 @@ int kdbus_cmd_conn_update(struct kdbus_conn *conn,
> {
> const struct kdbus_item *item;
> bool policy_provided = false;
> + u64 attach_flags = 0;
> int ret;
>
> KDBUS_ITEMS_FOREACH(item, cmd->items, KDBUS_ITEMS_SIZE(cmd, items)) {
> +
> + if (!KDBUS_ITEM_VALID(item, &cmd->items,
> + KDBUS_ITEMS_SIZE(cmd, items)))
> + return -EINVAL;
> +
> switch (item->type) {
> case KDBUS_ITEM_ATTACH_FLAGS:
> - conn->attach_flags = item->data64[0];
> + attach_flags = item->data64[0];
> break;
> case KDBUS_ITEM_NAME:
> case KDBUS_ITEM_POLICY_ACCESS:
> @@ -1798,6 +1804,11 @@ int kdbus_cmd_conn_update(struct kdbus_conn *conn,
> }
> }
>
> + if (!KDBUS_ITEMS_END(item, cmd->items, KDBUS_ITEMS_SIZE(cmd, items)))
> + return -EINVAL;
> +
> + conn->attach_flags = attach_flags;
> +
> if (!policy_provided)
> return 0;
>
> @@ -1862,6 +1873,11 @@ int kdbus_conn_new(struct kdbus_ep *ep,
>
> KDBUS_ITEMS_FOREACH(item, hello->items,
> KDBUS_ITEMS_SIZE(hello, items)) {
> +
> + if (!KDBUS_ITEM_VALID(item, &hello->items,
> + KDBUS_ITEMS_SIZE(hello, items)))
> + return -EINVAL;
> +
> switch (item->type) {
> case KDBUS_ITEM_NAME:
> if (!is_activator && !is_policy_holder)
> @@ -1916,6 +1932,9 @@ int kdbus_conn_new(struct kdbus_ep *ep,
> }
> }
>
> + if (!KDBUS_ITEMS_END(item, hello->items, KDBUS_ITEMS_SIZE(hello, items)))
> + return -EINVAL;
> +
> if ((is_activator || is_policy_holder) && !name)
> return -EINVAL;
>
> --
> 1.9.0
>
--
Djalal Harouni
http://opendz.org
-------------- next part --------------
diff --git a/test/Makefile b/test/Makefile
index 83cb736..0d57eed 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -20,7 +20,8 @@ TESTS= \
test-kdbus-chat \
test-kdbus-timeout \
test-kdbus-sync \
- test-kdbus-prio
+ test-kdbus-prio \
+ test-kdbus-policy-holder
all: $(TESTS)
diff --git a/test/kdbus-util.c b/test/kdbus-util.c
index 5316263..77bfa47 100644
--- a/test/kdbus-util.c
+++ b/test/kdbus-util.c
@@ -109,7 +109,7 @@ struct conn *kdbus_hello(const char *path, uint64_t flags)
return __kdbus_hello(path, flags, NULL, 0);
}
-static struct conn *
+struct conn *
kdbus_hello_registrar(const char *path, const char *name,
const struct kdbus_policy_access *access,
size_t num_access, uint64_t flags)
diff --git a/test/kdbus-util.h b/test/kdbus-util.h
index fde3f77..9771622 100644
--- a/test/kdbus-util.h
+++ b/test/kdbus-util.h
@@ -45,6 +45,9 @@ char *msg_id(uint64_t id, char *buf);
int msg_send(const struct conn *conn, const char *name, uint64_t cookie,
uint64_t flags, uint64_t timeout, int64_t priority, uint64_t dst_id);
struct conn *kdbus_hello(const char *path, uint64_t hello_flags);
+struct conn *kdbus_hello_registrar(const char *path, const char *name,
+ const struct kdbus_policy_access *access,
+ size_t num_access, uint64_t flags);
struct conn *kdbus_hello_activator(const char *path, const char *name,
const struct kdbus_policy_access *access,
size_t num_access);
-------------- next part --------------
#include <stdio.h>
#include <string.h>
#include <fcntl.h>
#include <pthread.h>
#include <poll.h>
#include <stdlib.h>
#include <stddef.h>
#include <stdint.h>
#include <stdbool.h>
#include <unistd.h>
#include <errno.h>
#include <sys/ioctl.h>
#include "kdbus-util.h"
#include "kdbus-enum.h"
#define MAX_CONN 128
#define POLICY_PATH "foo.test.policy-test"
struct conn_entry {
bool policy_holder;
bool cached_send;
struct conn *conn;
};
static int kdbus_register_policy(char *bus, struct conn_entry **e)
{
int ret = -1;
struct conn_entry *ce;
struct kdbus_policy_access access[2];
ce = malloc(sizeof(struct conn_entry));
if (!ce)
return -ENOMEM;
memset(ce, 0, sizeof(struct conn_entry));
access[0].type = KDBUS_POLICY_ACCESS_WORLD;
access[0].access = KDBUS_POLICY_TALK;
access[0].id = 1001;
access[1].type = KDBUS_POLICY_ACCESS_WORLD;
access[1].access = KDBUS_POLICY_TALK;
ce->conn = kdbus_hello_registrar(bus, POLICY_PATH, access, 2,
KDBUS_HELLO_POLICY_HOLDER);
if (!ce->conn)
return ret;
*e = ce;
return 0;
}
static void *kdbus_recv_echo(void *ptr)
{
int ret;
int cnt = 3;
struct pollfd fd;
struct conn *conn = ptr;
fd.fd = conn->fd;
fd.events = POLLIN | POLLPRI | POLLHUP;
fd.revents = 0;
while (cnt) {
cnt--;
ret = poll(&fd, 1, 2000);
if (ret == 0) {
ret = -ETIMEDOUT;
break;
}
if (ret > 0 && fd.revents & POLLIN) {
printf("-- Connection id: %lu new received message:\n",
conn->id);
ret = msg_recv(conn);
}
if (ret >= 0 || ret != -EAGAIN)
break;
}
return (void *)(long)ret;
}
static int kdbus_test_connections(struct conn_entry **conn_db)
{
int ret;
unsigned int i, tid;
unsigned long cookie = 1;
unsigned int thread_nr = MAX_CONN - 1;
pthread_t thread_id[MAX_CONN - 1] = {'\0'};
for (tid = 0, i = 1; tid < thread_nr; tid++, i++) {
ret = pthread_create(&thread_id[tid], NULL,
kdbus_recv_echo, (void *)conn_db[0]->conn);
if (ret < 0) {
fprintf(stderr, "error pthread_create: %d err %d (%m)\n",
ret, errno);
break;
}
ret = msg_send(conn_db[i]->conn, POLICY_PATH,
cookie++, 0, 0, 0,
conn_db[0]->conn->id);
if (ret != 0)
break;
}
for (tid = 0; tid < thread_nr; tid++) {
int thread_ret = 0;
if (thread_id[tid]) {
pthread_join(thread_id[tid], (void *)&thread_ret);
/* Update ret only if it is >= 0
* msg_send may finish with:
* EXIT_FAILURE or EXIT_SUCCESS */
if (thread_ret < 0 && ret >= 0)
ret = thread_ret;
}
}
return ret;
}
static void kdbus_free_conn_entry(struct conn_entry *e)
{
if (e) {
if (e->conn) {
close(e->conn->fd);
free(e->conn);
}
free(e);
}
}
static int kdbus_check_policy(char *bus)
{
int i;
int ret;
struct conn_entry **conn_db = NULL;
conn_db = calloc(MAX_CONN, sizeof(struct conn_entry *));
if (!conn_db)
return -ENOMEM;
memset(conn_db, 0, MAX_CONN * sizeof(struct conn_entry *));
printf("-- Round 1) creating connections:\n");
ret = kdbus_register_policy(bus, &conn_db[0]);
if (ret < 0)
goto out_free_connections;
for (i = 1; i < MAX_CONN; i++) {
struct conn_entry *e = malloc(sizeof(struct conn_entry));
if (!e) {
ret = -ENOMEM;
goto out_free_connections;
}
memset(e, 0, sizeof(struct conn_entry));
*(conn_db+i) = e;
e->conn = kdbus_hello(bus, 0);
if (!e->conn) {
ret = -1;
goto out_free_connections;
}
e->cached_send = true;
add_match_empty(e->conn->fd);
}
ret = kdbus_test_connections(conn_db);
printf("-- Round 2) testing connections............... ");
if (ret < 0) {
printf("FAILED\n");
goto out_free_connections;
}
printf("OK\n");
out_free_connections:
for (i = 0; i < MAX_CONN; i++)
kdbus_free_conn_entry(conn_db[i]);
free(conn_db);
return ret;
}
int main(int argc, char *argv[])
{
struct {
struct kdbus_cmd_make head;
/* bloom size item */
struct {
uint64_t size;
uint64_t type;
struct kdbus_bloom_parameter bloom;
} bs;
/* name item */
uint64_t n_size;
uint64_t n_type;
char name[64];
} bus_make;
int fdc, ret, i;
char *bus;
printf("-- opening /dev/" KBUILD_MODNAME "/control\n");
fdc = open("/dev/" KBUILD_MODNAME "/control", O_RDWR|O_CLOEXEC);
if (fdc < 0) {
fprintf(stderr, "--- error %d (%m)\n", fdc);
return EXIT_FAILURE;
}
memset(&bus_make, 0, sizeof(bus_make));
bus_make.bs.size = sizeof(bus_make.bs);
bus_make.bs.type = KDBUS_ITEM_BLOOM_PARAMETER;
bus_make.bs.bloom.size = 64;
bus_make.bs.bloom.n_hash = 1;
snprintf(bus_make.name, sizeof(bus_make.name), "%u-testbus", getuid());
bus_make.n_type = KDBUS_ITEM_MAKE_NAME;
bus_make.n_size = KDBUS_ITEM_HEADER_SIZE + strlen(bus_make.name) + 1;
bus_make.head.size = sizeof(struct kdbus_cmd_make) +
sizeof(bus_make.bs) +
bus_make.n_size;
printf("-- creating bus '%s'\n", bus_make.name);
ret = ioctl(fdc, KDBUS_CMD_BUS_MAKE, &bus_make);
if (ret) {
fprintf(stderr, "--- error %d (%m)\n", ret);
return EXIT_FAILURE;
}
if (asprintf(&bus, "/dev/" KBUILD_MODNAME "/%s/bus", bus_make.name) < 0)
return EXIT_FAILURE;
ret = kdbus_check_policy(bus);
printf("RUNNING TEST 'policy db check' ");
for (i = 0; i < 15; i++)
printf(".");
printf(" ");
if (ret < 0) {
printf("FAILED\n");
return EXIT_FAILURE;
}
printf("OK\n");
close(fdc);
free(bus);
return EXIT_SUCCESS;
}
More information about the systemd-devel
mailing list