All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ip: set the close-on-exec flag for descriptors
@ 2013-06-04  8:01 Andrey Vagin
  2013-06-04 16:11 ` Stephen Hemminger
  0 siblings, 1 reply; 9+ messages in thread
From: Andrey Vagin @ 2013-06-04  8:01 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev, Andrey Vagin

Otherwise a program executed by "ip netns exec" has two extra
descriptors.

$ ip netns exec test /bin/bash
$ lsof -p $$
...
bash    817 root    0u   CHR  136,0       0t0          3 /dev/pts/0
bash    817 root    1u   CHR  136,0       0t0          3 /dev/pts/0
bash    817 root    2u   CHR  136,0       0t0          3 /dev/pts/0
bash    817 root    3u  sock    0,6       0t0      13386 protocol: NETLINK
bash    817 root    4r   REG    0,3         0 4026532155 net
bash    817 root  255u   CHR  136,0       0t0          3 /dev/pts/0

Cc: Stephen Hemminger <stephen@networkplumber.org>
Reported-by: Dilip Daya <dilip.daya@hp.com>
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 ip/ipnetns.c     | 2 +-
 lib/libnetlink.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index c9bc20a..fa2b681 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -150,7 +150,7 @@ static int netns_exec(int argc, char **argv)
 	name = argv[0];
 	cmd = argv[1];
 	snprintf(net_path, sizeof(net_path), "%s/%s", NETNS_RUN_DIR, name);
-	netns = open(net_path, O_RDONLY);
+	netns = open(net_path, O_RDONLY | O_CLOEXEC);
 	if (netns < 0) {
 		fprintf(stderr, "Cannot open network namespace \"%s\": %s\n",
 			name, strerror(errno));
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index b17e1aa..9e2a795 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -43,7 +43,7 @@ int rtnl_open_byproto(struct rtnl_handle *rth, unsigned subscriptions,
 
 	memset(rth, 0, sizeof(*rth));
 
-	rth->fd = socket(AF_NETLINK, SOCK_RAW, protocol);
+	rth->fd = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, protocol);
 	if (rth->fd < 0) {
 		perror("Cannot open netlink socket");
 		return -1;
-- 
1.8.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] ip: set the close-on-exec flag for descriptors
  2013-06-04  8:01 [PATCH] ip: set the close-on-exec flag for descriptors Andrey Vagin
@ 2013-06-04 16:11 ` Stephen Hemminger
  2013-07-08 14:17   ` Eric Dumazet
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2013-06-04 16:11 UTC (permalink / raw)
  To: Andrey Vagin; +Cc: netdev

On Tue,  4 Jun 2013 12:01:14 +0400
Andrey Vagin <avagin@openvz.org> wrote:

> Otherwise a program executed by "ip netns exec" has two extra
> descriptors.
> 
> $ ip netns exec test /bin/bash
> $ lsof -p $$
> ...
> bash    817 root    0u   CHR  136,0       0t0          3 /dev/pts/0
> bash    817 root    1u   CHR  136,0       0t0          3 /dev/pts/0
> bash    817 root    2u   CHR  136,0       0t0          3 /dev/pts/0
> bash    817 root    3u  sock    0,6       0t0      13386 protocol: NETLINK
> bash    817 root    4r   REG    0,3         0 4026532155 net
> bash    817 root  255u   CHR  136,0       0t0          3 /dev/pts/0

Applied

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] ip: set the close-on-exec flag for descriptors
  2013-06-04 16:11 ` Stephen Hemminger
