Re: [PATCH] alternative^2 to sys_indirect: socket, socketpair

Re: [PATCH] alternative^2 to sys_indirect: socket, socketpair

[Alan Cox]

Comments all come down to implementation details and it basically looks
sound and fairly small clean change (especially when the various wrappers
and _flags changes were replaced by just fixing up the callers to
functions in a final implementation.

Add a new accept() and it looks good to me

> -	fd = get_unused_fd();
> +	fd = get_unused_fd_flags(flags);

Better to fix up the existing users who don't pass flags (and check why
they need not to), Implementation detail

> -int sock_map_fd(struct socket *sock)
> +static int sock_map_fd_flags(struct socket *sock, int flags)

No need to rename these, the compiler errors will let us quickly
find/fix any users.

> +int sock_map_fd(struct socket *sock)
> +{
> +	return sock_map_fd_flags(sock, 0);
> +}

Ditto

> +	/* Extract the close-on-exec flag.  */
> +	if ((type & SOCK_CLOEXEC) != 0) {
> +		fflags = O_CLOEXEC;
> +		type &= ~SOCK_CLOEXEC;

> +	/* Extract the close-on-exec flag.  */
> +	if ((type & SOCK_CLOEXEC) != 0) {
> +		fflags = O_CLOEXEC;
> +		type &= ~SOCK_CLOEXEC;
> +	}

Should probably be done once in one place in case we ever add future flags



-- 
--
	"Alan, I'm getting a bit worried about you."
				-- Linus Torvalds

[PATCH] alternative^2 to sys_indirect: socket, socketpair

[Ulrich Drepper]

Here is a patch with just the socket and socketpair changes which uses
a new SOCK_CLOEXEC flag.  I started the flags at the high end (ignoring
the sign bit) to avoid collisions.  accept() is not changed in this
patch.


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

#define SOCK_CLOEXEC 0x40000000

#define PORT 57392

int
main (void)
{
  int status = 0;
  int s;
  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]);
    }

  return status;
}


 include/linux/net.h |    3 +++
 net/socket.c        |   35 +++++++++++++++++++++++++++--------
 2 files changed, 30 insertions(+), 8 deletions(-)

Signed-off-by: Ulrich Drepper <drepper@redhat.com>

