File lock status may be empty

Submitted by Nicolas Viennot on Sept. 6, 2019, 7:28 p.m.

Details

Message ID 6d550b8afc1a4d89bb35aec47e7f9ac0@EXMBDFT10.ad.twosigma.com
State New
Series "File lock status may be empty"
Headers show

Commit Message

Nicolas Viennot Sept. 6, 2019, 7:28 p.m.
The lock status string may be empty. This can happen when the owner of
the lock is outside of our PID namespace. See the kernel function
fs/locks.c:lock_get_status() when fl_pid == 0.

Signed-off-by: Nicolas Viennot <nviennot@twosigma.com>
---
 criu/proc_parse.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/criu/proc_parse.c b/criu/proc_parse.c
index 0e8b6f20..749dfa40 100644
--- a/criu/proc_parse.c
+++ b/criu/proc_parse.c
@@ -1669,17 +1669,21 @@  static int parse_fdinfo_pid_s(int pid, int fd, int type, void *arg)
 		if (fdinfo_field(str, "lock")) {
 			struct file_lock *fl;
 			struct fdinfo_common *fdinfo = arg;
+			char *flock_buf = str+6;
 
 			if (type != FD_TYPES__UND)
 				continue;
 
+			if (flock_buf[0] == '\0')
+				continue;
+
 			fl = alloc_file_lock();
 			if (!fl) {
 				pr_perror("Alloc file lock failed!");
 				goto out;
 			}
 
-			if (parse_file_lock_buf(str + 6, fl, 0)) {
+			if (parse_file_lock_buf(flock_buf, fl, 0)) {
 				xfree(fl);
 				goto parse_err;
 			}

Comments

Andrei Vagin Sept. 11, 2019, 11:38 a.m.
On Fri, Sep 06, 2019 at 07:28:33PM +0000, Nicolas Viennot wrote:
> The lock status string may be empty. This can happen when the owner of
> the lock is outside of our PID namespace. See the kernel function
> fs/locks.c:lock_get_status() when fl_pid == 0.
> 

Could we introduce a test case for this?

> Signed-off-by: Nicolas Viennot <nviennot@twosigma.com>
> ---
>  criu/proc_parse.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/proc_parse.c b/criu/proc_parse.c
> index 0e8b6f20..749dfa40 100644
> --- a/criu/proc_parse.c
> +++ b/criu/proc_parse.c
> @@ -1669,17 +1669,21 @@ static int parse_fdinfo_pid_s(int pid, int fd, int type, void *arg)
>  		if (fdinfo_field(str, "lock")) {
>  			struct file_lock *fl;
>  			struct fdinfo_common *fdinfo = arg;
> +			char *flock_buf = str+6;
>  
>  			if (type != FD_TYPES__UND)
>  				continue;
>  
> +			if (flock_buf[0] == '\0')
> +				continue;
> +
>  			fl = alloc_file_lock();
>  			if (!fl) {
>  				pr_perror("Alloc file lock failed!");
>  				goto out;
>  			}
>  
> -			if (parse_file_lock_buf(str + 6, fl, 0)) {
> +			if (parse_file_lock_buf(flock_buf, fl, 0)) {
>  				xfree(fl);
>  				goto parse_err;
>  			}
> -- 
> 2.19.1
> 
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Andrei Vagin Sept. 11, 2019, 11:41 a.m.
On Fri, Sep 06, 2019 at 07:28:33PM +0000, Nicolas Viennot wrote:
> The lock status string may be empty. This can happen when the owner of
> the lock is outside of our PID namespace. See the kernel function
> fs/locks.c:lock_get_status() when fl_pid == 0.
> 
> Signed-off-by: Nicolas Viennot <nviennot@twosigma.com>
> ---
>  criu/proc_parse.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/proc_parse.c b/criu/proc_parse.c
> index 0e8b6f20..749dfa40 100644
> --- a/criu/proc_parse.c
> +++ b/criu/proc_parse.c
> @@ -1669,17 +1669,21 @@ static int parse_fdinfo_pid_s(int pid, int fd, int type, void *arg)
>  		if (fdinfo_field(str, "lock")) {
>  			struct file_lock *fl;
>  			struct fdinfo_common *fdinfo = arg;
> +			char *flock_buf = str+6;
>  
>  			if (type != FD_TYPES__UND)
>  				continue;
>

Could you add a comment which describes what's going on here?
  
> +			if (flock_buf[0] == '\0')
> +				continue;
> +
>  			fl = alloc_file_lock();
>  			if (!fl) {
>  				pr_perror("Alloc file lock failed!");
>  				goto out;
>  			}
>  
> -			if (parse_file_lock_buf(str + 6, fl, 0)) {
> +			if (parse_file_lock_buf(flock_buf, fl, 0)) {
>  				xfree(fl);
>  				goto parse_err;
>  			}
> -- 
> 2.19.1
> 
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Nicolas Viennot Sept. 19, 2019, 5:31 p.m.
Turns out, this problem has been solved in the kernel:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1cf8e5de4055f85383405a21a0a7c3c4348bf2ed

Instead of printing an empty "lock: " status when the owner of the lock is not
visible in the current namespace, the new behavior is to print the lock status
with pid=0 as the owner of the lock. This won't affect CRIU negatively as the
restoring logic goes as follow: each restored process acquires the locks that
belong to that process. Thus, locks of owner pid=0 will simply be ignored.

Also, I don't know how to reproduce this issue, even with an older kernel, and
thus can't write a test case for it.

Nico

-----Original Message-----
From: Andrei Vagin <avagin@gmail.com> 
Sent: Wednesday, September 11, 2019 7:38 AM
To: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
Cc: 'criu@openvz.org' <criu@openvz.org>
Subject: Re: [CRIU] [PATCH] File lock status may be empty

On Fri, Sep 06, 2019 at 07:28:33PM +0000, Nicolas Viennot wrote:
> The lock status string may be empty. This can happen when the owner of 
> the lock is outside of our PID namespace. See the kernel function
> fs/locks.c:lock_get_status() when fl_pid == 0.
> 

Could we introduce a test case for this?

> Signed-off-by: Nicolas Viennot <nviennot@twosigma.com>
> ---
>  criu/proc_parse.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/proc_parse.c b/criu/proc_parse.c index 
> 0e8b6f20..749dfa40 100644
> --- a/criu/proc_parse.c
> +++ b/criu/proc_parse.c
> @@ -1669,17 +1669,21 @@ static int parse_fdinfo_pid_s(int pid, int fd, int type, void *arg)
>  		if (fdinfo_field(str, "lock")) {
>  			struct file_lock *fl;
>  			struct fdinfo_common *fdinfo = arg;
> +			char *flock_buf = str+6;
>  
>  			if (type != FD_TYPES__UND)
>  				continue;
>  
> +			if (flock_buf[0] == '\0')
> +				continue;
> +
>  			fl = alloc_file_lock();
>  			if (!fl) {
>  				pr_perror("Alloc file lock failed!");
>  				goto out;
>  			}
>  
> -			if (parse_file_lock_buf(str + 6, fl, 0)) {
> +			if (parse_file_lock_buf(flock_buf, fl, 0)) {
>  				xfree(fl);
>  				goto parse_err;
>  			}
> --
> 2.19.1
> 
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://secure-web.cisco.com/1MKzXNYMzZwJn_J9RsFEUfPmDw8sZbUKn_Lw4lWDC
> VSP_Rxsl1razN-W-iH8LFtYlwXsepsT21iwSWQQ_urqq0znwbVNbQTsazCXrnKpi3IqJ4h
> bQZvWpcLyl6FetN7fr4x2wvg9TgZGFCQRC_-zyt0VmDRje5jze9OrR-YHlVoJ2erbrGodn
> _mZL6DLHJN2JUgdwLDa6gULq3cDj0PpR-LJ0_jUuV624yZakVi-ZtQ_LZgRogwy5wyEXT5
> DG89SUAjYLmkVC4Vg6o6Go-hh_GQ/https%3A%2F%2Flists.openvz.org%2Fmailman%
> 2Flistinfo%2Fcriu