On Wed, Mar 14, 2018 at 07:49:29PM -0500, Eric W. Biederman wrote:
> @@ -109,20 +109,13 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids
> *ids,
> {
> struct kern_ipc_perm *perm;
> int next_id;
> - int total, in_use;
>
> down_write(&ids->rwsem);
> -
> - in_use = ids->in_use;
> -
> - for (total = 0, next_id = 0; total < in_use; next_id++) {
> - perm = idr_find(&ids->ipcs_idr, next_id);
> - if (perm == NULL)
> - continue;
> + next_id = 0;
> + while ((perm = idr_get_next(&ids->ipcs_idr, &next_id))) {
> rcu_read_lock();
> ipc_lock_object(perm);
> free(ns, perm);
> - total++;
> }
> up_write(&ids->rwsem);
> }
We have a helper for this:
idr_for_each_entry(&ids->ipcs_idr, perm, next_id) {
rcu_read_lock();
ipc_lock_object(perm);
free(ns, perm);
}
(using idr_get_next() is tricky because you have to remember to increment
next_id yourself, and you didn't).
> +static int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
> {
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> + if (ids->next_id >= 0) {
> + idr_set_cursor(&ids->ipcs_idr, ids->next_id);
> ids->next_id = -1;
> }
> +#endif
> + return idr_alloc_cyclic(&ids->ipcs_idr, (new), 0, 0, GFP_NOWAIT);
> }
>
> -#endif /* CONFIG_CHECKPOINT_RESTORE */
That seems a little convoluted; is there a reason to not call idr_set_cursor()
instead of assigning to ids->next_id?
> @@ -757,30 +725,20 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct
> ipc_ids *ids, loff_t pos,
> loff_t *new_pos)
> {
> struct kern_ipc_perm *ipc;
> + int id;
>
> + /* Out of range - return NULL to terminate iteration */
> + if (pos > INT_MAX)
> return NULL;
>
> + ipc = idr_get_next(&ids->ipcs_idr, &id);
> + if (!ipc)
> + return NULL;
>
> + *new_pos = id + 1;
> + rcu_read_lock();
> + ipc_lock_object(ipc);
> + return ipc;
> }
I'm no expert on the IPC locking, but I would have thought you'd want to
call rcu_read_lock() before calling idr_get_next() to avoid a simultaneous
delete from freeing 'ipc'.
Oh, I see. proc_start takes the rwsem for read and proc_stop releases it.
The locking here seems quite shabby and in need of renovation.
