[v3,2/2] unix: don't drop the path on unix sockets if they don't exist

Submitted by Tycho Andersen on July 5, 2016, 3:15 p.m.

Details

Message ID 1467731707-1792-2-git-send-email-tycho.andersen@canonical.com
State Accepted
Series "Series without cover letter"
Commit 24da7e2c386d3e11d43915aa8bee8e2c96e8981e
Headers show

Commit Message

Tycho Andersen July 5, 2016, 3:15 p.m.
For standalone unix sockets, listen() will fail if we haven't called bind()
with an actual address. If we remove the name on dump, we won't call
bind(), and thus sockets in this state will fail to restore.

v2: temporarily rename a unix socket out of the way if necessary in order
    to bind() correctly and then delete it (e.g. when there are two unix
    sockets bound "on top" of each other)
v3: remove extra unlink(), do the real unlink() in bind_unix_sk() so we
    only need to do it once

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
---
 criu/sk-unix.c       | 73 ++++++++++++++++++++++++++++++++++++++++------------
 images/sk-unix.proto |  1 +
 2 files changed, 58 insertions(+), 16 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/sk-unix.c b/criu/sk-unix.c
index ca6673e..c5062d4 100644
--- a/criu/sk-unix.c
+++ b/criu/sk-unix.c
@@ -72,6 +72,7 @@  struct unix_sk_desc {
 	unsigned int		nr_icons;
 	unsigned int		*icons;
 	unsigned char		shutdown;
+	bool			deleted;
 
 	mode_t			mode;
 	uid_t			uid;
@@ -337,6 +338,11 @@  static int dump_one_unix_fd(int lfd, u32 id, const struct fd_parms *p)
 		perms->gid	= userns_gid(sk->gid);
 	}
 
+	if (sk->deleted) {
+		ue->has_deleted = true;
+		ue->deleted	= sk->deleted;
+	}
+
 	sk_encode_shutdown(ue, sk->shutdown);
 
 	if (ue->peer) {
@@ -503,7 +509,7 @@  static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
 
 	if (name[0] != '\0') {
 		struct unix_diag_vfs *uv;
-		bool drop_path = false;
+		bool deleted = false;
 		char rpath[PATH_MAX];
 		struct ns_id *ns;
 		struct stat st;
@@ -554,30 +560,21 @@  static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
 
 			pr_info("unix: Dropping path %s for unlinked sk %#x\n",
 				name, m->udiag_ino);
-			drop_path = true;
+			deleted = true;
 		} else if ((st.st_ino != uv->udiag_vfs_ino) ||
 			   !phys_stat_dev_match(st.st_dev, uv->udiag_vfs_dev, ns, name)) {
 			pr_info("unix: Dropping path %s for unlinked bound "
 				"sk %#x.%#x real %#x.%#x\n",
 				name, (int)st.st_dev, (int)st.st_ino,
 				(int)uv->udiag_vfs_dev, (int)uv->udiag_vfs_ino);
-			drop_path = true;
-		}
-
-		if (drop_path) {
-			/*
-			 * When a socket is bound to unlinked file, we
-			 * just drop his name, since no one will access
-			 * it via one.
-			 */
-			xfree(name);
-			len = 0;
-			name = NULL;
+			deleted = true;
 		}
 
 		d->mode = st.st_mode;
 		d->uid	= st.st_uid;
 		d->gid	= st.st_gid;
+
+		d->deleted = deleted;
 	}
 
 postprone:
