Messages in this thread | | | Date | Wed, 27 Sep 2017 18:28:08 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH v2 2/2] pid: Remove pidhash |
| |
If I was not clear...
in short, after this patch the very first idr_alloc_cyclic() is already wrong. Because, once again, the new not-fully-initialized pid can be found by find_pid_ns().
perhaps you should chane the previous patch to do idr_alloc_cyclic(ptr = NULL) and use idr_replace() in this patch after the PIDNS_HASH_ADDING check.
And I just noticed you didn't cc Eric Biederman <ebiederm@xmission.com>, please do next time.
On 09/27, Oleg Nesterov wrote: > > On 09/27, Gargi Sharma wrote: > > > > -#define find_next_offset(map, off) \ > > - find_next_zero_bit((map)->page, BITS_PER_PAGE, off) > > - > > this should go into the previous patch, but this is minor... > > > @@ -208,12 +200,10 @@ struct pid *alloc_pid(struct pid_namespace *ns) > > > > upid = pid->numbers + ns->level; > > spin_lock_irq(&pidmap_lock); > > - if (!(ns->nr_hashed & PIDNS_HASH_ADDING)) > > + if (!(ns->pid_allocated & PIDNS_ADDING)) > > goto out_unlock; > > for ( ; upid >= pid->numbers; --upid) { > > - hlist_add_head_rcu(&upid->pid_chain, > > - &pid_hash[pid_hashfn(upid->nr, upid->ns)]); > > - upid->ns->nr_hashed++; > > + upid->ns->pid_allocated++; > > No, this is wrong. > > It is too late to check PIDNS_HASH_ADDING/PIDNS_ADDING and increment pid_allocated, > once we call idr_alloc_cyclic() this pid is already "hashed" in that it can be found > by find_pid_ns() with this patch applied. > > And of course, it is too late to do atomic_set(&pid->count, 1) and initialize > pid->tasks[type] lists by the same reason. > > Oleg.
| |