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

Submitted by Tycho Andersen on June 29, 2016, 1:23 a.m.

Details

Message ID 1467163386-11266-2-git-send-email-tycho.andersen@canonical.com
State Rejected
Series "Series without cover letter"
Headers show

Commit Message

Tycho Andersen June 29, 2016, 1:23 a.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)

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

Patch hide | download patch | download mbox

diff --git a/criu/sk-unix.c b/criu/sk-unix.c
index ca6673e..2940c11 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,53 @@  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 = unlink(addr.sun_path);
+				if (ret < 0) {
+					pr_perror("Can't unlink socket after bind");
+					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) {
@@ -1225,6 +1267,11 @@  out:
 	if (restore_socket_opts(sk, ui->ue->opts))
 		return -1;
 
+	if (ui->ue->deleted && unlink((char *)ui->ue->name.data) < 0) {
+		pr_perror("failed to unlink %s\n", ui->ue->name.data);
+		return -1;
+	}
+
 	return sk;
 }
 
diff --git a/images/sk-unix.proto b/images/sk-unix.proto
index aa2bcf7..75ec3bf 100644
--- a/images/sk-unix.proto
+++ b/images/sk-unix.proto
@@ -45,4 +45,5 @@  message unix_sk_entry {
 	 * Relative socket name may have prefix.
 	 */
 	optional string			name_dir	= 14;
+	optional bool			deleted		= 15;
 }

Comments

Pavel Emelianov July 4, 2016, 5:38 p.m.
> @@ -962,8 +959,53 @@ 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 = unlink(addr.sun_path);

This unlink seems redundant, the rename below should unlink it.

> +				if (ret < 0) {
> +					pr_perror("Can't unlink socket after bind");
> +					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) {
> @@ -1225,6 +1267,11 @@ out:
>  	if (restore_socket_opts(sk, ui->ue->opts))
>  		return -1;
>  
> +	if (ui->ue->deleted && unlink((char *)ui->ue->name.data) < 0) {

I worry that we do this move/bind/unlink/move logic in bind_unix_sk(), which is
called in several places, but remove the deleted name only here. Is this safe?

> +		pr_perror("failed to unlink %s\n", ui->ue->name.data);
> +		return -1;
> +	}
> +
>  	return sk;
>  }
>  
> diff --git a/images/sk-unix.proto b/images/sk-unix.proto
> index aa2bcf7..75ec3bf 100644
> --- a/images/sk-unix.proto
> +++ b/images/sk-unix.proto
> @@ -45,4 +45,5 @@ message unix_sk_entry {
>  	 * Relative socket name may have prefix.
>  	 */
>  	optional string			name_dir	= 14;
> +	optional bool			deleted		= 15;
>  }
>
Tycho Andersen July 5, 2016, 2:53 p.m.
On Mon, Jul 04, 2016 at 08:38:42PM +0300, Pavel Emelyanov wrote:
> 
> > @@ -962,8 +959,53 @@ 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 = unlink(addr.sun_path);
> 
> This unlink seems redundant, the rename below should unlink it.

Ok, I'll drop it.

> > +				if (ret < 0) {
> > +					pr_perror("Can't unlink socket after bind");
> > +					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) {
> > @@ -1225,6 +1267,11 @@ out:
> >  	if (restore_socket_opts(sk, ui->ue->opts))
> >  		return -1;
> >  
> > +	if (ui->ue->deleted && unlink((char *)ui->ue->name.data) < 0) {
> 
> I worry that we do this move/bind/unlink/move logic in bind_unix_sk(), which is
> called in several places, but remove the deleted name only here. Is this safe?

Good point. I think we can just do the unlink in bind_unix_sk() to
avoid having to do this everywhere, as it looks like once the socket
is bound we do all operations via the FD and not the path. This way we
can avoid inserting another call to unlink everywhere.

I'll resend, thanks!

Tycho

> > +		pr_perror("failed to unlink %s\n", ui->ue->name.data);
> > +		return -1;
> > +	}
> > +
> >  	return sk;
> >  }
> >  
> > diff --git a/images/sk-unix.proto b/images/sk-unix.proto
> > index aa2bcf7..75ec3bf 100644
> > --- a/images/sk-unix.proto
> > +++ b/images/sk-unix.proto
> > @@ -45,4 +45,5 @@ message unix_sk_entry {
> >  	 * Relative socket name may have prefix.
> >  	 */
> >  	optional string			name_dir	= 14;
> > +	optional bool			deleted		= 15;
> >  }
> > 
>