@ 2013-07-08 14:17   ` Eric Dumazet
  2013-07-08 15:51     ` Andrey Wagin
  2013-07-08 19:57     ` Stephen Hemminger
  0 siblings, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2013-07-08 14:17 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Andrey Vagin, netdev

On Tue, 2013-06-04 at 09:11 -0700, Stephen Hemminger wrote:
> On Tue,  4 Jun 2013 12:01:14 +0400
> Andrey Vagin <avagin@openvz.org> wrote:
> 
> > Otherwise a program executed by "ip netns exec" has two extra
> > descriptors.
> > 
> > $ ip netns exec test /bin/bash
> > $ lsof -p $$
> > ...
> > bash    817 root    0u   CHR  136,0       0t0          3 /dev/pts/0
> > bash    817 root    1u   CHR  136,0       0t0          3 /dev/pts/0
> > bash    817 root    2u   CHR  136,0       0t0          3 /dev/pts/0
> > bash    817 root    3u  sock    0,6       0t0      13386 protocol: NETLINK
> > bash    817 root    4r   REG    0,3         0 4026532155 net
> > bash    817 root  255u   CHR  136,0       0t0          3 /dev/pts/0
> 
> Applied

It seems it could break ip command for old kernels, as SOCK_CLOEXEC is
supported from 2.6.27

Not sure if its worth a patch, to do the normal socket() call followed
by legacy fcntl() one.

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] ip: set the close-on-exec flag for descriptors
  2013-07-08 14:17   ` Eric Dumazet
@ 2013-07-08 15:51     ` Andrey Wagin
  2013-07-08 16:05       ` Eric Dumazet
  2013-07-08 19:57     ` Stephen Hemminger
  1 sibling, 1 reply; 9+ messages in thread
From: Andrey Wagin @ 2013-07-08 15:51 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Stephen Hemminger, netdev

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

Hi Eric,

2013/7/8 Eric Dumazet <eric.dumazet@gmail.com>:
>> Applied
>
> It seems it could break ip command for old kernels, as SOCK_CLOEXEC is
> supported from 2.6.27
>
> Not sure if its worth a patch, to do the normal socket() call followed
> by legacy fcntl() one.

fcntl is not safe for multithread applications. If libnetlink isn't
going to be used for them, I don't have objection. The patch is
attached.

Thanks.

[-- Attachment #2: 0001-ip-avoid-using-SOCK_CLOEXEC-to-save-backward-compati.patch --]
[-- Type: application/octet-stream, Size: 1277 bytes --]

From 970d24097323f5d316cf160f3bc711d5f740db36 Mon Sep 17 00:00:00 2001
From: Andrey Vagin <avagin@openvz.org>
Date: Mon, 8 Jul 2013 19:08:36 +0400
Subject: [PATCH] ip: avoid using SOCK_CLOEXEC to save backward compatibility

Eric Dumazet wrote, that SOCK_CLOEXEC could break ip command for old
kernels, as SOCK_CLOEXEC is supported from 2.6.27.

BTW: fcntl(SOCK_CLOEXEC) is not safe for multithread applications.

Reported-by: <eric.dumazet@gmail.com>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 lib/libnetlink.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 9e2a795..992aabd 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -43,12 +43,17 @@ int rtnl_open_byproto(struct rtnl_handle *rth, unsigned subscriptions,
 
 	memset(rth, 0, sizeof(*rth));
 
-	rth->fd = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, protocol);
+	rth->fd = socket(AF_NETLINK, SOCK_RAW, protocol);
 	if (rth->fd < 0) {
 		perror("Cannot open netlink socket");
 		return -1;
 	}
 
+	if (fcntl(rth->fd, F_SETFD, FD_CLOEXEC)) {
+		perror("Cannot set the close-on-exec flag");
+		return -1;
+	}
+
 	if (setsockopt(rth->fd,SOL_SOCKET,SO_SNDBUF,&sndbuf,sizeof(sndbuf)) < 0) {
 		perror("SO_SNDBUF");
 		return -1;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] ip: set the close-on-exec flag for descriptors
  2013-07-08 15:51     ` Andrey Wagin
@ 2013-07-08 16:05       ` Eric Dumazet
  2013-07-08 19:23         ` Andrey Wagin
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2013-07-08 16:05 UTC (permalink / raw)
  To: Andrey Wagin; +Cc: Stephen Hemminger, netdev

