[CRIU] [PATCH] files: fix clone_service_fd overlap handling

[Dmitry Safonov]

2018-04-17 16:01 GMT+01:00 Dmitry Safonov <0x7f454c46 at gmail.com>:
> 2018-04-17 15:44 GMT+01:00 Kirill Tkhai <ktkhai at 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 at 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 :)

-- 
             Dmitry