Re: [PATCH-next] ipc: Fix race condition in ipc_idr_alloc()

[Waiman Long <longman@redhat.com>]

On 03/10/2019 11:35 PM, Matthew Wilcox wrote:
> On Sun, Mar 10, 2019 at 09:25:36PM -0400, Waiman Long wrote:
>> @@ -221,15 +221,34 @@ 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 */
>> +		/*
>> +		 * It is possible that another thread of the same
>> +		 * kern_ipc_perm may have called ipc_obtain_object_check()
>> +		 * concurrently with a recently deleted IPC id (idx|seq).
>> +		 * If idr_alloc() happens to allocate this deleted idx value,
>> +		 * the other thread may incorrectly get a handle to the new
>> +		 * IPC id.
>> +		 *
>> +		 * To prevent this race condition from happening, we will
>> +		 * always store a new sequence number into the kern_ipc_perm
>> +		 * object before calling idr_alloc(). If we find out that we
>> +		 * don't need to change seq, we write back the right value.
>> +		 */
>> +		new->seq = ids->seq + 1;
>> +		if (new->seq > IPCID_SEQ_MAX)
>> +			new->seq = 0;
>> +
>>  		if (ipc_mni_extended)
>>  			idx = idr_alloc_cyclic(&ids->ipcs_idr, new, 0, ipc_mni,
>>  						GFP_NOWAIT);
>>  		else
>>  			idx = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
>>  
>> -		if ((idx <= ids->last_idx) && (++ids->seq > IPCID_SEQ_MAX))
>> -			ids->seq = 0;
>> -		new->seq = ids->seq;
>> +		/* Make ids->seq and new->seq stay in sync */
>> +		if (idx <= ids->last_idx)
>> +			ids->seq = new->seq;
>> +		else
>> +			new->seq = ids->seq;
> This can't possibly be right.  It's no better to occasionally find the
> wrong ID than to find an uninitialised ID.

The kern_ipc_perm object isn't uninitialized. The seq value, however, is
tentative rather than final.

> The normal pattern for solving this kind of problem is to idr_alloc()
> a NULL pointer, initialise new->seq, then call idr_replace() to turn
> that NULL pointer into the actual pointer you want.

That will work too, I think. My only concern is that it will slow down
ipc_idr_alloc() process as each idr_*() call can be pretty expensive.
Even though it is in the slow path, we still don't want to introduce
unnecessary overhead.

Actually, it is no different from what ipc_idr_alloc() used to be. The
seq value in kern_ipc_perm was updated every time ipc_idr_alloc() was
called. So older IPC id would fail ipc_checkid(). This patch guarantees
that it will happen again. The new IPC id won't be returned until the
kern_ipc_perm object is correctly set. So there is no danger of new IPC
id failing the test. I will update my patch to better discuss the
rationale for this change.

Cheers,
Longman