[systemd-devel] [PATCH 5/7] kdbus: improve user domain accounting
Djalal Harouni
tixxdz at opendz.org
Wed Jul 23 14:19:09 PDT 2014
Currently kdbus_domain_user_find_or_new() is used to find a user domain
or create a new one and link it into the domain.
kdbus_domain_user_find_or_new() may fail due to memory allocation
errors or if the domain was shutdown, but since callers will receive
only a NULL pointer on failure, they assume -ENOMEM and ignore
-ESHUTDOWN.
To fix this we make kdbus_domain_user_find_or_new() return an ERR_PTR()
on failure, and we update all callers.
The patch also simplifies kdbus_domain_user_find_or_new() by using the
previously added helpers.
Another point is that by using kdbus_domain_user_account() inside
kdbus_domain_user_find_or_new() we make user index start from 1 instead
of 0, this allows:
Other parts of the code to directly call kdbus_domain_user_account() and
to be sure that user was really accounted and its index is 1 instead of
0.
0 is the default value given to user->idr after user = kzalloc(), if we
want other parts of the code to direclty account users, we must
differenciate the two values:
1) 0 as a valid user index
user->idr = idr_alloc(&domain->user_idr, user, 0, 0, GFP_KERNEL)
And
2) 0 after kzalloc()
struct kdbus_domain_user *user = kzalloc()
So convert the former to 1, and adapt other parts of the code to treat 1
as the starting id of user indexes.
Signed-off-by: Djalal Harouni <tixxdz at opendz.org>
---
bus.c | 4 ++--
connection.c | 19 +++++++--------
domain.c | 76 +++++++++++++++++++++---------------------------------------
handle.c | 4 ++--
4 files changed, 40 insertions(+), 63 deletions(-)
diff --git a/bus.c b/bus.c
index d09e3c6..c329bef 100644
--- a/bus.c
+++ b/bus.c
@@ -271,8 +271,8 @@ int kdbus_bus_new(struct kdbus_domain *domain,
/* account the bus against the user */
b->user = kdbus_domain_user_find_or_new(domain, uid);
- if (!b->user) {
- ret = -ENOMEM;
+ if (IS_ERR(b->user)) {
+ ret = PTR_ERR(b->user);
goto exit_ep_unref;
}
diff --git a/connection.c b/connection.c
index 1658a92..c432286 100644
--- a/connection.c
+++ b/connection.c
@@ -60,7 +60,7 @@ struct kdbus_conn_reply;
* @dst_name_id: The sequence number of the name this message is
* addressed to, 0 for messages sent to an ID
* @reply: The reply block if a reply to this message is expected.
- * @user: Index in per-user message counter, -1 for unused
+ * @user: Index in per-user message counter, 0 for unused
*/
struct kdbus_conn_queue {
struct list_head entry;
@@ -78,7 +78,7 @@ struct kdbus_conn_queue {
u64 cookie;
u64 dst_name_id;
struct kdbus_conn_reply *reply;
- int user;
+ unsigned int user;
};
/**
@@ -397,10 +397,10 @@ static void kdbus_conn_queue_remove(struct kdbus_conn *conn,
conn->msg_count--;
/* user quota */
- if (queue->user >= 0) {
+ if (queue->user > 0) {
BUG_ON(conn->msg_users[queue->user] == 0);
conn->msg_users[queue->user]--;
- queue->user = -1;
+ queue->user = 0;
}
/* the queue is empty, remove the user quota accounting */
@@ -471,8 +471,6 @@ static int kdbus_conn_queue_alloc(struct kdbus_conn *conn,
if (!queue)
return -ENOMEM;
- queue->user = -1;
-
/* copy message properties we need for the queue management */
queue->src_id = kmsg->msg.src_id;
queue->cookie = kmsg->msg.cookie;
@@ -2092,12 +2090,13 @@ int kdbus_conn_new(struct kdbus_ep *ep,
*/
if (ep->user)
conn->user = kdbus_domain_user_ref(ep->user);
- else
+ else {
conn->user = kdbus_domain_user_find_or_new(ep->bus->domain,
current_fsuid());
- if (!conn->user) {
- ret = -ENOMEM;
- goto exit_free_meta;
+ if (IS_ERR(conn->user)) {
+ ret = PTR_ERR(conn->user);
+ goto exit_free_meta;
+ }
}
/* lock order: domain -> bus -> ep -> names -> conn */
diff --git a/domain.c b/domain.c
index a5abb2d..81a9667 100644
--- a/domain.c
+++ b/domain.c
@@ -3,6 +3,7 @@
* Copyright (C) 2013 Greg Kroah-Hartman <gregkh at linuxfoundation.org>
* Copyright (C) 2013 Daniel Mack <daniel at zonque.org>
* Copyright (C) 2013 Linux Foundation
+ * Copyright (C) 2014 Djalal Harouni
*
* kdbus is free software; you can redistribute it and/or modify it under
* the terms of the GNU Lesser General Public License as published by the
@@ -540,64 +541,38 @@ int kdbus_domain_user_account(struct kdbus_domain *domain,
* user like a custom endpoint
*
* Return: a kdbus_domain_user, either freshly allocated or with the reference
- * counter increased. In case of memory allocation failure, NULL is returned.
+ * counter increased. On failure, an ERR_PTR() that contains either -ENOMEM or
+ * -ESHUTDOWN is returned.
+ *
+ * On success the user is automatically accounted and linked into the
+ * domain.
*/
-struct kdbus_domain_user
-*kdbus_domain_user_find_or_new(struct kdbus_domain *domain, kuid_t uid)
+struct kdbus_domain_user *
+kdbus_domain_user_find_or_new(struct kdbus_domain *domain, kuid_t uid)
{
struct kdbus_domain_user *u;
int ret;
- /* find uid and reference it */
- if (uid_valid(uid)) {
- mutex_lock(&domain->lock);
- hash_for_each_possible(domain->user_hash, u,
- hentry, __kuid_val(uid)) {
- if (!uid_eq(u->uid, uid))
- continue;
-
- kref_get(&u->kref);
- mutex_unlock(&domain->lock);
- return u;
- }
- mutex_unlock(&domain->lock);
- }
+ u = kdbus_domain_user_find(domain, uid);
+ if (u)
+ return u;
- /* allocate a new user */
- u = kzalloc(sizeof(*u), GFP_KERNEL);
+ ret = -ENOMEM;
+ u = kdbus_domain_user_new(domain, uid);
if (!u)
- return NULL;
-
- kref_init(&u->kref);
- u->domain = kdbus_domain_ref(domain);
- u->uid = uid;
- atomic_set(&u->buses, 0);
- atomic_set(&u->connections, 0);
+ goto err;
/* link into domain */
- mutex_lock(&domain->lock);
- if (domain->disconnected) {
- mutex_unlock(&domain->lock);
- kfree(u);
- return NULL;
- }
-
- /*
- * Allocate the smallest possible index for this user; used
- * in arrays for accounting user quota in receiver queues.
- */
- ret = idr_alloc(&domain->user_idr, u, 0, 0, GFP_KERNEL);
+ ret = kdbus_domain_user_account(domain, u);
if (ret < 0) {
- mutex_unlock(&domain->lock);
- return NULL;
+ kfree(u);
+ goto err;
}
- u->idr = ret;
-
- /* UID hash map */
- hash_add(domain->user_hash, &u->hentry, __kuid_val(u->uid));
- mutex_unlock(&domain->lock);
return u;
+
+err:
+ return ERR_PTR(ret);
}
static void __kdbus_domain_user_free(struct kref *kref)
@@ -608,10 +583,13 @@ static void __kdbus_domain_user_free(struct kref *kref)
BUG_ON(atomic_read(&user->buses) > 0);
BUG_ON(atomic_read(&user->connections) > 0);
- mutex_lock(&user->domain->lock);
- idr_remove(&user->domain->user_idr, user->idr);
- hash_del(&user->hentry);
- mutex_unlock(&user->domain->lock);
+ /* if the user was accounted into the domain */
+ if (user->idr > 0) {
+ mutex_lock(&user->domain->lock);
+ idr_remove(&user->domain->user_idr, user->idr);
+ hash_del(&user->hentry);
+ mutex_unlock(&user->domain->lock);
+ }
kdbus_domain_unref(user->domain);
kfree(user);
diff --git a/handle.c b/handle.c
index bf32c6e..6e244c8 100644
--- a/handle.c
+++ b/handle.c
@@ -466,9 +466,9 @@ static long kdbus_handle_ioctl_ep(struct file *file, unsigned int cmd,
*/
ep->user = kdbus_domain_user_find_or_new(
handle->ep->bus->domain, INVALID_UID);
- if (!ep->user) {
+ if (IS_ERR(ep->user)) {
+ ret = PTR_ERR(ep->user);
kdbus_ep_unref(ep);
- ret = -ENOMEM;
break;
}
--
1.9.3
More information about the systemd-devel
mailing list