compel: check whether a parasite socket is prepared each time

Submitted by Andrei Vagin on Nov. 17, 2016, 6:45 a.m.

Details

Message ID 1479365131-14065-1-git-send-email-avagin@openvz.org
State Superseded
Series "compel: check whether a parasite socket is prepared each time"
Headers show

Commit Message

Andrei Vagin Nov. 17, 2016, 6:45 a.m.
From: Andrei Vagin <avagin@virtuozzo.com>

Currently we prepare a parasite socket only once and
save it in a static variable.

It's bad idea to use a static variable in a library.

In addition, it doesn't work if we have processes in
different network namespaces. In this case, we have to have
a separate socket for each namespace.

Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
---
 compel/src/lib/infect.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
index 0d462d8..af8555e 100644
--- a/compel/src/lib/infect.c
+++ b/compel/src/lib/infect.c
@@ -322,20 +322,27 @@  static int gen_parasite_saddr(struct sockaddr_un *saddr, int key)
 static int prepare_tsock(struct parasite_ctl *ctl, pid_t pid,
 		struct parasite_init_args *args)
 {
-	static int ssock = -1;
+	int ssock = -1;
+	socklen_t sk_len;
+	struct sockaddr_un addr;
 
 	pr_info("Putting tsock into pid %d\n", pid);
 	args->h_addr_len = gen_parasite_saddr(&args->h_addr, getpid());
 
-	if (ssock == -1) {
-		ssock = *ctl->ictx.p_sock;
+	ssock = *ctl->ictx.p_sock;
+	sk_len = sizeof(addr);
+
+	if (getsockname(ssock, &addr, &sk_len) < 0) {
+		pr_perror("Unable to get name for a socket");
+		return -1;
+	}
+
+	if (sk_len == sizeof(addr.sun_family)) {
 		if (ssock == -1) {
 			pr_err("No socket in ictx\n");
 			goto err;
 		}
 
-		*ctl->ictx.p_sock = -1;
-
 		if (bind(ssock, (struct sockaddr *)&args->h_addr, args->h_addr_len) < 0) {
 			pr_perror("Can't bind socket");
 			goto err;

Comments

Cyrill Gorcunov Nov. 17, 2016, 8 a.m.
On Thu, Nov 17, 2016 at 09:45:31AM +0300, Andrei Vagin wrote:
> From: Andrei Vagin <avagin@virtuozzo.com>
> 
> Currently we prepare a parasite socket only once and
> save it in a static variable.
> 
> It's bad idea to use a static variable in a library.
> 
> In addition, it doesn't work if we have processes in
> different network namespaces. In this case, we have to have
> a separate socket for each namespace.
> 
> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
Pavel Emelianov Nov. 17, 2016, 8:16 a.m.
On 11/17/2016 10:42 AM, Patchwork wrote:
> == Series Details ==
> 
> Series: compel: check whether a parasite socket is prepared each time
> URL   : https://patchwork.criu.org/series/884/
> State : failure
> 
> == Logs ==
> 
> For more details see: https://travis-ci.org/criupatchwork/criu/builds/176615305
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
> .


Alpine build failed.


compel/src/lib/infect.c: In function 'prepare_tsock':
compel/src/lib/infect.c:335:25: error: passing argument 2 of 'getsockname' from incompatible pointer type [-Werror=incompatible-pointer-types]
  if (getsockname(ssock, &addr, &sk_len) < 0) {
                         ^
In file included from /usr/include/fortify/sys/socket.h:20:0,
                 from compel/include/uapi/compel/infect-rpc.h:4,
                 from compel/include/uapi/compel/compel.h:11,
                 from compel/include/log.h:4,
                 from compel/src/lib/infect.c:12:
/usr/include/sys/socket.h:304:5: note: expected 'struct sockaddr * restrict' but argument is of type 'struct sockaddr_un *'
 int getsockname (int, struct sockaddr *__restrict, socklen_t *__restrict);
Pavel Emelianov Nov. 17, 2016, 8:19 a.m.
On 11/17/2016 09:45 AM, Andrei Vagin wrote:
> From: Andrei Vagin <avagin@virtuozzo.com>
> 
> Currently we prepare a parasite socket only once and
> save it in a static variable.
> 
> It's bad idea to use a static variable in a library.

Agreed.

> In addition, it doesn't work if we have processes in
> different network namespaces. In this case, we have to have
> a separate socket for each namespace.

Also agreed.

> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> ---
>  compel/src/lib/infect.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
> index 0d462d8..af8555e 100644
> --- a/compel/src/lib/infect.c
> +++ b/compel/src/lib/infect.c
> @@ -322,20 +322,27 @@ static int gen_parasite_saddr(struct sockaddr_un *saddr, int key)
>  static int prepare_tsock(struct parasite_ctl *ctl, pid_t pid,
>  		struct parasite_init_args *args)
>  {
> -	static int ssock = -1;
> +	int ssock = -1;
> +	socklen_t sk_len;
> +	struct sockaddr_un addr;
>  
>  	pr_info("Putting tsock into pid %d\n", pid);
>  	args->h_addr_len = gen_parasite_saddr(&args->h_addr, getpid());
>  
> -	if (ssock == -1) {
> -		ssock = *ctl->ictx.p_sock;
> +	ssock = *ctl->ictx.p_sock;
> +	sk_len = sizeof(addr);
> +
> +	if (getsockname(ssock, &addr, &sk_len) < 0) {
> +		pr_perror("Unable to get name for a socket");
> +		return -1;
> +	}
> +
> +	if (sk_len == sizeof(addr.sun_family)) {

The kernel's unix_getname places here sizeof(short). Maybe we do the
same here, to bee 100% explicit?

>  		if (ssock == -1) {
>  			pr_err("No socket in ictx\n");
>  			goto err;
>  		}
>  
> -		*ctl->ictx.p_sock = -1;
> -
>  		if (bind(ssock, (struct sockaddr *)&args->h_addr, args->h_addr_len) < 0) {
>  			pr_perror("Can't bind socket");
>  			goto err;
>
Pavel Emelianov Nov. 17, 2016, 8:20 a.m.
On 11/17/2016 09:45 AM, Andrei Vagin wrote:
> From: Andrei Vagin <avagin@virtuozzo.com>
> 
> Currently we prepare a parasite socket only once and
> save it in a static variable.
> 
> It's bad idea to use a static variable in a library.
> 
> In addition, it doesn't work if we have processes in
> different network namespaces. In this case, we have to have
> a separate socket for each namespace.
> 
> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> ---
>  compel/src/lib/infect.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
> index 0d462d8..af8555e 100644
> --- a/compel/src/lib/infect.c
> +++ b/compel/src/lib/infect.c
> @@ -322,20 +322,27 @@ static int gen_parasite_saddr(struct sockaddr_un *saddr, int key)
>  static int prepare_tsock(struct parasite_ctl *ctl, pid_t pid,
>  		struct parasite_init_args *args)
>  {
> -	static int ssock = -1;
> +	int ssock = -1;
> +	socklen_t sk_len;
> +	struct sockaddr_un addr;
>  
>  	pr_info("Putting tsock into pid %d\n", pid);
>  	args->h_addr_len = gen_parasite_saddr(&args->h_addr, getpid());
>  
> -	if (ssock == -1) {
> -		ssock = *ctl->ictx.p_sock;
> +	ssock = *ctl->ictx.p_sock;
> +	sk_len = sizeof(addr);
> +
> +	if (getsockname(ssock, &addr, &sk_len) < 0) {
> +		pr_perror("Unable to get name for a socket");
> +		return -1;
> +	}
> +
> +	if (sk_len == sizeof(addr.sun_family)) {
>  		if (ssock == -1) {
>  			pr_err("No socket in ictx\n");
>  			goto err;
>  		}
>  
> -		*ctl->ictx.p_sock = -1;

BTW, since we don't erase this thing any longer, would you change
the ictx.p_sock from pointer to just int with the name .sock?

-- Pavel

> -
>  		if (bind(ssock, (struct sockaddr *)&args->h_addr, args->h_addr_len) < 0) {
>  			pr_perror("Can't bind socket");
>  			goto err;
>