diff --git a/include/linux/net.h b/include/linux/net.h
index 71f7dd5..f5dddd8 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -91,6 +91,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)
diff --git a/net/socket.c b/net/socket.c
index 9b5c917..e144f8a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -348,11 +348,11 @@ static struct dentry_operations sockfs_dentry_operations = {
  *	but we take care of internal coherence yet.
  */
 
-static int sock_alloc_fd(struct file **filep)
+static int sock_alloc_fd(struct file **filep, int flags)
 {
 	int fd;
 
-	fd = get_unused_fd();
+	fd = get_unused_fd_flags(flags);
 	if (likely(fd >= 0)) {
 		struct file *file = get_empty_filp();
 
@@ -395,10 +395,10 @@ static int sock_attach_fd(struct socket *sock, struct file *file)
 	return 0;
 }
 
-int sock_map_fd(struct socket *sock)
+static int sock_map_fd_flags(struct socket *sock, int flags)
 {
 	struct file *newfile;
-	int fd = sock_alloc_fd(&newfile);
+	int fd = sock_alloc_fd(&newfile, flags);
 
 	if (likely(fd >= 0)) {
 		int err = sock_attach_fd(sock, newfile);
@@ -413,6 +413,11 @@ int sock_map_fd(struct socket *sock)
 	return fd;
 }
 
+int sock_map_fd(struct socket *sock)
+{
+	return sock_map_fd_flags(sock, 0);
+}
+
 static struct socket *sock_from_file(struct file *file, int *err)
 {
 	if (file->f_op == &socket_file_ops)
@@ -1217,12 +1222,19 @@ asmlinkage long sys_socket(int family, int type, int protocol)
 {
 	int retval;
 	struct socket *sock;
+	int fflags = 0;
+
+	/* Extract the close-on-exec flag.  */
+	if ((type & SOCK_CLOEXEC) != 0) {
+		fflags = O_CLOEXEC;
+		type &= ~SOCK_CLOEXEC;
+	}
 
 	retval = sock_create(family, type, protocol, &sock);
 	if (retval < 0)
 		goto out;
 
-	retval = sock_map_fd(sock);
+	retval = sock_map_fd_flags(sock, fflags);
 	if (retval < 0)
 		goto out_release;
 
@@ -1245,6 +1257,13 @@ asmlinkage long sys_socketpair(int family, int type, int protocol,
 	struct socket *sock1, *sock2;
 	int fd1, fd2, err;
 	struct file *newfile1, *newfile2;
+	int fflags = 0;
+
+	/* Extract the close-on-exec flag.  */
+	if ((type & SOCK_CLOEXEC) != 0) {
+		fflags = O_CLOEXEC;
+		type &= ~SOCK_CLOEXEC;
+	}
 
 	/*
 	 * Obtain the first socket and check if the underlying protocol
@@ -1263,13 +1282,13 @@ asmlinkage long sys_socketpair(int family, int type, int protocol,
 	if (err < 0)
 		goto out_release_both;
 
-	fd1 = sock_alloc_fd(&newfile1);
+	fd1 = sock_alloc_fd(&newfile1, fflags);
 	if (unlikely(fd1 < 0)) {
 		err = fd1;
 		goto out_release_both;
 	}
 
-	fd2 = sock_alloc_fd(&newfile2);
+	fd2 = sock_alloc_fd(&newfile2, fflags);
 	if (unlikely(fd2 < 0)) {
 		err = fd2;
 		put_filp(newfile1);
@@ -1425,7 +1444,7 @@ asmlinkage long sys_accept(int fd, struct sockaddr __user *upeer_sockaddr,
 	 */
 	__module_get(newsock->ops->owner);
 
-	newfd = sock_alloc_fd(&newfile);
+	newfd = sock_alloc_fd(&newfile, 0);
 	if (unlikely(newfd < 0)) {
 		err = newfd;
 		sock_release(newsock);

Re: [PATCH] alternative^2 to sys_indirect: socket, socketpair

[Ulrich Drepper]

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

Alan Cox wrote:

>> -int sock_map_fd(struct socket *sock)
>> +static int sock_map_fd_flags(struct socket *sock, int flags)

sock_map_fd is exported to modules.  Isn't it a big no-no to change
those interfaces?

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

iD8DBQFIELda2ijCOnn/RHQRAoXZAKCAnv7JAup2v6KukBp59LDELfNUeACgvuET
eBYx7ErX8E27VLxlYOdeXjs=
=D+sf
-----END PGP SIGNATURE-----

Re: [PATCH] alternative^2 to sys_indirect: socket, socketpair

[Alan Cox]

On Thu, 24 Apr 2008 09:37:47 -0700
Ulrich Drepper <drepper@redhat.com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Alan Cox wrote:
> 
> >> -int sock_map_fd(struct socket *sock)
> >> +static int sock_map_fd_flags(struct socket *sock, int flags)
> 
> sock_map_fd is exported to modules.  Isn't it a big no-no to change
> those interfaces?

Upstream no. We just fix the users. The aversion to changing exported
symbols between kernels is strictly an enterprise vendor thing. 

Alan

Re: [PATCH] alternative^2 to sys_indirect: socket, socketpair

[David Miller]

From: Ulrich Drepper <drepper@redhat.com>
Date: Thu, 24 Apr 2008 09:37:47 -0700

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Alan Cox wrote:
> 
> >> -int sock_map_fd(struct socket *sock)
> >> +static int sock_map_fd_flags(struct socket *sock, int flags)
> 
> sock_map_fd is exported to modules.  Isn't it a big no-no to change
> those interfaces?

It's perfectly acceptable to change the interface however
you like when necessary, and merely fix up all the in-tree users.