On Mon, Jul 9, 2018 at 8:22 PM, Manfred Spraul <manf...@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 <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
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 >