On Mon, 2013-07-08 at 19:51 +0400, Andrey Wagin wrote:
> Hi Eric,
> 
> 2013/7/8 Eric Dumazet <eric.dumazet@gmail.com>:
> >> Applied
> >
> > It seems it could break ip command for old kernels, as SOCK_CLOEXEC is
> > supported from 2.6.27
> >
> > Not sure if its worth a patch, to do the normal socket() call followed
> > by legacy fcntl() one.
> 
> fcntl is not safe for multithread applications. If libnetlink isn't
> going to be used for them, I don't have objection. The patch is
> attached.
> 

If you want multithread safety, you could attempt the SOCK_CLOEXEC, and
fall back in case of error to legacy socket()/fcntl()

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] ip: set the close-on-exec flag for descriptors
  2013-07-08 16:05       ` Eric Dumazet
@ 2013-07-08 19:23         ` Andrey Wagin
  2013-07-08 19:50           ` Stephen Hemminger
  0 siblings, 1 reply; 9+ messages in thread
From: Andrey Wagin @ 2013-07-08 19:23 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Stephen Hemminger, netdev

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

2013/7/8 Eric Dumazet <eric.dumazet@gmail.com>:
> On Mon, 2013-07-08 at 19:51 +0400, Andrey Wagin wrote:
>> Hi Eric,
>>
>> 2013/7/8 Eric Dumazet <eric.dumazet@gmail.com>:
>> >> Applied
>> >
>> > It seems it could break ip command for old kernels, as SOCK_CLOEXEC is
>> > supported from 2.6.27
>> >
>> > Not sure if its worth a patch, to do the normal socket() call followed
>> > by legacy fcntl() one.
>>
>> fcntl is not safe for multithread applications. If libnetlink isn't
>> going to be used for them, I don't have objection. The patch is
>> attached.
>>
>
> If you want multithread safety, you could attempt the SOCK_CLOEXEC, and
> fall back in case of error to legacy socket()/fcntl()
>

Yes, it's good idea. Look at the attached patch.

Thanks.

>
>

[-- Attachment #2: 0001-ip-try-to-create-socket-w-o-SOCK_CLOEXEC-if-previous.patch --]
[-- Type: application/octet-stream, Size: 1389 bytes --]

From 9e4bc17d457aded05c0cbb9c58285f15f4f1c406 Mon Sep 17 00:00:00 2001
From: Andrey Vagin <avagin@openvz.org>
Date: Mon, 8 Jul 2013 19:08:36 +0400
Subject: [PATCH] ip: try to create socket w/o SOCK_CLOEXEC, if previous
 attempt failed

SOCK_CLOEXEC is supported from 2.6.27

Create socket with SOCK_CLOEXEC, if this operation failed with EINVAL,
try to create socket w/o SOCK_CLOEXEC and set SOCK_CLOEXEC with help
fcntl.

The second way is not safe for multithreaded application, but it's
better than don't close fd at all.

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 lib/libnetlink.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 9e2a795..48a4da2 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -44,6 +44,16 @@ int rtnl_open_byproto(struct rtnl_handle *rth, unsigned subscriptions,
 	memset(rth, 0, sizeof(*rth));
 
 	rth->fd = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, protocol);
+	if (rth->fd < 0 && errno == EINVAL) {
+		/* SOCK_CLOEXEC is supported from 2.6.27 */
+		rth->fd = socket(AF_NETLINK, SOCK_RAW, protocol);
+		if (rth->fd >= 0)
+			if (fcntl(rth->fd, F_SETFD, FD_CLOEXEC)) {
+				perror("Cannot set the close-on-exec flag");
+				return -1;
+			}
+	}
+
 	if (rth->fd < 0) {
 		perror("Cannot open netlink socket");
 		return -1;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] ip: set the close-on-exec flag for descriptors
  2013-07-08 19:23         ` Andrey Wagin
