[PATCH 00/44] kdbus cleanups

[PATCH 00/44] kdbus cleanups

[Sergei Zviagintsev]

Hi all,

This is a set of various kdbus code cleanups. Patches are ordered by
increasing complexity, starting with docs and comments fixes and
one-liners.

Patch 29 is the revised version of
http://lkml.kernel.org/g/1435497454-10464-6-git-send-email-sergei@s15v.net

Feel free to ask to change layout of this, split/join, etc if necessary.

Thanks, Sergei

Sergei Zviagintsev (44):
  Documentation/kdbus: Document new name registry flags
  uapi: kdbus.h: Kernel-doc fixes
  kdbus: Kernel-docs and comments trivial fixes
  kdbus: Update kernel-doc for struct kdbus_pool
  kdbus: Add comment on merging free pool slices
  kdbus: Fix kernel-doc for struct kdbus_gaps
  kdbus: Fix comment on translation of caps between namespaces
  kdbus: Rename var in kdbus_meta_export_caps()
  kdbus: Remove unused KDBUS_MSG_MAX_SIZE constant
  kdbus: Use conditional operator
  kdbus: Cosmetic fix of kdbus_name_is_valid()
  kdbus: Use conventional list macros in __kdbus_pool_slice_release()
  kdbus: Use list_next_entry() in kdbus_queue_entry_unlink()
  kdbus: Simplify expression in kdbus_get_memfd()
  kdbus: Simplify bitwise expression in kdbus_meta_get_mask()
  kdbus: Drop redundant code from kdbus_name_acquire()
  kdbus: Drop duplicated code from kdbus_pool_slice_alloc()
  kdbus: Add var initialization to kdbus_conn_entry_insert()
  kdbus: Drop useless initialization from kdbus_conn_reply()
  kdbus: Drop useless initialization from kdbus_cmd_hello()
  kdbus: Cleanup tests in kdbus_cmd_send()
  kdbus: Cleanup error path in kdbus_staging_new_user()
  kdbus: Cleanup kdbus_conn_call()
  kdbus: Cleanup kdbus_conn_unicast()
  kdbus: Cleanup kdbus_cmd_conn_info()
  kdbus: Cleanup kdbus_pin_dst()
  kdbus: Cleanup kdbus_conn_new()
  kdbus: Cleanup kdbus_queue_entry_new()
  kdbus: Improve tests on incrementing quota
  kdbus: Cleanup kdbus_meta_proc_mask()
  kdbus: Cleanup kdbus_conn_move_messages()
  kdbus: Remove duplicated code from kdbus_conn_lock2()
  kdbus: Improve kdbus_staging_reserve()
  kdbus: Improve kdbus_conn_entry_sync_attach()
  kdbus: Drop goto from kdbus_queue_entry_link()
  kdbus: Improve kdbus_name_release()
  kdbus: Fix error path in kdbus_meta_proc_collect_cgroup()
  kdbus: Fix error path in kdbus_user_lookup()
  kdbus: Cleanup kdbus_user_lookup()
  kdbus: Cleanup kdbus_item_validate_name()
  kdbus: Fix memfd install algorithm
  kdbus: Check if fd is allocated before trying to free it
  kdbus: Give up on failed fd allocation
  kdbus: Cleanup kdbus_gaps_install()

 Documentation/kdbus/kdbus.name.xml |  42 +++++++++-
 include/uapi/linux/kdbus.h         |  43 +++++-----
 ipc/kdbus/connection.c             | 157 +++++++++++++++----------------------
 ipc/kdbus/connection.h             |  19 ++---
 ipc/kdbus/domain.c                 |  38 +++++----
 ipc/kdbus/fs.c                     |   2 +-
 ipc/kdbus/item.c                   |  26 +++---
 ipc/kdbus/limits.h                 |   3 -
 ipc/kdbus/message.c                |  81 +++++++++----------
 ipc/kdbus/message.h                |   9 ++-
 ipc/kdbus/metadata.c               |  79 ++++++++++---------
 ipc/kdbus/names.c                  |  32 ++++----
 ipc/kdbus/node.c                   |   4 +-
 ipc/kdbus/pool.c                   |  26 +++---
 ipc/kdbus/queue.c                  |  51 ++++++------
 ipc/kdbus/queue.h                  |   2 +-
 16 files changed, 298 insertions(+), 316 deletions(-)

-- 
1.8.3.1

[PATCH 01/44] Documentation/kdbus: Document new name registry flags

[Sergei Zviagintsev]

