[v5,10/11] unix: Correctly restore DGRAM promiscuos queue

Submitted by Kirill Tkhai on June 16, 2016, 1:54 p.m.

Details

Message ID 146608527493.15781.9726162354620803720.stgit@pro
State Rejected
Series "Support for packet's msg_name in receive queue of promiscous DGRAM sockets"
Headers show

Commit Message

Kirill Tkhai June 16, 2016, 1:54 p.m.
In case of DGRAM, bound, queuerless sockets, restore
their queues using restore_promisc_queue().

This function forces to use socket address, and
do not pass a queuer_fd. The last leads to that
fd, used for sendto(), will be choosen by get_pkt_sender_fd(),
and correct pkt msg name will be restored.

v5: New

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 criu/include/sk-queue.h |    1 +
 criu/sk-queue.c         |   15 ++++++++++-
 criu/sk-unix.c          |   63 ++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 76 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/include/sk-queue.h b/criu/include/sk-queue.h
index a16edc6..78a5677 100644
--- a/criu/include/sk-queue.h
+++ b/criu/include/sk-queue.h
@@ -13,6 +13,7 @@  struct sk_packet {
 };
 
 extern struct list_head packets_list;
+extern int get_pkt_sender_fd(SkPacketEntry *entry, int *noname_fd);
 
 extern struct collect_image_info sk_queues_cinfo;
 extern int dump_sk_queue(int sock_fd, int sock_id, u64 (*get_sender)(const char *, int));