@ 2013-07-08 19:50           ` Stephen Hemminger
  0 siblings, 0 replies; 9+ messages in thread
From: Stephen Hemminger @ 2013-07-08 19:50 UTC (permalink / raw)
  To: Andrey Wagin; +Cc: Eric Dumazet, netdev

On Mon, 8 Jul 2013 23:23:35 +0400
Andrey Wagin <avagin@gmail.com> wrote:

> 2013/7/8 Eric Dumazet <eric.dumazet@gmail.com>:
> > On Mon, 2013-07-08 at 19:51 +0400, Andrey Wagin wrote:
> >> Hi Eric,
> >>
> >> 2013/7/8 Eric Dumazet <eric.dumazet@gmail.com>:
> >> >> Applied
> >> >
> >> > It seems it could break ip command for old kernels, as SOCK_CLOEXEC is
> >> > supported from 2.6.27
> >> >
> >> > Not sure if its worth a patch, to do the normal socket() call followed
> >> > by legacy fcntl() one.
> >>
> >> fcntl is not safe for multithread applications. If libnetlink isn't
> >> going to be used for them, I don't have objection. The patch is
> >> attached.
> >>
> >
> > If you want multithread safety, you could attempt the SOCK_CLOEXEC, and
> > fall back in case of error to legacy socket()/fcntl()
> >
> 
> Yes, it's good idea. Look at the attached patch.
> 
> Thanks.
> 
> >
> >

Just always do the fcntl(), it makes more sense rather than having
two code paths.

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] ip: set the close-on-exec flag for descriptors
  2013-07-08 14:17   ` Eric Dumazet
  2013-07-08 15:51     ` Andrey Wagin
@ 2013-07-08 19:57     ` Stephen Hemminger
  2013-07-08 20:00       ` Eric Dumazet
  1 sibling, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2013-07-08 19:57 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Andrey Vagin, netdev

On Mon, 08 Jul 2013 07:17:16 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Tue, 2013-06-04 at 09:11 -0700, Stephen Hemminger wrote:
> > On Tue,  4 Jun 2013 12:01:14 +0400
> > Andrey Vagin <avagin@openvz.org> wrote:
> > 
> > > Otherwise a program executed by "ip netns exec" has two extra
> > > descriptors.
> > > 
> > > $ ip netns exec test /bin/bash
> > > $ lsof -p $$
> > > ...
> > > bash    817 root    0u   CHR  136,0       0t0          3 /dev/pts/0
> > > bash    817 root    1u   CHR  136,0       0t0          3 /dev/pts/0
> > > bash    817 root    2u   CHR  136,0       0t0          3 /dev/pts/0
> > > bash    817 root    3u  sock    0,6       0t0      13386 protocol: NETLINK
> > > bash    817 root    4r   REG    0,3         0 4026532155 net
> > > bash    817 root  255u   CHR  136,0       0t0          3 /dev/pts/0
> > 
> > Applied
> 
> It seems it could break ip command for old kernels, as SOCK_CLOEXEC is
> supported from 2.6.27
> 
> Not sure if its worth a patch, to do the normal socket() call followed
> by legacy fcntl() one.


Nevermind, original patch is fine since the set namespace (NR_setns) system
call used did not show up into 3.0!

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] ip: set the close-on-exec flag for descriptors
  2013-07-08 19:57     ` Stephen Hemminger
@ 2013-07-08 20:00       ` Eric Dumazet
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Dumazet @ 2013-07-08 20:00 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Andrey Vagin, netdev

On Mon, 2013-07-08 at 12:57 -0700, Stephen Hemminger wrote:

> Nevermind, original patch is fine since the set namespace (NR_setns) system
> call used did not show up into 3.0!

I was concerned by the ip batch facility, not the netns.

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-07-08 20:00 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-04  8:01 [PATCH] ip: set the close-on-exec flag for descriptors Andrey Vagin
2013-06-04 16:11 ` Stephen Hemminger
2013-07-08 14:17   ` Eric Dumazet
2013-07-08 15:51     ` Andrey Wagin
2013-07-08 16:05       ` Eric Dumazet
2013-07-08 19:23         ` Andrey Wagin
2013-07-08 19:50           ` Stephen Hemminger
2013-07-08 19:57     ` Stephen Hemminger
2013-07-08 20:00       ` Eric Dumazet

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.