linux-kernel - Re: [PATCH 1/1] kernel/pid.c: Masking the flag out to get the actual value.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8761xkt9fh.fsf@xmission.com>
Date:	Tue, 11 Jun 2013 18:16:50 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	"Raphael S. Carvalho" <raphael.scarv@...il.com>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] kernel/pid.c: Masking the flag out to get the actual value.

Andrew Morton <akpm@...ux-foundation.org> writes:

> On Mon, 10 Jun 2013 18:56:38 -0300 "Raphael S. Carvalho" <raphael.scarv@...il.com> wrote:
>
>> This patch shouldn't be applied if those branches must only be taken when
>> the pid_allocation(PIDNS_HASH_ADDING) flag was turned off.

That is correct.  We should not encounter the last pid in the pid
namespace while still allowing processes to be created in the pid
namespace.

So the patch I am seeing quoted below is unnecessary.

> Well yes - hopefully this is the case.  Otherwise we're missing some
> rather important-looking wakeups.
>
>
>> Otherwise, we must turn the PIDNS_HASH_ADDING flag (1U << 31) off
>> before getting into the switch-cases.
>> 
>> ...
>>
>> --- a/kernel/pid.c
>> +++ b/kernel/pid.c
>> @@ -263,7 +263,10 @@ void free_pid(struct pid *pid)
>>  		struct upid *upid = pid->numbers + i;
>>  		struct pid_namespace *ns = upid->ns;
>>  		hlist_del_rcu(&upid->pid_chain);
>> -		switch(--ns->nr_hashed) {
>> +
>> +		/* We must turn the PIDNS_HASH_ADDING flag off to
>> +		   get the actual value of nr_hashed */
>> +		switch ((--ns->nr_hashed) & ~(PIDNS_HASH_ADDING)) {
>>  		case 1:
>>  			/* When all that is left in the pid namespace
>>  			 * is the reaper wake up the reaper.  The reaper
>
> Eric, can you please take a look?  Presumably and hopefully
> PIDNS_HASH_ADDING cannot be set here, but what guarantees this?

The init process has not exited, and called zap_pid_ns_processes.

In fact there is a case where nr_hashed can be 0 | PIDNS_HASH_ADDING
where we absolutely don't want to do these things.   Of course there
are no pids allocated in that case to free so we can't possible get 
to the switch in free pid either.

> Hopefully we can fix this one by adding the missing comment.

Perhaps we can fix this one by having people who care read the code and 
think about what it means?  Seriously if we are adding pids/processes in
the pid namespace why would to clean up the pid namespace?

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/