@@ -962,8 +959,47 @@  static int bind_unix_sk(int sk, struct unix_sk_info *ui)
 		ret = bind(sk, (struct sockaddr *)&addr,
 				sizeof(addr.sun_family) + ui->ue->name.len);
 		if (ret < 0) {
-			pr_perror("Can't bind socket");
-			goto done;
+			if (ui->ue->has_deleted && ui->ue->deleted && errno == EADDRINUSE) {
+				char temp[PATH_MAX];
+
+				pr_info("found duplicate unix socket bound at %s\n", addr.sun_path);
+
+				ret = snprintf(temp, sizeof(temp), "%s-%s-%d", addr.sun_path, "criu-temp", getpid());
+				/* this shouldn't happen, since sun_addr is only 108 chars long */
+				if (ret < 0 || ret >= sizeof(temp)) {
+					pr_err("snprintf of %s failed?\n", addr.sun_path);
+					goto done;
+				}
+
+				ret = rename(addr.sun_path, temp);
+				if (ret < 0) {
+					pr_perror("couldn't move socket for binding");
+					goto done;
+				}
+
+				ret = bind(sk, (struct sockaddr *)&addr,
+						sizeof(addr.sun_family) + ui->ue->name.len);
+				if (ret < 0) {
+					pr_perror("Can't bind socket after move");
+					goto done;
+				}
+
+				ret = rename(temp, addr.sun_path);
+				if (ret < 0) {
+					pr_perror("couldn't move socket back");
+					goto done;
+				}
+
+				/* we've handled the deleted-ness of this
+				 * socket and we don't want to delete it later
+				 * since it's not /this/ socket.
+				 */
+				ui->ue->deleted = false;
+
+			} else {
+				pr_perror("Can't bind socket");
+				goto done;
+			}
 		}
 
 		if (*ui->name && ui->ue->file_perms) {
@@ -988,6 +1024,11 @@  static int bind_unix_sk(int sk, struct unix_sk_info *ui)
 				goto done;
 			}
 		}
+
+		if (ui->ue->deleted && unlink((char *)ui->ue->name.data) < 0) {
+			pr_perror("failed to unlink %s\n", ui->ue->name.data);
+			return -1;
+		}
 	}
 
 	if (ui->ue->state != TCP_LISTEN)
diff --git a/images/sk-unix.proto b/images/sk-unix.proto
index 9c90376..3026214 100644
--- a/images/sk-unix.proto
+++ b/images/sk-unix.proto
@@ -47,4 +47,5 @@  message unix_sk_entry {
 	 * Relative socket name may have prefix.
 	 */
 	optional string			name_dir	= 14;
+	optional bool			deleted		= 15;
 }

Comments

Pavel Emelianov July 13, 2016, 1:01 p.m.
Both applied, thanks :)

Would you also check whether this thing git fixed:

static int bind_unix_sk(int sk, struct unix_sk_info *ui)
{
        struct sockaddr_un addr;
        int cwd_fd = -1;
        int ret = -1;

        if ((ui->ue->type == SOCK_STREAM) && (ui->ue->state == TCP_ESTABLISHED)) {
                /*
                 * FIXME this can be done, but for doing this properly we
                 * need to bind socket to its name, then rename one to
                 * some temporary unique one and after all the sockets are
                 * restored we should walk those temp names and rename
                 * some of them back to real ones.
                 */
                ret = 0;
                goto done;
        }

this is for the case when we have established socket that inherited its name
from the listening one. Presumably it has, and, id I'm right, would you add
a test for this and drop this comment? :)

-- Pavel
Tycho Andersen July 13, 2016, 2:25 p.m.
Hi Pavel,

On Wed, Jul 13, 2016 at 04:01:59PM +0300, Pavel Emelyanov wrote:
> Both applied, thanks :)
> 
> Would you also check whether this thing git fixed:
> 
> static int bind_unix_sk(int sk, struct unix_sk_info *ui)
> {
>         struct sockaddr_un addr;
>         int cwd_fd = -1;
>         int ret = -1;
> 
>         if ((ui->ue->type == SOCK_STREAM) && (ui->ue->state == TCP_ESTABLISHED)) {
>                 /*
>                  * FIXME this can be done, but for doing this properly we
>                  * need to bind socket to its name, then rename one to
>                  * some temporary unique one and after all the sockets are
>                  * restored we should walk those temp names and rename
>                  * some of them back to real ones.
>                  */
>                 ret = 0;
>                 goto done;
>         }
> 
> this is for the case when we have established socket that inherited its name
> from the listening one. Presumably it has, and, id I'm right, would you add
> a test for this and drop this comment? :)

Won't established sockets have the same inode as listen sockets? I
think in this case we have to do something a little more clever, as
the code won't detect them as "deleted". Anyway, I can figure it out
and send some patches.

