files: fix clone_service_fd overlap handling

Submitted by Pavel Tikhomirov on April 17, 2018, 2:07 p.m.

Details

Message ID 20180417140732.2810-1-ptikhomirov@virtuozzo.com
State New
Series "files: fix clone_service_fd overlap handling"
Headers show

Commit Message

Pavel Tikhomirov April 17, 2018, 2:07 p.m.
Though LOG_FD_OFF < IMG_FD_OFF, get_service_fd(LOG_FD_OFF) is > than
get_service_fd(IMG_FD_OFF), see __get_service_fd, so the check here
should be twisted. Also add bug_on to track possible __get_service_fd
change which can break these check again.

We have a problem when USERNSD_SK replaces LOG_FD_OFF, latter when
writing to log, instead we actually send crazy commands to usernsd,
which failes to handle them and BUG or crash.

https://jira.sw.ru/browse/PSBM-83472

Also we had similar problem when __userns_call received bad repsonse,
likely it has the same background.

https://api.travis-ci.org/v3/job/352164661/log.txt

fixes commit 129bb14611c3 ("files: Prepare clone_service_fd() for
overlaping ranges.")

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

Patch hide | download patch | download mbox

diff --git a/criu/util.c b/criu/util.c
index b19bf5175..4af542a4f 100644
--- a/criu/util.c
+++ b/criu/util.c
@@ -617,7 +617,8 @@  int clone_service_fd(struct pstree_item *me)
 		return 0;
 
 	/* Dup sfds in memmove() style: they may overlap */
-	if (get_service_fd(LOG_FD_OFF) > new_base - LOG_FD_OFF - SERVICE_FD_MAX * id)
+	BUG_ON(get_service_fd(LOG_FD_OFF) < get_service_fd(IMG_FD_OFF));
+	if (get_service_fd(LOG_FD_OFF) < new_base - LOG_FD_OFF - SERVICE_FD_MAX * id)
 		for (i = SERVICE_FD_MIN + 1; i < SERVICE_FD_MAX; i++)
 			move_service_fd(me, i, id, new_base);
 	else

Comments

Kirill Tkhai April 17, 2018, 2:44 p.m.
On 17.04.2018 17:07, Pavel Tikhomirov wrote:
> Though LOG_FD_OFF < IMG_FD_OFF, get_service_fd(LOG_FD_OFF) is > than
> get_service_fd(IMG_FD_OFF), see __get_service_fd, so the check here
> should be twisted. Also add bug_on to track possible __get_service_fd
> change which can break these check again.
> 
> We have a problem when USERNSD_SK replaces LOG_FD_OFF, latter when
> writing to log, instead we actually send crazy commands to usernsd,
> which failes to handle them and BUG or crash.
> 
> https://jira.sw.ru/browse/PSBM-83472
> 
> Also we had similar problem when __userns_call received bad repsonse,
> likely it has the same background.
> 
> https://api.travis-ci.org/v3/job/352164661/log.txt
> 
> fixes commit 129bb14611c3 ("files: Prepare clone_service_fd() for
> overlaping ranges.")
> 
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  criu/util.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/util.c b/criu/util.c
> index b19bf5175..4af542a4f 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -617,7 +617,8 @@ int clone_service_fd(struct pstree_item *me)
>  		return 0;
>  
>  	/* Dup sfds in memmove() style: they may overlap */
> -	if (get_service_fd(LOG_FD_OFF) > new_base - LOG_FD_OFF - SERVICE_FD_MAX * id)

Good catch!

> +	BUG_ON(get_service_fd(LOG_FD_OFF) < get_service_fd(IMG_FD_OFF));

I don't think we should check this in runtime. If we need this, this should be made
once in one of the init functions.

> +	if (get_service_fd(LOG_FD_OFF) < new_base - LOG_FD_OFF - SERVICE_FD_MAX * id)
>  		for (i = SERVICE_FD_MIN + 1; i < SERVICE_FD_MAX; i++)
>  			move_service_fd(me, i, id, new_base);
>  	else
> 

Kirill
Dmitry Safonov April 17, 2018, 3:01 p.m.
2018-04-17 15:44 GMT+01:00 Kirill Tkhai <ktkhai@virtuozzo.com>:
> On 17.04.2018 17:07, Pavel Tikhomirov wrote:
>> Though LOG_FD_OFF < IMG_FD_OFF, get_service_fd(LOG_FD_OFF) is > than
>> get_service_fd(IMG_FD_OFF), see __get_service_fd, so the check here
>> should be twisted. Also add bug_on to track possible __get_service_fd
>> change which can break these check again.
>>
>> We have a problem when USERNSD_SK replaces LOG_FD_OFF, latter when
>> writing to log, instead we actually send crazy commands to usernsd,
>> which failes to handle them and BUG or crash.
>>
>> https://jira.sw.ru/browse/PSBM-83472
>>
>> Also we had similar problem when __userns_call received bad repsonse,
>> likely it has the same background.
>>
>> https://api.travis-ci.org/v3/job/352164661/log.txt
>>
>> fixes commit 129bb14611c3 ("files: Prepare clone_service_fd() for
>> overlaping ranges.")
>>
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>> ---
>>  criu/util.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/criu/util.c b/criu/util.c
>> index b19bf5175..4af542a4f 100644
>> --- a/criu/util.c
>> +++ b/criu/util.c
>> @@ -617,7 +617,8 @@ int clone_service_fd(struct pstree_item *me)
>>               return 0;
>>
>>       /* Dup sfds in memmove() style: they may overlap */
>> -     if (get_service_fd(LOG_FD_OFF) > new_base - LOG_FD_OFF - SERVICE_FD_MAX * id)
>
> Good catch!
>
>> +     BUG_ON(get_service_fd(LOG_FD_OFF) < get_service_fd(IMG_FD_OFF));
>
> I don't think we should check this in runtime. If we need this, this should be made
> once in one of the init functions.

I might be wrong, but it looks to me the same as:
BUILD_BUG_ON(LOG_FD_OFF > IMG_FD_OFF)

so we can omit runtime penalty and keep it in place if I'm not mistaken.
But maybe you want to check the blackbox of get_service_fd(), not
what it implies.
Dmitry Safonov April 17, 2018, 3:04 p.m.
2018-04-17 16:01 GMT+01:00 Dmitry Safonov <0x7f454c46@gmail.com>:
> 2018-04-17 15:44 GMT+01:00 Kirill Tkhai <ktkhai@virtuozzo.com>:
>> On 17.04.2018 17:07, Pavel Tikhomirov wrote:
>>> Though LOG_FD_OFF < IMG_FD_OFF, get_service_fd(LOG_FD_OFF) is > than
>>> get_service_fd(IMG_FD_OFF), see __get_service_fd, so the check here
>>> should be twisted. Also add bug_on to track possible __get_service_fd
>>> change which can break these check again.
>>>
>>> We have a problem when USERNSD_SK replaces LOG_FD_OFF, latter when
>>> writing to log, instead we actually send crazy commands to usernsd,
>>> which failes to handle them and BUG or crash.
>>>
>>> https://jira.sw.ru/browse/PSBM-83472
>>>
>>> Also we had similar problem when __userns_call received bad repsonse,
>>> likely it has the same background.
>>>
>>> https://api.travis-ci.org/v3/job/352164661/log.txt
>>>
>>> fixes commit 129bb14611c3 ("files: Prepare clone_service_fd() for
>>> overlaping ranges.")
>>>
>>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>>> ---
>>>  criu/util.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/criu/util.c b/criu/util.c
>>> index b19bf5175..4af542a4f 100644
>>> --- a/criu/util.c
>>> +++ b/criu/util.c
>>> @@ -617,7 +617,8 @@ int clone_service_fd(struct pstree_item *me)
>>>               return 0;
>>>
>>>       /* Dup sfds in memmove() style: they may overlap */
>>> -     if (get_service_fd(LOG_FD_OFF) > new_base - LOG_FD_OFF - SERVICE_FD_MAX * id)
>>
>> Good catch!
>>
>>> +     BUG_ON(get_service_fd(LOG_FD_OFF) < get_service_fd(IMG_FD_OFF));
>>
>> I don't think we should check this in runtime. If we need this, this should be made
>> once in one of the init functions.
>
> I might be wrong, but it looks to me the same as:
> BUILD_BUG_ON(LOG_FD_OFF > IMG_FD_OFF)
>
> so we can omit runtime penalty and keep it in place if I'm not mistaken.
> But maybe you want to check the blackbox of get_service_fd(), not
> what it implies.

Ok, I guess, it's about the internals of get_service_fd(), drop my comment :)