diff --git a/criu/sk-queue.c b/criu/sk-queue.c
index 9e9a484..d0e5862 100644
--- a/criu/sk-queue.c
+++ b/criu/sk-queue.c
@@ -192,11 +192,12 @@  int __restore_sk_queue(int queuer_fd, unsigned int peer_id, struct sockaddr *dst
 {
 	struct sk_packet *pkt, *tmp;
 	struct cr_img *img;
+	int noname_fd = -1;
 	int fd, ret;
 
 	pr_info("Trying to restore recv queue for %u\n", peer_id);
 
-	if (restore_prepare_socket(queuer_fd) < 0)
+	if (queuer_fd >=0 && restore_prepare_socket(queuer_fd) < 0)
 		return -1;
 
 	img = open_image(CR_FD_SK_QUEUES, O_RSTR);
@@ -235,7 +236,14 @@  int __restore_sk_queue(int queuer_fd, unsigned int peer_id, struct sockaddr *dst
 			goto err;
 		}
 
-		fd = queuer_fd;
+		if (queuer_fd >= 0)
+			fd = queuer_fd;
+		else
+			fd = get_pkt_sender_fd(entry, &noname_fd);
+		if (fd < 0) {
+			ret = -1;
+			goto err;
+		}
 		ret = sendto(fd, buf, entry->length, 0, dst_addr, dst_addrlen);
 		xfree(buf);
 		if (ret < 0) {
@@ -252,6 +260,9 @@  int __restore_sk_queue(int queuer_fd, unsigned int peer_id, struct sockaddr *dst
 		xfree(pkt);
 	}
 
+	if (noname_fd >= 0)
+		close(noname_fd);
+
 	close_image(img);
 	return 0;
 err:
diff --git a/criu/sk-unix.c b/criu/sk-unix.c
index b221a71..759a261 100644
--- a/criu/sk-unix.c
+++ b/criu/sk-unix.c
@@ -907,6 +907,63 @@  static int prep_unix_sk_cwd(struct unix_sk_info *ui, int *prev_cwd_fd)
 	return 0;
 }
 
+static struct file_desc_ops unix_desc_ops;
+
+int get_pkt_sender_fd(SkPacketEntry *e, int *noname_fd)
+{
+	struct fdinfo_list_entry *fle;
+	struct unix_sk_info *ui;
+	u32 ino;
+
+	if (!e->has_sender_ino) {
+		if (*noname_fd >= 0)
+			return *noname_fd;
+		*noname_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
+		return *noname_fd;
+	}
+
+	ino = e->sender_ino;
+
+	list_for_each_entry(fle, &rsti(current)->used, used_list) {
+		if (fle->desc->ops != &unix_desc_ops)
+			continue;
+		ui = container_of(fle->desc, struct unix_sk_info, d);
+		if (ui->ue->ino != ino || ui->ue->type != SOCK_DGRAM)
+			continue;
+		if (!list_empty(&ui->d.fd_info_head))
+			futex_wait_while(&ui->prepared, 0);
+		return fle->fe->fd;
+	}
+
+	pr_err("Can't find sender fd: pid=%u, ino=%u\n", current->pid.virt, ino);
+	return -1;
+}
+
+static int restore_promisc_queue(struct unix_sk_info *ui, int fd)
+{
+	struct sockaddr_un addr;
+	socklen_t addrlen;
+	int cwd_fd = -1;
+
+	addrlen = ui->ue->name.len + sizeof(addr.sun_family);
+	memcpy(&addr.sun_path, ui->ue->name.data, ui->ue->name.len);
+	addr.sun_family = AF_UNIX;
+
+	if (prep_unix_sk_cwd(ui, &cwd_fd))
+		return -1;
+
+	if (__restore_sk_queue(-1, ui->ue->id,
+			       (struct sockaddr *)&addr, addrlen) < 0) {
+		pr_err("Can't restore rcv queue\n");
+		revert_unix_sk_cwd(&cwd_fd);
+		return -1;
+	}
+
+	revert_unix_sk_cwd(&cwd_fd);
+
+	return 0;
+}
+
 static int post_open_unix_sk(struct file_desc *d, int fd)
 {
 	struct unix_sk_info *ui;
@@ -918,6 +975,10 @@  static int post_open_unix_sk(struct file_desc *d, int fd)
 	if (ui->flags & (USK_PAIR_MASTER | USK_PAIR_SLAVE))
 		return 0;
 
+	if (ui->ue->type == SOCK_DGRAM && !ui->queuer && ui->ue->name.len &&
+	    restore_promisc_queue(ui, fd))
+		return -1;
+
 	peer = ui->peer;
 
 	if (peer == NULL)
@@ -1177,7 +1238,7 @@  static int open_unixsk_standalone(struct unix_sk_info *ui)
 
 		close(sks[1]);
 		sk = sks[0];
-	} else if (ui->ue->type == SOCK_DGRAM && !ui->queuer) {
+	} else if (ui->ue->type == SOCK_DGRAM && !ui->queuer && !ui->ue->name.len) {
 		struct sockaddr_un addr;
 		int sks[2];
 

Comments

Pavel Emelyanov June 28, 2016, 12:27 p.m.
On 06/16/2016 04:54 PM, Kirill Tkhai wrote:
> In case of DGRAM, bound, queuerless sockets, restore
> their queues using restore_promisc_queue().
> 
> This function forces to use socket address, and
> do not pass a queuer_fd. The last leads to that
> fd, used for sendto(), will be choosen by get_pkt_sender_fd(),
> and correct pkt msg name will be restored.
> 
> v5: New
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  criu/include/sk-queue.h |    1 +
>  criu/sk-queue.c         |   15 ++++++++++-
>  criu/sk-unix.c          |   63 ++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/criu/include/sk-queue.h b/criu/include/sk-queue.h
> index a16edc6..78a5677 100644
> --- a/criu/include/sk-queue.h
> +++ b/criu/include/sk-queue.h
> @@ -13,6 +13,7 @@ struct sk_packet {
>  };
>  
>  extern struct list_head packets_list;
> +extern int get_pkt_sender_fd(SkPacketEntry *entry, int *noname_fd);
>  
>  extern struct collect_image_info sk_queues_cinfo;
>  extern int dump_sk_queue(int sock_fd, int sock_id, u64 (*get_sender)(const char *, int));
> diff --git a/criu/sk-queue.c b/criu/sk-queue.c
> index 9e9a484..d0e5862 100644
> --- a/criu/sk-queue.c
> +++ b/criu/sk-queue.c
> @@ -192,11 +192,12 @@ int __restore_sk_queue(int queuer_fd, unsigned int peer_id, struct sockaddr *dst
>  {
>  	struct sk_packet *pkt, *tmp;
>  	struct cr_img *img;
> +	int noname_fd = -1;
>  	int fd, ret;
>  
>  	pr_info("Trying to restore recv queue for %u\n", peer_id);
>  
> -	if (restore_prepare_socket(queuer_fd) < 0)
> +	if (queuer_fd >=0 && restore_prepare_socket(queuer_fd) < 0)

Runaway from patch #9?

>  		return -1;
>  
>  	img = open_image(CR_FD_SK_QUEUES, O_RSTR);
> @@ -235,7 +236,14 @@ int __restore_sk_queue(int queuer_fd, unsigned int peer_id, struct sockaddr *dst
>  			goto err;
>  		}
>  
> -		fd = queuer_fd;
> +		if (queuer_fd >= 0)
> +			fd = queuer_fd;
> +		else
> +			fd = get_pkt_sender_fd(entry, &noname_fd);

Can we somehow relax this logic? The noname_fd is magically not-re-created by the
get_pkt_sender_fd() internals which makes it confusing to read this code. The fd
seems to leak.

> +		if (fd < 0) {
> +			ret = -1;
> +			goto err;
> +		}
>  		ret = sendto(fd, buf, entry->length, 0, dst_addr, dst_addrlen);
>  		xfree(buf);
>  		if (ret < 0) {
> @@ -252,6 +260,9 @@ int __restore_sk_queue(int queuer_fd, unsigned int peer_id, struct sockaddr *dst
>  		xfree(pkt);
>  	}
>  
> +	if (noname_fd >= 0)
> +		close(noname_fd);
> +
>  	close_image(img);
>  	return 0;
>  err:
> diff --git a/criu/sk-unix.c b/criu/sk-unix.c
> index b221a71..759a261 100644
> --- a/criu/sk-unix.c
> +++ b/criu/sk-unix.c
> @@ -907,6 +907,63 @@ static int prep_unix_sk_cwd(struct unix_sk_info *ui, int *prev_cwd_fd)
>  	return 0;
>  }
>  
> +static struct file_desc_ops unix_desc_ops;
> +
> +int get_pkt_sender_fd(SkPacketEntry *e, int *noname_fd)
> +{
> +	struct fdinfo_list_entry *fle;
> +	struct unix_sk_info *ui;
> +	u32 ino;
> +
> +	if (!e->has_sender_ino) {
> +		if (*noname_fd >= 0)
> +			return *noname_fd;
> +		*noname_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
> +		return *noname_fd;
> +	}
> +
> +	ino = e->sender_ino;
> +
> +	list_for_each_entry(fle, &rsti(current)->used, used_list) {
> +		if (fle->desc->ops != &unix_desc_ops)
> +			continue;
> +		ui = container_of(fle->desc, struct unix_sk_info, d);
> +		if (ui->ue->ino != ino || ui->ue->type != SOCK_DGRAM)
> +			continue;
> +		if (!list_empty(&ui->d.fd_info_head))
> +			futex_wait_while(&ui->prepared, 0);
> +		return fle->fe->fd;
> +	}
> +
> +	pr_err("Can't find sender fd: pid=%u, ino=%u\n", current->pid.virt, ino);
> +	return -1;
> +}
> +
> +static int restore_promisc_queue(struct unix_sk_info *ui, int fd)
> +{
> +	struct sockaddr_un addr;
> +	socklen_t addrlen;
> +	int cwd_fd = -1;
> +
> +	addrlen = ui->ue->name.len + sizeof(addr.sun_family);
> +	memcpy(&addr.sun_path, ui->ue->name.data, ui->ue->name.len);
> +	addr.sun_family = AF_UNIX;
> +
> +	if (prep_unix_sk_cwd(ui, &cwd_fd))
> +		return -1;
> +
> +	if (__restore_sk_queue(-1, ui->ue->id,
> +			       (struct sockaddr *)&addr, addrlen) < 0) {
> +		pr_err("Can't restore rcv queue\n");
> +		revert_unix_sk_cwd(&cwd_fd);
> +		return -1;
> +	}
> +
> +	revert_unix_sk_cwd(&cwd_fd);
> +
> +	return 0;
> +}
> +
>  static int post_open_unix_sk(struct file_desc *d, int fd)
>  {
>  	struct unix_sk_info *ui;
> @@ -918,6 +975,10 @@ static int post_open_unix_sk(struct file_desc *d, int fd)
>  	if (ui->flags & (USK_PAIR_MASTER | USK_PAIR_SLAVE))
>  		return 0;
>  
> +	if (ui->ue->type == SOCK_DGRAM && !ui->queuer && ui->ue->name.len &&
> +	    restore_promisc_queue(ui, fd))
> +		return -1;
> +
>  	peer = ui->peer;
>  
>  	if (peer == NULL)
> @@ -1177,7 +1238,7 @@ static int open_unixsk_standalone(struct unix_sk_info *ui)
>  
>  		close(sks[1]);
>  		sk = sks[0];
> -	} else if (ui->ue->type == SOCK_DGRAM && !ui->queuer) {
> +	} else if (ui->ue->type == SOCK_DGRAM && !ui->queuer && !ui->ue->name.len) {
>  		struct sockaddr_un addr;
>  		int sks[2];
>  
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
> .
>
Kirill Tkhai June 28, 2016, 1:21 p.m.
On 28.06.2016 15:27, Pavel Emelyanov wrote:
> On 06/16/2016 04:54 PM, Kirill Tkhai wrote:
>> In case of DGRAM, bound, queuerless sockets, restore
>> their queues using restore_promisc_queue().
>>
>> This function forces to use socket address, and
>> do not pass a queuer_fd. The last leads to that
>> fd, used for sendto(), will be choosen by get_pkt_sender_fd(),
>> and correct pkt msg name will be restored.
>>
>> v5: New
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  criu/include/sk-queue.h |    1 +
>>  criu/sk-queue.c         |   15 ++++++++++-
>>  criu/sk-unix.c          |   63 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 76 insertions(+), 3 deletions(-)
>>
>> diff --git a/criu/include/sk-queue.h b/criu/include/sk-queue.h
>> index a16edc6..78a5677 100644
>> --- a/criu/include/sk-queue.h
>> +++ b/criu/include/sk-queue.h
>> @@ -13,6 +13,7 @@ struct sk_packet {
>>  };
>>  
>>  extern struct list_head packets_list;
>> +extern int get_pkt_sender_fd(SkPacketEntry *entry, int *noname_fd);
>>  
>>  extern struct collect_image_info sk_queues_cinfo;
>>  extern int dump_sk_queue(int sock_fd, int sock_id, u64 (*get_sender)(const char *, int));
>> diff --git a/criu/sk-queue.c b/criu/sk-queue.c
>> index 9e9a484..d0e5862 100644
>> --- a/criu/sk-queue.c
>> +++ b/criu/sk-queue.c
>> @@ -192,11 +192,12 @@ int __restore_sk_queue(int queuer_fd, unsigned int peer_id, struct sockaddr *dst
>>  {
>>  	struct sk_packet *pkt, *tmp;
>>  	struct cr_img *img;
>> +	int noname_fd = -1;
>>  	int fd, ret;
>>  
>>  	pr_info("Trying to restore recv queue for %u\n", peer_id);
>>  
>> -	if (restore_prepare_socket(queuer_fd) < 0)
>> +	if (queuer_fd >=0 && restore_prepare_socket(queuer_fd) < 0)
> 
> Runaway from patch #9?

Patch #9 does not allow negative queuers.

> 
>>  		return -1;
>>  
>>  	img = open_image(CR_FD_SK_QUEUES, O_RSTR);
>> @@ -235,7 +236,14 @@ int __restore_sk_queue(int queuer_fd, unsigned int peer_id, struct sockaddr *dst
>>  			goto err;
>>  		}
>>  
>> -		fd = queuer_fd;
>> +		if (queuer_fd >= 0)
>> +			fd = queuer_fd;
>> +		else
>> +			fd = get_pkt_sender_fd(entry, &noname_fd);
> 
> Can we somehow relax this logic? The noname_fd is magically not-re-created by the
> get_pkt_sender_fd() internals which makes it confusing to read this code.

Ok, I'll try. Do you have an idea what is the best way?

> The fd seems to leak.

How does this happen? Have you seen "close(noname_fd)" below?

> 
>> +		if (fd < 0) {
>> +			ret = -1;
>> +			goto err;
>> +		}
>>  		ret = sendto(fd, buf, entry->length, 0, dst_addr, dst_addrlen);
>>  		xfree(buf);
>>  		if (ret < 0) {
>> @@ -252,6 +260,9 @@ int __restore_sk_queue(int queuer_fd, unsigned int peer_id, struct sockaddr *dst
>>  		xfree(pkt);
>>  	}
>>  
>> +	if (noname_fd >= 0)
>> +		close(noname_fd);
>> +
>>  	close_image(img);
>>  	return 0;
>>  err:
>> diff --git a/criu/sk-unix.c b/criu/sk-unix.c
>> index b221a71..759a261 100644
>> --- a/criu/sk-unix.c
>> +++ b/criu/sk-unix.c
>> @@ -907,6 +907,63 @@ static int prep_unix_sk_cwd(struct unix_sk_info *ui, int *prev_cwd_fd)
>>  	return 0;
>>  }
>>  
>> +static struct file_desc_ops unix_desc_ops;
>> +
>> +int get_pkt_sender_fd(SkPacketEntry *e, int *noname_fd)
>> +{
>> +	struct fdinfo_list_entry *fle;
>> +	struct unix_sk_info *ui;
>> +	u32 ino;
>> +
>> +	if (!e->has_sender_ino) {
>> +		if (*noname_fd >= 0)
>> +			return *noname_fd;
>> +		*noname_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
>> +		return *noname_fd;
>> +	}
>> +
>> +	ino = e->sender_ino;
>> +
>> +	list_for_each_entry(fle, &rsti(current)->used, used_list) {
>> +		if (fle->desc->ops != &unix_desc_ops)
>> +			continue;
>> +		ui = container_of(fle->desc, struct unix_sk_info, d);
>> +		if (ui->ue->ino != ino || ui->ue->type != SOCK_DGRAM)
>> +			continue;
>> +		if (!list_empty(&ui->d.fd_info_head))
>> +			futex_wait_while(&ui->prepared, 0);
>> +		return fle->fe->fd;
>> +	}
>> +
>> +	pr_err("Can't find sender fd: pid=%u, ino=%u\n", current->pid.virt, ino);
>> +	return -1;
>> +}
>> +
>> +static int restore_promisc_queue(struct unix_sk_info *ui, int fd)
>> +{
>> +	struct sockaddr_un addr;
>> +	socklen_t addrlen;
>> +	int cwd_fd = -1;
>> +
>> +	addrlen = ui->ue->name.len + sizeof(addr.sun_family);
>> +	memcpy(&addr.sun_path, ui->ue->name.data, ui->ue->name.len);
>> +	addr.sun_family = AF_UNIX;
>> +
>> +	if (prep_unix_sk_cwd(ui, &cwd_fd))
>> +		return -1;
>> +
>> +	if (__restore_sk_queue(-1, ui->ue->id,
>> +			       (struct sockaddr *)&addr, addrlen) < 0) {
>> +		pr_err("Can't restore rcv queue\n");
>> +		revert_unix_sk_cwd(&cwd_fd);
>> +		return -1;
>> +	}
>> +
>> +	revert_unix_sk_cwd(&cwd_fd);
>> +
>> +	return 0;
>> +}
>> +
>>  static int post_open_unix_sk(struct file_desc *d, int fd)
>>  {
>>  	struct unix_sk_info *ui;
>> @@ -918,6 +975,10 @@ static int post_open_unix_sk(struct file_desc *d, int fd)
>>  	if (ui->flags & (USK_PAIR_MASTER | USK_PAIR_SLAVE))
>>  		return 0;
>>  
>> +	if (ui->ue->type == SOCK_DGRAM && !ui->queuer && ui->ue->name.len &&
>> +	    restore_promisc_queue(ui, fd))
>> +		return -1;
>> +
>>  	peer = ui->peer;
>>  
>>  	if (peer == NULL)
>> @@ -1177,7 +1238,7 @@ static int open_unixsk_standalone(struct unix_sk_info *ui)
>>  
>>  		close(sks[1]);
>>  		sk = sks[0];
>> -	} else if (ui->ue->type == SOCK_DGRAM && !ui->queuer) {
>> +	} else if (ui->ue->type == SOCK_DGRAM && !ui->queuer && !ui->ue->name.len) {
>>  		struct sockaddr_un addr;
>>  		int sks[2];
>>  
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
>> .
>>
>