Add description of KDBUS_NAME_PRIMARY and KDBUS_NAME_ACQUIRED flags
which were added in commit 0486b859f05a ("kdbus: inform caller about
exact updates on NAME_ACQUIRE").

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 Documentation/kdbus/kdbus.name.xml | 42 +++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/Documentation/kdbus/kdbus.name.xml b/Documentation/kdbus/kdbus.name.xml
index 3f5f6a6c5ed6..85c8c30c0edd 100644
--- a/Documentation/kdbus/kdbus.name.xml
+++ b/Documentation/kdbus/kdbus.name.xml
@@ -209,6 +209,32 @@ struct kdbus_cmd {
                 </para>
               </listitem>
             </varlistentry>
+
+            <varlistentry>
+              <term><constant>KDBUS_NAME_PRIMARY</constant></term>
+              <listitem>
+                <para>
+		  The connection is currently the primary owner of the name.
+		  This flag is the negation of
+		  <constant>KDBUS_NAME_IN_QUEUE</constant>, but is required to
+		  distinguish the case from the situation where the connection
+		  is neither queued nor the primary owner of the name.
+                </para>
+              </listitem>
+            </varlistentry>
+
+            <varlistentry>
+              <term><constant>KDBUS_NAME_ACQUIRED</constant></term>
+              <listitem>
+                <para>
+                  This flag is used to let the caller know whether
+		  <emphasis>this</emphasis> exact call actually queued the
+		  connection on the name. If the flag is not set, the connection
+		  was either already queued and only the flags were updated, or
+		  the connection is not queued at all.
+                </para>
+              </listitem>
+            </varlistentry>
           </variablelist>
         </listitem>
       </varlistentry>
@@ -489,9 +515,19 @@ struct kdbus_info {
               <listitem>
                 <para>
                   When retrieving a list of currently acquired names in the
-                  registry, this flag indicates whether the connection
-                  actually owns the name or is currently waiting for it to
-                  become available.
+                  registry, this flag indicates that the connection is currently
+                  waiting for the name to become available.
+                </para>
+              </listitem>
+            </varlistentry>
+
+            <varlistentry>
+              <term><constant>KDBUS_NAME_PRIMARY</constant></term>
+              <listitem>
+                <para>
+                  When retrieving a list of currently acquired names in the
+                  registry, this flag indicates that the connection is the
+                  primary owner of the name.
                 </para>
               </listitem>
             </varlistentry>
-- 
1.8.3.1

[PATCH 02/44] uapi: kdbus.h: Kernel-doc fixes

[Sergei Zviagintsev]

 - 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 <sergei@s15v.net>
---
 include/uapi/linux/kdbus.h | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/include/uapi/linux/kdbus.h b/include/uapi/linux/kdbus.h
index 4fc44cb1d4a8..09b23af87ce6 100644
--- a/include/uapi/linux/kdbus.h
+++ b/include/uapi/linux/kdbus.h
@@ -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,
@@ -308,7 +308,7 @@ enum kdbus_attach_flags {
  *					connection, carries a struct
  *					kdbus_bloom_filter
  * @KDBUS_ITEM_BLOOM_MASK:		Bloom mask used to match against a
- *					message'sbloom filter
+ *					message's bloom filter
  * @KDBUS_ITEM_DST_NAME:		Destination's well-known name
  * @KDBUS_ITEM_MAKE_NAME:		Name of domain, bus, endpoint
  * @KDBUS_ITEM_ATTACH_FLAGS_SEND:	Attach-flags, used for updating which
@@ -409,20 +409,23 @@ enum kdbus_item_type {
 /**
  * struct kdbus_item - chain of data blocks
  * @size:		Overall data record size
- * @type:		Kdbus_item type of data
+ * @type:		kdbus_item type of data
  * @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
+ *			KDBUS_ITEM_PAYLOAD_OFF
  * @creds:		KDBUS_ITEM_CREDS
+ * @pids:		KDBUS_ITEM_PIDS
  * @audit:		KDBUS_ITEM_AUDIT
  * @timestamp:		KDBUS_ITEM_TIMESTAMP
  * @name:		KDBUS_ITEM_NAME
  * @bloom_parameter:	KDBUS_ITEM_BLOOM_PARAMETER
  * @bloom_filter:	KDBUS_ITEM_BLOOM_FILTER
  * @memfd:		KDBUS_ITEM_PAYLOAD_MEMFD
+ * @fsd:		KDBUS_ITEM_FDS
  * @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

Re: [PATCH 02/44] uapi: kdbus.h: Kernel-doc fixes

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
>  - 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 <sergei@s15v.net>
> ---
>  include/uapi/linux/kdbus.h | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/include/uapi/linux/kdbus.h b/include/uapi/linux/kdbus.h
> index 4fc44cb1d4a8..09b23af87ce6 100644
> --- a/include/uapi/linux/kdbus.h
> +++ b/include/uapi/linux/kdbus.h
> @@ -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,
> @@ -308,7 +308,7 @@ enum kdbus_attach_flags {
>   *                                     connection, carries a struct
>   *                                     kdbus_bloom_filter
>   * @KDBUS_ITEM_BLOOM_MASK:             Bloom mask used to match against a
> - *                                     message'sbloom filter
> + *                                     message's bloom filter

This actually ought to be "against the bloom filter of a message".

>   * @KDBUS_ITEM_DST_NAME:               Destination's well-known name
>   * @KDBUS_ITEM_MAKE_NAME:              Name of domain, bus, endpoint
>   * @KDBUS_ITEM_ATTACH_FLAGS_SEND:      Attach-flags, used for updating which
> @@ -409,20 +409,23 @@ enum kdbus_item_type {
>  /**
>   * struct kdbus_item - chain of data blocks
>   * @size:              Overall data record size
> - * @type:              Kdbus_item type of data
> + * @type:              kdbus_item type of data

What's "kdbus_item" doing here, anyway? I'd just drop it entirely and
make it say "Type of data".

>   * @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
> + *                     KDBUS_ITEM_PAYLOAD_OFF
>   * @creds:             KDBUS_ITEM_CREDS
> + * @pids:              KDBUS_ITEM_PIDS
>   * @audit:             KDBUS_ITEM_AUDIT
>   * @timestamp:         KDBUS_ITEM_TIMESTAMP
>   * @name:              KDBUS_ITEM_NAME
>   * @bloom_parameter:   KDBUS_ITEM_BLOOM_PARAMETER
>   * @bloom_filter:      KDBUS_ITEM_BLOOM_FILTER
>   * @memfd:             KDBUS_ITEM_PAYLOAD_MEMFD
> + * @fsd:               KDBUS_ITEM_FDS

Typo: @fds

Thanks
David

>   * @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
>

[PATCH 03/44] kdbus: Kernel-docs and comments trivial fixes

[Sergei Zviagintsev]

 - 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 <sergei@s15v.net>
---
 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(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index ef63d6533273..4f3cd370ecd9 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -317,7 +317,7 @@ struct kdbus_conn *kdbus_conn_ref(struct kdbus_conn *conn)
 
 /**
  * kdbus_conn_unref() - drop a connection reference
- * @conn:		Connection (may be NULL)
+ * @conn:		Connection, may be %NULL
  *
  * 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,
diff --git a/ipc/kdbus/connection.h b/ipc/kdbus/connection.h
index 1ad082014faa..679f393d3e68 100644
--- a/ipc/kdbus/connection.h
+++ b/ipc/kdbus/connection.h
@@ -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
diff --git a/ipc/kdbus/fs.c b/ipc/kdbus/fs.c
index 09c480924b9e..47e652f2ee0e 100644
--- a/ipc/kdbus/fs.c
+++ b/ipc/kdbus/fs.c
@@ -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.
  *
diff --git a/ipc/kdbus/node.c b/ipc/kdbus/node.c
index 89f58bc85433..133fb01a35cc 100644
--- a/ipc/kdbus/node.c
+++ b/ipc/kdbus/node.c
@@ -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.
 	 *
diff --git a/ipc/kdbus/pool.c b/ipc/kdbus/pool.c
index 63ccd55713c7..0433e26b777e 100644
--- a/ipc/kdbus/pool.c
+++ b/ipc/kdbus/pool.c
@@ -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
diff --git a/ipc/kdbus/queue.h b/ipc/kdbus/queue.h
index bf686d182ce1..92f7549d8c9b 100644
--- a/ipc/kdbus/queue.h
+++ b/ipc/kdbus/queue.h
@@ -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

Re: [PATCH 03/44] kdbus: Kernel-docs and comments trivial fixes

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
>  - 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 <sergei@s15v.net>
> ---
>  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(-)
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index ef63d6533273..4f3cd370ecd9 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -317,7 +317,7 @@ struct kdbus_conn *kdbus_conn_ref(struct kdbus_conn *conn)
>
>  /**
>   * kdbus_conn_unref() - drop a connection reference
> - * @conn:              Connection (may be NULL)
> + * @conn:              Connection, may be %NULL

We usually use "Foobar, or NULL" in kdbus docs, and then describe the
case for NULL in the description.

Otherwise, looks good, thanks!
David

>   *
>   * 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,
> diff --git a/ipc/kdbus/connection.h b/ipc/kdbus/connection.h
> index 1ad082014faa..679f393d3e68 100644
> --- a/ipc/kdbus/connection.h
> +++ b/ipc/kdbus/connection.h
> @@ -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
> diff --git a/ipc/kdbus/fs.c b/ipc/kdbus/fs.c
> index 09c480924b9e..47e652f2ee0e 100644
> --- a/ipc/kdbus/fs.c
> +++ b/ipc/kdbus/fs.c
> @@ -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.
>   *
> diff --git a/ipc/kdbus/node.c b/ipc/kdbus/node.c
> index 89f58bc85433..133fb01a35cc 100644
> --- a/ipc/kdbus/node.c
> +++ b/ipc/kdbus/node.c
> @@ -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.
>          *
> diff --git a/ipc/kdbus/pool.c b/ipc/kdbus/pool.c
> index 63ccd55713c7..0433e26b777e 100644
> --- a/ipc/kdbus/pool.c
> +++ b/ipc/kdbus/pool.c
> @@ -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
> diff --git a/ipc/kdbus/queue.h b/ipc/kdbus/queue.h
> index bf686d182ce1..92f7549d8c9b 100644
> --- a/ipc/kdbus/queue.h
> +++ b/ipc/kdbus/queue.h
> @@ -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
>

[PATCH 04/44] kdbus: Update kernel-doc for struct kdbus_pool

[Sergei Zviagintsev]

Add words on how free and busy slices trees are sorted.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/pool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipc/kdbus/pool.c b/ipc/kdbus/pool.c
index 0433e26b777e..84afe96fbc22 100644
--- a/ipc/kdbus/pool.c
+++ b/ipc/kdbus/pool.c
@@ -38,8 +38,8 @@
  * @accounted_size:	Currently accounted memory in bytes
  * @lock:		Pool data lock
  * @slices:		All slices sorted by address
- * @slices_busy:	Tree of allocated slices
- * @slices_free:	Tree of free slices
+ * @slices_busy:	Tree of allocated slices, sorted by slice offset
+ * @slices_free:	Tree of free slices, sorted by slice size
  *
  * The receiver's buffer, managed as a pool of allocated and free
  * slices containing the queued messages.
-- 
1.8.3.1

[PATCH 05/44] kdbus: Add comment on merging free pool slices

[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 <sergei@s15v.net>
---
 ipc/kdbus/pool.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ipc/kdbus/pool.c b/ipc/kdbus/pool.c
index 84afe96fbc22..c26ef963d8d1 100644
--- a/ipc/kdbus/pool.c
+++ b/ipc/kdbus/pool.c
@@ -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.
+			 */
 			rb_erase(&s->rb_node, &pool->slices_free);
 			list_del(&slice->entry);
 			s->size += slice->size;
-- 
1.8.3.1

Re: [PATCH 05/44] kdbus: Add comment on merging free pool slices

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> 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 <sergei@s15v.net>
> ---
>  ipc/kdbus/pool.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/ipc/kdbus/pool.c b/ipc/kdbus/pool.c
> index 84afe96fbc22..c26ef963d8d1 100644
> --- a/ipc/kdbus/pool.c
> +++ b/ipc/kdbus/pool.c
> @@ -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

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

Re: [PATCH 05/44] kdbus: Add comment on merging free pool slices

[Sergei Zviagintsev]

Hi,

On Thu, Oct 08, 2015 at 03:50:46PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > 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 <sergei@s15v.net>
> > ---
> >  ipc/kdbus/pool.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/ipc/kdbus/pool.c b/ipc/kdbus/pool.c
> > index 84afe96fbc22..c26ef963d8d1 100644
> > --- a/ipc/kdbus/pool.c
> > +++ b/ipc/kdbus/pool.c
> > @@ -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 */

It wasn't obvious for me from the first sight (but don't tell anyone),
so I decided that some words on why we're doing re-insert would be
helpful. It's up to you to decide whether it would be useful for the
others. BTW, I find your shorter variant better.

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

[PATCH 06/44] kdbus: Fix kernel-doc for struct kdbus_gaps

[Sergei Zviagintsev]

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/message.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/ipc/kdbus/message.h b/ipc/kdbus/message.h
index 298f9c99dfcf..405a5668f9d5 100644
--- a/ipc/kdbus/message.h
+++ b/ipc/kdbus/message.h
@@ -27,11 +27,12 @@ struct kdbus_pool_slice;
 /**
  * struct kdbus_gaps - gaps in message to be filled later
  * @kref:		Reference counter
- * @n_memfd_offs:	Number of memfds
- * @memfd_offs:		Offsets of kdbus_memfd items in target slice
+ * @n_memfds:		Number of memfds
+ * @memfd_offsets:	Offsets of kdbus_memfd items in target slice
+ * @memfd_files:	Array of struct file pointers representing memfds
  * @n_fds:		Number of fds
- * @fds:		Array of sent fds
- * @fds_offset:		Offset of fd-array in target slice
+ * @fd_files:		Array of struct file pointers representing fds
+ * @fd_offset:		Offset of fd-array in target slice
  *
  * The 'gaps' object is used to track data that is needed to fill gaps in a
  * message at RECV time. Usually, we try to compile the whole message at SEND
-- 
1.8.3.1

[PATCH 07/44] kdbus: Fix comment on translation of caps between namespaces

[Sergei Zviagintsev]

Update the comment to keep it in sync with the algorithm. The current
one lacks words on that in order to have all capabilities in the owned
user namespace the process must stay in the parent of that namespace.
Also (obvious, but should be mentioned for completeness) the mask is
copied verbatim if the process is a member of the given userns.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/metadata.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/ipc/kdbus/metadata.c b/ipc/kdbus/metadata.c
index 71ca475a80d5..4ff4b99a40e0 100644
--- a/ipc/kdbus/metadata.c
+++ b/ipc/kdbus/metadata.c
@@ -730,15 +730,21 @@ static void kdbus_meta_export_caps(struct kdbus_meta_caps *out,
 
 	/*
 	 * This translates the effective capabilities of 'cred' into the given
-	 * user-namespace. If the given user-namespace is a child-namespace of
-	 * the user-namespace of 'cred', the mask can be copied verbatim. If
-	 * not, the mask is cleared.
-	 * There's one exception: If 'cred' is the owner of any user-namespace
-	 * in the path between the given user-namespace and the user-namespace
-	 * of 'cred', then it has all effective capabilities set. This means,
-	 * the user who created a user-namespace always has all effective
-	 * capabilities in any child namespaces. Note that this is based on the
-	 * uid of the namespace creator, not the task hierarchy.
+	 * user namespace according to the following rules:
+	 *
+	 * - If 'cred' is a member of the given user namespace or any of its
+	 *   parent user namespaces, the mask is copied verbatim. That is, if
+	 *   a process has a capability in a user namespace, then it has it in
+	 *   all child user namespaces too.
+	 *
+	 * - If the effective UID of 'cred' matches the owner of the given user
+	 *   namespace or any of its parent user namespaces and 'cred' itself
+	 *   resides in the parent of that user namespace which it owns, then
+	 *   it has all effective capabilities set. This means that the user
+	 *   who created a user namespace always has all effective capabilities
+	 *   in all child namespaces while staying in the parent of the user
+	 *   namespace which it owns. Note that this is based on the UID of the
+	 *   namespace creator, not the task hierarchy.
 	 */
 	for (iter = user_ns; iter; iter = iter->parent) {
 		if (iter == cred->user_ns) {
-- 
1.8.3.1

[PATCH 08/44] kdbus: Rename var in kdbus_meta_export_caps()

[Sergei Zviagintsev]

Rename `parent' to `member'. We currently set `parent' to true during
iteration over namespaces if `cred' resides in one of parent namespaces
of `user_ns'. But if `cred' is a member of `user_ns' itself, `parent'
name is not the best choice. Also new `member' name is more consistent
with the `owner' var, as it can be read "cred is a member of user_ns or
one of its parents" and "cred is the owner of ...".

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/metadata.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ipc/kdbus/metadata.c b/ipc/kdbus/metadata.c
index 4ff4b99a40e0..788b4d9c7ecb 100644
--- a/ipc/kdbus/metadata.c
+++ b/ipc/kdbus/metadata.c
@@ -725,7 +725,7 @@ static void kdbus_meta_export_caps(struct kdbus_meta_caps *out,
 {
 	struct user_namespace *iter;
 	const struct cred *cred = mp->cred;
-	bool parent = false, owner = false;
+	bool member = false, owner = false;
 	int i;
 
 	/*
@@ -748,7 +748,7 @@ static void kdbus_meta_export_caps(struct kdbus_meta_caps *out,
 	 */
 	for (iter = user_ns; iter; iter = iter->parent) {
 		if (iter == cred->user_ns) {
-			parent = true;
+			member = true;
 			break;
 		}
 
@@ -765,7 +765,7 @@ static void kdbus_meta_export_caps(struct kdbus_meta_caps *out,
 	out->last_cap = CAP_LAST_CAP;
 
 	CAP_FOR_EACH_U32(i) {
-		if (parent) {
+		if (member) {
 			out->set[0].caps[i] = cred->cap_inheritable.cap[i];
 			out->set[1].caps[i] = cred->cap_permitted.cap[i];
 			out->set[2].caps[i] = cred->cap_effective.cap[i];
-- 
1.8.3.1

[PATCH 09/44] kdbus: Remove unused KDBUS_MSG_MAX_SIZE constant

[Sergei Zviagintsev]

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/limits.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/ipc/kdbus/limits.h b/ipc/kdbus/limits.h
index bd47119cdf1b..5e4d2b5b0a9d 100644
--- a/ipc/kdbus/limits.h
+++ b/ipc/kdbus/limits.h
@@ -16,9 +16,6 @@
 
 #include <linux/kernel.h>
 
-/* maximum size of message header and items */
-#define KDBUS_MSG_MAX_SIZE		SZ_8K
-
 /* maximum number of memfd items per message */
 #define KDBUS_MSG_MAX_MEMFD_ITEMS	16
 
-- 
1.8.3.1

[PATCH 10/44] kdbus: Use conditional operator

[Sergei Zviagintsev]

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/names.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/ipc/kdbus/names.c b/ipc/kdbus/names.c
index bf44ca3f12b6..6b31b38ac2ad 100644
--- a/ipc/kdbus/names.c
+++ b/ipc/kdbus/names.c
@@ -438,10 +438,7 @@ static void kdbus_name_release_unlocked(struct kdbus_name_owner *owner)
 		name->activator = NULL;
 
 	if (!primary || owner == primary) {
-		next = kdbus_name_entry_first(name);
-		if (!next)
-			next = name->activator;
-
+		next = kdbus_name_entry_first(name) ?: name->activator;
 		if (next) {
 			/* hand to next in queue */
 			next->flags &= ~KDBUS_NAME_IN_QUEUE;
-- 
1.8.3.1

[PATCH 11/44] kdbus: Cosmetic fix of kdbus_name_is_valid()

[Sergei Zviagintsev]

Initialize `dot' during declaration but not in the for-loop, as it is
not used as a loop cursor.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/names.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipc/kdbus/names.c b/ipc/kdbus/names.c
index 6b31b38ac2ad..4dea83defc8e 100644
--- a/ipc/kdbus/names.c
+++ b/ipc/kdbus/names.c
@@ -530,10 +530,10 @@ void kdbus_name_release_all(struct kdbus_name_registry *reg,
  */
 bool kdbus_name_is_valid(const char *p, bool allow_wildcard)
 {
-	bool dot, found_dot = false;
+	bool dot = true, found_dot = false;
 	const char *q;
 
-	for (dot = true, q = p; *q; q++) {
+	for (q = p; *q; q++) {
 		if (*q == '.') {
 			if (dot)
 				return false;
-- 
1.8.3.1

[PATCH 12/44] kdbus: Use conventional list macros in __kdbus_pool_slice_release()

[Sergei Zviagintsev]

Use list_next_entry() and list_prev_entry().

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/pool.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/ipc/kdbus/pool.c b/ipc/kdbus/pool.c
index c26ef963d8d1..cb692a35755b 100644
--- a/ipc/kdbus/pool.c
+++ b/ipc/kdbus/pool.c
@@ -287,8 +287,7 @@ static void __kdbus_pool_slice_release(struct kdbus_pool_slice *slice)
 	if (!list_is_last(&slice->entry, &pool->slices)) {
 		struct kdbus_pool_slice *s;
 
-		s = list_entry(slice->entry.next,
-			       struct kdbus_pool_slice, entry);
+		s = list_next_entry(slice, entry);
 		if (s->free) {
 			rb_erase(&s->rb_node, &pool->slices_free);
 			list_del(&s->entry);
@@ -301,8 +300,7 @@ static void __kdbus_pool_slice_release(struct kdbus_pool_slice *slice)
 	if (pool->slices.next != &slice->entry) {
 		struct kdbus_pool_slice *s;
 
-		s = list_entry(slice->entry.prev,
-			       struct kdbus_pool_slice, entry);
+		s = list_prev_entry(slice, entry);
 		if (s->free) {
 			/*
 			 * As size of slice increases after merge and free
-- 
1.8.3.1

[PATCH 13/44] kdbus: Use list_next_entry() in kdbus_queue_entry_unlink()

[Sergei Zviagintsev]

Use list_next_entry() instead of list_first_entry().

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/queue.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ipc/kdbus/queue.c b/ipc/kdbus/queue.c
index f9c44d7bae6d..90e8d16f5967 100644
--- a/ipc/kdbus/queue.c
+++ b/ipc/kdbus/queue.c
@@ -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

Re: [PATCH 13/44] kdbus: Use list_next_entry() in kdbus_queue_entry_unlink()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> Use list_next_entry() instead of list_first_entry().

This commit-message is completely useless. This is not a cosmetic
change, so please justify your changes. Sure, in this case both
list_next_entry() and list_first_entry() perform the same operation,
but you should really describe such changes in the commit message to
allow reviewers to understand the intent of your change.

Anyway, patch looks good, thanks!
David

> Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> ---
>  ipc/kdbus/queue.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/ipc/kdbus/queue.c b/ipc/kdbus/queue.c
> index f9c44d7bae6d..90e8d16f5967 100644
> --- a/ipc/kdbus/queue.c
> +++ b/ipc/kdbus/queue.c
> @@ -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
>

[PATCH 14/44] kdbus: Simplify expression in kdbus_get_memfd()

[Sergei Zviagintsev]

'(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 <sergei@s15v.net>
---
 ipc/kdbus/message.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
index ae565cd343f8..c7ef23d40471 100644
--- a/ipc/kdbus/message.c
+++ b/ipc/kdbus/message.c
@@ -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

Re: [PATCH 14/44] kdbus: Simplify expression in kdbus_get_memfd()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> '(s & m) != m' means that mask 'm' contains some bits which are not set
> in 's', and this is literally equal to '~s & m'.

Sure, but you make the code look much less obvious. Checking a bit is
set is "a & b", checking if not set is "!(a & b)". If you check
whether a whole mask is set, you run "(a & m) == m", checking whether
it not set should be the negation, which is "(a & m) != m".

I'd prefer keeping the current code, unless the compiler cannot figure
it out on its own.
David

> Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> ---
>  ipc/kdbus/message.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
> index ae565cd343f8..c7ef23d40471 100644
> --- a/ipc/kdbus/message.c
> +++ b/ipc/kdbus/message.c
> @@ -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
>

[PATCH 15/44] kdbus: Simplify bitwise expression in kdbus_meta_get_mask()

[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

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/metadata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/kdbus/metadata.c b/ipc/kdbus/metadata.c
index 788b4d9c7ecb..61215a078359 100644
--- a/ipc/kdbus/metadata.c
+++ b/ipc/kdbus/metadata.c
@@ -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

Re: [PATCH 15/44] kdbus: Simplify bitwise expression in kdbus_meta_get_mask()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> 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

> Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> ---
>  ipc/kdbus/metadata.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ipc/kdbus/metadata.c b/ipc/kdbus/metadata.c
> index 788b4d9c7ecb..61215a078359 100644
> --- a/ipc/kdbus/metadata.c
> +++ b/ipc/kdbus/metadata.c
> @@ -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
>

Re: [PATCH 15/44] kdbus: Simplify bitwise expression in kdbus_meta_get_mask()

[Sergei Zviagintsev]

Hi David,

On Thu, Oct 08, 2015 at 04:24:30PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > 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?

The variant I propose has one to one correspondence to the description,
but is shorter and has no multi levels of parentheses, thus easier to
read, IMO. The comment says "... set of metadata that is not granted
implicitly" (which is ~impl_mask), "... nor by the sender" (~prv_mask),
"... but still requested by the receiver" (req_mask).

We can leave parentheses, i.e. 'req_mask & ~(prv_mask | impl_mask)', but
even in this case the original code does one extra bitwise AND.

> David
> 
> > Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> > ---
> >  ipc/kdbus/metadata.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ipc/kdbus/metadata.c b/ipc/kdbus/metadata.c
> > index 788b4d9c7ecb..61215a078359 100644
> > --- a/ipc/kdbus/metadata.c
> > +++ b/ipc/kdbus/metadata.c
> > @@ -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
> >

[PATCH 16/44] kdbus: Drop redundant code from kdbus_name_acquire()

[Sergei Zviagintsev]

We already reached the end of function at this point, so remove useless
goto.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/names.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ipc/kdbus/names.c b/ipc/kdbus/names.c
index 4dea83defc8e..a1c3442f8e53 100644
--- a/ipc/kdbus/names.c
+++ b/ipc/kdbus/names.c
@@ -412,8 +412,6 @@ int kdbus_name_acquire(struct kdbus_name_registry *reg,
 		ret = kdbus_name_become_activator(owner, return_flags);
 	else
 		ret = kdbus_name_update(owner, flags, return_flags);
-	if (ret < 0)
-		goto exit;
 
 exit:
 	if (owner && !kdbus_name_owner_is_used(owner))
-- 
1.8.3.1

[PATCH 17/44] kdbus: Drop duplicated code from kdbus_pool_slice_alloc()

[Sergei Zviagintsev]

Check the value of `ret' instead of keeping separate exit point.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/pool.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/ipc/kdbus/pool.c b/ipc/kdbus/pool.c
index cb692a35755b..75a7366baa34 100644
--- a/ipc/kdbus/pool.c
+++ b/ipc/kdbus/pool.c
@@ -261,13 +261,10 @@ struct kdbus_pool_slice *kdbus_pool_slice_alloc(struct kdbus_pool *pool,
 	s->accounted = accounted;
 	if (accounted)
 		pool->accounted_size += s->size;
-	mutex_unlock(&pool->lock);
-
-	return s;
 
 exit_unlock:
 	mutex_unlock(&pool->lock);
-	return ERR_PTR(ret);
+	return ret < 0 ? ERR_PTR(ret) : s;
 }
 
 static void __kdbus_pool_slice_release(struct kdbus_pool_slice *slice)
-- 
1.8.3.1

[PATCH 18/44] kdbus: Add var initialization to kdbus_conn_entry_insert()

[Sergei Zviagintsev]

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

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index 4f3cd370ecd9..185ed3ba1bce 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -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);
 
@@ -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;
-
 exit_unlock:
 	kdbus_conn_unlock2(conn_src, conn_dst);
 	return ret;
-- 
1.8.3.1

Re: [PATCH 18/44] kdbus: Add var initialization to kdbus_conn_entry_insert()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> Assign zero to `ret' in the beginning of function instead of doing it
> in the end.
>
> Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> ---
>  ipc/kdbus/connection.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index 4f3cd370ecd9..185ed3ba1bce 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -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);
>
> @@ -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

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

Re: [PATCH 18/44] kdbus: Add var initialization to kdbus_conn_entry_insert()

[Sergei Zviagintsev]

Hi,

On Thu, Oct 08, 2015 at 04:28:29PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > Assign zero to `ret' in the beginning of function instead of doing it
> > in the end.
> >
> > Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> > ---
> >  ipc/kdbus/connection.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index 4f3cd370ecd9..185ed3ba1bce 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -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);
> >
> > @@ -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?

The rationale is to keep things simple. That `ret' var is used only once
to deliver the error code, and the function itself has only two local
vars and fits into my 12.5 inch thinkpad screen, so IMO that extra line
with assignment is redundant. I agree that in some cases we need to
handle 'ret > 0', but using same templates for every particular case
produces boring code :)

And BTW we have this style in number of places over kdbus code. For
example see kdbus_handle_ioctl_control(), kdbus_handle_ioctl_ep(),
kdbus_name_update(), kdbus_name_release(), kdbus_pool_release_offset(),
kdbus_pool_slice_copy().

> 
> Thanks
> David
> 
> >  exit_unlock:
> >         kdbus_conn_unlock2(conn_src, conn_dst);
> >         return ret;
> > --
> > 1.8.3.1
> >

[PATCH 19/44] kdbus: Drop useless initialization from kdbus_conn_reply()

[Sergei Zviagintsev]

`name' is assigned the value at first use by kdbus_pin_dst(). Do not
initialize it during declaration.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index 185ed3ba1bce..4b5ed4bb59c7 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -1104,7 +1104,7 @@ static int kdbus_conn_reply(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 *reply, *wake = NULL;
 	struct kdbus_conn *dst = NULL;
 	struct kdbus_bus *bus = src->ep->bus;
-- 
1.8.3.1

[PATCH 20/44] kdbus: Drop useless initialization from kdbus_cmd_hello()

[Sergei Zviagintsev]

Do not initialize `c' as it is assigned a return value of
kdbus_conn_new() at the first use.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index 4b5ed4bb59c7..93da7f539f74 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -1591,7 +1591,7 @@ struct kdbus_conn *kdbus_cmd_hello(struct kdbus_ep *ep, struct file *file,
 				   void __user *argp)
 {
 	struct kdbus_cmd_hello *cmd;
-	struct kdbus_conn *c = NULL;
+	struct kdbus_conn *c;
 	const char *item_name;
 	int ret;
 
-- 
1.8.3.1

[PATCH 21/44] kdbus: Cleanup tests in kdbus_cmd_send()

[Sergei Zviagintsev]

Rearrange tests a little to make them look cleaner.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index 93da7f539f74..a4d7414ecaea 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -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

Re: [PATCH 21/44] kdbus: Cleanup tests in kdbus_cmd_send()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> Rearrange tests a little to make them look cleaner.

What's wrong with 'else if'?

Thanks
David

> Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> ---
>  ipc/kdbus/connection.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index 93da7f539f74..a4d7414ecaea 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -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
>

Re: [PATCH 21/44] kdbus: Cleanup tests in kdbus_cmd_send()

[Sergei Zviagintsev]

On Thu, Oct 08, 2015 at 04:30:59PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > Rearrange tests a little to make them look cleaner.
> 
> What's wrong with 'else if'?

Less verbose constructions are easier for our brain to accept :)
Moreover, we usually don't write code in the 'else' branch of
if-statement which handles an error-path.

But I don't mind dropping this patch, it really doesn't save the
universe.

> 
> Thanks
> David
> 
> > Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> > ---
> >  ipc/kdbus/connection.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index 93da7f539f74..a4d7414ecaea 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -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
> >

[PATCH 22/44] kdbus: Cleanup error path in kdbus_staging_new_user()

[Sergei Zviagintsev]

Replace duplicated cleanup code with jump to exit point.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/message.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
index c7ef23d40471..e337b1b1024a 100644
--- a/ipc/kdbus/message.c
+++ b/ipc/kdbus/message.c
@@ -800,8 +800,7 @@ struct kdbus_staging *kdbus_staging_new_user(struct kdbus_bus *bus,
 		if (IS_ERR(staging->gaps)) {
 			ret = PTR_ERR(staging->gaps);
 			staging->gaps = NULL;
-			kdbus_staging_free(staging);
-			return ERR_PTR(ret);
+			goto error;
 		}
 	}
 
-- 
1.8.3.1

[PATCH 23/44] kdbus: Cleanup kdbus_conn_call()

[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 <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index a4d7414ecaea..db49f282a1bf 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -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,
 	}
 
 	/* 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;
 
 exit:
 	up_read(&bus->name_registry->rwlock);
-- 
1.8.3.1

Re: [PATCH 23/44] kdbus: Cleanup kdbus_conn_call()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> 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 <sergei@s15v.net>
> ---
>  ipc/kdbus/connection.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index a4d7414ecaea..db49f282a1bf 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -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,
>         }
>
>         /* 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

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

Re: [PATCH 23/44] kdbus: Cleanup kdbus_conn_call()

[Sergei Zviagintsev]

On Thu, Oct 08, 2015 at 04:32:47PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > 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 <sergei@s15v.net>
> > ---
> >  ipc/kdbus/connection.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index a4d7414ecaea..db49f282a1bf 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -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,
> >         }
> >
> >         /* 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.

That is clearly written in its kernel-doc and its code. In this
particular case 'ret > 0' situation doesn't matter at all as we only do
'ret < 0' test latter and return `wait' var (the commit message isn't
clear about that).

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

[PATCH 24/44] kdbus: Cleanup kdbus_conn_unicast()

[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 <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index db49f282a1bf..b3c5f20a57d8 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -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);
 
 	/* find and pin destination */
-
 	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;
 
 	/* signals are treated like broadcasts, recv-errors are ignored */
-	ret = 0;
+	if (is_signal)
+		ret = 0;
 
 exit:
 	up_read(&bus->name_registry->rwlock);
-- 
1.8.3.1

Re: [PATCH 24/44] kdbus: Cleanup kdbus_conn_unicast()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> 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 <sergei@s15v.net>
> ---
>  ipc/kdbus/connection.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index db49f282a1bf..b3c5f20a57d8 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -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);
>
>         /* 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.

>         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;
>
>         /* 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

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

Re: [PATCH 24/44] kdbus: Cleanup kdbus_conn_unicast()

[Sergei Zviagintsev]

On Thu, Oct 08, 2015 at 04:34:27PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > 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 <sergei@s15v.net>
> > ---
> >  ipc/kdbus/connection.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index db49f282a1bf..b3c5f20a57d8 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -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);
> >
> >         /* 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.

Sorry, this change is here by mistake and shouldn't be in the patch at
all.

> 
> >         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;
> >
> >         /* 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.

IMO, it's easier to read as it's exacly what the comment says: ignore an
error in the case of signal. But I don't mind omitting this change from
the next submission.

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

[PATCH 25/44] kdbus: Cleanup kdbus_cmd_conn_info()

[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 <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index b3c5f20a57d8..6a73ac3f444d 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -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;
 
 	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;
+
 		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);
 
 	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;
 
 exit:
 	up_read(&bus->name_registry->rwlock);
-- 
1.8.3.1

Re: [PATCH 25/44] kdbus: Cleanup kdbus_cmd_conn_info()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
>  - 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 <sergei@s15v.net>
> ---
>  ipc/kdbus/connection.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index b3c5f20a57d8..6a73ac3f444d 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -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;
>
>         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.

>                 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);
>
>         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

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

Re: [PATCH 25/44] kdbus: Cleanup kdbus_cmd_conn_info()

[Sergei Zviagintsev]

Hi,

On Thu, Oct 08, 2015 at 04:38:11PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> >  - 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 <sergei@s15v.net>
> > ---
> >  ipc/kdbus/connection.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index b3c5f20a57d8..6a73ac3f444d 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -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;
> >
> >         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.
> 
> >                 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);
> >
> >         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.

This change was intended to stress to one who is reading the code that
kdbus_pool_slice_copy_kvec() returns >= 0 on success (as it returns the
number of bytes copied in contrast to most of functions which return 0 on
success). The only reason we have 'ret = 0' in the end of the function
is to reset `ret' after kdbus_pool_slice_copy_kvec(), so I decided to
group it together. Without kdbus_pool_slice_copy_kvec() we could return
`ret' as is, because it is always zero on success path.

BTW, kdbus_node_link() for example does the same: uses `ret' to handle
the return val of ida_simple_get() and then reset it to zero
immediately.

But that's too much words on such a simple change. I don't mind omitting
it from v2.

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

[PATCH 26/44] kdbus: Cleanup kdbus_pin_dst()

[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 <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index 6a73ac3f444d..ace587ee951a 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -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);
 
@@ -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;
 		}
 	} else {
+		struct kdbus_name_owner *owner;
+
 		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;
 		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;
 }
 
 static int kdbus_conn_reply(struct kdbus_conn *src,
-- 
1.8.3.1

Re: [PATCH 26/44] kdbus: Cleanup kdbus_pin_dst()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
>  - 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 <sergei@s15v.net>
> ---
>  ipc/kdbus/connection.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index 6a73ac3f444d..ace587ee951a 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -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);
>
> @@ -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.

>                 }
>         } else {
> +               struct kdbus_name_owner *owner;
> +

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

>                 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.

>                 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

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

Re: [PATCH 26/44] kdbus: Cleanup kdbus_pin_dst()

[Sergei Zviagintsev]

Hi,

On Thu, Oct 08, 2015 at 04:40:33PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> >  - 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 <sergei@s15v.net>
> > ---
> >  ipc/kdbus/connection.c | 17 ++++++-----------
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> >
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index 6a73ac3f444d..ace587ee951a 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -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);
> >
> > @@ -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.
> 
> >                 }
> >         } else {
> > +               struct kdbus_name_owner *owner;
> > +
> 
> I'd prefer if you avoid making this block-local.

What is the rule of keeping (or not) things block-local? I always
thought the less vars we keep in the mind at time, the easier is a
piece of code to read.

> 
> >                 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.
> 
> >                 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
> 
> >  }
> >
> >  static int kdbus_conn_reply(struct kdbus_conn *src,
> > --
> > 1.8.3.1
> >

[PATCH 27/44] kdbus: Cleanup kdbus_conn_new()

[Sergei Zviagintsev]

 - Replace two tests with one. Sequence of tests

       name && !is_activator && !is_policy_holder
       !name && (is_activator || is_policy_holder)

   is the same as

       name XOR (is_activator || is_policy_holder)

   Replace these two expressions with

       !name == (is_activator || is_policy_holder)

 - Drop `privileged' var which is used only once to set value of
   conn->privileged.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index ace587ee951a..c57ff2c846ee 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -73,7 +73,6 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep,
 	bool is_policy_holder;
 	bool is_activator;
 	bool is_monitor;
-	bool privileged;
 	bool owner;
 	struct kvec kvec;
 	int ret;
@@ -84,9 +83,7 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep,
 		struct kdbus_bloom_parameter bloom;
 	} bloom_item;
 
-	privileged = kdbus_ep_is_privileged(ep, file);
 	owner = kdbus_ep_is_owner(ep, file);
-
 	is_monitor = hello->flags & KDBUS_HELLO_MONITOR;
 	is_activator = hello->flags & KDBUS_HELLO_ACTIVATOR;
 	is_policy_holder = hello->flags & KDBUS_HELLO_POLICY_HOLDER;
@@ -95,9 +92,7 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep,
 		return ERR_PTR(-EINVAL);
 	if (is_monitor + is_activator + is_policy_holder > 1)
 		return ERR_PTR(-EINVAL);
-	if (name && !is_activator && !is_policy_holder)
-		return ERR_PTR(-EINVAL);
-	if (!name && (is_activator || is_policy_holder))
+	if (!name == (is_activator || is_policy_holder))
 		return ERR_PTR(-EINVAL);
 	if (name && !kdbus_name_is_valid(name, true))
 		return ERR_PTR(-EINVAL);
@@ -138,7 +133,7 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep,
 	get_fs_root(current->fs, &conn->root_path);
 	init_waitqueue_head(&conn->wait);
 	kdbus_queue_init(&conn->queue);
-	conn->privileged = privileged;
+	conn->privileged = kdbus_ep_is_privileged(ep, file);
 	conn->owner = owner;
 	conn->ep = kdbus_ep_ref(ep);
 	conn->id = atomic64_inc_return(&bus->domain->last_id);
-- 
1.8.3.1

[PATCH 28/44] kdbus: Cleanup kdbus_queue_entry_new()

[Sergei Zviagintsev]

Remove separate exit point as the only error case in the function is
when kdbus_staging_emit() fails. Assign a slice to temporary var first
to not clear it explicitly on error and to return an error code without
`ret' variable.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/queue.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/ipc/kdbus/queue.c b/ipc/kdbus/queue.c
index 90e8d16f5967..3c0fb3bb55da 100644
--- a/ipc/kdbus/queue.c
+++ b/ipc/kdbus/queue.c
@@ -185,7 +185,7 @@ struct kdbus_queue_entry *kdbus_queue_entry_new(struct kdbus_conn *src,
 						struct kdbus_staging *s)
 {
 	struct kdbus_queue_entry *entry;
-	int ret;
+	struct kdbus_pool_slice *slice;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry)
@@ -196,19 +196,15 @@ struct kdbus_queue_entry *kdbus_queue_entry_new(struct kdbus_conn *src,
 	entry->conn = kdbus_conn_ref(dst);
 	entry->gaps = kdbus_gaps_ref(s->gaps);
 
-	entry->slice = kdbus_staging_emit(s, src, dst);
-	if (IS_ERR(entry->slice)) {
-		ret = PTR_ERR(entry->slice);
-		entry->slice = NULL;
-		goto error;
+	slice = kdbus_staging_emit(s, src, dst);
+	if (IS_ERR(slice)) {
+		kdbus_queue_entry_free(entry);
+		return ERR_CAST(slice);
 	}
 
+	entry->slice = slice;
 	entry->user = src ? kdbus_user_ref(src->user) : NULL;
 	return entry;
-
-error:
-	kdbus_queue_entry_free(entry);
-	return ERR_PTR(ret);
 }
 
 /**
-- 
1.8.3.1

[PATCH 29/44] kdbus: Improve tests on incrementing quota

[Sergei Zviagintsev]

 - Rewrite

       quota->memory + memory > U32_MAX

   as

       U32_MAX - quota->memory < memory

   There is no overflow issue in the original expression on 32-bit
   because the previous one (available - quota->memory < memory)
   guarantees that quota->memory + memory doesn't exceed `available'
   which is <= U32_MAX in that case. But lets replace it with less
   ambiguous variant.

 - Replace

       quota->fds + fds < quota->fds ||
       quota->fds + fds > KDBUS_CONN_MAX_FDS_PER_USER

   with

       quota->fds > KDBUS_CONN_MAX_FDS_PER_USER ||
       KDBUS_CONN_MAX_FDS_PER_USER - quota->fds < fds

   Reading the code, one can assume that the first original expression
   is there to ensure that quota->fds won't overflow after
   quota->fds += fds, but what it really does is testing for
   size_t overflow in `quota->fds + fds' to be safe in the second
   expression (as fds is size_t, quota->fds is converted to bigger
   type).

   Rewrite it in more obvious way. KDBUS_CONN_MAX_FDS_PER_USER is
   checked at compile time to fill in quota->fds type (there is
   BUILD_BUG_ON), so no further checks for quota->fds overflow are
   needed.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index c57ff2c846ee..b32b4f981618 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -717,12 +717,12 @@ int kdbus_conn_quota_inc(struct kdbus_conn *c, struct kdbus_user *u,
 
 	if (available < quota->memory ||
 	    available - quota->memory < memory ||
-	    quota->memory + memory > U32_MAX)
+	    U32_MAX - quota->memory < memory)
 		return -ENOBUFS;
 	if (quota->msgs >= KDBUS_CONN_MAX_MSGS)
 		return -ENOBUFS;
-	if (quota->fds + fds < quota->fds ||
-	    quota->fds + fds > KDBUS_CONN_MAX_FDS_PER_USER)
+	if (quota->fds > KDBUS_CONN_MAX_FDS_PER_USER ||
+	    KDBUS_CONN_MAX_FDS_PER_USER - quota->fds < fds)
 		return -EMFILE;
 
 	quota->memory += memory;
-- 
1.8.3.1

[PATCH 30/44] kdbus: Cleanup kdbus_meta_proc_mask()

[Sergei Zviagintsev]

 - 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 <sergei@s15v.net>
---
 ipc/kdbus/metadata.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/ipc/kdbus/metadata.c b/ipc/kdbus/metadata.c
index 61215a078359..b8d094d9fb56 100644
--- a/ipc/kdbus/metadata.c
+++ b/ipc/kdbus/metadata.c
@@ -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 |
-			    KDBUS_ATTACH_PID_COMM |
-			    KDBUS_ATTACH_SECLABEL |
-			    KDBUS_ATTACH_CMDLINE |
-			    KDBUS_ATTACH_CGROUP |
-			    KDBUS_ATTACH_AUDIT |
-			    KDBUS_ATTACH_CAPS |
-			    KDBUS_ATTACH_EXE);
+	if (prv_pid != req_pid) {
+		unsigned int 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)
+			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

Re: [PATCH 30/44] kdbus: Cleanup kdbus_meta_proc_mask()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
>  - 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 <sergei@s15v.net>
> ---
>  ipc/kdbus/metadata.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
>
> diff --git a/ipc/kdbus/metadata.c b/ipc/kdbus/metadata.c
> index 61215a078359..b8d094d9fb56 100644
> --- a/ipc/kdbus/metadata.c
> +++ b/ipc/kdbus/metadata.c
> @@ -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 |
> -                           KDBUS_ATTACH_PID_COMM |
> -                           KDBUS_ATTACH_SECLABEL |
> -                           KDBUS_ATTACH_CMDLINE |
> -                           KDBUS_ATTACH_CGROUP |
> -                           KDBUS_ATTACH_AUDIT |
> -                           KDBUS_ATTACH_CAPS |
> -                           KDBUS_ATTACH_EXE);
> +       if (prv_pid != req_pid) {
> +               unsigned int proc = kdbus_proc_permission(req_ns, req_cred,
> +                                                         prv_pid);

Please keep "proc" declared non-local. Also, we don't do direct
assignments of non-static data in declarations.

Thanks
David

> +
> +               /* 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
>

[PATCH 31/44] kdbus: Cleanup kdbus_conn_move_messages()

[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 <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index b32b4f981618..6ee688d3de53 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -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;
 
-	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;
+
 		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) {
 			if (r->reply_src != conn_src)
 				continue;
 
@@ -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;
+
 		/* 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;
 		}
 	}
 	kdbus_conn_unlock2(conn_src, conn_dst);
-- 
1.8.3.1

Re: [PATCH 31/44] kdbus: Cleanup kdbus_conn_move_messages()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
>  - 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 <sergei@s15v.net>
> ---
>  ipc/kdbus/connection.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index b32b4f981618..6ee688d3de53 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -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;
>
> -       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?

>                 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.

>                         if (r->reply_src != conn_src)
>                                 continue;
>
> @@ -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?

>                 /* 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

>                 }
>         }
>         kdbus_conn_unlock2(conn_src, conn_dst);
> --
> 1.8.3.1
>

Re: [PATCH 31/44] kdbus: Cleanup kdbus_conn_move_messages()

[Sergei Zviagintsev]

Hi,

On Thu, Oct 08, 2015 at 04:50:31PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> >  - 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 <sergei@s15v.net>
> > ---
> >  ipc/kdbus/connection.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index b32b4f981618..6ee688d3de53 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -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;
> >
> > -       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?

So what is the rule to keep some vars block-local and some not? This
function has a number of local vars, so the rationale is to keep things
simple. We don't use `r' function wide, but use it only in this block,
so why don't have it block-local?

> 
> >                 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.
> 
> >                         if (r->reply_src != conn_src)
> >                                 continue;
> >
> > @@ -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?

Same thing here. We have the only use of this `ret' var, so why don't
keep it right here?

> 
> >                 /* 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
> 
> >                 }
> >         }
> >         kdbus_conn_unlock2(conn_src, conn_dst);
> > --
> > 1.8.3.1
> >

[PATCH 32/44] kdbus: Remove duplicated code from kdbus_conn_lock2()

[Sergei Zviagintsev]

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/connection.h | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/ipc/kdbus/connection.h b/ipc/kdbus/connection.h
index 679f393d3e68..3b382b604348 100644
--- a/ipc/kdbus/connection.h
+++ b/ipc/kdbus/connection.h
@@ -215,17 +215,13 @@ static inline int kdbus_conn_is_monitor(const struct kdbus_conn *conn)
  */
 static inline void kdbus_conn_lock2(struct kdbus_conn *a, struct kdbus_conn *b)
 {
-	if (a < b) {
-		if (a)
-			mutex_lock(&a->lock);
-		if (b && b != a)
-			mutex_lock_nested(&b->lock, !!a);
-	} else {
-		if (b)
-			mutex_lock(&b->lock);
-		if (a && a != b)
-			mutex_lock_nested(&a->lock, !!b);
-	}
+	struct kdbus_conn *lo = min(a, b);
+	struct kdbus_conn *hi = max(a, b);
+
+	if (lo)
+		mutex_lock(&lo->lock);
+	if (hi && hi != lo)
+		mutex_lock_nested(&hi->lock, !!lo);
 }
 
 /**
-- 
1.8.3.1

[PATCH 33/44] kdbus: Improve kdbus_staging_reserve()

[Sergei Zviagintsev]

Allow to reserve N elements in row. This eliminates duplicated code.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/message.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
index e337b1b1024a..f2176796390d 100644
--- a/ipc/kdbus/message.c
+++ b/ipc/kdbus/message.c
@@ -647,13 +647,14 @@ static int kdbus_staging_import(struct kdbus_staging *staging)
 	return 0;
 }
 
-static void kdbus_staging_reserve(struct kdbus_staging *staging)
+static void kdbus_staging_reserve(struct kdbus_staging *staging, size_t n)
 {
-	struct iovec *part;
+	while (n--) {
+		struct iovec *part = &staging->parts[staging->n_parts++];
 
-	part = &staging->parts[staging->n_parts++];
-	part->iov_base = (void __user *)zeros;
-	part->iov_len = 0;
+		part->iov_base = (void __user *)zeros;
+		part->iov_len = 0;
+	}
 }
 
 static struct kdbus_staging *kdbus_staging_new(struct kdbus_bus *bus,
@@ -701,16 +702,9 @@ static struct kdbus_staging *kdbus_staging_new(struct kdbus_bus *bus,
 	 *   * iovec for possible padding after the items
 	 *   * iovec for metadata items
 	 *   * iovec for possible padding after the items
-	 *
-	 * Make sure to update @reserved_parts if you add more parts here.
 	 */
 
-	kdbus_staging_reserve(staging); /* msg.size */
-	kdbus_staging_reserve(staging); /* msg (minus msg.size) plus items */
-	kdbus_staging_reserve(staging); /* msg padding */
-	kdbus_staging_reserve(staging); /* meta */
-	kdbus_staging_reserve(staging); /* meta padding */
-
+	kdbus_staging_reserve(staging, reserved_parts);
 	return staging;
 
 error:
@@ -814,7 +808,7 @@ struct kdbus_staging *kdbus_staging_new_user(struct kdbus_bus *bus,
 	 */
 
 	ret = kdbus_staging_import(staging); /* payload */
-	kdbus_staging_reserve(staging); /* payload padding */
+	kdbus_staging_reserve(staging, reserved_parts); /* payload padding */
 
 	if (ret < 0)
 		goto error;
-- 
1.8.3.1

[PATCH 34/44] kdbus: Improve kdbus_conn_entry_sync_attach()

[Sergei Zviagintsev]

Use goto to handle error paths in conventional way. Use conditional
operator instead of `remote_ret' var. Update the comment on waking up
remote peer.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 52 ++++++++++++++++++++------------------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index 6ee688d3de53..081f248339f5 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -820,47 +820,37 @@ static int kdbus_conn_entry_sync_attach(struct kdbus_conn *conn_dst,
 					struct kdbus_reply *reply_wake)
 {
 	struct kdbus_queue_entry *entry;
-	int remote_ret, ret = 0;
+	int ret = 0;
 
 	mutex_lock(&reply_wake->reply_dst->lock);
 
-	/*
-	 * If we are still waiting then proceed, allocate a queue
-	 * entry and attach it to the reply object
-	 */
-	if (reply_wake->waiting) {
-		entry = kdbus_conn_entry_make(reply_wake->reply_src, conn_dst,
-					      staging);
-		if (IS_ERR(entry))
-			ret = PTR_ERR(entry);
-		else
-			/* Attach the entry to the reply object */
-			reply_wake->queue_entry = entry;
-	} else {
+	if (!reply_wake->waiting) {
 		ret = -ECONNRESET;
+		goto wake_up_remote;
 	}
 
 	/*
-	 * Update the reply object and wake up remote peer only
-	 * on appropriate return codes
-	 *
-	 * * -ECOMM: if the replying connection failed with -ECOMM
-	 *           then wakeup remote peer with -EREMOTEIO
-	 *
-	 *           We do this to differenciate between -ECOMM errors
-	 *           from the original sender perspective:
-	 *           -ECOMM error during the sync send and
-	 *           -ECOMM error during the sync reply, this last
-	 *           one is rewritten to -EREMOTEIO
-	 *
-	 * * Wake up on all other return codes.
+	 * We are still waiting. Allocate a queue entry and attach it to the
+	 * reply object.
 	 */
-	remote_ret = ret;
+	entry = kdbus_conn_entry_make(reply_wake->reply_src, conn_dst, staging);
+	if (IS_ERR(entry)) {
+		ret = PTR_ERR(entry);
+		goto wake_up_remote;
+	}
 
-	if (ret == -ECOMM)
-		remote_ret = -EREMOTEIO;
+	reply_wake->queue_entry = entry;
 
-	kdbus_sync_reply_wakeup(reply_wake, remote_ret);
+	/*
+	 * If the replying connection failed with -ECOMM then wakeup remote peer
+	 * with -EREMOTEIO. We do this to differentiate between -ECOMM errors
+	 * from the original sender perspective:
+	 *   * -ECOMM error during the sync send and
+	 *   * -ECOMM error during the sync reply, this last one is rewritten
+	 *     to -EREMOTEIO
+	 */
+wake_up_remote:
+	kdbus_sync_reply_wakeup(reply_wake, (ret == -ECOMM) ? -EREMOTEIO : ret);
 	kdbus_reply_unlink(reply_wake);
 	mutex_unlock(&reply_wake->reply_dst->lock);
 
-- 
1.8.3.1

[PATCH 35/44] kdbus: Drop goto from kdbus_queue_entry_link()

[Sergei Zviagintsev]

Use conventional if-else logic instead of goto, which makes the function
easier to read.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/queue.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/ipc/kdbus/queue.c b/ipc/kdbus/queue.c
index 3c0fb3bb55da..5158f00e7148 100644
--- a/ipc/kdbus/queue.c
+++ b/ipc/kdbus/queue.c
@@ -87,6 +87,7 @@ struct kdbus_queue_entry *kdbus_queue_peek(struct kdbus_queue *queue,
 static void kdbus_queue_entry_link(struct kdbus_queue_entry *entry)
 {
 	struct kdbus_queue *queue = &entry->conn->queue;
+	struct kdbus_queue_entry *e;
 	struct rb_node **n, *pn = NULL;
 	bool highest = true;
 
@@ -97,16 +98,11 @@ static void kdbus_queue_entry_link(struct kdbus_queue_entry *entry)
 	/* sort into priority entry tree */
 	n = &queue->msg_prio_queue.rb_node;
 	while (*n) {
-		struct kdbus_queue_entry *e;
-
 		pn = *n;
 		e = rb_entry(pn, struct kdbus_queue_entry, prio_node);
 
-		/* existing node for this priority, add to its list */
-		if (likely(entry->priority == e->priority)) {
-			list_add_tail(&entry->prio_entry, &e->prio_entry);
-			goto prio_done;
-		}
+		if (likely(entry->priority == e->priority))
+			break;
 
 		if (entry->priority < e->priority) {
 			n = &pn->rb_left;
@@ -116,16 +112,20 @@ static void kdbus_queue_entry_link(struct kdbus_queue_entry *entry)
 		}
 	}
 
-	/* cache highest-priority entry */
-	if (highest)
-		queue->msg_prio_highest = &entry->prio_node;
-
-	/* new node for this priority */
-	rb_link_node(&entry->prio_node, pn, n);
-	rb_insert_color(&entry->prio_node, &queue->msg_prio_queue);
-	INIT_LIST_HEAD(&entry->prio_entry);
+	if (*n) {
+		/* existing node for this priority, add to its list */
+		list_add_tail(&entry->prio_entry, &e->prio_entry);
+	} else {
+		/* cache highest-priority entry */
+		if (highest)
+			queue->msg_prio_highest = &entry->prio_node;
+
+		/* new node for this priority */
+		rb_link_node(&entry->prio_node, pn, n);
+		rb_insert_color(&entry->prio_node, &queue->msg_prio_queue);
+		INIT_LIST_HEAD(&entry->prio_entry);
+	}
 
-prio_done:
 	/* add to unsorted fifo list */
 	list_add_tail(&entry->entry, &queue->msg_list);
 }
-- 
1.8.3.1

[PATCH 36/44] kdbus: Improve kdbus_name_release()

[Sergei Zviagintsev]

Use goto to handle error paths in conventional way.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/names.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/ipc/kdbus/names.c b/ipc/kdbus/names.c
index a1c3442f8e53..8e53681d1b7b 100644
--- a/ipc/kdbus/names.c
+++ b/ipc/kdbus/names.c
@@ -474,18 +474,23 @@ static int kdbus_name_release(struct kdbus_name_registry *reg,
 	int ret = 0;
 
 	down_write(&reg->rwlock);
+
 	name = kdbus_name_entry_find(reg, kdbus_strhash(name_str), name_str);
-	if (name) {
-		owner = kdbus_name_owner_find(name, conn);
-		if (owner)
-			kdbus_name_release_unlocked(owner);
-		else
-			ret = -EADDRINUSE;
-	} else {
+	if (!name) {
 		ret = -ESRCH;
+		goto exit;
 	}
-	up_write(&reg->rwlock);
 
+	owner = kdbus_name_owner_find(name, conn);
+	if (!owner) {
+		ret = -EADDRINUSE;
+		goto exit;
+	}
+
+	kdbus_name_release_unlocked(owner);
+
+exit:
+	up_write(&reg->rwlock);
 	kdbus_notify_flush(conn->ep->bus);
 	return ret;
 }
-- 
1.8.3.1

[PATCH 37/44] kdbus: Fix error path in kdbus_meta_proc_collect_cgroup()

[Sergei Zviagintsev]

Current code checks return value of task_cgroup_path(), which can be
NULL if provided buffer isn't long enough to store path there, but
alters mp->valid in case of error, producing inconsistency. Return
-ENAMETOOLONG if task_cgroup_path() fails.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/metadata.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/ipc/kdbus/metadata.c b/ipc/kdbus/metadata.c
index b8d094d9fb56..f4f2b1af81a7 100644
--- a/ipc/kdbus/metadata.c
+++ b/ipc/kdbus/metadata.c
@@ -269,12 +269,15 @@ static int kdbus_meta_proc_collect_cgroup(struct kdbus_meta_proc *mp)
 		return -ENOMEM;
 
 	s = task_cgroup_path(current, page, PAGE_SIZE);
-	if (s) {
-		mp->cgroup = kstrdup(s, GFP_KERNEL);
-		if (!mp->cgroup) {
-			free_page((unsigned long)page);
-			return -ENOMEM;
-		}
+	if (!s) {
+		free_page((unsigned long)page);
+		return -ENAMETOOLONG;
+	}
+
+	mp->cgroup = kstrdup(s, GFP_KERNEL);
+	if (!mp->cgroup) {
+		free_page((unsigned long)page);
+		return -ENOMEM;
 	}
 
 	free_page((unsigned long)page);
-- 
1.8.3.1

[PATCH 38/44] kdbus: Fix error path in kdbus_user_lookup()

[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 <sergei@s15v.net>
---
 ipc/kdbus/domain.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/ipc/kdbus/domain.c b/ipc/kdbus/domain.c
index ac9f760c150d..31cd09fb572f 100644
--- a/ipc/kdbus/domain.c
+++ b/ipc/kdbus/domain.c
@@ -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;
 		}
 	}
 
@@ -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;
 
 	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

Re: [PATCH 38/44] kdbus: Fix error path in kdbus_user_lookup()

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> 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 <sergei@s15v.net>
> ---
>  ipc/kdbus/domain.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/ipc/kdbus/domain.c b/ipc/kdbus/domain.c
> index ac9f760c150d..31cd09fb572f 100644
> --- a/ipc/kdbus/domain.c
> +++ b/ipc/kdbus/domain.c
> @@ -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;
>                 }
>         }
>
> @@ -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

>         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
>

Re: [PATCH 38/44] kdbus: Fix error path in kdbus_user_lookup()

[Sergei Zviagintsev]

Hi David,

On Thu, Oct 08, 2015 at 05:06:57PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > 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 <sergei@s15v.net>
> > ---
> >  ipc/kdbus/domain.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/ipc/kdbus/domain.c b/ipc/kdbus/domain.c
> > index ac9f760c150d..31cd09fb572f 100644
> > --- a/ipc/kdbus/domain.c
> > +++ b/ipc/kdbus/domain.c
> > @@ -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;
> >                 }
> >         }
> >
> > @@ -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?

If I understand it right, in this case we have to firstly assign
INVALID_UID to u->uid (to check it with uid_valid() in the error path)
and then do 'u->uid = uid'. But from the first sight it would be not so
obvious and may require adding some comment on it. Isn't it better to
stay explicit here by maintaining several goto labels?

> 
> Thanks
> David
> 
> >         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
> >

[PATCH 39/44] kdbus: Cleanup kdbus_user_lookup()

[Sergei Zviagintsev]

 - Do not initialize `u' with NULL as value is assigned to it at the
   first use.

 - Simplify if-statement. If `old' is non-NULL, it means that provided
   @uid is valid, so there is no need to check both.

 - Use `uid' and `domain' instead of `u->uid' and `u->domain' in error
   path, which is equivalent but more concise and readable (as we used
   same vars in the code above).

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/domain.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/ipc/kdbus/domain.c b/ipc/kdbus/domain.c
index 31cd09fb572f..1da893089889 100644
--- a/ipc/kdbus/domain.c
+++ b/ipc/kdbus/domain.c
@@ -188,7 +188,7 @@ int kdbus_domain_populate(struct kdbus_domain *domain, unsigned int access)
  */
 struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
 {
-	struct kdbus_user *u = NULL, *old = NULL;
+	struct kdbus_user *u, *old = NULL;
 	int ret;
 
 	mutex_lock(&domain->lock);
@@ -217,16 +217,14 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
 	atomic_set(&u->buses, 0);
 	atomic_set(&u->connections, 0);
 
-	if (uid_valid(uid)) {
-		if (old) {
-			idr_replace(&domain->user_idr, u, __kuid_val(uid));
-			old->uid = INVALID_UID; /* mark old as removed */
-		} else {
-			ret = idr_alloc(&domain->user_idr, u, __kuid_val(uid),
-					__kuid_val(uid) + 1, GFP_KERNEL);
-			if (ret < 0)
-				goto exit_free;
-		}
+	if (old) {
+		idr_replace(&domain->user_idr, u, __kuid_val(uid));
+		old->uid = INVALID_UID; /* mark old as removed */
+	} else if (uid_valid(uid)) {
+		ret = idr_alloc(&domain->user_idr, u, __kuid_val(uid),
+				__kuid_val(uid) + 1, GFP_KERNEL);
+		if (ret < 0)
+			goto exit_free;
 	}
 
 	/*
@@ -242,10 +240,10 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
 	return u;
 
 exit_idr:
-	if (uid_valid(u->uid))
-		idr_remove(&domain->user_idr, __kuid_val(u->uid));
+	if (uid_valid(uid))
+		idr_remove(&domain->user_idr, __kuid_val(uid));
 exit_free:
-	kdbus_domain_unref(u->domain);
+	kdbus_domain_unref(domain);
 	kfree(u);
 exit_unlock:
 	mutex_unlock(&domain->lock);
-- 
1.8.3.1

[PATCH 40/44] kdbus: Cleanup kdbus_item_validate_name()

[Sergei Zviagintsev]

Calculate length of the string directly from item's payload size and
simplify tests. Do not use strlen(), as kdbus_str_valid() guarantees
that '\0' is located in the end of provided buffer. When examining
contents of the string, use pointer instead of array index to stay more
concise and readable.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/item.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/ipc/kdbus/item.c b/ipc/kdbus/item.c
index ce78dba03426..e6b6e2ca7e05 100644
--- a/ipc/kdbus/item.c
+++ b/ipc/kdbus/item.c
@@ -37,32 +37,26 @@ static bool kdbus_str_valid(const char *str, size_t size)
  */
 int kdbus_item_validate_name(const struct kdbus_item *item)
 {
-	const char *name = item->str;
-	unsigned int i;
-	size_t len;
+	const char *p, *name = item->str;
+	size_t len = KDBUS_ITEM_PAYLOAD_SIZE(item) - 1;
 
-	if (item->size < KDBUS_ITEM_HEADER_SIZE + 2)
+	if (len > item->size || len < 1)
 		return -EINVAL;
 
-	if (item->size > KDBUS_ITEM_HEADER_SIZE +
-			 KDBUS_SYSNAME_MAX_LEN + 1)
+	if (len > KDBUS_SYSNAME_MAX_LEN)
 		return -ENAMETOOLONG;
 
-	if (!kdbus_str_valid(name, KDBUS_ITEM_PAYLOAD_SIZE(item)))
+	if (!kdbus_str_valid(name, len + 1))
 		return -EINVAL;
 
-	len = strlen(name);
-	if (len == 0)
-		return -EINVAL;
-
-	for (i = 0; i < len; i++) {
-		if (isalpha(name[i]))
+	for (p = name; *p; ++p) {
+		if (isalpha(*p))
 			continue;
-		if (isdigit(name[i]))
+		if (isdigit(*p))
 			continue;
-		if (name[i] == '_')
+		if (*p == '_')
 			continue;
-		if (i > 0 && i + 1 < len && (name[i] == '-' || name[i] == '.'))
+		if (p > name && *(p + 1) && (*p == '-' || *p == '.'))
 			continue;
 
 		return -EINVAL;
-- 
1.8.3.1

[PATCH 41/44] kdbus: Fix memfd install algorithm

[Sergei Zviagintsev]

If file descriptor allocation for memfd fails, we do not fill the
corresponding position in `fds' array with -1. Later when we install
memfds, fds[gaps->n_fds + i] will contain garbage which we pass then
to fd_install(). Fix it by adding -1 to `fds' in case when we can't
get free file descriptor for memfd.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/message.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
index f2176796390d..0653a085c104 100644
--- a/ipc/kdbus/message.c
+++ b/ipc/kdbus/message.c
@@ -181,6 +181,7 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 		memfd = get_unused_fd_flags(O_CLOEXEC);
 		if (memfd < 0) {
 			incomplete_fds = true;
+			fds[n_fds++] = -1;
 			/* memfds are initialized to -1, skip copying it */
 			continue;
 		}
-- 
1.8.3.1

[PATCH 42/44] kdbus: Check if fd is allocated before trying to free it

[Sergei Zviagintsev]

Elements of `fds' array were set to -1 in case if we couldn't allocate
a fd. Verify that element contains a valid fd number before submitting
it to put_unused_fd().

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/message.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
index 0653a085c104..da685049d66c 100644
--- a/ipc/kdbus/message.c
+++ b/ipc/kdbus/message.c
@@ -221,7 +221,8 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 exit:
 	if (ret < 0)
 		for (i = 0; i < n_fds; ++i)
-			put_unused_fd(fds[i]);
+			if (fds[i] >= 0)
+				put_unused_fd(fds[i]);
 	kfree(fds);
 	*out_incomplete = incomplete_fds;
 	return ret;
-- 
1.8.3.1

[PATCH 43/44] kdbus: Give up on failed fd allocation

[Sergei Zviagintsev]

If we failed to allocate a file descriptor, do not try to do it again on
the next iteration. There are few chances that we will have success, and
we can simplify the algorithm assuming that valid fd numbers are not
mixed with -1 values.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/message.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
index da685049d66c..75e6213e7ed5 100644
--- a/ipc/kdbus/message.c
+++ b/ipc/kdbus/message.c
@@ -123,7 +123,7 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 {
 	bool incomplete_fds = false;
 	struct kvec kvec;
-	size_t i, n_fds;
+	size_t i, n_fds, n_memfds;
 	int ret, *fds;
 
 	if (!gaps) {
@@ -140,25 +140,31 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 	}
 
 	fds = kmalloc_array(n_fds, sizeof(*fds), GFP_TEMPORARY);
-	n_fds = 0;
 	if (!fds)
 		return -ENOMEM;
 
 	/* 1) allocate fds and copy them over */
 
+	n_fds = 0; /* n_fds now tracks the number of allocated fds */
 	if (gaps->n_fds > 0) {
 		for (i = 0; i < gaps->n_fds; ++i) {
 			int fd;
 
 			fd = get_unused_fd_flags(O_CLOEXEC);
-			if (fd < 0)
+			if (fd < 0) {
 				incomplete_fds = true;
+				break;
+			}
 
 			WARN_ON(!gaps->fd_files[i]);
 
-			fds[n_fds++] = fd < 0 ? -1 : fd;
+			fds[n_fds++] = fd;
 		}
 
+		/* If we couldn't allocate a fd, fill the rest with -1 */
+		for ( ; i < gaps->n_fds; ++i)
+			fds[i] = -1;
+
 		/*
 		 * The file-descriptor array can only be present once per
 		 * message. Hence, prepare all fds and then copy them over with
@@ -175,18 +181,18 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 			goto exit;
 	}
 
-	for (i = 0; i < gaps->n_memfds; ++i) {
+	n_memfds = 0;
+	for (i = 0; i < gaps->n_memfds && !incomplete_fds; ++i) {
 		int memfd;
 
 		memfd = get_unused_fd_flags(O_CLOEXEC);
 		if (memfd < 0) {
 			incomplete_fds = true;
-			fds[n_fds++] = -1;
 			/* memfds are initialized to -1, skip copying it */
-			continue;
+			break;
 		}
 
-		fds[n_fds++] = memfd;
+		fds[gaps->n_fds + n_memfds++] = memfd;
 
 		/*
 		 * memfds have to be copied individually as they each are put
@@ -208,21 +214,21 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 
 	/* 2) install fds now that everything was successful */
 
-	for (i = 0; i < gaps->n_fds; ++i)
-		if (fds[i] >= 0)
-			fd_install(fds[i], get_file(gaps->fd_files[i]));
-	for (i = 0; i < gaps->n_memfds; ++i)
-		if (fds[gaps->n_fds + i] >= 0)
-			fd_install(fds[gaps->n_fds + i],
-				   get_file(gaps->memfd_files[i]));
+	for (i = 0; i < n_fds; ++i)
+		fd_install(fds[i], get_file(gaps->fd_files[i]));
+	for (i = 0; i < n_memfds; ++i)
+		fd_install(fds[gaps->n_fds + i],
+			   get_file(gaps->memfd_files[i]));
 
 	ret = 0;
 
 exit:
-	if (ret < 0)
+	if (ret < 0) {
 		for (i = 0; i < n_fds; ++i)
-			if (fds[i] >= 0)
-				put_unused_fd(fds[i]);
+			put_unused_fd(fds[i]);
+		for (i = 0; i < n_memfds; ++i)
+			put_unused_fd(fds[gaps->n_fds + i]);
+	}
 	kfree(fds);
 	*out_incomplete = incomplete_fds;
 	return ret;
-- 
1.8.3.1

Re: [PATCH 43/44] kdbus: Give up on failed fd allocation

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> If we failed to allocate a file descriptor, do not try to do it again on
> the next iteration. There are few chances that we will have success, and
> we can simplify the algorithm assuming that valid fd numbers are not
> mixed with -1 values.

Why? This is no a fast-path, and the penalty is accounted on the
actual culprit (the caller). I don't see why we should optimize for
that case. If the fd-allocation fails, you clearly did something
wrong.

Thanks
David

> Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> ---
>  ipc/kdbus/message.c | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
> index da685049d66c..75e6213e7ed5 100644
> --- a/ipc/kdbus/message.c
> +++ b/ipc/kdbus/message.c
> @@ -123,7 +123,7 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
>  {
>         bool incomplete_fds = false;
>         struct kvec kvec;
> -       size_t i, n_fds;
> +       size_t i, n_fds, n_memfds;
>         int ret, *fds;
>
>         if (!gaps) {
> @@ -140,25 +140,31 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
>         }
>
>         fds = kmalloc_array(n_fds, sizeof(*fds), GFP_TEMPORARY);
> -       n_fds = 0;
>         if (!fds)
>                 return -ENOMEM;
>
>         /* 1) allocate fds and copy them over */
>
> +       n_fds = 0; /* n_fds now tracks the number of allocated fds */
>         if (gaps->n_fds > 0) {
>                 for (i = 0; i < gaps->n_fds; ++i) {
>                         int fd;
>
>                         fd = get_unused_fd_flags(O_CLOEXEC);
> -                       if (fd < 0)
> +                       if (fd < 0) {
>                                 incomplete_fds = true;
> +                               break;
> +                       }
>
>                         WARN_ON(!gaps->fd_files[i]);
>
> -                       fds[n_fds++] = fd < 0 ? -1 : fd;
> +                       fds[n_fds++] = fd;
>                 }
>
> +               /* If we couldn't allocate a fd, fill the rest with -1 */
> +               for ( ; i < gaps->n_fds; ++i)
> +                       fds[i] = -1;
> +
>                 /*
>                  * The file-descriptor array can only be present once per
>                  * message. Hence, prepare all fds and then copy them over with
> @@ -175,18 +181,18 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
>                         goto exit;
>         }
>
> -       for (i = 0; i < gaps->n_memfds; ++i) {
> +       n_memfds = 0;
> +       for (i = 0; i < gaps->n_memfds && !incomplete_fds; ++i) {
>                 int memfd;
>
>                 memfd = get_unused_fd_flags(O_CLOEXEC);
>                 if (memfd < 0) {
>                         incomplete_fds = true;
> -                       fds[n_fds++] = -1;
>                         /* memfds are initialized to -1, skip copying it */
> -                       continue;
> +                       break;
>                 }
>
> -               fds[n_fds++] = memfd;
> +               fds[gaps->n_fds + n_memfds++] = memfd;
>
>                 /*
>                  * memfds have to be copied individually as they each are put
> @@ -208,21 +214,21 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
>
>         /* 2) install fds now that everything was successful */
>
> -       for (i = 0; i < gaps->n_fds; ++i)
> -               if (fds[i] >= 0)
> -                       fd_install(fds[i], get_file(gaps->fd_files[i]));
> -       for (i = 0; i < gaps->n_memfds; ++i)
> -               if (fds[gaps->n_fds + i] >= 0)
> -                       fd_install(fds[gaps->n_fds + i],
> -                                  get_file(gaps->memfd_files[i]));
> +       for (i = 0; i < n_fds; ++i)
> +               fd_install(fds[i], get_file(gaps->fd_files[i]));
> +       for (i = 0; i < n_memfds; ++i)
> +               fd_install(fds[gaps->n_fds + i],
> +                          get_file(gaps->memfd_files[i]));
>
>         ret = 0;
>
>  exit:
> -       if (ret < 0)
> +       if (ret < 0) {
>                 for (i = 0; i < n_fds; ++i)
> -                       if (fds[i] >= 0)
> -                               put_unused_fd(fds[i]);
> +                       put_unused_fd(fds[i]);
> +               for (i = 0; i < n_memfds; ++i)
> +                       put_unused_fd(fds[gaps->n_fds + i]);
> +       }
>         kfree(fds);
>         *out_incomplete = incomplete_fds;
>         return ret;
> --
> 1.8.3.1
>

Re: [PATCH 43/44] kdbus: Give up on failed fd allocation

[Sergei Zviagintsev]

Hi,

On Thu, Oct 08, 2015 at 05:14:24PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > If we failed to allocate a file descriptor, do not try to do it again on
> > the next iteration. There are few chances that we will have success, and
> > we can simplify the algorithm assuming that valid fd numbers are not
> > mixed with -1 values.
> 
> Why? This is no a fast-path, and the penalty is accounted on the
> actual culprit (the caller). I don't see why we should optimize for
> that case. If the fd-allocation fails, you clearly did something
> wrong.

I agree, but this is optimization for readability and not for speed.
If we don't care whether fd-allocation failed or not, why should we keep
less readable variant which handles those -1 values mixed with valid fds
numbers?

> 
> Thanks
> David
> 
> > Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> > ---
> >  ipc/kdbus/message.c | 42 ++++++++++++++++++++++++------------------
> >  1 file changed, 24 insertions(+), 18 deletions(-)
> >
> > diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
> > index da685049d66c..75e6213e7ed5 100644
> > --- a/ipc/kdbus/message.c
> > +++ b/ipc/kdbus/message.c
> > @@ -123,7 +123,7 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
> >  {
> >         bool incomplete_fds = false;
> >         struct kvec kvec;
> > -       size_t i, n_fds;
> > +       size_t i, n_fds, n_memfds;
> >         int ret, *fds;
> >
> >         if (!gaps) {
> > @@ -140,25 +140,31 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
> >         }
> >
> >         fds = kmalloc_array(n_fds, sizeof(*fds), GFP_TEMPORARY);
> > -       n_fds = 0;
> >         if (!fds)
> >                 return -ENOMEM;
> >
> >         /* 1) allocate fds and copy them over */
> >
> > +       n_fds = 0; /* n_fds now tracks the number of allocated fds */
> >         if (gaps->n_fds > 0) {
> >                 for (i = 0; i < gaps->n_fds; ++i) {
> >                         int fd;
> >
> >                         fd = get_unused_fd_flags(O_CLOEXEC);
> > -                       if (fd < 0)
> > +                       if (fd < 0) {
> >                                 incomplete_fds = true;
> > +                               break;
> > +                       }
> >
> >                         WARN_ON(!gaps->fd_files[i]);
> >
> > -                       fds[n_fds++] = fd < 0 ? -1 : fd;
> > +                       fds[n_fds++] = fd;
> >                 }
> >
> > +               /* If we couldn't allocate a fd, fill the rest with -1 */
> > +               for ( ; i < gaps->n_fds; ++i)
> > +                       fds[i] = -1;
> > +
> >                 /*
> >                  * The file-descriptor array can only be present once per
> >                  * message. Hence, prepare all fds and then copy them over with
> > @@ -175,18 +181,18 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
> >                         goto exit;
> >         }
> >
> > -       for (i = 0; i < gaps->n_memfds; ++i) {
> > +       n_memfds = 0;
> > +       for (i = 0; i < gaps->n_memfds && !incomplete_fds; ++i) {
> >                 int memfd;
> >
> >                 memfd = get_unused_fd_flags(O_CLOEXEC);
> >                 if (memfd < 0) {
> >                         incomplete_fds = true;
> > -                       fds[n_fds++] = -1;
> >                         /* memfds are initialized to -1, skip copying it */
> > -                       continue;
> > +                       break;
> >                 }
> >
> > -               fds[n_fds++] = memfd;
> > +               fds[gaps->n_fds + n_memfds++] = memfd;
> >
> >                 /*
> >                  * memfds have to be copied individually as they each are put
> > @@ -208,21 +214,21 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
> >
> >         /* 2) install fds now that everything was successful */
> >
> > -       for (i = 0; i < gaps->n_fds; ++i)
> > -               if (fds[i] >= 0)
> > -                       fd_install(fds[i], get_file(gaps->fd_files[i]));
> > -       for (i = 0; i < gaps->n_memfds; ++i)
> > -               if (fds[gaps->n_fds + i] >= 0)
> > -                       fd_install(fds[gaps->n_fds + i],
> > -                                  get_file(gaps->memfd_files[i]));
> > +       for (i = 0; i < n_fds; ++i)
> > +               fd_install(fds[i], get_file(gaps->fd_files[i]));
> > +       for (i = 0; i < n_memfds; ++i)
> > +               fd_install(fds[gaps->n_fds + i],
> > +                          get_file(gaps->memfd_files[i]));
> >
> >         ret = 0;
> >
> >  exit:
> > -       if (ret < 0)
> > +       if (ret < 0) {
> >                 for (i = 0; i < n_fds; ++i)
> > -                       if (fds[i] >= 0)
> > -                               put_unused_fd(fds[i]);
> > +                       put_unused_fd(fds[i]);
> > +               for (i = 0; i < n_memfds; ++i)
> > +                       put_unused_fd(fds[gaps->n_fds + i]);
> > +       }
> >         kfree(fds);
> >         *out_incomplete = incomplete_fds;
> >         return ret;
> > --
> > 1.8.3.1
> >

[PATCH 44/44] kdbus: Cleanup kdbus_gaps_install()

[Sergei Zviagintsev]

 - Assign `false' to *out_incomplete directly on no-op cases.

 - Initialize `ret' with zero during declaration instead of doing it in
   the end of the function.

 - Initialize `fd' and `memfd' vars during declaration to be more
   concise.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/message.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
index 75e6213e7ed5..ca20852f4ecf 100644
--- a/ipc/kdbus/message.c
+++ b/ipc/kdbus/message.c
@@ -124,18 +124,18 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 	bool incomplete_fds = false;
 	struct kvec kvec;
 	size_t i, n_fds, n_memfds;
-	int ret, *fds;
+	int *fds, ret = 0;
 
 	if (!gaps) {
 		/* nothing to do */
-		*out_incomplete = incomplete_fds;
+		*out_incomplete = false;
 		return 0;
 	}
 
 	n_fds = gaps->n_fds + gaps->n_memfds;
 	if (n_fds < 1) {
 		/* nothing to do */
-		*out_incomplete = incomplete_fds;
+		*out_incomplete = false;
 		return 0;
 	}
 
@@ -148,9 +148,7 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 	n_fds = 0; /* n_fds now tracks the number of allocated fds */
 	if (gaps->n_fds > 0) {
 		for (i = 0; i < gaps->n_fds; ++i) {
-			int fd;
-
-			fd = get_unused_fd_flags(O_CLOEXEC);
+			int fd = get_unused_fd_flags(O_CLOEXEC);
 			if (fd < 0) {
 				incomplete_fds = true;
 				break;
@@ -183,9 +181,7 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 
 	n_memfds = 0;
 	for (i = 0; i < gaps->n_memfds && !incomplete_fds; ++i) {
-		int memfd;
-
-		memfd = get_unused_fd_flags(O_CLOEXEC);
+		int memfd = get_unused_fd_flags(O_CLOEXEC);
 		if (memfd < 0) {
 			incomplete_fds = true;
 			/* memfds are initialized to -1, skip copying it */
@@ -220,8 +216,6 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 		fd_install(fds[gaps->n_fds + i],
 			   get_file(gaps->memfd_files[i]));
 
-	ret = 0;
-
 exit:
 	if (ret < 0) {
 		for (i = 0; i < n_fds; ++i)
-- 
1.8.3.1

Re: [PATCH 00/44] kdbus cleanups

[David Herrmann]

Hi

On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> Hi all,
>
> This is a set of various kdbus code cleanups. Patches are ordered by
> increasing complexity, starting with docs and comments fixes and
> one-liners.
>
> Patch 29 is the revised version of
> http://lkml.kernel.org/g/1435497454-10464-6-git-send-email-sergei@s15v.net
>
> Feel free to ask to change layout of this, split/join, etc if necessary.

So I reviewed all of the patches, most of them look good. Some comments:
 - Please justify your changes in the commit-message. Always.
 - Please don't split patches based on modified functions. If you fix
typos, do them subsystem-wide. If you fix common errors like "don't
init 'name' to NULL before calling kdbus_pin_dst()", then please do
that for all functions. In other words, group your changes logically,
not based on location.
 - If you do cleanups, explain why you do them. I commented on some of
the changes, which IMO reduce readability.

Anyway, looks good.

Thanks a lot!
David

> Thanks, Sergei
>
> Sergei Zviagintsev (44):
>   Documentation/kdbus: Document new name registry flags
>   uapi: kdbus.h: Kernel-doc fixes
>   kdbus: Kernel-docs and comments trivial fixes
>   kdbus: Update kernel-doc for struct kdbus_pool
>   kdbus: Add comment on merging free pool slices
>   kdbus: Fix kernel-doc for struct kdbus_gaps
>   kdbus: Fix comment on translation of caps between namespaces
>   kdbus: Rename var in kdbus_meta_export_caps()
>   kdbus: Remove unused KDBUS_MSG_MAX_SIZE constant
>   kdbus: Use conditional operator
>   kdbus: Cosmetic fix of kdbus_name_is_valid()
>   kdbus: Use conventional list macros in __kdbus_pool_slice_release()
>   kdbus: Use list_next_entry() in kdbus_queue_entry_unlink()
>   kdbus: Simplify expression in kdbus_get_memfd()
>   kdbus: Simplify bitwise expression in kdbus_meta_get_mask()
>   kdbus: Drop redundant code from kdbus_name_acquire()
>   kdbus: Drop duplicated code from kdbus_pool_slice_alloc()
>   kdbus: Add var initialization to kdbus_conn_entry_insert()
>   kdbus: Drop useless initialization from kdbus_conn_reply()
>   kdbus: Drop useless initialization from kdbus_cmd_hello()
>   kdbus: Cleanup tests in kdbus_cmd_send()
>   kdbus: Cleanup error path in kdbus_staging_new_user()
>   kdbus: Cleanup kdbus_conn_call()
>   kdbus: Cleanup kdbus_conn_unicast()
>   kdbus: Cleanup kdbus_cmd_conn_info()
>   kdbus: Cleanup kdbus_pin_dst()
>   kdbus: Cleanup kdbus_conn_new()
>   kdbus: Cleanup kdbus_queue_entry_new()
>   kdbus: Improve tests on incrementing quota
>   kdbus: Cleanup kdbus_meta_proc_mask()
>   kdbus: Cleanup kdbus_conn_move_messages()
>   kdbus: Remove duplicated code from kdbus_conn_lock2()
>   kdbus: Improve kdbus_staging_reserve()
>   kdbus: Improve kdbus_conn_entry_sync_attach()
>   kdbus: Drop goto from kdbus_queue_entry_link()
>   kdbus: Improve kdbus_name_release()
>   kdbus: Fix error path in kdbus_meta_proc_collect_cgroup()
>   kdbus: Fix error path in kdbus_user_lookup()
>   kdbus: Cleanup kdbus_user_lookup()
>   kdbus: Cleanup kdbus_item_validate_name()
>   kdbus: Fix memfd install algorithm
>   kdbus: Check if fd is allocated before trying to free it
>   kdbus: Give up on failed fd allocation
>   kdbus: Cleanup kdbus_gaps_install()
>
>  Documentation/kdbus/kdbus.name.xml |  42 +++++++++-
>  include/uapi/linux/kdbus.h         |  43 +++++-----
>  ipc/kdbus/connection.c             | 157 +++++++++++++++----------------------
>  ipc/kdbus/connection.h             |  19 ++---
>  ipc/kdbus/domain.c                 |  38 +++++----
>  ipc/kdbus/fs.c                     |   2 +-
>  ipc/kdbus/item.c                   |  26 +++---
>  ipc/kdbus/limits.h                 |   3 -
>  ipc/kdbus/message.c                |  81 +++++++++----------
>  ipc/kdbus/message.h                |   9 ++-
>  ipc/kdbus/metadata.c               |  79 ++++++++++---------
>  ipc/kdbus/names.c                  |  32 ++++----
>  ipc/kdbus/node.c                   |   4 +-
>  ipc/kdbus/pool.c                   |  26 +++---
>  ipc/kdbus/queue.c                  |  51 ++++++------
>  ipc/kdbus/queue.h                  |   2 +-
>  16 files changed, 298 insertions(+), 316 deletions(-)
>
> --
> 1.8.3.1
>

Re: [PATCH 00/44] kdbus cleanups

[Sergei Zviagintsev]

Hi David,

On Thu, Oct 08, 2015 at 05:20:34PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > Hi all,
> >
> > This is a set of various kdbus code cleanups. Patches are ordered by
> > increasing complexity, starting with docs and comments fixes and
> > one-liners.
> >
> > Patch 29 is the revised version of
> > http://lkml.kernel.org/g/1435497454-10464-6-git-send-email-sergei@s15v.net
> >
> > Feel free to ask to change layout of this, split/join, etc if necessary.
> 
> So I reviewed all of the patches, most of them look good. Some comments:
>  - Please justify your changes in the commit-message. Always.
>  - Please don't split patches based on modified functions. If you fix
> typos, do them subsystem-wide. If you fix common errors like "don't
> init 'name' to NULL before calling kdbus_pin_dst()", then please do
> that for all functions. In other words, group your changes logically,
> not based on location.
>  - If you do cleanups, explain why you do them. I commented on some of
> the changes, which IMO reduce readability.

Thank you for immediate response and helpful comments! I will
address your notes in v2 shortly after discussing some details on
individual patches.

Best regards,
Sergei

> 
> Anyway, looks good.
> 
> Thanks a lot!
> David
> 
> > Thanks, Sergei
> >
> > Sergei Zviagintsev (44):
> >   Documentation/kdbus: Document new name registry flags
> >   uapi: kdbus.h: Kernel-doc fixes
> >   kdbus: Kernel-docs and comments trivial fixes
> >   kdbus: Update kernel-doc for struct kdbus_pool
> >   kdbus: Add comment on merging free pool slices
> >   kdbus: Fix kernel-doc for struct kdbus_gaps
> >   kdbus: Fix comment on translation of caps between namespaces
> >   kdbus: Rename var in kdbus_meta_export_caps()
> >   kdbus: Remove unused KDBUS_MSG_MAX_SIZE constant
> >   kdbus: Use conditional operator
> >   kdbus: Cosmetic fix of kdbus_name_is_valid()
> >   kdbus: Use conventional list macros in __kdbus_pool_slice_release()
> >   kdbus: Use list_next_entry() in kdbus_queue_entry_unlink()
> >   kdbus: Simplify expression in kdbus_get_memfd()
> >   kdbus: Simplify bitwise expression in kdbus_meta_get_mask()
> >   kdbus: Drop redundant code from kdbus_name_acquire()
> >   kdbus: Drop duplicated code from kdbus_pool_slice_alloc()
> >   kdbus: Add var initialization to kdbus_conn_entry_insert()
> >   kdbus: Drop useless initialization from kdbus_conn_reply()
> >   kdbus: Drop useless initialization from kdbus_cmd_hello()
> >   kdbus: Cleanup tests in kdbus_cmd_send()
> >   kdbus: Cleanup error path in kdbus_staging_new_user()
> >   kdbus: Cleanup kdbus_conn_call()
> >   kdbus: Cleanup kdbus_conn_unicast()
> >   kdbus: Cleanup kdbus_cmd_conn_info()
> >   kdbus: Cleanup kdbus_pin_dst()
> >   kdbus: Cleanup kdbus_conn_new()
> >   kdbus: Cleanup kdbus_queue_entry_new()
> >   kdbus: Improve tests on incrementing quota
> >   kdbus: Cleanup kdbus_meta_proc_mask()
> >   kdbus: Cleanup kdbus_conn_move_messages()
> >   kdbus: Remove duplicated code from kdbus_conn_lock2()
> >   kdbus: Improve kdbus_staging_reserve()
> >   kdbus: Improve kdbus_conn_entry_sync_attach()
> >   kdbus: Drop goto from kdbus_queue_entry_link()
> >   kdbus: Improve kdbus_name_release()
> >   kdbus: Fix error path in kdbus_meta_proc_collect_cgroup()
> >   kdbus: Fix error path in kdbus_user_lookup()
> >   kdbus: Cleanup kdbus_user_lookup()
> >   kdbus: Cleanup kdbus_item_validate_name()
> >   kdbus: Fix memfd install algorithm
> >   kdbus: Check if fd is allocated before trying to free it
> >   kdbus: Give up on failed fd allocation
> >   kdbus: Cleanup kdbus_gaps_install()
> >
> >  Documentation/kdbus/kdbus.name.xml |  42 +++++++++-
> >  include/uapi/linux/kdbus.h         |  43 +++++-----
> >  ipc/kdbus/connection.c             | 157 +++++++++++++++----------------------
> >  ipc/kdbus/connection.h             |  19 ++---
> >  ipc/kdbus/domain.c                 |  38 +++++----
> >  ipc/kdbus/fs.c                     |   2 +-
> >  ipc/kdbus/item.c                   |  26 +++---
> >  ipc/kdbus/limits.h                 |   3 -
> >  ipc/kdbus/message.c                |  81 +++++++++----------
> >  ipc/kdbus/message.h                |   9 ++-
> >  ipc/kdbus/metadata.c               |  79 ++++++++++---------
> >  ipc/kdbus/names.c                  |  32 ++++----
> >  ipc/kdbus/node.c                   |   4 +-
> >  ipc/kdbus/pool.c                   |  26 +++---
> >  ipc/kdbus/queue.c                  |  51 ++++++------
> >  ipc/kdbus/queue.h                  |   2 +-
> >  16 files changed, 298 insertions(+), 316 deletions(-)
> >
> > --
> > 1.8.3.1
> >