[PATCH 0/2] 26c4caea "don't allow duplicate entries in listener mode" fix

[PATCH 0/2] 26c4caea "don't allow duplicate entries in listener mode" fix

[Oleg Nesterov]

Hello.

I noticed 26c4caea by accident. Afaics it deserves a couple
of minor fixes/cleanups.

COMPILE TESTED ONLY. Needs review and acks ;)

Can't we kill listener->valid? It seems we can set ->pid = 0
to mark it invalid.

Oleg.

[PATCH 1/2] taskstats: add_del_listener() shouldn't use the wrong node

[Oleg Nesterov]

1. 26c4caea "don't allow duplicate entries in listener mode"
   changed add_del_listener(REGISTER) so that "next_cpu:" can
   reuse the listener allocated for the previous cpu, this
   doesn't look exactly right even if minor.

   Change the code to kfree() in the already-registered case,
   this case is unlikely anyway so the extra kmalloc_node()
   shouldn't hurt but looke more correct and clean.

2. use the plain list_for_each_entry() instead of _safe() to
   scan listeners->list.

3. Remove the unneeded INIT_LIST_HEAD(&s->list), we are going
   to list_add(&s->list).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/taskstats.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

--- ts/kernel/taskstats.c~1_cleanup	2011-07-03 16:27:57.000000000 +0200
+++ ts/kernel/taskstats.c	2011-07-20 19:35:38.000000000 +0200
@@ -291,30 +291,28 @@ static int add_del_listener(pid_t pid, c
 	if (!cpumask_subset(mask, cpu_possible_mask))
 		return -EINVAL;
 
-	s = NULL;
 	if (isadd == REGISTER) {
 		for_each_cpu(cpu, mask) {
-			if (!s)
-				s = kmalloc_node(sizeof(struct listener),
-						 GFP_KERNEL, cpu_to_node(cpu));
+			s = kmalloc_node(sizeof(struct listener),
+					GFP_KERNEL, cpu_to_node(cpu));
 			if (!s)
 				goto cleanup;
+
 			s->pid = pid;
-			INIT_LIST_HEAD(&s->list);
 			s->valid = 1;
 
 			listeners = &per_cpu(listener_array, cpu);
 			down_write(&listeners->sem);
-			list_for_each_entry_safe(s2, tmp, &listeners->list, list) {
+			list_for_each_entry(s2, &listeners->list, list) {
 				if (s2->pid == pid)
-					goto next_cpu;
+					goto exists;
 			}
 			list_add(&s->list, &listeners->list);
 			s = NULL;
-next_cpu:
+exists:
 			up_write(&listeners->sem);
+			kfree(s); /* nop if NULL */
 		}
-		kfree(s);
 		return 0;
 	}
 

[PATCH 2/2] taskstats: add_del_listener() should ignore !valid listener's

[Oleg Nesterov]

When send_cpu_listeners() finds the orphaned listener it marks
it as !valid and drops listeners->sem. Before it takes this sem
for wrinting, s->pid can be reused and add_del_listener() can
wrongly try to re-use this entry.

Change add_del_listener() to check ->valid = T.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/taskstats.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- ts/kernel/taskstats.c~2_ck_valid	2011-07-20 20:18:19.000000000 +0200
+++ ts/kernel/taskstats.c	2011-07-20 20:18:47.000000000 +0200
@@ -304,7 +304,7 @@ static int add_del_listener(pid_t pid, c
 			listeners = &per_cpu(listener_array, cpu);
 			down_write(&listeners->sem);
 			list_for_each_entry(s2, &listeners->list, list) {
-				if (s2->pid == pid)
+				if (s2->pid == pid && s2->valid)
 					goto exists;
 			}
 			list_add(&s->list, &listeners->list);

Re: [PATCH 2/2] taskstats: add_del_listener() should ignore !valid listener's

[Vasiliy Kulikov]

On Wed, Jul 20, 2011 at 21:00 +0200, Oleg Nesterov wrote:
> When send_cpu_listeners() finds the orphaned listener it marks
> it as !valid and drops listeners->sem. Before it takes this sem
> for wrinting, s->pid can be reused and add_del_listener() can
> wrongly try to re-use this entry.
> 
> Change add_del_listener() to check ->valid = T.

I think the current logic is wrong.  If s was claimed is invalid, it
then points to a dead task, it is unknown when it died.  As the same pid
can be reused by a new process BEFORE add_del_listener(), it is unknown
whether ->pid points to the actual owner task.

Rather than optimizing the wrong algorithm it's better to change the
processes keeping way.

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH 1/2] taskstats: add_del_listener() shouldn't use the wrong node

[Vasiliy Kulikov]

On Wed, Jul 20, 2011 at 21:00 +0200, Oleg Nesterov wrote:
> 1. 26c4caea "don't allow duplicate entries in listener mode"
>    changed add_del_listener(REGISTER) so that "next_cpu:" can
>    reuse the listener allocated for the previous cpu, this
>    doesn't look exactly right even if minor.
> 
>    Change the code to kfree() in the already-registered case,
>    this case is unlikely anyway so the extra kmalloc_node()
>    shouldn't hurt but looke more correct and clean.
> 
> 2. use the plain list_for_each_entry() instead of _safe() to
>    scan listeners->list.
> 
> 3. Remove the unneeded INIT_LIST_HEAD(&s->list), we are going
>    to list_add(&s->list).

I don't know whether (1) is needed, looks very minor to me.  Other than
that:

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Vasiliy Kulikov <segoon@openwall.com>

> ---
> 
>  kernel/taskstats.c |   16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> --- ts/kernel/taskstats.c~1_cleanup	2011-07-03 16:27:57.000000000 +0200
> +++ ts/kernel/taskstats.c	2011-07-20 19:35:38.000000000 +0200
> @@ -291,30 +291,28 @@ static int add_del_listener(pid_t pid, c
>  	if (!cpumask_subset(mask, cpu_possible_mask))
>  		return -EINVAL;
>  
> -	s = NULL;
>  	if (isadd == REGISTER) {
>  		for_each_cpu(cpu, mask) {
> -			if (!s)
> -				s = kmalloc_node(sizeof(struct listener),
> -						 GFP_KERNEL, cpu_to_node(cpu));
> +			s = kmalloc_node(sizeof(struct listener),
> +					GFP_KERNEL, cpu_to_node(cpu));
>  			if (!s)
>  				goto cleanup;
> +
>  			s->pid = pid;
> -			INIT_LIST_HEAD(&s->list);
>  			s->valid = 1;
>  
>  			listeners = &per_cpu(listener_array, cpu);
>  			down_write(&listeners->sem);
> -			list_for_each_entry_safe(s2, tmp, &listeners->list, list) {
> +			list_for_each_entry(s2, &listeners->list, list) {
>  				if (s2->pid == pid)
> -					goto next_cpu;
> +					goto exists;
>  			}
>  			list_add(&s->list, &listeners->list);
>  			s = NULL;
> -next_cpu:
> +exists:
>  			up_write(&listeners->sem);
> +			kfree(s); /* nop if NULL */
>  		}
> -		kfree(s);
>  		return 0;
>  	}
>  
> 

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH 2/2] taskstats: add_del_listener() should ignore !valid listener's

[Oleg Nesterov]

On 07/20, Vasiliy Kulikov wrote:
>
> On Wed, Jul 20, 2011 at 21:00 +0200, Oleg Nesterov wrote:
> > When send_cpu_listeners() finds the orphaned listener it marks
> > it as !valid and drops listeners->sem. Before it takes this sem
> > for wrinting, s->pid can be reused and add_del_listener() can
> > wrongly try to re-use this entry.
> >
> > Change add_del_listener() to check ->valid = T.
>
> I think the current logic is wrong.

me too ;)

still I think this patch makes sense anyway.

> If s was claimed is invalid, it
> then points to a dead task,

I forgot everything I knew about netlink. But, it seems, netlink_bind()
can use nl_pid != current->pid. But even if this is not true,

> it is unknown when it died.  As the same pid
> can be reused by a new process BEFORE add_del_listener(), it is unknown
> whether ->pid points to the actual owner task.

And? Why should we try to reuse the !valid entries? struct listener
is just the "addr" we should send the message to. If someone calls
add_listener() it should works regardless of dead entries with the
same ->pid.

> Rather than optimizing the wrong algorithm it's better to change the
> processes keeping way.

Not sure I understand... This is not optimizing, the patch tries to
fix the unlikely (in fact mostly theoretical) and minor problem. Still
this is the problem, no?

Thanks,

Oleg.

Re: [PATCH 1/2] taskstats: add_del_listener() shouldn't use the wrong node

[Jerome Marchand]

On 07/20/2011 09:00 PM, Oleg Nesterov wrote:
> 1. 26c4caea "don't allow duplicate entries in listener mode"
>    changed add_del_listener(REGISTER) so that "next_cpu:" can
>    reuse the listener allocated for the previous cpu, this
>    doesn't look exactly right even if minor.

That construction is somewhat unusual. I agree that this code looks cleaner
and probably reduces the risk that someone in the future misunderstand the
code.

> 
>    Change the code to kfree() in the already-registered case,
>    this case is unlikely anyway so the extra kmalloc_node()
>    shouldn't hurt but looke more correct and clean.
> 
> 2. use the plain list_for_each_entry() instead of _safe() to
>    scan listeners->list.
> 
> 3. Remove the unneeded INIT_LIST_HEAD(&s->list), we are going
>    to list_add(&s->list).
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Jerome Marchand <jmarchan@redhat.com>

> ---
> 
>  kernel/taskstats.c |   16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> --- ts/kernel/taskstats.c~1_cleanup	2011-07-03 16:27:57.000000000 +0200
> +++ ts/kernel/taskstats.c	2011-07-20 19:35:38.000000000 +0200
> @@ -291,30 +291,28 @@ static int add_del_listener(pid_t pid, c
>  	if (!cpumask_subset(mask, cpu_possible_mask))
>  		return -EINVAL;
>  
> -	s = NULL;
>  	if (isadd == REGISTER) {
>  		for_each_cpu(cpu, mask) {
> -			if (!s)
> -				s = kmalloc_node(sizeof(struct listener),
> -						 GFP_KERNEL, cpu_to_node(cpu));
> +			s = kmalloc_node(sizeof(struct listener),
> +					GFP_KERNEL, cpu_to_node(cpu));
>  			if (!s)
>  				goto cleanup;
> +
>  			s->pid = pid;
> -			INIT_LIST_HEAD(&s->list);
>  			s->valid = 1;
>  
>  			listeners = &per_cpu(listener_array, cpu);
>  			down_write(&listeners->sem);
> -			list_for_each_entry_safe(s2, tmp, &listeners->list, list) {
> +			list_for_each_entry(s2, &listeners->list, list) {
>  				if (s2->pid == pid)
> -					goto next_cpu;
> +					goto exists;
>  			}
>  			list_add(&s->list, &listeners->list);
>  			s = NULL;
> -next_cpu:
> +exists:
>  			up_write(&listeners->sem);
> +			kfree(s); /* nop if NULL */
>  		}
> -		kfree(s);
>  		return 0;
>  	}
>  
> 

Re: [PATCH 2/2] taskstats: add_del_listener() should ignore !valid listener's

[Vasiliy Kulikov]

On Wed, Jul 20, 2011 at 23:01 +0200, Oleg Nesterov wrote:
> > it is unknown when it died.  As the same pid
> > can be reused by a new process BEFORE add_del_listener(), it is unknown
> > whether ->pid points to the actual owner task.
> 
> And? Why should we try to reuse the !valid entries? struct listener
> is just the "addr" we should send the message to. If someone calls
> add_listener() it should works regardless of dead entries with the
> same ->pid.

True.

> > Rather than optimizing the wrong algorithm it's better to change the
> > processes keeping way.
> 
> Not sure I understand... This is not optimizing, the patch tries to
> fix the unlikely (in fact mostly theoretical) and minor problem.

Ah, I see the problem here.  Yes, then it surely makes sense.

Reviewed-by: Vasiliy Kulikov <segoon@openwall.com>


Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH 2/2] taskstats: add_del_listener() should ignore !valid listener's

[Oleg Nesterov]

On 07/21, Vasiliy Kulikov wrote:
>
> On Wed, Jul 20, 2011 at 23:01 +0200, Oleg Nesterov wrote:
> >
> > Not sure I understand... This is not optimizing, the patch tries to
> > fix the unlikely (in fact mostly theoretical) and minor problem.
>
> Ah, I see the problem here.  Yes, then it surely makes sense.

Yes, agreed.

Just in case... please note that "s->pid can be reused" doesn't
necessarily mean this pid was reused by kernel. For example, a
task can close the socket without DEREGISTER, then create another
one and do REGISTER which can race with send_cpu_listeners().
Afaics.

All this register/deregister logic doesn't look very right, imho.

> Reviewed-by: Vasiliy Kulikov <segoon@openwall.com>

Thanks,

Oleg.

Re: [PATCH 2/2] taskstats: add_del_listener() should ignore !valid listener's

[Balbir Singh]

On Thu, Jul 21, 2011 at 12:30 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> When send_cpu_listeners() finds the orphaned listener it marks
> it as !valid and drops listeners->sem. Before it takes this sem
> for wrinting, s->pid can be reused and add_del_listener() can
> wrongly try to re-use this entry.
>
> Change add_del_listener() to check ->valid = T.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Balbir Singh <bsingharora@gmail.com>

BTW, some namespace changes are pending from my side. I need to get
it, its on my TODO list

Balbir Singh