26c4caea "don't allow duplicate entries in listener mode" fix

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 <[email protected]>

---

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

kernel/taskstats.c renamed from kernel/taskstats.c~1_cleanup +7 -9

@@ -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;
 	}
 

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 <[email protected]>

---

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

kernel/taskstats.c renamed from kernel/taskstats.c~1_cleanup +7 -9

@@ -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;
 	}
 

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.

Quoted 6 lines · 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.

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 3 lines · Oleg Nesterov

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 <[email protected]>

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 <[email protected]>