Tycho
Pavel Emelianov July 14, 2016, 10:55 a.m.
On 07/13/2016 05:25 PM, Tycho Andersen wrote:
> Hi Pavel,
> 
> On Wed, Jul 13, 2016 at 04:01:59PM +0300, Pavel Emelyanov wrote:
>> Both applied, thanks :)
>>
>> Would you also check whether this thing git fixed:
>>
>> static int bind_unix_sk(int sk, struct unix_sk_info *ui)
>> {
>>         struct sockaddr_un addr;
>>         int cwd_fd = -1;
>>         int ret = -1;
>>
>>         if ((ui->ue->type == SOCK_STREAM) && (ui->ue->state == TCP_ESTABLISHED)) {
>>                 /*
>>                  * FIXME this can be done, but for doing this properly we
>>                  * need to bind socket to its name, then rename one to
>>                  * some temporary unique one and after all the sockets are
>>                  * restored we should walk those temp names and rename
>>                  * some of them back to real ones.
>>                  */
>>                 ret = 0;
>>                 goto done;
>>         }
>>
>> this is for the case when we have established socket that inherited its name
>> from the listening one. Presumably it has, and, id I'm right, would you add
>> a test for this and drop this comment? :)
> 
> Won't established sockets have the same inode as listen sockets?

No, the new pair of sockets will be created upon connect()/accept() calls.

> I think in this case we have to do something a little more clever, as
> the code won't detect them as "deleted". Anyway, I can figure it out
> and send some patches.

Awesome :)

