pstree: use RB_EMPTY_NODE to check that node is not linked

Submitted by Pavel Tikhomirov on May 24, 2017, 2:50 p.m.

Details

Message ID 20170524145050.24153-1-ptikhomirov@virtuozzo.com
State Accepted
Series "pstree: use RB_EMPTY_NODE to check that node is not linked"
Headers show

Commit Message

Pavel Tikhomirov May 24, 2017, 2:50 p.m.
When new rb_root is created for pidns it is initialized with
RB_ROOT, so ns->pid.rb_root.rb_node is NULL at first. Later
then insert first node in lookup_create_pid() to these rb-tree
it will have (NULL & color) in node->rb_parent_color.

So the check "!rb_parent(&found->ns[i].node)" will be true for
the rb-tree's root node, and criu will fail lookup these node.

We haven't hit that yet as to get to these check we need task in
at least two levels of pidns which at the same time is the root
in rb-tree on e.g. level 0.

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 criu/pstree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/criu/pstree.c b/criu/pstree.c
index 3206cf1..efff3d7 100644
--- a/criu/pstree.c
+++ b/criu/pstree.c
@@ -522,7 +522,7 @@  static struct pid *lookup_create_pid(pid_t *pid, int level, struct pid *pid_node
 	found = find_pid_or_place_in_hier(&ns->pid.rb_root.rb_node, pid[level-1], level-1, &parent, &new);
 	if (found) {
 		for (i = level - 2; i >= 0; i--)
-			if (pid[i] != found->ns[i].virt || !rb_parent(&found->ns[i].node)) {
+			if (pid[i] != found->ns[i].virt || RB_EMPTY_NODE(&found->ns[i].node)) {
 				pr_err("Wrong pid\n");
 				return NULL;
 			}

Comments

Kirill Tkhai May 29, 2017, 9:27 a.m.
On 24.05.2017 17:50, Pavel Tikhomirov wrote:
> When new rb_root is created for pidns it is initialized with
> RB_ROOT, so ns->pid.rb_root.rb_node is NULL at first. Later
> then insert first node in lookup_create_pid() to these rb-tree
> it will have (NULL & color) in node->rb_parent_color.
> 
> So the check "!rb_parent(&found->ns[i].node)" will be true for
> the rb-tree's root node, and criu will fail lookup these node.
> 
> We haven't hit that yet as to get to these check we need task in
> at least two levels of pidns which at the same time is the root
> in rb-tree on e.g. level 0.
> 
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
>  criu/pstree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/criu/pstree.c b/criu/pstree.c
> index 3206cf1..efff3d7 100644
> --- a/criu/pstree.c
> +++ b/criu/pstree.c
> @@ -522,7 +522,7 @@ static struct pid *lookup_create_pid(pid_t *pid, int level, struct pid *pid_node
>  	found = find_pid_or_place_in_hier(&ns->pid.rb_root.rb_node, pid[level-1], level-1, &parent, &new);
>  	if (found) {
>  		for (i = level - 2; i >= 0; i--)
> -			if (pid[i] != found->ns[i].virt || !rb_parent(&found->ns[i].node)) {
> +			if (pid[i] != found->ns[i].virt || RB_EMPTY_NODE(&found->ns[i].node)) {
>  				pr_err("Wrong pid\n");
>  				return NULL;
>  			}
>
Pavel Tikhomirov May 29, 2017, 9:37 a.m.
note to avoid confusion: I've also sent these patch as 01 in scope of 
patch-set "[PATCH 00/11] rework init child-reaper reparent handling for 
pidnses" as the latter does not work without it it.

On 05/29/2017 12:27 PM, Kirill Tkhai wrote:
> On 24.05.2017 17:50, Pavel Tikhomirov wrote:
>> When new rb_root is created for pidns it is initialized with
>> RB_ROOT, so ns->pid.rb_root.rb_node is NULL at first. Later
>> then insert first node in lookup_create_pid() to these rb-tree
>> it will have (NULL & color) in node->rb_parent_color.
>>
>> So the check "!rb_parent(&found->ns[i].node)" will be true for
>> the rb-tree's root node, and criu will fail lookup these node.
>>
>> We haven't hit that yet as to get to these check we need task in
>> at least two levels of pidns which at the same time is the root
>> in rb-tree on e.g. level 0.
>>
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> 
> Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> 
>> ---
>>   criu/pstree.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/criu/pstree.c b/criu/pstree.c
>> index 3206cf1..efff3d7 100644
>> --- a/criu/pstree.c
>> +++ b/criu/pstree.c
>> @@ -522,7 +522,7 @@ static struct pid *lookup_create_pid(pid_t *pid, int level, struct pid *pid_node
>>   	found = find_pid_or_place_in_hier(&ns->pid.rb_root.rb_node, pid[level-1], level-1, &parent, &new);
>>   	if (found) {
>>   		for (i = level - 2; i >= 0; i--)
>> -			if (pid[i] != found->ns[i].virt || !rb_parent(&found->ns[i].node)) {
>> +			if (pid[i] != found->ns[i].virt || RB_EMPTY_NODE(&found->ns[i].node)) {
>>   				pr_err("Wrong pid\n");
>>   				return NULL;
>>   			}
>>
Andrey Vagin June 2, 2017, 5:45 a.m.
Applied, thanks!

On Wed, May 24, 2017 at 05:50:50PM +0300, Pavel Tikhomirov wrote:
> When new rb_root is created for pidns it is initialized with
> RB_ROOT, so ns->pid.rb_root.rb_node is NULL at first. Later
> then insert first node in lookup_create_pid() to these rb-tree
> it will have (NULL & color) in node->rb_parent_color.
> 
> So the check "!rb_parent(&found->ns[i].node)" will be true for
> the rb-tree's root node, and criu will fail lookup these node.
> 
> We haven't hit that yet as to get to these check we need task in
> at least two levels of pidns which at the same time is the root
> in rb-tree on e.g. level 0.
> 
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  criu/pstree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/criu/pstree.c b/criu/pstree.c
> index 3206cf1..efff3d7 100644
> --- a/criu/pstree.c
> +++ b/criu/pstree.c
> @@ -522,7 +522,7 @@ static struct pid *lookup_create_pid(pid_t *pid, int level, struct pid *pid_node
>  	found = find_pid_or_place_in_hier(&ns->pid.rb_root.rb_node, pid[level-1], level-1, &parent, &new);
>  	if (found) {
>  		for (i = level - 2; i >= 0; i--)
> -			if (pid[i] != found->ns[i].virt || !rb_parent(&found->ns[i].node)) {
> +			if (pid[i] != found->ns[i].virt || RB_EMPTY_NODE(&found->ns[i].node)) {
>  				pr_err("Wrong pid\n");
>  				return NULL;
>  			}
> -- 
> 2.9.3
>