lkml.org 
[lkml]   [2018]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 12/12] ipc/util.c: Further ipc_idr_alloc cleanups.
On Mon, Jul 9, 2018 at 8:22 PM, Manfred Spraul <manfred@colorfullife.com> wrote:
> Hello Dmitry,
>
>
> On 07/09/2018 07:05 PM, Dmitry Vyukov wrote:
>>
>> On Mon, Jul 9, 2018 at 5:10 PM, Manfred Spraul <manfred@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 <manfred@colorfullife.com>
>>> Cc: Dmitry Vyukov <dvyukov@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

Right!

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

\
 
 \ /
  Last update: 2018-07-09 20:32    [W:1.173 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site