Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Manfred Spraul <manfred@colorfullife.com>
To:     LKML <linux-kernel@vger.kernel.org>,
        Davidlohr Bueso <dbueso@suse.de>,
        Waiman Long <llong@redhat.com>,
        Rafael Aquini <aquini@redhat.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, 1vier1@web.de,
        Manfred Spraul <manfred@colorfullife.com>
Subject: [PATCH] ipc/util.c: Cleanup and improve sysvipc_find_ipc()
Date:   Sun, 29 Aug 2021 14:04:50 +0200
Message-Id: <20210829120450.49971-1-manfred@colorfullife.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

1) sysvipc_find_ipc() is a bit difficult to understand:
> static struct kern_ipc_perm *sysvipc_find_ipc(
>                          (struct ipc_ids *ids, loff_t pos,
>                          loff_t *new_pos)

@pos is not a sequence file position, it is an index in the idr.
@new_pos is the sequence file position.

In addition, there are not commented conversions from pos to idr.

e.g.:
> return sysvipc_find_ipc(&iter->ns->ids[iface->ids], *pos, pos);

The call tries to find the successor to the sequence file entry
with position *pos.
Thus:
> *pos +1 /* +1 for successor */ -1 /* -1 for conversion to idx */
which is shortened to *pos.

2) sysvipc_find_ipc() uses a for loop to find the next entry,
instead of using idr_get_next().

The patch tries to fix both issues.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/util.c | 75 ++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 19 deletions(-)

diff --git a/ipc/util.c b/ipc/util.c
index d48d8cfa1f3f..469d369310f1 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -782,28 +782,64 @@ struct pid_namespace *ipc_seq_pid_ns(struct seq_file *s)
 	return iter->pid_ns;
 }
 
-/*
- * This routine locks the ipc structure found at least at position pos.
+/**
+ * sysvipc_pos_to_index - convert seqfile pos to idr index
+ * @pos: position
+ *
+ * For the sequence file, pos=0 is used for the header line.
+ * Thus "real" ipc entries start at pos=1, with index = pos-1;
+ *
+ */
+static inline int sysvipc_pos_to_index(int pos)
+{
+	return pos-1;
+}
+
+/**
+ * sysvipc_index_to_pos - convert idr index to seqfile pos
+ * @pos: position
+ *
+ * For the sequence file, pos=0 is used for the header line.
+ * Thus "real" ipc entries start at pos=1, with pos = index+1;
+ */
+static inline int sysvipc_index_to_pos(int idx)
+{
+	return idx+1;
+}
+
+/**
+ * sysvipc_find_ipc - Find and lock the ipc structure based on seq pos
+ * @ids: ipc identifier set
+ * @pos: expected position
+ * @new_pos: actual position of the found ipc structure
+ *
+ * The function finds an ipc structure, based on the sequence file
+ * position @pos. If there is no ipc structure at position @pos, then
+ * the successor is selected.
+ * If a structure is found, then it is locked (both rcu_read_lock() and
+ * ipc_lock_object()) and  @new_pos is set to the position needed to locate
+ * the found ipc structure.
+ * If nothing is found (i.e. EOF), @new_pos is set to the input position.
+ *
+ * The function returns the found ipc structure, or NULL at EOF.
  */
 static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
 					      loff_t *new_pos)
 {
-	struct kern_ipc_perm *ipc = NULL;
-	int max_idx = ipc_get_maxidx(ids);
+	int tmpidx;
+	struct kern_ipc_perm *ipc;
 
-	if (max_idx == -1 || pos > max_idx)
-		goto out;
+	tmpidx = sysvipc_pos_to_index(pos);
 
-	for (; pos <= max_idx; pos++) {
-		ipc = idr_find(&ids->ipcs_idr, pos);
-		if (ipc != NULL) {
-			rcu_read_lock();
-			ipc_lock_object(ipc);
-			break;
-		}
+	ipc = idr_get_next(&ids->ipcs_idr, &tmpidx);
+	if (ipc != NULL) {
+		rcu_read_lock();
+		ipc_lock_object(ipc);
+
+		*new_pos = sysvipc_index_to_pos(tmpidx);
+	} else {
+		*new_pos = pos;
 	}
-out:
-	*new_pos = pos + 1;
 	return ipc;
 }
 
@@ -817,11 +853,12 @@ static void *sysvipc_proc_next(struct seq_file *s, void *it, loff_t *pos)
 	if (ipc && ipc != SEQ_START_TOKEN)
 		ipc_unlock(ipc);
 
-	return sysvipc_find_ipc(&iter->ns->ids[iface->ids], *pos, pos);
+	/* Next -> search for *pos+1 */
+	return sysvipc_find_ipc(&iter->ns->ids[iface->ids], *pos+1, pos);
 }
 
 /*
- * File positions: pos 0 -> header, pos n -> ipc id = n - 1.
+ * File positions: pos 0 -> header, pos n -> ipc idx = n - 1.
  * SeqFile iterator: iterator value locked ipc pointer or SEQ_TOKEN_START.
  */
 static void *sysvipc_proc_start(struct seq_file *s, loff_t *pos)
@@ -846,8 +883,8 @@ static void *sysvipc_proc_start(struct seq_file *s, loff_t *pos)
 	if (*pos == 0)
 		return SEQ_START_TOKEN;
 
-	/* Find the (pos-1)th ipc */
-	return sysvipc_find_ipc(ids, *pos - 1, pos);
+	/* Otherwise return the correct ipc structure */
+	return sysvipc_find_ipc(ids, *pos, pos);
 }
 
 static void sysvipc_proc_stop(struct seq_file *s, void *it)
-- 
2.31.1