[v2] files: fix clone_service_fd overlap handling

Submitted by Pavel Tikhomirov on April 17, 2018, 3:15 p.m.

Details

Message ID 20180417151506.27286-1-ptikhomirov@virtuozzo.com
State Accepted
Series "files: fix clone_service_fd overlap handling"
Commit 87b84e59c57a20ef35009192227b3c07cbf5ba23
Headers show

Commit Message

Pavel Tikhomirov April 17, 2018, 3:15 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 fails to handle them and BUGs or crashes.

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

Also we had similar problem when __userns_call receives 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.")

v2: move BUG_ON to main() to check it only once, use min+1 and max-1

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

Patch hide | download patch | download mbox

diff --git a/criu/crtools.c b/criu/crtools.c
index 62adeec35..9ba5832bd 100644
--- a/criu/crtools.c
+++ b/criu/crtools.c
@@ -501,6 +501,9 @@  int main(int argc, char *argv[], char *envp[])
 	BUILD_BUG_ON(PAGE_SIZE != PAGE_IMAGE_SIZE);
 	BUILD_BUG_ON(CTL_32 != SYSCTL_TYPE__CTL_32);
 	BUILD_BUG_ON(__CTL_STR != SYSCTL_TYPE__CTL_STR);
+	/* We use it for fd overlap handling in clone_service_fd() */
+	BUG_ON(get_service_fd(SERVICE_FD_MIN+1) <
+	       get_service_fd(SERVICE_FD_MAX-1));
 
 	if (fault_injection_init())
 		return 1;
diff --git a/criu/util.c b/criu/util.c
index b19bf5175..15bcac03d 100644
--- a/criu/util.c
+++ b/criu/util.c
@@ -617,7 +617,7 @@  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)
+	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, 3:20 p.m.
On 17.04.2018 18:15, 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 fails to handle them and BUGs or crashes.
> 
> https://jira.sw.ru/browse/PSBM-83472
> 
> Also we had similar problem when __userns_call receives 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.")
> 
> v2: move BUG_ON to main() to check it only once, use min+1 and max-1
> 
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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

> ---
>  criu/crtools.c | 3 +++
>  criu/util.c    | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/crtools.c b/criu/crtools.c
> index 62adeec35..9ba5832bd 100644
> --- a/criu/crtools.c
> +++ b/criu/crtools.c
> @@ -501,6 +501,9 @@ int main(int argc, char *argv[], char *envp[])
>  	BUILD_BUG_ON(PAGE_SIZE != PAGE_IMAGE_SIZE);
>  	BUILD_BUG_ON(CTL_32 != SYSCTL_TYPE__CTL_32);
>  	BUILD_BUG_ON(__CTL_STR != SYSCTL_TYPE__CTL_STR);
> +	/* We use it for fd overlap handling in clone_service_fd() */
> +	BUG_ON(get_service_fd(SERVICE_FD_MIN+1) <
> +	       get_service_fd(SERVICE_FD_MAX-1));
>  
>  	if (fault_injection_init())
>  		return 1;
> diff --git a/criu/util.c b/criu/util.c
> index b19bf5175..15bcac03d 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -617,7 +617,7 @@ 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)
> +	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
>
Andrey Vagin April 25, 2018, 6:22 p.m.
Applied, thanks!

On Tue, Apr 17, 2018 at 06:15:06PM +0300, 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 fails to handle them and BUGs or crashes.
> 
> https://jira.sw.ru/browse/PSBM-83472
> 
> Also we had similar problem when __userns_call receives 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.")
> 
> v2: move BUG_ON to main() to check it only once, use min+1 and max-1
> 
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  criu/crtools.c | 3 +++
>  criu/util.c    | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/crtools.c b/criu/crtools.c
> index 62adeec35..9ba5832bd 100644
> --- a/criu/crtools.c
> +++ b/criu/crtools.c
> @@ -501,6 +501,9 @@ int main(int argc, char *argv[], char *envp[])
>  	BUILD_BUG_ON(PAGE_SIZE != PAGE_IMAGE_SIZE);
>  	BUILD_BUG_ON(CTL_32 != SYSCTL_TYPE__CTL_32);
>  	BUILD_BUG_ON(__CTL_STR != SYSCTL_TYPE__CTL_STR);
> +	/* We use it for fd overlap handling in clone_service_fd() */
> +	BUG_ON(get_service_fd(SERVICE_FD_MIN+1) <
> +	       get_service_fd(SERVICE_FD_MAX-1));
>  
>  	if (fault_injection_init())
>  		return 1;
> diff --git a/criu/util.c b/criu/util.c
> index b19bf5175..15bcac03d 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -617,7 +617,7 @@ 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)
> +	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
> -- 
> 2.14.3
>