Hello Dmitry,
On 07/09/2018 07:05 PM, Dmitry Vyukov wrote:
On Mon, Jul 9, 2018 at 5:10 PM, Manfred Spraul <manf...@colorfullife.com> wrote:
If idr_alloc within ipc_idr_alloc fails, then the return value (-ENOSPC)
is used to calculate new->id.
Technically, this is not a bug, because new->id is never accessed.
But: Clean it up anyways: On error, just return, do not set new->id.
And improve the documentation.
Signed-off-by: Manfred Spraul <manf...@colorfullife.com>
Cc: Dmitry Vyukov <dvyu...@google.com>
---
ipc/util.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/ipc/util.c b/ipc/util.c
index d474f2b3b299..302c18fc846b 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -182,11 +182,20 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids
*ids, key_t key)
}
/*
- * Specify desired id for next allocated IPC object.
+ * Insert new IPC object into idr tree, and set sequence number and id
+ * in the correct order.
+ * Especially:
+ * - the sequence number must be set before inserting the object into the idr,
+ * because the sequence number is accessed without a lock.
+ * - the id can/must be set after inserting the object into the idr.
+ * All accesses must be done after getting kern_ipc_perm.lock.
+ *
+ * The caller must own kern_ipc_perm.lock.of the new object.
+ * On error, the function returns a (negative) error code.
*/
static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm
*new)
{
- int key, next_id = -1;
+ int id, next_id = -1;
/\/\/\/\
Looks good to me. I was also confused by how key transforms into id,
and then key name is used for something else.
Let's see if there are further findings, perhaps I'll rework the series,
it may make sense to standardize the variable names:
id: user space id. Called semid, shmid, msgid if the type is known.
Most functions use "id" already.
Exception: ipc_checkid(), the function calls is uid.
idx: "index" for the idr lookup
Right now, ipc_rmid() use lid, ipc_addid() use id as variable name
seq: sequence counter, to avoid quick collisions of the user space id
In the comments, it got a mixture of sequence counter and sequence
number.
key: user space key, used for the rhash tree
#ifdef CONFIG_CHECKPOINT_RESTORE
next_id = ids->next_id;
@@ -197,14 +206,15 @@ static inline int ipc_idr_alloc(struct ipc_ids *ids,
struct kern_ipc_perm *new)
new->seq = ids->seq++;
if (ids->seq > IPCID_SEQ_MAX)
ids->seq = 0;
- key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
+ id = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
} else {
new->seq = ipcid_to_seqx(next_id);
- key = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
+ id = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
0, GFP_NOWAIT);
}
- new->id = SEQ_MULTIPLIER * new->seq + key;
- return key;
+ if (id >= 0)
+ new->id = SEQ_MULTIPLIER * new->seq + id;
We still initialize seq in this case. I guess it's ok because the
object is not published at all. But if we are doing this, then perhaps
store seq into a local var first and then:
if (id >= 0) {
new->id = SEQ_MULTIPLIER * seq + id;
new->seq = seq:
}
?
No!!!
We must initialize ->seq before publication. Otherwise we end up with
the syzcall findings, or in the worst case a strange rare failure of an
ipc operation.
The difference between ->id and ->seq is that we have the valid number
for ->seq.
For the user space ID we cannot have the valid number unless the
idr_alloc is successful.
The patch only avoids that this line is executed:
new->id = SEQ_MULTIPLIER * new->seq + (-ENOSPC)
As I wrote, the line shouldn't cause any damage, the code is more or less:
new->id = SEQ_MULTIPLIER * new->seq + (-ENOSPC)
kfree(new);
But this is ugly, it asks for problems.
--
Manfred