> Tycho
> .
>
Andrey Vagin July 16, 2016, 5:20 a.m.
On Tue, Jul 05, 2016 at 03:15:07PM +0000, Tycho Andersen wrote:
> For standalone unix sockets, listen() will fail if we haven't called bind()
> with an actual address. If we remove the name on dump, we won't call
> bind(), and thus sockets in this state will fail to restore.
> 
> v2: temporarily rename a unix socket out of the way if necessary in order
>     to bind() correctly and then delete it (e.g. when there are two unix
>     sockets bound "on top" of each other)
> v3: remove extra unlink(), do the real unlink() in bind_unix_sk() so we
>     only need to do it once
> 
> Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
> ---
>  criu/sk-unix.c       | 73 ++++++++++++++++++++++++++++++++++++++++------------
>  images/sk-unix.proto |  1 +
>  2 files changed, 58 insertions(+), 16 deletions(-)
> 
> diff --git a/criu/sk-unix.c b/criu/sk-unix.c
> index ca6673e..c5062d4 100644
> --- a/criu/sk-unix.c
> +++ b/criu/sk-unix.c
> @@ -72,6 +72,7 @@ struct unix_sk_desc {
>  	unsigned int		nr_icons;
>  	unsigned int		*icons;
>  	unsigned char		shutdown;
> +	bool			deleted;
>  
>  	mode_t			mode;
>  	uid_t			uid;
> @@ -337,6 +338,11 @@ static int dump_one_unix_fd(int lfd, u32 id, const struct fd_parms *p)
>  		perms->gid	= userns_gid(sk->gid);
>  	}
>  
> +	if (sk->deleted) {
> +		ue->has_deleted = true;
> +		ue->deleted	= sk->deleted;
> +	}
> +
>  	sk_encode_shutdown(ue, sk->shutdown);
>  
>  	if (ue->peer) {
> @@ -503,7 +509,7 @@ static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
>  
>  	if (name[0] != '\0') {
>  		struct unix_diag_vfs *uv;
> -		bool drop_path = false;
> +		bool deleted = false;
>  		char rpath[PATH_MAX];
>  		struct ns_id *ns;
>  		struct stat st;
> @@ -554,30 +560,21 @@ static int unix_process_name(struct unix_sk_desc *d, const struct unix_diag_msg
>  
>  			pr_info("unix: Dropping path %s for unlinked sk %#x\n",
>  				name, m->udiag_ino);
> -			drop_path = true;
> +			deleted = true;
>  		} else if ((st.st_ino != uv->udiag_vfs_ino) ||
>  			   !phys_stat_dev_match(st.st_dev, uv->udiag_vfs_dev, ns, name)) {
>  			pr_info("unix: Dropping path %s for unlinked bound "
>  				"sk %#x.%#x real %#x.%#x\n",
>  				name, (int)st.st_dev, (int)st.st_ino,
>  				(int)uv->udiag_vfs_dev, (int)uv->udiag_vfs_ino);
> -			drop_path = true;
> -		}
> -
> -		if (drop_path) {
> -			/*
> -			 * When a socket is bound to unlinked file, we
> -			 * just drop his name, since no one will access
> -			 * it via one.
> -			 */
> -			xfree(name);
> -			len = 0;
> -			name = NULL;
> +			deleted = true;
>  		}
>  
>  		d->mode = st.st_mode;
>  		d->uid	= st.st_uid;
>  		d->gid	= st.st_gid;
> +
> +		d->deleted = deleted;
>  	}
>  
>  postprone:
> @@ -962,8 +959,47 @@ static int bind_unix_sk(int sk, struct unix_sk_info *ui)
>  		ret = bind(sk, (struct sockaddr *)&addr,
>  				sizeof(addr.sun_family) + ui->ue->name.len);
>  		if (ret < 0) {
> -			pr_perror("Can't bind socket");
> -			goto done;
> +			if (ui->ue->has_deleted && ui->ue->deleted && errno == EADDRINUSE) {
> +				char temp[PATH_MAX];
> +
> +				pr_info("found duplicate unix socket bound at %s\n", addr.sun_path);
> +
> +				ret = snprintf(temp, sizeof(temp), "%s-%s-%d", addr.sun_path, "criu-temp", getpid());
> +				/* this shouldn't happen, since sun_addr is only 108 chars long */
> +				if (ret < 0 || ret >= sizeof(temp)) {
> +					pr_err("snprintf of %s failed?\n", addr.sun_path);
> +					goto done;
> +				}
> +
> +				ret = rename(addr.sun_path, temp);
> +				if (ret < 0) {
> +					pr_perror("couldn't move socket for binding");
> +					goto done;
> +				}
> +
> +				ret = bind(sk, (struct sockaddr *)&addr,
> +						sizeof(addr.sun_family) + ui->ue->name.len);
> +				if (ret < 0) {
> +					pr_perror("Can't bind socket after move");
> +					goto done;
> +				}
> +
> +				ret = rename(temp, addr.sun_path);
> +				if (ret < 0) {
> +					pr_perror("couldn't move socket back");
> +					goto done;
> +				}
> +
> +				/* we've handled the deleted-ness of this
> +				 * socket and we don't want to delete it later
> +				 * since it's not /this/ socket.
> +				 */
> +				ui->ue->deleted = false;
> +
> +			} else {
> +				pr_perror("Can't bind socket");
> +				goto done;
> +			}
>  		}
>  
>  		if (*ui->name && ui->ue->file_perms) {
> @@ -988,6 +1024,11 @@ static int bind_unix_sk(int sk, struct unix_sk_info *ui)
>  				goto done;
>  			}
>  		}
> +
> +		if (ui->ue->deleted && unlink((char *)ui->ue->name.data) < 0) {
> +			pr_perror("failed to unlink %s\n", ui->ue->name.data);
> +			return -1;

*** CID 164720:  Resource leaks  (RESOURCE_LEAK)
/criu/sk-unix.c: 1030 in bind_unix_sk()
1024                                    goto done;
1025                            }
1026                    }
1027
1028                    if (ui->ue->deleted && unlink((char
*)ui->ue->name.data) < 0) {
1029                            pr_perror("failed to unlink %s\n",
ui->ue->name.data);
>>>     CID 164720:  Resource leaks  (RESOURCE_LEAK)
>>>     Handle variable "cwd_fd" going out of scope leaks the handle.
1030                            return -1;
1031                    }
1032            }
1033
1034            if (ui->ue->state != TCP_LISTEN)
1035                    futex_set_and_wake(&ui->prepared, 1);

> +		}
>  	}
>  
>  	if (ui->ue->state != TCP_LISTEN)
> diff --git a/images/sk-unix.proto b/images/sk-unix.proto
> index 9c90376..3026214 100644
> --- a/images/sk-unix.proto
> +++ b/images/sk-unix.proto
> @@ -47,4 +47,5 @@ message unix_sk_entry {
>  	 * Relative socket name may have prefix.
>  	 */
>  	optional string			name_dir	= 14;
> +	optional bool			deleted		= 15;
>  }
> -- 
> 2.7.4
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu