paccept, socket, socketpair w/flags

This version of the patch hopefully integrates all of the requested changes:

- the sock_map_fd interface is changed and all in-tree users changed. No additional function anymore

- there is a helper function to convert the new socket flags into file flags. Shared by all three functions

- the new accept interface is now paccept() which adds the flag parameter as well as a signal mask.

- not changed from the last version: the paccet() function takes a flags parameter with the same value as the ORed flags for socket() and socketpair(). Seems cleaner.

I tried to write the paccept() code as similar as possible to the pselect() code. It seems to work. Somebody might want to look over it, though.

Unless somebody has more interface requests I'd hope this version is ready for integration.

As usual, a test program is included. Adjust for archs != x86/x86-64.

#include <fcntl.h>
#include <pthread.h>
#include <signal.h>
#include <stdio.h>
#include <unistd.h>
#include <netinet/in.h>
#include <sys/socket.h>
#include <sys/syscall.h>

#define SOCK_CLOEXEC 0x40000000

#ifdef __x86_64__
#define __NR_paccept 288
#elif __i386__
#define SYS_PACCEPT 18
#define USE_SOCKETCALL 1
#else
#error "define syscall numbers for this architecture" #endif

#define PORT 57392

static pthread_barrier_t b;
static int status;

static void *
tf (void *arg)
{
pthread_barrier_wait (&b);
int s = socket (AF_INET, SOCK_STREAM, 0);
struct sockaddr_in sin;
sin.sin_family = AF_INET;
sin.sin_addr.s_addr = htonl (INADDR_LOOPBACK); sin.sin_port = htons (PORT);
connect (s, (const struct sockaddr *) &sin, sizeof (sin)); close (s);
pthread_barrier_wait (&b);

pthread_barrier_wait (&b);
s = socket (AF_INET, SOCK_STREAM, 0); sin.sin_family = AF_INET;
sin.sin_addr.s_addr = htonl (INADDR_LOOPBACK); sin.sin_port = htons (PORT + 1);
connect (s, (const struct sockaddr *) &sin, sizeof (sin)); close (s);
return NULL;
}

static void
h(int sig)
{
_exit(status);
}

int
main (void)
{
int s;
int s2;
int sp[2];

  s = socket (PF_UNIX, SOCK_STREAM, 0);

if (s < 0)

    {
      puts ("socket failed");
      status = 1;
    }
  else
    {
      int fl = fcntl (s, F_GETFD);
      if ((fl & FD_CLOEXEC) != 0)
	{
	  puts ("socket did set CLOEXEC");
	  status = 1;
	}

      close (s);
    }

  s = socket (PF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0);

if (s < 0)

    {
      puts ("socket(SOCK_CLOEXEC) failed");
      status = 1;
    }
  else
    {
      int fl = fcntl (s, F_GETFD);
      if ((fl & FD_CLOEXEC) == 0)
	{
	  puts ("socket(SOCK_CLOEXEC) did not set CLOEXEC");
	  status = 1;
	}

      close (s);
    }

if (socketpair (PF_UNIX, SOCK_STREAM, 0, sp) < 0)

    {
      puts ("socketpair failed");
      status = 1;
    }
  else
    {
      int fl1 = fcntl (sp[0], F_GETFD);
      int fl2 = fcntl (sp[1], F_GETFD);
      if ((fl1 & FD_CLOEXEC) != 0 || (fl2 & FD_CLOEXEC) != 0)
	{
	  puts ("socketpair did set CLOEXEC");
	  status = 1;
	}

      close (sp[0]);
      close (sp[1]);
    }

if (socketpair (PF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sp) < 0)

    {
      puts ("socketpair(SOCK_CLOEXEC) failed");
      status = 1;
    }
  else
    {
      int fl1 = fcntl (sp[0], F_GETFD);
      int fl2 = fcntl (sp[1], F_GETFD);
      if ((fl1 & FD_CLOEXEC) == 0 || (fl2 & FD_CLOEXEC) == 0)
	{
	  puts ("socketpair(SOCK_CLOEXEC) did not set CLOEXEC");
	  status = 1;
	}

      close (sp[0]);
      close (sp[1]);
    }

  pthread_barrier_init (&b, NULL, 2);
  
  struct sockaddr_in sin;
  pthread_t th;
  if (pthread_create (&th, NULL, tf, NULL) != 0)
    {
      puts ("pthread_create failed");
      status = 1;
    }
  else
    {
      int s = socket (AF_INET, SOCK_STREAM, 0);
      sin.sin_family = AF_INET;
      sin.sin_addr.s_addr = htonl (INADDR_LOOPBACK);
      sin.sin_port = htons (PORT);
      bind (s, (struct sockaddr *) &sin, sizeof (sin));
      listen (s, SOMAXCONN);

      pthread_barrier_wait (&b);

      s2 = accept (s, NULL, 0);
      if (s2 < 0)
	{
	  puts ("accept failed");
	  status = 1;
	}
      else
	{
	  int fl = fcntl (s2, F_GETFD);
	  if ((fl & FD_CLOEXEC) != 0)
	    {
	      puts ("accept did set CLOEXEC");
	      status = 1;
	    }

	  close (s2);
        }

      close (s);

      pthread_barrier_wait (&b);

      sin.sin_family = AF_INET;
      sin.sin_addr.s_addr = htonl (INADDR_LOOPBACK);
      sin.sin_port = htons (PORT + 1);
      s = socket (AF_INET, SOCK_STREAM, 0);
      bind (s, (struct sockaddr *) &sin, sizeof (sin));
      listen (s, SOMAXCONN);

      pthread_barrier_wait (&b);

#if USE_SOCKETCALL
      s2 = syscall (__NR_socketcall, SYS_PACCEPT, s, NULL, 0, NULL, 0, SOCK_CLOEXEC);
#else
      s2 = syscall (__NR_paccept, s, NULL, 0, NULL, 0, SOCK_CLOEXEC);
#endif
      if (s2 < 0)
	{
	  puts ("paccept failed");
	  status = 1;
	}
      else
	{
	  int fl = fcntl (s2, F_GETFD);
	  if ((fl & FD_CLOEXEC) == 0)
	    {
	      puts ("paccept did not set CLOEXEC");
	      status = 1;
	    }

	  close (s2);
        }

      close (s);
    }

  sigblock(sigmask(SIGALRM));

  struct sigaction sa;
  sa.sa_handler = h;
  sigemptyset(&sa.sa_mask);
  sa.sa_flags = 0;
  sigaction(SIGALRM, &sa, NULL);

  sigset_t ss;
  sigprocmask(SIG_SETMASK, NULL, &ss);
  sigdelset(&ss, SIGALRM);
  alarm(2);

  sin.sin_family = AF_INET;
  sin.sin_addr.s_addr = htonl (INADDR_LOOPBACK);
  sin.sin_port = htons (PORT + 2);
  s = socket (AF_INET, SOCK_STREAM, 0);
  bind (s, (struct sockaddr *) &sin, sizeof (sin));
  listen (s, SOMAXCONN);

#if USE_SOCKETCALL
s2 = syscall (__NR_socketcall, SYS_PACCEPT, s, NULL, 0, &ss, 8, 0);
#else
s2 = syscall (__NR_paccept, s, NULL, 0, &ss, 8, 0);
#endif

// Should not be reached.
printf ("paccept returned with %d\n", s2);
return 1;
}

 include/asm-x86/unistd_64.h |    2 
 include/linux/net.h         |    8 +++
 include/linux/syscalls.h    |    2 
 net/9p/trans_fd.c           |    2 
 net/compat.c                |   52 ++++++++++++++++++++--
 net/sctp/socket.c           |    2 
 net/socket.c                |  102 +++++++++++++++++++++++++++++++++++++-------
 7 files changed, 148 insertions(+), 22 deletions(-)

