[1/5] files: Fix crossing unused and service fds of shared fd tables

Submitted by Kirill Tkhai on June 7, 2017, 3:15 p.m.

Details

Message ID 149684851206.12941.9434108039579487927.stgit@localhost.localdomain
State Rejected
Series "Use fdstore to keep mnt_ns descriptors"
Headers show

Commit Message

Kirill Tkhai June 7, 2017, 3:15 p.m.
service_fd_id is id of a specific task, while other tasks
in shared fd table group may have bigger id numbers.
In this case given unused fd intersects with service fds
of such tasks. This leads to undefined behaviour. Fix that.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 criu/files.c             |    4 ++--
 criu/include/servicefd.h |    4 +++-
 criu/servicefd.c         |   12 ++++++++++--
 3 files changed, 15 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/files.c b/criu/files.c
index 6aa88ba8b..73672b87e 100644
--- a/criu/files.c
+++ b/criu/files.c
@@ -138,7 +138,7 @@  unsigned int find_unused_fd(struct pstree_item *task, int hint_fd)
 		goto out;
 	}
 
-	prev_fd = service_fd_min_fd() - 1;
+	prev_fd = service_fd_min_fd(task) - 1;
 	head = &rsti(task)->fds;
 
 	list_for_each_entry_reverse(fle, head, ps_list) {
@@ -799,7 +799,7 @@  int prepare_fd_pid(struct pstree_item *item)
 		if (ret <= 0)
 			break;
 
-		if (e->fd >= service_fd_min_fd()) {
+		if (e->fd >= service_fd_min_fd(item)) {
 			ret = -1;
 			pr_err("Too big FD number to restore %d\n", e->fd);
 			break;
diff --git a/criu/include/servicefd.h b/criu/include/servicefd.h
index b77922394..c14e8ec87 100644
--- a/criu/include/servicefd.h
+++ b/criu/include/servicefd.h
@@ -28,6 +28,8 @@  enum sfd_type {
 	SERVICE_FD_MAX
 };
 
+struct pstree_item;
+
 extern int clone_service_fd(int id);
 extern int init_service_fd(void);
 extern int get_service_fd(enum sfd_type type);
@@ -36,6 +38,6 @@  extern int install_service_fd(enum sfd_type type, int fd);
 extern int close_service_fd(enum sfd_type type);
 extern bool is_service_fd(int fd, enum sfd_type type);
 extern bool is_any_service_fd(int fd);
-extern int service_fd_min_fd(void);
+extern int service_fd_min_fd(struct pstree_item *item);
 
 #endif /* __CR_SERVICE_FD_H__ */
diff --git a/criu/servicefd.c b/criu/servicefd.c
index 245891b79..207693fb2 100644
--- a/criu/servicefd.c
+++ b/criu/servicefd.c
@@ -9,6 +9,8 @@ 
 #include "servicefd.h"
 #include "bitops.h"
 #include "criu-log.h"
+#include "rst_info.h"
+#include "pstree.h"
 
 #include "common/bug.h"
 
@@ -72,9 +74,15 @@  static int __get_service_fd(enum sfd_type type, int service_fd_id)
 	return service_fd_rlim_cur - type - SERVICE_FD_MAX * service_fd_id;
 }
 
-int service_fd_min_fd(void)
+int service_fd_min_fd(struct pstree_item *item)
 {
-	return service_fd_rlim_cur - (SERVICE_FD_MAX - 1) - SERVICE_FD_MAX * service_fd_id;
+	struct fdt *fdt = rsti(item)->fdt;
+	int id = 0;
+
+	if (fdt)
+		id = fdt->nr - 1;
+
+	return service_fd_rlim_cur - (SERVICE_FD_MAX - 1) - SERVICE_FD_MAX * id;
 }
 
 int reserve_service_fd(enum sfd_type type)

Comments

Kirill Gorkunov June 7, 2017, 3:26 p.m.
On Wed, Jun 07, 2017 at 06:15:12PM +0300, Kirill Tkhai wrote:
> service_fd_id is id of a specific task, while other tasks
> in shared fd table group may have bigger id numbers.
> In this case given unused fd intersects with service fds
> of such tasks. This leads to undefined behaviour. Fix that.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> +int service_fd_min_fd(struct pstree_item *item)
>  {
> -	return service_fd_rlim_cur - (SERVICE_FD_MAX - 1) - SERVICE_FD_MAX * service_fd_id;
> +	struct fdt *fdt = rsti(item)->fdt;
> +	int id = 0;
> +
> +	if (fdt)
> +		id = fdt->nr - 1;

Why -1 here?
Kirill Tkhai June 8, 2017, 11:33 a.m.
On 07.06.2017 18:26, Cyrill Gorcunov wrote:
> On Wed, Jun 07, 2017 at 06:15:12PM +0300, Kirill Tkhai wrote:
>> service_fd_id is id of a specific task, while other tasks
>> in shared fd table group may have bigger id numbers.
>> In this case given unused fd intersects with service fds
>> of such tasks. This leads to undefined behaviour. Fix that.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> +int service_fd_min_fd(struct pstree_item *item)
>>  {
>> -	return service_fd_rlim_cur - (SERVICE_FD_MAX - 1) - SERVICE_FD_MAX * service_fd_id;
>> +	struct fdt *fdt = rsti(item)->fdt;
>> +	int id = 0;
>> +
>> +	if (fdt)
>> +		id = fdt->nr - 1;
> 
> Why -1 here?

service_fd has numbers from 0 to fdt->nr-1, so we have to take the biggest number,
and it is be the biggest offset. I based on this. Is there wrong assumption?
Cyrill Gorcunov June 9, 2017, 7:17 p.m.
On Thu, Jun 08, 2017 at 02:33:31PM +0300, Kirill Tkhai wrote:
> >> +int service_fd_min_fd(struct pstree_item *item)
> >>  {
> >> -	return service_fd_rlim_cur - (SERVICE_FD_MAX - 1) - SERVICE_FD_MAX * service_fd_id;
> >> +	struct fdt *fdt = rsti(item)->fdt;
> >> +	int id = 0;
> >> +
> >> +	if (fdt)
> >> +		id = fdt->nr - 1;
> > 
> > Why -1 here?
> 
> service_fd has numbers from 0 to fdt->nr-1, so we have to take the biggest number,
> and it is be the biggest offset. I based on this. Is there wrong assumption?

That's fine. Thanks.
Andrey Vagin June 21, 2017, 12:14 a.m.
This patch isn't a part of this series. Pls, rebase and send it
separately.

Thanks,
Andrei

On Wed, Jun 07, 2017 at 06:15:12PM +0300, Kirill Tkhai wrote:
> service_fd_id is id of a specific task, while other tasks
> in shared fd table group may have bigger id numbers.
> In this case given unused fd intersects with service fds
> of such tasks. This leads to undefined behaviour. Fix that.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  criu/files.c             |    4 ++--
>  criu/include/servicefd.h |    4 +++-
>  criu/servicefd.c         |   12 ++++++++++--
>  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/criu/files.c b/criu/files.c
> index 6aa88ba8b..73672b87e 100644
> --- a/criu/files.c
> +++ b/criu/files.c
> @@ -138,7 +138,7 @@ unsigned int find_unused_fd(struct pstree_item *task, int hint_fd)
>  		goto out;
>  	}
>  
> -	prev_fd = service_fd_min_fd() - 1;
> +	prev_fd = service_fd_min_fd(task) - 1;
>  	head = &rsti(task)->fds;
>  
>  	list_for_each_entry_reverse(fle, head, ps_list) {
> @@ -799,7 +799,7 @@ int prepare_fd_pid(struct pstree_item *item)
>  		if (ret <= 0)
>  			break;
>  
> -		if (e->fd >= service_fd_min_fd()) {
> +		if (e->fd >= service_fd_min_fd(item)) {
>  			ret = -1;
>  			pr_err("Too big FD number to restore %d\n", e->fd);
>  			break;
> diff --git a/criu/include/servicefd.h b/criu/include/servicefd.h
> index b77922394..c14e8ec87 100644
> --- a/criu/include/servicefd.h
> +++ b/criu/include/servicefd.h
> @@ -28,6 +28,8 @@ enum sfd_type {
>  	SERVICE_FD_MAX
>  };
>  
> +struct pstree_item;
> +
>  extern int clone_service_fd(int id);
>  extern int init_service_fd(void);
>  extern int get_service_fd(enum sfd_type type);
> @@ -36,6 +38,6 @@ extern int install_service_fd(enum sfd_type type, int fd);
>  extern int close_service_fd(enum sfd_type type);
>  extern bool is_service_fd(int fd, enum sfd_type type);
>  extern bool is_any_service_fd(int fd);
> -extern int service_fd_min_fd(void);
> +extern int service_fd_min_fd(struct pstree_item *item);
>  
>  #endif /* __CR_SERVICE_FD_H__ */
> diff --git a/criu/servicefd.c b/criu/servicefd.c
> index 245891b79..207693fb2 100644
> --- a/criu/servicefd.c
> +++ b/criu/servicefd.c
> @@ -9,6 +9,8 @@
>  #include "servicefd.h"
>  #include "bitops.h"
>  #include "criu-log.h"
> +#include "rst_info.h"
> +#include "pstree.h"
>  
>  #include "common/bug.h"
>  
> @@ -72,9 +74,15 @@ static int __get_service_fd(enum sfd_type type, int service_fd_id)
>  	return service_fd_rlim_cur - type - SERVICE_FD_MAX * service_fd_id;
>  }
>  
> -int service_fd_min_fd(void)
> +int service_fd_min_fd(struct pstree_item *item)
>  {
> -	return service_fd_rlim_cur - (SERVICE_FD_MAX - 1) - SERVICE_FD_MAX * service_fd_id;
> +	struct fdt *fdt = rsti(item)->fdt;
> +	int id = 0;
> +
> +	if (fdt)
> +		id = fdt->nr - 1;
> +
> +	return service_fd_rlim_cur - (SERVICE_FD_MAX - 1) - SERVICE_FD_MAX * id;
>  }
>  
>  int reserve_service_fd(enum sfd_type type)
>