Re: [PATCH v12 2/3] ipc: Conserve sequence numbers in ipcmni_extend mode

[Manfred Spraul]

This is a multi-part message in MIME format.
Hi,

On 2/28/19 7:47 PM, Waiman Long wrote:
> @@ -216,10 +221,11 @@ static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
>   	 */
>   
>   	if (next_id < 0) { /* !CHECKPOINT_RESTORE or next_id is unset */
> -		new->seq = ids->seq++;
> -		if (ids->seq > IPCID_SEQ_MAX)
> -			ids->seq = 0;
>   		idx = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
> +		if ((idx <= ids->last_idx) && (++ids->seq > IPCID_SEQ_MAX))
> +			ids->seq = 0;

I'm always impressed by such lines:

Everything in just two lines, use "++a", etc.

But: How did you test it?

idr_alloc() can fail, the code doesn't handle that :-(


> +		new->seq = ids->seq;

As written this morning:

Writing new->seq after inserting "new" into the idr creates races 
without any good reason.

I could not spot a bug, even find_alloc_undo() appears to be safe, but 
why should we take this risk?


Attached is:

- proposed replacement for this patch.

- the test patch that I have used to check the error handling.


--

ÂÂÂ Manfred

diff --git a/ipc/util.c b/ipc/util.c
index 6e0fe3410423..5dafe4bc78a1 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -309,6 +309,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
 		}
 	}
 	if (idx < 0) {
+pr_info("failed allocation.\n");
 		new->deleted = true;
 		spin_unlock(&new->lock);
 		rcu_read_unlock();
diff --git a/lib/idr.c b/lib/idr.c
index cb1db9b8d3f6..ba274baa87e3 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -83,6 +83,17 @@ int idr_alloc(struct idr *idr, void *ptr, int start, int end, gfp_t gfp)
 	if (WARN_ON_ONCE(start < 0))
 		return -EINVAL;
 
+	{
+		u64 a = get_jiffies_64();
+
+		if (time_after64(a, (u64)INITIAL_JIFFIES+40*HZ)) {
+			if (a%5 < 2) {
+				pr_info("idr_alloc:Failing.\n");
+				return -ENOSPC;
+			}
+		}
+	}
+
 	ret = idr_alloc_u32(idr, ptr, &id, end > 0 ? end - 1 : INT_MAX, gfp);
 	if (ret)
 		return ret;