index fe26e36..d751135 100644
@@ -639,6 +639,8 @@ __SYSCALL(__NR_fallocate, sys_fallocate)
 __SYSCALL(__NR_timerfd_settime, sys_timerfd_settime)
 #define __NR_timerfd_gettime			287
 __SYSCALL(__NR_timerfd_gettime, sys_timerfd_gettime)
+#define __NR_paccept				288
+__SYSCALL(__NR_paccept, sys_paccept)
 
 
 #ifndef __NO_STUBS

index 71f7dd5..5110e1b 100644
@@ -46,6 +46,7 @@ struct net;
 #define SYS_GETSOCKOPT	15		/* sys_getsockopt(2)		*/
 #define SYS_SENDMSG	16		/* sys_sendmsg(2)		*/
 #define SYS_RECVMSG	17		/* sys_recvmsg(2)		*/
+#define SYS_PACCEPT	18		/* sys_paccept(2)		*/
 
 typedef enum {
 	SS_FREE = 0,			/* not allocated		*/

@@ -91,6 +92,9 @@ enum sock_type {
 	SOCK_SEQPACKET	= 5,
 	SOCK_DCCP	= 6,
 	SOCK_PACKET	= 10,
+
+	/* Flag values, ORed to the types above.  */
+	SOCK_CLOEXEC	= 0x40000000
 };
 
 #define SOCK_MAX (SOCK_PACKET + 1)

@@ -208,10 +212,12 @@ extern int   	     sock_sendmsg(struct socket *sock, struct msghdr *msg,
 				  size_t len);
 extern int	     sock_recvmsg(struct socket *sock, struct msghdr *msg,
 				  size_t size, int flags);
-extern int 	     sock_map_fd(struct socket *sock);
+extern int 	     sock_map_fd(struct socket *sock, int flags);
 extern struct socket *sockfd_lookup(int fd, int *err);
 #define		     sockfd_put(sock) fput(sock->file)
 extern int	     net_ratelimit(void);
+extern long	     do_accept(int fd, struct sockaddr __user *upeer_sockaddr,
+			       int __user *upeer_addrlen, int flags);
 
 #define net_random()		random32()
 #define net_srandom(seed)	srandom32((__force u32)seed)

index 8df6d13..14ba94c 100644
@@ -407,6 +407,8 @@ asmlinkage long sys_getsockopt(int fd, int level, int optname,
 asmlinkage long sys_bind(int, struct sockaddr __user *, int);
 asmlinkage long sys_connect(int, struct sockaddr __user *, int);
 asmlinkage long sys_accept(int, struct sockaddr __user *, int __user *);
+asmlinkage long sys_paccept(int, struct sockaddr __user *, int __user *,
+			    const sigset_t *, size_t, int);
 asmlinkage long sys_getsockname(int, struct sockaddr __user *, int __user *);
 asmlinkage long sys_getpeername(int, struct sockaddr __user *, int __user *);
 asmlinkage long sys_send(int, void __user *, size_t, unsigned);

index f624dff..25931c0 100644
@@ -1173,7 +1173,7 @@ static int p9_socket_open(struct p9_trans *trans, struct socket *csocket)
 	int fd, ret;
 
 	csocket->sk->sk_allocation = GFP_NOIO;
-	fd = sock_map_fd(csocket);
+	fd = sock_map_fd(csocket, 0);
 	if (fd < 0) {
 		P9_EPRINTK(KERN_ERR, "p9_socket_open: failed to map fd\n");
 		return fd;

index e7e3baf..5034b1d 100644
@@ -3812,7 +3812,7 @@ static int sctp_getsockopt_peeloff(struct sock *sk, int len, char __user *optval
 		goto out;
 
 	/* Map the socket to an unused fd that can be returned to the user.  */
-	retval = sock_map_fd(newsock);
+	retval = sock_map_fd(newsock, 0);
 	if (retval < 0) {
 		sock_release(newsock);
 		goto out;

[CC+=Alan Cox]

Ulrich,

This is ugly. Why invent a diffent set of flags here. I agree with your earlier statement that new syscalls would be cleaner. If, as seems to be the case, we are going to create new syscalls for

eventfd()
signalfd()
accept()
dup2()
epoll_create()
pipe()
inotify_init()
(I've not seen those last two yet, but I assume you are going to do them.)

then *please* let's go the hwole way cleanly, and have new syscalls also for socketpair() and socket(), and make all of the new syscalls use the same flags. Creating a different set of flags just to avoid a couple of extra sycalls is ugly. (You yourself asserted similar in saying that dup3() is better than adding an extra flag to fcntl(), and I don't disagree.)

Cheers,

Michael

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Don't lay words in my mouth. I do think that different flags are needed. The name of the flag must indicate what it is for and you don't want to mix flags with different prefixes for the same flags parameter.
Not introducing separate flags would mean all the functions would have
to accept the same set of flags which will sooner or later create problems. I really don't see the problem here.

Hell, no, that's worse than everything else proposed. We don't have the luxury to have a separate parameter to indicate close-on-exec or not. For efficiency it has to be a multi-purpose flags parameter and the flags each syscall takes are different since the functionality is different.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖ -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)

iD8DBQFIFduL2ijCOnn/RHQRAhV5AKDK924ANx9HO5qDbPyB6m4uegbABACgpkFA AOO7/HaCFNe/GNmNlEJXBag=
=18Bc
-----END PGP SIGNATURE-----

Okay -- to some extent I seem to have misunderstood you. I'll step back a little and start again.

You have said it would be cleaner to have new syscalls for socket() and socketpair(). I understand your reasoning for that to be that doing so is cleaner than overloading the type argument of the existing versions of these syscalls. I agree. So, since you are adding new syscalls for all of the other interfaces, why not do it for these two syscalls also?

Okay -- I see your point (and I didn't read your patches closely enough). However, the rationale for the implementations proposed to date isn't very clear. So far we have the following new flags:

timerfd_create() -- TFD_CLOEXEC
signalfd4() -- SFD_CLOEXEC
eventfd2() -- EFD_CLOEXEC
epoll_create2() -- EPOLL_CLOEXEC
open() (and openat()), dup3() -- all use O_CLOEXEC socketpair(), socket(), paccept -- all use SOCK_CLOEXEC pipe2() -- to be determined
inotify_init2() -- to be determined

It would help if you laid out your reasoning for creating distinct flags for some syscalls but not others. For example, is the assumption here that socketpair(), socket(), and paccept() will always use the same set of flags? If so, what makes that group special with respect to your arguments about the need for separate flags for each syscall?

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

If the range of the existing parameter can indeed be restricted as needed for the flags it is the better approach for programmers since all that is needed for a programmer to do is to write

   fd = socket(PF_INET, SOCK_STREAM|SOCK_CLOEXEC, 0);
   if (fd == -1 && errno == EINVAL)
     {
       fd = socket(PF_INET, SOCK_STREAM, 0);
       if (fd != -1)
         fcntl(fd, F_SETFD, FD_CLOEXEC);
     }

No autoconf code needed to detect the presence of a new function. You can even imagine that convenience libraries will do the above automatically (not libc, though, where you want to recognize this situation).

Use O_CLOEXEC, normal filesystem operation.

New flag.

Yes, they are all related interfaces. Using the same name indicates similarity.

And just to complicate things even more:

if we'd go with sys_indirect we can have single way to specify close-on-exec for all syscalls. That would be at the syscall level. At the API level we'd still need separate names and possibly values.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖ -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)

iD8DBQFIFemb2ijCOnn/RHQRAjNCAJ0QCvEJoynwbPveUZQqsyVnEAHtmgCZAe0R jNlcvQFI06n2ckFhEqUcEbE=
=sJhN
-----END PGP SIGNATURE-----

Okay -- makes sense.

What happens come the day that we want to have a flag that is specific to, say, pipe2()? Will it be a new O_ flag? or a new PIPE_ flag?

Sorry -- I still don't get this -- how is it guaranteed that these syscalls will always use the same set of flags? For example, I could imagine that one day, we might want a flag that is specific to paccept() -- you seem to be saying that would never be the case?

Yes.

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html