kdbus cleanups

- Fix typos and spelling errors, use proper case.

- struct kdbus_pids: Add PPID to description.

- struct kdbus_item: Add missing @pids, @fsd and
KDBUS_ITEM_PAYLOAD_OFF.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 include/uapi/linux/kdbus.h | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

include/uapi/linux/kdbus.h +4 -4

index 4fc44cb1d4a8..09b23af87ce6 100644
@@ -21,7 +21,7 @@
 /**
  * struct kdbus_notify_id_change - name registry change message
  * @id:                        New or former owner of the name
- * @flags:             flags field from KDBUS_HELLO_*
+ * @flags:             Flags field from KDBUS_HELLO_*
  *
  * Sent from kernel to userspace when the owner or activator of
  * a well-known name changes.

@@ -86,7 +86,7 @@ struct kdbus_creds {
  * @tid:               Thread ID
  * @ppid:              Parent process ID
  *
- * The PID and TID of a process.
+ * The PID, TID and PPID of a process.
  *
  * Attached to:
  *   KDBUS_ITEM_PIDS

@@ -212,7 +212,7 @@ struct kdbus_name {
  * enum kdbus_policy_access_type - permissions of a policy record
  * @_KDBUS_POLICY_ACCESS_NULL: Uninitialized/invalid
  * @KDBUS_POLICY_ACCESS_USER:  Grant access to a uid
- * @KDBUS_POLICY_ACCESS_GROUP: Grant access to gid
+ * @KDBUS_POLICY_ACCESS_GROUP: Grant access to a gid
  * @KDBUS_POLICY_ACCESS_WORLD: World-accessible
  */
 enum kdbus_policy_access_type {

@@ -267,7 +267,7 @@ struct kdbus_policy_access {
  * @KDBUS_ATTACH_CONN_DESCRIPTION:     The human-readable connection name
  * @_KDBUS_ATTACH_ALL:                 All of the above
  * @_KDBUS_ATTACH_ANY:                 Wildcard match to enable any kind of
- *                                     metatdata.
+ *                                     metadata
  */
 enum kdbus_attach_flags {
        KDBUS_ATTACH_TIMESTAMP          =  1ULL <<  0,

include/uapi/linux/kdbus.h +2 -2

  * @data:              Generic bytes
- * @data32:            Generic 32 bit array
- * @data64:            Generic 64 bit array
+ * @data32:            Generic 32-bit array
+ * @data64:            Generic 64-bit array
  * @str:               Generic string
  * @id:                        Connection ID
  * @vec:               KDBUS_ITEM_PAYLOAD_VEC

include/uapi/linux/kdbus.h +12 -13

  * @name_change:       KDBUS_ITEM_NAME_ADD
  *                     KDBUS_ITEM_NAME_REMOVE
  *                     KDBUS_ITEM_NAME_CHANGE

@@ -721,10 +724,10 @@ struct kdbus_cmd_hello {
 
 /**
  * struct kdbus_info - connection information
- * @size:              total size of the struct
- * @id:                        64bit object ID
- * @flags:             object creation flags
- * @items:             list of items
+ * @size:              Total size of the struct
+ * @id:                        64-bit object ID
+ * @flags:             Object creation flags
+ * @items:             List of items
  *
  * Note that the user is responsible for freeing the allocated memory with
  * the KDBUS_CMD_FREE ioctl.

@@ -738,10 +741,10 @@ struct kdbus_info {
 
 /**
  * enum kdbus_list_flags - what to include into the returned list
- * @KDBUS_LIST_UNIQUE:         active connections
- * @KDBUS_LIST_ACTIVATORS:     activator connections
- * @KDBUS_LIST_NAMES:          known well-known names
- * @KDBUS_LIST_QUEUED:         queued-up names
+ * @KDBUS_LIST_UNIQUE:         Active connections
+ * @KDBUS_LIST_ACTIVATORS:     Activator connections
+ * @KDBUS_LIST_NAMES:          Known well-known names
+ * @KDBUS_LIST_QUEUED:         Queued-up names
  */
 enum kdbus_list_flags {
        KDBUS_LIST_UNIQUE               = 1ULL <<  0,

@@ -752,14 +755,14 @@ enum kdbus_list_flags {
 
 /**
  * struct kdbus_cmd_list - list connections
- * @size:              overall size of this object
- * @flags:             flags for the query (KDBUS_LIST_*), userspace → kernel
- * @return_flags:      command return flags, kernel → userspace
+ * @size:              Overall size of this object
+ * @flags:             Flags for the query (KDBUS_LIST_*), userspace → kernel
+ * @return_flags:      Command return flags, kernel → userspace
  * @offset:            Offset in the caller's pool buffer where an array of
  *                     kdbus_info objects is stored.
  *                     The user must use KDBUS_CMD_FREE to free the
  *                     allocated memory.
- * @list_size:         size of returned list in bytes
+ * @list_size:         Size of returned list in bytes
  * @items:             Items for the command. Reserved for future use.
  *
  * This structure is used with the KDBUS_CMD_LIST ioctl.
--

1.8.3.1

- kdbus_conn_unref(): Fix style issue to stay similar to other kernel-docs.

- kdbus_conn_disconnect(): Update the name of parameter:

   "ensure_msg_list_empty" -> "ensure_queue_empty".

- kdbus_conn_entry_insert(): Fix typo: dot -> comma.

- struct kdbus_conn: Remove lost "activator for" string.

- kdbus_fs_init(): Fix typo: "nameing" -> "naming".

- kdbus_node_deactivate(): Fix typo in comment:
"node as children" -> "node has children".

- kdbus_pool_slice_alloc(): Remove description of kvec and iovec as they relate to the old code.

- struct kdbus_queue_entry: Fix typo: "messages" -> "message".

- kdbus_pool_slice_publish(): Remove obsolete line from the description.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 6 +++---
 ipc/kdbus/connection.h | 1 -
 ipc/kdbus/fs.c         | 2 +-
 ipc/kdbus/node.c       | 4 ++--
 ipc/kdbus/pool.c       | 5 +----
 ipc/kdbus/queue.h      | 2 +-
 6 files changed, 8 insertions(+), 12 deletions(-)

ipc/kdbus/connection.c +0 -0

index ef63d6533273..4f3cd370ecd9 100644

ipc/kdbus/connection.c +2 -2

  *
  * When the last reference is dropped, the connection's internal structure
  * is freed.

@@ -490,7 +490,7 @@ exit_disconnect:
  *                     case the connection's message list is not
  *                     empty
  *
- * If @ensure_msg_list_empty is true, and the connection has pending messages,
+ * If @ensure_queue_empty is true, and the connection has pending messages,
  * -EBUSY is returned.
  *
  * Return: 0 on success, negative errno on failure

@@ -880,7 +880,7 @@ static int kdbus_conn_entry_sync_attach(struct kdbus_conn *conn_dst,
  * @reply:             The reply tracker to attach to the queue entry
  * @name:              Destination name this msg is sent to, or NULL
  *
- * Return: 0 on success. negative error otherwise.
+ * Return: 0 on success, negative error otherwise.
  */
 int kdbus_conn_entry_insert(struct kdbus_conn *conn_src,
                            struct kdbus_conn *conn_dst,

ipc/kdbus/connection.h +0 -1

index 1ad082014faa..679f393d3e68 100644
@@ -53,7 +53,6 @@ struct kdbus_staging;
  * @reply_list:                List of connections this connection should
  *                     reply to
  * @work:              Delayed work to handle timeouts
- *                     activator for
  * @match_db:          Subscription filter to broadcast messages
  * @meta_proc:         Process metadata of connection creator, or NULL
  * @meta_fake:         Faked metadata, or NULL

ipc/kdbus/fs.c +1 -1

index 09c480924b9e..47e652f2ee0e 100644
@@ -383,7 +383,7 @@ static struct file_system_type fs_type = {
  * kdbus_fs_init() - register kdbus filesystem
  *
  * This registers a filesystem with the VFS layer. The filesystem is called
- * `KBUILD_MODNAME "fs"', which usually resolves to `kdbusfs'. The nameing
+ * `KBUILD_MODNAME "fs"', which usually resolves to `kdbusfs'. The naming
  * scheme allows to set KBUILD_MODNAME to "kdbus2" and you will get an
  * independent filesystem for developers.
  *

ipc/kdbus/node.c +2 -2

index 89f58bc85433..133fb01a35cc 100644
@@ -557,8 +557,8 @@ void kdbus_node_deactivate(struct kdbus_node *node)
        /*
         * To avoid recursion, we perform back-tracking while deactivating
         * nodes. For each node we enter, we first mark the active-counter as
-        * deactivated by adding BIAS. If the node as children, we set the first
-        * child as current position and start over. If the node has no
+        * deactivated by adding BIAS. If the node has children, we set the
+        * first child as current position and start over. If the node has no
         * children, we drain the node by waiting for all active refs to be
         * dropped and then releasing the node.
         *

ipc/kdbus/pool.c +1 -4

index 63ccd55713c7..0433e26b777e 100644
@@ -192,9 +192,7 @@ static struct kdbus_pool_slice *kdbus_pool_find_slice(struct kdbus_pool *pool,
  * @accounted: Whether this slice should be accounted for
  *
  * The returned slice is used for kdbus_pool_slice_release() to
- * free the allocated memory. If either @kvec or @iovec is non-NULL, the data
- * will be copied from kernel or userspace memory into the new slice at
- * offset 0.
+ * free the allocated memory.
  *
  * Return: the allocated slice on success, ERR_PTR on failure.
  */

@@ -411,7 +409,6 @@ void kdbus_pool_publish_empty(struct kdbus_pool *pool, u64 *off, u64 *size)
  * This prepares a slice to be published to user-space.
  *
  * This call combines the following operations:
- *   * the memory region is flushed so the user's memory view is consistent
  *   * the slice is marked as referenced by user-space, so user-space has to
  *     call KDBUS_CMD_FREE to release it
  *   * the offset and size of the slice are written to the given output

ipc/kdbus/queue.h +1 -2

index bf686d182ce1..92f7549d8c9b 100644
@@ -38,7 +38,7 @@ struct kdbus_queue {
 };
 
 /**
- * struct kdbus_queue_entry - messages waiting to be read
+ * struct kdbus_queue_entry - message waiting to be read
  * @entry:             Entry in the connection's list
  * @prio_node:         Entry in the priority queue tree
  * @prio_entry:                Queue tree node entry in the list of one priority
--

1.8.3.1

Add comment on why we remove the same slice from free slices tree and then add it back again when merging the slice to be released with previous free slice.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/pool.c | 6 ++++++
 1 file changed, 6 insertions(+)

ipc/kdbus/pool.c +0 -0

index 84afe96fbc22..c26ef963d8d1 100644

ipc/kdbus/pool.c +0 -1

                        rb_erase(&s->rb_node, &pool->slices_free);
                        list_del(&slice->entry);
                        s->size += slice->size;
--

1.8.3.1

Hi

Quoted 14 lines · Sergei Zviagintsev

Add comment on why we remove the same slice from free slices tree and then add it back again when merging the slice to be released with previous free slice.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/pool.c | 6 ++++++
 1 file changed, 6 insertions(+)

ipc/kdbus/pool.c +0 -0

index 84afe96fbc22..c26ef963d8d1 100644

ipc/kdbus/pool.c static void __kdbus_pool_slice_release(struct kdbus_pool_slice *slice) · line 312

Quoted 9 lines

ipc/kdbus/pool.c +6 -0

@@ -304,6 +304,12 @@ static void __kdbus_pool_slice_release(struct kdbus_pool_slice *slice)
                s = list_entry(slice->entry.prev,
                               struct kdbus_pool_slice, entry);
                if (s->free) {
+                       /*
+                        * As size of slice increases after merge and free
+                        * slices tree is ordered by slice size, we have to
+                        * remove the slice from free slices tree and then add
+                        * it again to keep the tree balanced.
+                        */

Mhh, isn't this obvious? "slices_free" is ordered by s->size, so a change of the key requires a re-insert. If you disagree, maybe keep it simple:

        /* s->size changed, re-insert slice in rbtree */

Thanks
David

Quoted 6 lines

ipc/kdbus/pool.c +0 -1

                        rb_erase(&s->rb_node, &pool->slices_free);
                        list_del(&slice->entry);
                        s->size += slice->size;
--

1.8.3.1

Use list_next_entry() instead of list_first_entry().

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/queue.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

ipc/kdbus/queue.c +1 -3

index f9c44d7bae6d..90e8d16f5967 100644
@@ -157,8 +157,7 @@ static void kdbus_queue_entry_unlink(struct kdbus_queue_entry *entry)
                 * the list. Update cached highest-priority entry, store the
                 * new one as the tree node.
                 */
-               q = list_first_entry(&entry->prio_entry,
-                                    struct kdbus_queue_entry, prio_entry);
+               q = list_next_entry(entry, prio_entry);
                list_del(&entry->prio_entry);
 
                if (queue->msg_prio_highest == &entry->prio_node)
--

1.8.3.1

'(s & m) != m' means that mask 'm' contains some bits which are not set in 's', and this is literally equal to '~s & m'.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/message.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

ipc/kdbus/message.c +1 -2

index ae565cd343f8..c7ef23d40471 100644
@@ -273,7 +273,7 @@ static struct file *kdbus_get_memfd(const struct kdbus_memfd *memfd)
        s = shmem_get_seals(f);
        if (s < 0)
                ret = ERR_PTR(-EMEDIUMTYPE);
-       else if ((s & m) != m)
+       else if (~s & m)
                ret = ERR_PTR(-ETXTBSY);
        else if (memfd->start + memfd->size > (u64)i_size_read(file_inode(f)))
                ret = ERR_PTR(-EFAULT);
--

1.8.3.1

Replace the expression with more concise and readable equivalent. It can be proven by opening parentheses:

    r & ~((p | i) & r) == r & (~(p | i) | ~r) ==
        (r & ~(p | i)) | (r & ~r) == r & ~(p | i) == r & ~p & ~i

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/metadata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

ipc/kdbus/metadata.c +1 -2

index 788b4d9c7ecb..61215a078359 100644
@@ -1321,7 +1321,7 @@ static u64 kdbus_meta_get_mask(struct pid *prv_pid, u64 prv_mask,
         * the sender, but still requested by the receiver. If any are left,
         * perform rather expensive /proc access checks for them.
         */
-       missing = req_mask & ~((prv_mask | impl_mask) & req_mask);
+       missing = req_mask & ~prv_mask & ~impl_mask;
        if (missing)
                proc_mask = kdbus_meta_proc_mask(prv_pid, req_pid, req_cred,
                                                 missing);
--

1.8.3.1

Hi

Quoted 5 lines · Sergei Zviagintsev

Replace the expression with more concise and readable equivalent. It can be proven by opening parentheses:

    r & ~((p | i) & r) == r & (~(p | i) | ~r) ==
        (r & ~(p | i)) | (r & ~r) == r & ~(p | i) == r & ~p & ~i

But why? The current code follows the description, and does exactly the same. It shows that it merges the "provided" and "implied" masks, and then extracts the flags that are missing compared to the required mask.

I cannot follow why your code is more obvious?

David

Quoted 21 lines

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/metadata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

ipc/kdbus/metadata.c +1 -2

index 788b4d9c7ecb..61215a078359 100644
@@ -1321,7 +1321,7 @@ static u64 kdbus_meta_get_mask(struct pid *prv_pid, u64 prv_mask,
         * the sender, but still requested by the receiver. If any are left,
         * perform rather expensive /proc access checks for them.
         */
-       missing = req_mask & ~((prv_mask | impl_mask) & req_mask);
+       missing = req_mask & ~prv_mask & ~impl_mask;
        if (missing)
                proc_mask = kdbus_meta_proc_mask(prv_pid, req_pid, req_cred,
                                                 missing);
--

1.8.3.1

Assign zero to `ret' in the beginning of function instead of doing it in the end.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

ipc/kdbus/connection.c +1 -1

index 4f3cd370ecd9..185ed3ba1bce 100644
@@ -889,7 +889,7 @@ int kdbus_conn_entry_insert(struct kdbus_conn *conn_src,
                            const struct kdbus_name_entry *name)
 {
        struct kdbus_queue_entry *entry;
-       int ret;
+       int ret = 0;
 
        kdbus_conn_lock2(conn_src, conn_dst);
 

ipc/kdbus/connection.c +0 -1

 exit_unlock:
        kdbus_conn_unlock2(conn_src, conn_dst);
        return ret;
--

1.8.3.1

Hi

Quoted 22 lines · Sergei Zviagintsev

Assign zero to `ret' in the beginning of function instead of doing it in the end.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

ipc/kdbus/connection.c +1 -1

index 4f3cd370ecd9..185ed3ba1bce 100644
@@ -889,7 +889,7 @@ int kdbus_conn_entry_insert(struct kdbus_conn *conn_src,
                            const struct kdbus_name_entry *name)
 {
        struct kdbus_queue_entry *entry;
-       int ret;
+       int ret = 0;
 
        kdbus_conn_lock2(conn_src, conn_dst);
 

ipc/kdbus/connection.c int kdbus_conn_entry_insert(struct kdbus_conn *conn_src, · line 920

Quoted 5 lines

ipc/kdbus/connection.c +0 -2

@@ -916,8 +916,6 @@ int kdbus_conn_entry_insert(struct kdbus_conn *conn_src,
        kdbus_queue_entry_enqueue(entry, reply);
        wake_up_interruptible(&conn_dst->wait);
 
-       ret = 0;
-

Not a big fan of this. It makes it less obvious, and this style is wrong in several cases (but not here). We often only check for "ret < 0", but generally want >0 to be turned into 0 on return.

It does not matter in this specific case, but I prefer making return codes explicit, rather than relying on a previous initialization to be still valid.

What's your rationale here?

Thanks
David

Quoted 6 lines

ipc/kdbus/connection.c +0 -1

 exit_unlock:
        kdbus_conn_unlock2(conn_src, conn_dst);
        return ret;
--

1.8.3.1

Rearrange tests a little to make them look cleaner.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

ipc/kdbus/connection.c +5 -5

index 93da7f539f74..a4d7414ecaea 100644
@@ -1977,7 +1977,7 @@ int kdbus_cmd_send(struct kdbus_conn *conn, struct file *f, void __user *argp)
        ret = kdbus_args_parse(&args, argp, &cmd);
        if (ret < 0)
                goto exit;
-       else if (ret > 0 && !cmd->msg_address) /* negotiation without msg */
+       if (ret > 0 && !cmd->msg_address) /* negotiation without msg */
                goto exit;
 
        ret2 = kdbus_args_parse_msg(&msg_args, KDBUS_PTR(cmd->msg_address),

@@ -1985,10 +1985,11 @@ int kdbus_cmd_send(struct kdbus_conn *conn, struct file *f, void __user *argp)
        if (ret2 < 0) { /* cannot parse message */
                ret = ret2;
                goto exit;
-       } else if (ret2 > 0 && !ret) { /* msg-negot implies cmd-negot */
-               ret = -EINVAL;
+       }
+       if (ret > 0) /* negotiation */
                goto exit;
-       } else if (ret > 0) { /* negotiation */
+       if (ret2 > 0) { /* msg-negot implies cmd-negot */
+               ret = -EINVAL;
                goto exit;
        }
 
--

1.8.3.1

Hi

Quoted 1 line · Sergei Zviagintsev

Rearrange tests a little to make them look cleaner.

What's wrong with 'else if'?

Thanks
David

Quoted 36 lines

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

ipc/kdbus/connection.c +5 -5

index 93da7f539f74..a4d7414ecaea 100644
@@ -1977,7 +1977,7 @@ int kdbus_cmd_send(struct kdbus_conn *conn, struct file *f, void __user *argp)
        ret = kdbus_args_parse(&args, argp, &cmd);
        if (ret < 0)
                goto exit;
-       else if (ret > 0 && !cmd->msg_address) /* negotiation without msg */
+       if (ret > 0 && !cmd->msg_address) /* negotiation without msg */
                goto exit;
 
        ret2 = kdbus_args_parse_msg(&msg_args, KDBUS_PTR(cmd->msg_address),

@@ -1985,10 +1985,11 @@ int kdbus_cmd_send(struct kdbus_conn *conn, struct file *f, void __user *argp)
        if (ret2 < 0) { /* cannot parse message */
                ret = ret2;
                goto exit;
-       } else if (ret2 > 0 && !ret) { /* msg-negot implies cmd-negot */
-               ret = -EINVAL;
+       }
+       if (ret > 0) /* negotiation */
                goto exit;
-       } else if (ret > 0) { /* negotiation */
+       if (ret2 > 0) { /* msg-negot implies cmd-negot */
+               ret = -EINVAL;
                goto exit;
        }
 
--

1.8.3.1

Do not initialize wait' and name' as values are assigned to them at first use: wait' gets its value from kdbus_reply_find(), name' is set by kdbus_pin_dst().

Remove redundant code. goto isn't required as we reached exit point already. Setting `ret' to zero is unnecessary because kdbus_conn_entry_insert() returns 0 on success.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

ipc/kdbus/connection.c +2 -2

index a4d7414ecaea..db49f282a1bf 100644
@@ -1159,8 +1159,8 @@ static struct kdbus_reply *kdbus_conn_call(struct kdbus_conn *src,
                                           ktime_t exp)
 {
        const struct kdbus_msg *msg = staging->msg;
-       struct kdbus_name_entry *name = NULL;
-       struct kdbus_reply *wait = NULL;
+       struct kdbus_name_entry *name;
+       struct kdbus_reply *wait;
        struct kdbus_conn *dst = NULL;
        struct kdbus_bus *bus = src->ep->bus;
        int ret;

@@ -1212,14 +1212,8 @@ static struct kdbus_reply *kdbus_conn_call(struct kdbus_conn *src,
        }

ipc/kdbus/connection.c +0 -1

 exit:
        up_read(&bus->name_registry->rwlock);
--

1.8.3.1

Hi

Quoted 31 lines · Sergei Zviagintsev

Do not initialize wait' and name' as values are assigned to them at first use: wait' gets its value from kdbus_reply_find(), name' is set by kdbus_pin_dst().

Remove redundant code. goto isn't required as we reached exit point already. Setting `ret' to zero is unnecessary because kdbus_conn_entry_insert() returns 0 on success.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

ipc/kdbus/connection.c +2 -2

index a4d7414ecaea..db49f282a1bf 100644
@@ -1159,8 +1159,8 @@ static struct kdbus_reply *kdbus_conn_call(struct kdbus_conn *src,
                                           ktime_t exp)
 {
        const struct kdbus_msg *msg = staging->msg;
-       struct kdbus_name_entry *name = NULL;
-       struct kdbus_reply *wait = NULL;
+       struct kdbus_name_entry *name;
+       struct kdbus_reply *wait;
        struct kdbus_conn *dst = NULL;
        struct kdbus_bus *bus = src->ep->bus;
        int ret;

@@ -1212,14 +1212,8 @@ static struct kdbus_reply *kdbus_conn_call(struct kdbus_conn *src,
        }
 

ipc/kdbus/connection.c static struct kdbus_reply *kdbus_conn_call(struct kdbus_conn *src, · line 1222

Quoted 9 lines

ipc/kdbus/connection.c +0 -6

        /* send message */
-
        kdbus_bus_eavesdrop(bus, src, staging);
-
        ret = kdbus_conn_entry_insert(src, dst, staging, wait, name);
-       if (ret < 0)
-               goto exit;
-
-       ret = 0;

Who says kdbus_conn_entry_insert() returns 0? It might be >0. I'd prefer the explicit check.

Thanks
David

Quoted 6 lines

ipc/kdbus/connection.c +0 -1

 exit:
        up_read(&bus->name_registry->rwlock);
--

1.8.3.1

Do not initialize name' and ret' as values are assigned to them at the first use by kdbus_pin_dst(). Simplify handling of kdbus_conn_entry_insert() return value and drop useless goto.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

ipc/kdbus/connection.c +2 -2

index db49f282a1bf..b3c5f20a57d8 100644
@@ -1229,12 +1229,12 @@ static int kdbus_conn_unicast(struct kdbus_conn *src,
                              struct kdbus_staging *staging)
 {
        const struct kdbus_msg *msg = staging->msg;
-       struct kdbus_name_entry *name = NULL;
+       struct kdbus_name_entry *name;
        struct kdbus_reply *wait = NULL;
        struct kdbus_conn *dst = NULL;
        struct kdbus_bus *bus = src->ep->bus;
        bool is_signal = (msg->flags & KDBUS_MSG_SIGNAL);
-       int ret = 0;
+       int ret;
 
        if (WARN_ON(msg->dst_id == KDBUS_DST_ID_BROADCAST) ||
            WARN_ON(!(msg->flags & KDBUS_MSG_EXPECT_REPLY) &&

ipc/kdbus/connection.c +0 -0

        ret = kdbus_pin_dst(bus, staging, &name, &dst);
        if (ret < 0)
                goto exit;

ipc/kdbus/connection.c +0 -1

 exit:
        up_read(&bus->name_registry->rwlock);
--

1.8.3.1

Hi

Quoted 31 lines · Sergei Zviagintsev

Do not initialize name' and ret' as values are assigned to them at the first use by kdbus_pin_dst(). Simplify handling of kdbus_conn_entry_insert() return value and drop useless goto.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

ipc/kdbus/connection.c +2 -2

index db49f282a1bf..b3c5f20a57d8 100644
@@ -1229,12 +1229,12 @@ static int kdbus_conn_unicast(struct kdbus_conn *src,
                              struct kdbus_staging *staging)
 {
        const struct kdbus_msg *msg = staging->msg;
-       struct kdbus_name_entry *name = NULL;
+       struct kdbus_name_entry *name;
        struct kdbus_reply *wait = NULL;
        struct kdbus_conn *dst = NULL;
        struct kdbus_bus *bus = src->ep->bus;
        bool is_signal = (msg->flags & KDBUS_MSG_SIGNAL);
-       int ret = 0;
+       int ret;
 
        if (WARN_ON(msg->dst_id == KDBUS_DST_ID_BROADCAST) ||
            WARN_ON(!(msg->flags & KDBUS_MSG_EXPECT_REPLY) &&

@@ -1245,7 +1245,6 @@ static int kdbus_conn_unicast(struct kdbus_conn *src,
        down_read(&bus->name_registry->rwlock);
 

ipc/kdbus/connection.c static int kdbus_conn_unicast(struct kdbus_conn *src, · line 1248

Quoted 2 lines

ipc/kdbus/connection.c +0 -1

        /* find and pin destination */
-

If a comment is addressed to a whole following block, we usually put a newline after it. Only if the comment is only addressed at the next code-line, we don't.

Quoted 11 lines

ipc/kdbus/connection.c +0 -2

        ret = kdbus_pin_dst(bus, staging, &name, &dst);
        if (ret < 0)
                goto exit;

@@ -1276,11 +1275,10 @@ static int kdbus_conn_unicast(struct kdbus_conn *src,
                kdbus_bus_eavesdrop(bus, src, staging);
 
        ret = kdbus_conn_entry_insert(src, dst, staging, wait, name);
-       if (ret < 0 && !is_signal)
-               goto exit;
 

ipc/kdbus/connection.c static int kdbus_conn_unicast(struct kdbus_conn *src, · line 1281

Quoted 4 lines

ipc/kdbus/connection.c +2 -1

        /* signals are treated like broadcasts, recv-errors are ignored */
-       ret = 0;
+       if (is_signal)
+               ret = 0;

Why? Just to reduce the line-count? You break the code-flow here, by making the success-path conditional, instead of the error-path.

Thanks
David

Quoted 6 lines

ipc/kdbus/connection.c +0 -1

 exit:
        up_read(&bus->name_registry->rwlock);
--

1.8.3.1

- Move entry' and owner' to the scope where they are used. Drop redundand initialization of entry'. Use conditional operator to set the value of owner'.

- Set `ret' to zero right after call to kdbus_pool_slice_copy_kvec(), not in the end of function.

- Remove redundant goto.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

ipc/kdbus/connection.c +0 -2

index b3c5f20a57d8..6a73ac3f444d 100644
@@ -1702,8 +1702,6 @@ int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp)
 {
        struct kdbus_meta_conn *conn_meta = NULL;
        struct kdbus_pool_slice *slice = NULL;
-       struct kdbus_name_entry *entry = NULL;
-       struct kdbus_name_owner *owner = NULL;
        struct kdbus_conn *owner_conn = NULL;
        struct kdbus_item *meta_items = NULL;
        struct kdbus_info info = {};

@@ -1739,9 +1737,12 @@ int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp)
        name = argv[1].item ? argv[1].item->str : NULL;

ipc/kdbus/connection.c +1 -0

                if (!owner ||
                    !kdbus_conn_policy_see_name(conn, current_cred(), name) ||
                    (cmd->id != 0 && owner->conn->id != cmd->id)) {

@@ -1804,17 +1805,14 @@ int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp)
        ret = kdbus_pool_slice_copy_kvec(slice, 0, kvec, cnt, size);
        if (ret < 0)
                goto exit;
+       ret = 0;
 
        kdbus_pool_slice_publish(slice, &cmd->offset, &cmd->info_size);

ipc/kdbus/connection.c +0 -1

 exit:
        up_read(&bus->name_registry->rwlock);
--

1.8.3.1

Hi

Quoted 29 lines · Sergei Zviagintsev

- Move entry' and owner' to the scope where they are used. Drop redundand initialization of entry'. Use conditional operator to set the value of owner'.

- Set `ret' to zero right after call to kdbus_pool_slice_copy_kvec(), not in the end of function.

- Remove redundant goto.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

ipc/kdbus/connection.c +0 -2

index b3c5f20a57d8..6a73ac3f444d 100644
@@ -1702,8 +1702,6 @@ int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp)
 {
        struct kdbus_meta_conn *conn_meta = NULL;
        struct kdbus_pool_slice *slice = NULL;
-       struct kdbus_name_entry *entry = NULL;
-       struct kdbus_name_owner *owner = NULL;
        struct kdbus_conn *owner_conn = NULL;
        struct kdbus_item *meta_items = NULL;
        struct kdbus_info info = {};

@@ -1739,9 +1737,12 @@ int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp)
        name = argv[1].item ? argv[1].item->str : NULL;

ipc/kdbus/connection.c int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp) · line 1745

Quoted 10 lines

ipc/kdbus/connection.c +5 -2

 
        if (name) {
+               struct kdbus_name_entry *entry;
+               struct kdbus_name_owner *owner;
+
                entry = kdbus_name_lookup_unlocked(bus->name_registry, name);
-               if (entry)
-                       owner = kdbus_name_get_owner(entry);
+               owner = entry ? kdbus_name_get_owner(entry) : NULL;
+

This looks good to me.

Quoted 11 lines

ipc/kdbus/connection.c +1 -0

                if (!owner ||
                    !kdbus_conn_policy_see_name(conn, current_cred(), name) ||
                    (cmd->id != 0 && owner->conn->id != cmd->id)) {

@@ -1804,17 +1805,14 @@ int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp)
        ret = kdbus_pool_slice_copy_kvec(slice, 0, kvec, cnt, size);
        if (ret < 0)
                goto exit;
+       ret = 0;
 
        kdbus_pool_slice_publish(slice, &cmd->offset, &cmd->info_size);
 

ipc/kdbus/connection.c int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp) · line 1817

Quoted 9 lines

ipc/kdbus/connection.c +1 -5

        if (kdbus_member_set_user(&cmd->offset, argp, typeof(*cmd), offset) ||
            kdbus_member_set_user(&cmd->info_size, argp,
-                                 typeof(*cmd), info_size)) {
+                                 typeof(*cmd), info_size))
                ret = -EFAULT;
-               goto exit;
-       }
-
-       ret = 0;

Again, why? Now you have a random "ret = 0;" somewhere in between, instead of directly at the tail of the success-path.

Thanks
David

Quoted 6 lines

ipc/kdbus/connection.c +0 -1

 exit:
        up_read(&bus->name_registry->rwlock);
--

1.8.3.1

- Reduce scope of `owner' var to the block where it is actually used. Use conditional operator to set its value.

- Drop initialization of `dst' as it gets value later.

- Drop separate exit point as we have only one use case of it.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

ipc/kdbus/connection.c +1 -3

index 6a73ac3f444d..ace587ee951a 100644
@@ -1048,10 +1048,8 @@ static int kdbus_pin_dst(struct kdbus_bus *bus,
                         struct kdbus_conn **out_dst)
 {
        const struct kdbus_msg *msg = staging->msg;
-       struct kdbus_name_owner *owner = NULL;
        struct kdbus_name_entry *name = NULL;
-       struct kdbus_conn *dst = NULL;
-       int ret;
+       struct kdbus_conn *dst;
 
        lockdep_assert_held(&bus->name_registry->rwlock);
 

ipc/kdbus/connection.c +0 -1

 }
 
 static int kdbus_conn_reply(struct kdbus_conn *src,
--

1.8.3.1

Hi

Quoted 29 lines · Sergei Zviagintsev

- Reduce scope of `owner' var to the block where it is actually used. Use conditional operator to set its value.

- Drop initialization of `dst' as it gets value later.

- Drop separate exit point as we have only one use case of it.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

ipc/kdbus/connection.c +1 -3

index 6a73ac3f444d..ace587ee951a 100644
@@ -1048,10 +1048,8 @@ static int kdbus_pin_dst(struct kdbus_bus *bus,
                         struct kdbus_conn **out_dst)
 {
        const struct kdbus_msg *msg = staging->msg;
-       struct kdbus_name_owner *owner = NULL;
        struct kdbus_name_entry *name = NULL;
-       struct kdbus_conn *dst = NULL;
-       int ret;
+       struct kdbus_conn *dst;
 
        lockdep_assert_held(&bus->name_registry->rwlock);
 

ipc/kdbus/connection.c static int kdbus_pin_dst(struct kdbus_bus *bus, · line 1063

Quoted 7 lines

ipc/kdbus/connection.c +2 -2

@@ -1061,14 +1059,15 @@ static int kdbus_pin_dst(struct kdbus_bus *bus,
                        return -ENXIO;
 
                if (!kdbus_conn_is_ordinary(dst)) {
-                       ret = -ENXIO;
-                       goto error;
+                       kdbus_conn_unref(dst);
+                       return -ENXIO;

Looks good.

ipc/kdbus/connection.c static int kdbus_pin_dst(struct kdbus_bus *bus, · line 1067

Quoted 4 lines

ipc/kdbus/connection.c +2 -0

                }
        } else {
+               struct kdbus_name_owner *owner;
+

I'd prefer if you avoid making this block-local.

ipc/kdbus/connection.c static int kdbus_pin_dst(struct kdbus_bus *bus, · line 1070

Quoted 6 lines

ipc/kdbus/connection.c +1 -2

                name = kdbus_name_lookup_unlocked(bus->name_registry,
                                                  staging->dst_name);
-               if (name)
-                       owner = kdbus_name_get_owner(name);
+               owner = name ? kdbus_name_get_owner(name) : NULL;

Looks good.

ipc/kdbus/connection.c static int kdbus_pin_dst(struct kdbus_bus *bus, · line 1100

Quoted 11 lines

ipc/kdbus/connection.c +0 -4

                if (!owner)
                        return -ESRCH;
 

@@ -1094,10 +1093,6 @@ static int kdbus_pin_dst(struct kdbus_bus *bus,
        *out_name = name;
        *out_dst = dst;
        return 0;
-
-error:
-       kdbus_conn_unref(dst);
-       return ret;

Looks good.

Thanks
David

Quoted 6 lines

ipc/kdbus/connection.c +0 -1

 }
 
 static int kdbus_conn_reply(struct kdbus_conn *src,
--

1.8.3.1

- Simplify if-statements

'proc < KDBUS_META_PROC_NORMAL' test only makes sense when we call kdbus_proc_permission(). Include it into 'prv_pid != req_pid' branch, remove redundant assignment of `proc' var and reduce its scope.

- Drop redundant binary operation

In 'proc < KDBUS_META_PROC_NORMAL' case we firstly do 'wanted &= ~flags' and then 'wanted & flags' in the return statement, which is the same as 'wanted & 0'. Return 0 instead.

- Cosmetic cleanup of the return statement.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/metadata.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

ipc/kdbus/metadata.c +0 -9

index 61215a078359..b8d094d9fb56 100644
@@ -1224,7 +1224,6 @@ static u64 kdbus_meta_proc_mask(struct pid *prv_pid,
                                u64 wanted)
 {
        struct pid_namespace *prv_ns, *req_ns;
-       unsigned int proc;
 
        prv_ns = ns_of_pid(prv_pid);
        req_ns = ns_of_pid(req_pid);

@@ -1243,30 +1242,23 @@ static u64 kdbus_meta_proc_mask(struct pid *prv_pid,
         * provider and requestor are the same. If not, we perform rather
         * expensive /proc permission checks.
         */
-       if (prv_pid == req_pid)
-               proc = KDBUS_META_PROC_NORMAL;
-       else
-               proc = kdbus_proc_permission(req_ns, req_cred, prv_pid);
-
-       /* you need /proc access to read standard process attributes */
-       if (proc < KDBUS_META_PROC_NORMAL)
-               wanted &= ~(KDBUS_ATTACH_TID_COMM |

ipc/kdbus/metadata.c +9 -5

+
+               /* you need /proc access to read standard process attributes */
+               if (proc < KDBUS_META_PROC_NORMAL)
+                       return 0;
+       }
 
        /* clear all non-/proc flags */
        return wanted & (KDBUS_ATTACH_TID_COMM |
                         KDBUS_ATTACH_PID_COMM |
                         KDBUS_ATTACH_SECLABEL |
-                        KDBUS_ATTACH_CMDLINE |
-                        KDBUS_ATTACH_CGROUP |
-                        KDBUS_ATTACH_AUDIT |
-                        KDBUS_ATTACH_CAPS |
+                        KDBUS_ATTACH_CMDLINE  |
+                        KDBUS_ATTACH_CGROUP   |
+                        KDBUS_ATTACH_AUDIT    |
+                        KDBUS_ATTACH_CAPS     |
                         KDBUS_ATTACH_EXE);
 }
 
--

1.8.3.1

- Move r' and ret' to scopes where they are used. Drop redundant initialization of `ret'.

- Initialize `bus' on declaration.

- Replace list_for_each_entry_safe() with list_for_each_entry() when iterating over list of replies.

- Drop redundant `continue'.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

ipc/kdbus/connection.c +2 -3

index b32b4f981618..6ee688d3de53 100644
@@ -1293,25 +1293,24 @@ void kdbus_conn_move_messages(struct kdbus_conn *conn_dst,
                              u64 name_id)
 {
        struct kdbus_queue_entry *e, *e_tmp;
-       struct kdbus_reply *r, *r_tmp;
-       struct kdbus_bus *bus;
+       struct kdbus_bus *bus = conn_src->ep->bus;
        struct kdbus_conn *c;
        LIST_HEAD(msg_list);
-       int i, ret = 0;
+       int i;
 

ipc/kdbus/connection.c +0 -1

                }
        }
        kdbus_conn_unlock2(conn_src, conn_dst);
--

1.8.3.1

Hi

Quoted 34 lines · Sergei Zviagintsev

- Move r' and ret' to scopes where they are used. Drop redundant initialization of `ret'.

- Initialize `bus' on declaration.

- Replace list_for_each_entry_safe() with list_for_each_entry() when iterating over list of replies.

- Drop redundant `continue'.

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/connection.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

ipc/kdbus/connection.c +2 -3

index b32b4f981618..6ee688d3de53 100644
@@ -1293,25 +1293,24 @@ void kdbus_conn_move_messages(struct kdbus_conn *conn_dst,
                              u64 name_id)
 {
        struct kdbus_queue_entry *e, *e_tmp;
-       struct kdbus_reply *r, *r_tmp;
-       struct kdbus_bus *bus;
+       struct kdbus_bus *bus = conn_src->ep->bus;
        struct kdbus_conn *c;
        LIST_HEAD(msg_list);
-       int i, ret = 0;
+       int i;
 
        if (WARN_ON(conn_src == conn_dst))
                return;
 

ipc/kdbus/connection.c void kdbus_conn_move_messages(struct kdbus_conn *conn_dst, · line 1308

Quoted 7 lines

ipc/kdbus/connection.c +2 -2

-       bus = conn_src->ep->bus;
-
        /* lock order: domain -> bus -> ep -> names -> conn */
        down_read(&bus->conn_rwlock);
        hash_for_each(bus->conn_hash, i, c, hentry) {
+               struct kdbus_reply *r;
+

Why make 'r' local?

ipc/kdbus/connection.c void kdbus_conn_move_messages(struct kdbus_conn *conn_dst, · line 1313

Quoted 7 lines

ipc/kdbus/connection.c +1 -1

                if (c == conn_src || c == conn_dst)
                        continue;
 
                mutex_lock(&c->lock);
-               list_for_each_entry_safe(r, r_tmp, &c->reply_list, entry) {
+               list_for_each_entry(r, &c->reply_list, entry) {

Looks good.

Quoted 3 lines

ipc/kdbus/connection.c +0 -0

                        if (r->reply_src != conn_src)
                                continue;
 

ipc/kdbus/connection.c void kdbus_conn_move_messages(struct kdbus_conn *conn_dst, · line 1331

Quoted 6 lines

ipc/kdbus/connection.c +2 -0

@@ -1328,6 +1327,8 @@ void kdbus_conn_move_messages(struct kdbus_conn *conn_dst,
 
        kdbus_conn_lock2(conn_src, conn_dst);
        list_for_each_entry_safe(e, e_tmp, &conn_src->queue.msg_list, entry) {
+               int ret;
+

Why make it local?

ipc/kdbus/connection.c void kdbus_conn_move_messages(struct kdbus_conn *conn_dst, · line 1346

Quoted 9 lines

ipc/kdbus/connection.c +0 -1

                /* filter messages for a specific name */
                if (name_id > 0 && e->dst_name_id != name_id)
                        continue;

@@ -1343,7 +1344,6 @@ void kdbus_conn_move_messages(struct kdbus_conn *conn_dst,
                if (ret < 0) {
                        kdbus_conn_lost_message(conn_dst);
                        kdbus_queue_entry_free(e);
-                       continue;

Looks good.

Thanks
David

Quoted 6 lines

ipc/kdbus/connection.c +0 -1

                }
        }
        kdbus_conn_unlock2(conn_src, conn_dst);
--

1.8.3.1

If idr_alloc() fails, we shouldn't call idr_remove() as latter produces warning when called on non-allocated ids. Split cleanup code into three parts for differrent cleanup scenarios before and after idr_alloc().

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/domain.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

ipc/kdbus/domain.c +2 -2

index ac9f760c150d..31cd09fb572f 100644
@@ -208,7 +208,7 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
        u = kzalloc(sizeof(*u), GFP_KERNEL);
        if (!u) {
                ret = -ENOMEM;
-               goto exit;
+               goto exit_unlock;
        }
 
        kref_init(&u->kref);

@@ -225,7 +225,7 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
                        ret = idr_alloc(&domain->user_idr, u, __kuid_val(uid),
                                        __kuid_val(uid) + 1, GFP_KERNEL);
                        if (ret < 0)
-                               goto exit;
+                               goto exit_free;
                }
        }
 

ipc/kdbus/domain.c +7 -8

        u->id = ret;
        mutex_unlock(&domain->lock);
        return u;
 
-exit:
-       if (u) {
-               if (uid_valid(u->uid))
-                       idr_remove(&domain->user_idr, __kuid_val(u->uid));
-               kdbus_domain_unref(u->domain);
-               kfree(u);
-       }
+exit_idr:
+       if (uid_valid(u->uid))
+               idr_remove(&domain->user_idr, __kuid_val(u->uid));
+exit_free:
+       kdbus_domain_unref(u->domain);
+       kfree(u);
+exit_unlock:
        mutex_unlock(&domain->lock);
        return ERR_PTR(ret);
 }
--

1.8.3.1

Hi

Quoted 31 lines · Sergei Zviagintsev

If idr_alloc() fails, we shouldn't call idr_remove() as latter produces warning when called on non-allocated ids. Split cleanup code into three parts for differrent cleanup scenarios before and after idr_alloc().

Signed-off-by Sergei Zviagintsev <[email protected]>

---
 ipc/kdbus/domain.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

ipc/kdbus/domain.c +2 -2

index ac9f760c150d..31cd09fb572f 100644
@@ -208,7 +208,7 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
        u = kzalloc(sizeof(*u), GFP_KERNEL);
        if (!u) {
                ret = -ENOMEM;
-               goto exit;
+               goto exit_unlock;
        }
 
        kref_init(&u->kref);

@@ -225,7 +225,7 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
                        ret = idr_alloc(&domain->user_idr, u, __kuid_val(uid),
                                        __kuid_val(uid) + 1, GFP_KERNEL);
                        if (ret < 0)
-                               goto exit;
+                               goto exit_free;
                }
        }
 

ipc/kdbus/domain.c struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid) · line 239

Quoted 7 lines

ipc/kdbus/domain.c +1 -1

@@ -235,19 +235,19 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
         */
        ret = ida_simple_get(&domain->user_ida, 1, 0, GFP_KERNEL);
        if (ret < 0)
-               goto exit;
+               goto exit_idr;
 

Why not simply assign "u->uid = uid;" _after_ doing the idr operations?

Thanks
David

Quoted 24 lines

ipc/kdbus/domain.c +7 -8

        u->id = ret;
        mutex_unlock(&domain->lock);
        return u;
 
-exit:
-       if (u) {
-               if (uid_valid(u->uid))
-                       idr_remove(&domain->user_idr, __kuid_val(u->uid));
-               kdbus_domain_unref(u->domain);
-               kfree(u);
-       }
+exit_idr:
+       if (uid_valid(u->uid))
+               idr_remove(&domain->user_idr, __kuid_val(u->uid));
+exit_free:
+       kdbus_domain_unref(u->domain);
+       kfree(u);
+exit_unlock:
        mutex_unlock(&domain->lock);
        return ERR_PTR(ret);
 }
--

1.8.3.1