Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751469AbdI3Pm3 (ORCPT ); Sat, 30 Sep 2017 11:42:29 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:37680 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751099AbdI3Pm2 (ORCPT ); Sat, 30 Sep 2017 11:42:28 -0400 X-Google-Smtp-Source: AOwi7QBWrpY+lwuXFoX0FrsxKOkVryJFbJkDGcDSxvWsSgkNxJGqCkWpECuWJq/TWk+k6fzmU+zk368SelHp5Q18qXI= MIME-Version: 1.0 In-Reply-To: <20170927162808.GA5680@redhat.com> References: <20170927154543.GA3788@redhat.com> <20170927162808.GA5680@redhat.com> From: Gargi Sharma Date: Sat, 30 Sep 2017 21:11:56 +0530 Message-ID: Subject: Re: [PATCH v2 2/2] pid: Remove pidhash To: Oleg Nesterov Cc: linux-kernel@vger.kernel.org, Rik van Riel , Julia Lawall , akpm@linux-foundation.org, mingo@kernel.org, pasha.tatashin@oracle.com, ktkhai@virtuozzo.com, Eric Biederman Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2289 Lines: 67 On Wed, Sep 27, 2017 at 9:58 PM, Oleg Nesterov wrote: > 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(). If the PIDNS_ADDING check fails, I jump to the flag that performs this while (++i <= ns->level) idr_remove(&ns->idr, (pid->numbers + i)->nr); So when find_pid_ns() is called, it will not find this pid. > > 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. I'm not sure if I understand this. Do we want to do this to make sure the pid namespace is initialized before the first process enters into the namespace? If yes, how does idr_alloc_cyclic(ptr = NULL) help? > > And I just noticed you didn't cc Eric Biederman , > please do next time. Sorry for missing out on this. Will do with the next version. Thanks! Gargi > > > 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. >