lkml.org 
[lkml]   [2018]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC][PATCH] ipc: Remove IPCMNI
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.

\
 
 \ /
  Last update: 2018-03-15 20:46    [W:0.080 / U:0.784 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site