linux-kernel - Re: [PATCH review 2/3] pidns: Stop pid allocation when init dies

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]

Date:	Sat, 22 Dec 2012 17:54:38 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Linux Containers <containers@...ts.linux-foundation.org>,
	linux-kernel@...r.kernel.org, "Serge E. Hallyn" <serge@...lyn.com>
Subject: Re: [PATCH review 2/3] pidns: Stop pid allocation when init dies

On 12/21, Eric W. Biederman wrote:
>
> --- a/include/linux/pid_namespace.h
> +++ b/include/linux/pid_namespace.h
> @@ -21,7 +21,7 @@ struct pid_namespace {
>  	struct kref kref;
>  	struct pidmap pidmap[PIDMAP_ENTRIES];
>  	int last_pid;
> -	int nr_hashed;
> +	unsigned int nr_hashed;
>  	struct task_struct *child_reaper;
>  	struct kmem_cache *pid_cachep;
>  	unsigned int level;
> @@ -42,6 +42,8 @@ struct pid_namespace {
>
>  extern struct pid_namespace init_pid_ns;
>
> +#define PIDNS_HASH_ADDING (1U << 31)

Yes, agreed. We can't rely on PF_EXITING/whatever, we need the explicit
flag.

1/2 looks fine too. Only one nit about init_pid_ns below...

> @@ -319,7 +318,7 @@ struct pid *alloc_pid(struct pid_namespace *ns)
>
>  	upid = pid->numbers + ns->level;
>  	spin_lock_irq(&pidmap_lock);
> -	if (ns->nr_hashed < 0)
> +	if (ns->nr_hashed < PIDNS_HASH_ADDING)

I won't insist, but perhaps if "(!(nr_hashed & PIDNS_HASH_ADDING))"
looks more understandable.

> +void disable_pid_allocation(struct pid_namespace *ns)
> +{
> +	spin_lock_irq(&pidmap_lock);
> +	if (ns->nr_hashed >= PIDNS_HASH_ADDING)

Do we really need this check? It seems that PIDNS_HASH_ADDING
bit must be always set when disable_pid_allocation() is called.

> +		ns->nr_hashed -= PIDNS_HASH_ADDING;

Anyway, nr_hashed &= ~PIDNS_HASH_ADDING looks simpler and doesn't
need a check.

But again, I won't insist this is minor and subjective.

>  struct pid *find_pid_ns(int nr, struct pid_namespace *ns)
>  {
>  	struct hlist_node *elem;
> @@ -584,7 +591,7 @@ void __init pidmap_init(void)
>  	/* Reserve PID 0. We never call free_pidmap(0) */
>  	set_bit(0, init_pid_ns.pidmap[0].page);
>  	atomic_dec(&init_pid_ns.pidmap[0].nr_free);
> -	init_pid_ns.nr_hashed = 1;
> +	init_pid_ns.nr_hashed = 1 + PIDNS_HASH_ADDING;

The obly chunk which doesn't look exactly correct to me, although this
doesn't really matter. Hmm, actually the code was already wrong before
this patch.

I think init_pid_ns.nr_hashed should be PIDNS_HASH_ADDING, we should not
add 1 to account the unused zero pid, and kernel_thread(kernel_init) was
not called yet.

Oleg.

--
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/