[PATCH] cleanup paccept - Kernel

This is a discussion on [PATCH] cleanup paccept - Kernel ; This patch doesn't change *any* functionality from what is currently in the upstream kernel. There is no reason to start arguing again and for me to explain everything the way I did it months ago. This is just a *cleanup ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: [PATCH] cleanup paccept

  1. [PATCH] cleanup paccept

    This patch doesn't change *any* functionality from what is currently
    in the upstream kernel. There is no reason to start arguing again and
    for me to explain everything the way I did it months ago. This is just
    a *cleanup patch*, nothing else. If you want to know why this cleanup
    is needed read the discussions of

    commit 2d4c8266774188cda7f7e612e6dfb8ad12c579d5
    Author: Michael Kerrisk
    Date: Mon Sep 22 13:57:49 2008 -0700

    sys_paccept: disable paccept() until API design is resolved


    For example, here:

    http://kerneltrap.org/mailarchive/li...8/9/16/3310534


    I don't agree with the change but I'm also tired arguing. So just
    clean up the mess created by change initiated by the posting mentioned
    above. The actual patch which is in the kernel is different from
    the patch in the mail: only the signal mask handling has been disabled.
    This is why this is only a cleanup patch.


    I've updated the test program which now looks as follows:

    #include
    #include
    #include
    #include
    #include
    #include
    #include

    #ifdef __x86_64__
    #define __NR_accept4 288
    #define SOCK_CLOEXEC O_CLOEXEC
    #elif __i386__
    #define SYS_ACCEPT4 18
    #define USE_SOCKETCALL 1
    #define SOCK_CLOEXEC O_CLOEXEC
    #else
    #error "define syscall numbers for this architecture"
    #endif

    #define PORT 57392

    static pthread_barrier_t b;

    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;
    }

    int
    main (void)
    {
    alarm (5);

    int status = 0;

    pthread_barrier_init (&b, NULL, 2);

    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);
    int fl = 1;
    setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &fl, sizeof (fl));
    struct sockaddr_in sin;
    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);

    int 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_ACCEPT4, s, NULL, 0, SOCK_CLOEXEC);
    #else
    s2 = syscall (__NR_accept4, s, NULL, 0, SOCK_CLOEXEC);
    #endif
    if (s2 < 0)
    {
    puts ("accept4 failed");
    status = 1;
    }
    else
    {
    int fl = fcntl (s2, F_GETFD);
    if ((fl & FD_CLOEXEC) == 0)
    {
    puts ("accept4 did not set CLOEXEC");
    status = 1;
    }

    close (s2);
    }

    close (s);
    }

    return status;
    }


    arch/x86/include/asm/unistd_64.h | 4 -
    include/linux/net.h | 6 --
    include/linux/syscalls.h | 3 -
    kernel/sys_ni.c | 2
    net/compat.c | 50 ++----------------------
    net/socket.c | 80 ++++-----------------------------------
    6 files changed, 21 insertions(+), 124 deletions(-)


    Signed-off-by: Ulrich Drepper

    diff --git a/arch/x86/include/asm/unistd_64.h b/arch/x86/include/asm/unistd_64.h
    index 834b2c1..d2e415e 100644
    --- a/arch/x86/include/asm/unistd_64.h
    +++ b/arch/x86/include/asm/unistd_64.h
    @@ -639,8 +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)
    +#define __NR_accept4 288
    +__SYSCALL(__NR_accept4, sys_accept4)
    #define __NR_signalfd4 289
    __SYSCALL(__NR_signalfd4, sys_signalfd4)
    #define __NR_eventfd2 290
    diff --git a/include/linux/net.h b/include/linux/net.h
    index 6dc14a2..4515efa 100644
    --- a/include/linux/net.h
    +++ b/include/linux/net.h
    @@ -40,7 +40,7 @@
    #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) */
    +#define SYS_ACCEPT4 18 /* sys_accept4(2) */

    typedef enum {
    SS_FREE = 0, /* not allocated */
    @@ -100,7 +100,7 @@ enum sock_type {
    * remaining bits are used as flags. */
    #define SOCK_TYPE_MASK 0xf

    -/* Flags for socket, socketpair, paccept */
    +/* Flags for socket, socketpair, accept4 */
    #define SOCK_CLOEXEC O_CLOEXEC
    #ifndef SOCK_NONBLOCK
    #define SOCK_NONBLOCK O_NONBLOCK
    @@ -223,8 +223,6 @@ 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)
    diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
    index d6ff145..04fb47b 100644
    --- a/include/linux/syscalls.h
    +++ b/include/linux/syscalls.h
    @@ -410,8 +410,7 @@ 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 __user sigset_t *, size_t, int);
    +asmlinkage long sys_accept4(int, struct sockaddr __user *, int __user *, 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);
    diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
    index a77b27b..e14a232 100644
    --- a/kernel/sys_ni.c
    +++ b/kernel/sys_ni.c
    @@ -31,7 +31,7 @@ cond_syscall(sys_socketpair);
    cond_syscall(sys_bind);
    cond_syscall(sys_listen);
    cond_syscall(sys_accept);
    -cond_syscall(sys_paccept);
    +cond_syscall(sys_accept4);
    cond_syscall(sys_connect);
    cond_syscall(sys_getsockname);
    cond_syscall(sys_getpeername);
    diff --git a/net/compat.c b/net/compat.c
    index 67fb6a3..50ce0a9 100644
    --- a/net/compat.c
    +++ b/net/compat.c
    @@ -725,7 +725,7 @@ EXPORT_SYMBOL(compat_mc_getsockopt);
    static unsigned char nas[19]={AL(0),AL(3),AL(3),AL(3),AL(2),AL(3),
    AL(3),AL(3),AL(4),AL(4),AL(4),AL(6),
    AL(6),AL(2),AL(5),AL(5),AL(3),AL(3),
    - AL(6)};
    + AL(4)};
    #undef AL

    asmlinkage long compat_sys_sendmsg(int fd, struct compat_msghdr __user *msg, unsigned flags)
    @@ -738,52 +738,13 @@ asmlinkage long compat_sys_recvmsg(int fd, struct compat_msghdr __user *msg, uns
    return sys_recvmsg(fd, (struct msghdr __user *)msg, flags | MSG_CMSG_COMPAT);
    }

    -asmlinkage long compat_sys_paccept(int fd, struct sockaddr __user *upeer_sockaddr,
    - int __user *upeer_addrlen,
    - const compat_sigset_t __user *sigmask,
    - compat_size_t sigsetsize, int flags)
    -{
    - compat_sigset_t ss32;
    - sigset_t ksigmask, sigsaved;
    - int ret;
    -
    - if (sigmask) {
    - if (sigsetsize != sizeof(compat_sigset_t))
    - return -EINVAL;
    - if (copy_from_user(&ss32, sigmask, sizeof(ss32)))
    - return -EFAULT;
    - sigset_from_compat(&ksigmask, &ss32);
    -
    - sigdelsetmask(&ksigmask, sigmask(SIGKILL)|sigmask(SIGSTOP));
    - sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
    - }
    -
    - ret = do_accept(fd, upeer_sockaddr, upeer_addrlen, flags);
    -
    - if (ret == -ERESTARTNOHAND) {
    - /*
    - * Don't restore the signal mask yet. Let do_signal() deliver
    - * the signal on the way back to userspace, before the signal
    - * mask is restored.
    - */
    - if (sigmask) {
    - memcpy(&current->saved_sigmask, &sigsaved,
    - sizeof(sigsaved));
    - set_restore_sigmask();
    - }
    - } else if (sigmask)
    - sigprocmask(SIG_SETMASK, &sigsaved, NULL);
    -
    - return ret;
    -}
    -
    asmlinkage long compat_sys_socketcall(int call, u32 __user *args)
    {
    int ret;
    u32 a[6];
    u32 a0, a1;

    - if (call < SYS_SOCKET || call > SYS_PACCEPT)
    + if (call < SYS_SOCKET || call > SYS_ACCEPT4)
    return -EINVAL;
    if (copy_from_user(a, args, nas[call]))
    return -EFAULT;
    @@ -804,7 +765,7 @@ asmlinkage long compat_sys_socketcall(int call, u32 __user *args)
    ret = sys_listen(a0, a1);
    break;
    case SYS_ACCEPT:
    - ret = do_accept(a0, compat_ptr(a1), compat_ptr(a[2]), 0);
    + ret = sys_accept4(a0, compat_ptr(a1), compat_ptr(a[2]), 0);
    break;
    case SYS_GETSOCKNAME:
    ret = sys_getsockname(a0, compat_ptr(a1), compat_ptr(a[2]));
    @@ -844,9 +805,8 @@ asmlinkage long compat_sys_socketcall(int call, u32 __user *args)
    case SYS_RECVMSG:
    ret = compat_sys_recvmsg(a0, compat_ptr(a1), a[2]);
    break;
    - case SYS_PACCEPT:
    - ret = compat_sys_paccept(a0, compat_ptr(a1), compat_ptr(a[2]),
    - compat_ptr(a[3]), a[4], a[5]);
    + case SYS_ACCEPT4:
    + ret = sys_accept4(a0, compat_ptr(a1), compat_ptr(a[2]), a[3]);
    break;
    default:
    ret = -EINVAL;
    diff --git a/net/socket.c b/net/socket.c
    index 2b7a4b5..8e72806 100644
    --- a/net/socket.c
    +++ b/net/socket.c
    @@ -1427,8 +1427,8 @@ asmlinkage long sys_listen(int fd, int backlog)
    * clean when we restucture accept also.
    */

    -long do_accept(int fd, struct sockaddr __user *upeer_sockaddr,
    - int __user *upeer_addrlen, int flags)
    +asmlinkage long sys_accept4(int fd, struct sockaddr __user *upeer_sockaddr,
    + int __user *upeer_addrlen, int flags)
    {
    struct socket *sock, *newsock;
    struct file *newfile;
    @@ -1511,66 +1511,10 @@ out_fd:
    goto out_put;
    }

    -#if 0
    -#ifdef HAVE_SET_RESTORE_SIGMASK
    -asmlinkage long sys_paccept(int fd, struct sockaddr __user *upeer_sockaddr,
    - int __user *upeer_addrlen,
    - const sigset_t __user *sigmask,
    - size_t sigsetsize, int flags)
    -{
    - sigset_t ksigmask, sigsaved;
    - int ret;
    -
    - if (sigmask) {
    - /* XXX: Don't preclude handling different sized sigset_t's. */
    - if (sigsetsize != sizeof(sigset_t))
    - return -EINVAL;
    - if (copy_from_user(&ksigmask, sigmask, sizeof(ksigmask)))
    - return -EFAULT;
    -
    - sigdelsetmask(&ksigmask, sigmask(SIGKILL)|sigmask(SIGSTOP));
    - sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
    - }
    -
    - ret = do_accept(fd, upeer_sockaddr, upeer_addrlen, flags);
    -
    - if (ret < 0 && signal_pending(current)) {
    - /*
    - * Don't restore the signal mask yet. Let do_signal() deliver
    - * the signal on the way back to userspace, before the signal
    - * mask is restored.
    - */
    - if (sigmask) {
    - memcpy(&current->saved_sigmask, &sigsaved,
    - sizeof(sigsaved));
    - set_restore_sigmask();
    - }
    - } else if (sigmask)
    - sigprocmask(SIG_SETMASK, &sigsaved, NULL);
    -
    - return ret;
    -}
    -#else
    -asmlinkage long sys_paccept(int fd, struct sockaddr __user *upeer_sockaddr,
    - int __user *upeer_addrlen,
    - const sigset_t __user *sigmask,
    - size_t sigsetsize, int flags)
    -{
    - /* The platform does not support restoring the signal mask in the
    - * return path. So we do not allow using paccept() with a signal
    - * mask. */
    - if (sigmask)
    - return -EINVAL;
    -
    - return do_accept(fd, upeer_sockaddr, upeer_addrlen, flags);
    -}
    -#endif
    -#endif
    -
    asmlinkage long sys_accept(int fd, struct sockaddr __user *upeer_sockaddr,
    int __user *upeer_addrlen)
    {
    - return do_accept(fd, upeer_sockaddr, upeer_addrlen, 0);
    + return sys_accept4(fd, upeer_sockaddr, upeer_addrlen, 0);
    }

    /*
    @@ -2097,7 +2041,7 @@ static const unsigned char nargs[19]={
    AL(0),AL(3),AL(3),AL(3),AL(2),AL(3),
    AL(3),AL(3),AL(4),AL(4),AL(4),AL(6),
    AL(6),AL(2),AL(5),AL(5),AL(3),AL(3),
    - AL(6)
    + AL(4)
    };

    #undef AL
    @@ -2116,7 +2060,7 @@ asmlinkage long sys_socketcall(int call, unsigned long __user *args)
    unsigned long a0, a1;
    int err;

    - if (call < 1 || call > SYS_PACCEPT)
    + if (call < 1 || call > SYS_ACCEPT4)
    return -EINVAL;

    /* copy_from_user should be SMP safe. */
    @@ -2144,9 +2088,8 @@ asmlinkage long sys_socketcall(int call, unsigned long __user *args)
    err = sys_listen(a0, a1);
    break;
    case SYS_ACCEPT:
    - err =
    - do_accept(a0, (struct sockaddr __user *)a1,
    - (int __user *)a[2], 0);
    + err = sys_accept4(a0, (struct sockaddr __user *)a1,
    + (int __user *)a[2], 0);
    break;
    case SYS_GETSOCKNAME:
    err =
    @@ -2193,12 +2136,9 @@ asmlinkage long sys_socketcall(int call, unsigned long __user *args)
    case SYS_RECVMSG:
    err = sys_recvmsg(a0, (struct msghdr __user *)a1, a[2]);
    break;
    - case SYS_PACCEPT:
    - err =
    - sys_paccept(a0, (struct sockaddr __user *)a1,
    - (int __user *)a[2],
    - (const sigset_t __user *) a[3],
    - a[4], a[5]);
    + case SYS_ACCEPT4:
    + err = sys_accept4(a0, (struct sockaddr __user *)a1,
    + (int __user *)a[2], a[3]);
    break;
    default:
    err = -EINVAL;
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] cleanup paccept

    On Tue, Oct 28, 2008 at 03:09:51PM -0400, Ulrich Drepper wrote:
    > This patch doesn't change *any* functionality from what is currently
    > in the upstream kernel. There is no reason to start arguing again and
    > for me to explain everything the way I did it months ago. This is just
    > a *cleanup patch*, nothing else. If you want to know why this cleanup
    > is needed read the discussions of


    Umm, it does enabled functionality that wasn't enabled before. And that
    functionality includes a syscall. So any chance you could write a
    proper changelog that describes what you actually do instead of lots of
    ranting and references to old posts?

    And no "[PATCH] cleanup paccept" isn't really a good subject for adding
    a new syscalls either.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] cleanup paccept

    On Tue, Oct 28, 2008 at 2:09 PM, Ulrich Drepper wrote:
    > This patch doesn't change *any* functionality from what is currently
    > in the upstream kernel. There is no reason to start arguing again and
    > for me to explain everything the way I did it months ago. This is just
    > a *cleanup patch*, nothing else. If you want to know why this cleanup
    > is needed read the discussions of
    >
    > commit 2d4c8266774188cda7f7e612e6dfb8ad12c579d5
    > Author: Michael Kerrisk
    > Date: Mon Sep 22 13:57:49 2008 -0700
    >
    > sys_paccept: disable paccept() until API design is resolved
    >
    >
    > For example, here:
    >
    > http://kerneltrap.org/mailarchive/li...8/9/16/3310534
    >
    >
    > I don't agree with the change but I'm also tired arguing. So just
    > clean up the mess created by change initiated by the posting mentioned
    > above.


    Oh, please! The "mess" arose because you refused to explain things in
    a simple and coherent fashion, even when politely asked.

    And now you compound the mess by running up two threads in the last 24
    hours with versions of the patch (are they the same -- who knows? You
    give us no clue.) So I'll repeat here, what I said in the other:

    1. Please take the trouble to CC interested parties. For example, the
    people CCed in recent threads that discussed earlier versions of the
    the patch. This is basic LKML basic etiquette, since almost no one
    reads all LKML, nor reads it hourly. Doing this allows interested
    parties to comment on the patch, and also avoids patches hitting the
    kernel "under the radar".

    2. Don't assume anyone has cached earlier discussions of the topic.
    Write a clear, complete description of the patch that could be read by
    someone new to the topic, something like a summary of
    http://lwn.net/Articles/281965/ and http://udrepper.livejournal.com/20407.html

    [If you had done this the first time round, then there'd be very
    little work other than cut and paste this time round.]

    3. Explain what changes have been made from earlier versions of the
    patch, and why.
    E.g., some discussion that summarizes this:
    http://thread.gmane.org/gmane.linux.network/106071

    4. CC linux-api@vger.kernel.org on patches that are API changes. This
    requirement was added to SubmitChecklist in the last few weeks.

    5. CC me or linux-man@vger.kernel.org, so that something makes its way
    into man-pages.

    > The actual patch which is in the kernel is different from
    > the patch in the mail: only the signal mask handling has been disabled.
    > This is why this is only a cleanup patch.


    And that does not explain to the world at large what this
    to-be-enabled system call does, or why it's needed.

    Cheers,

    Michael
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH] cleanup paccept

    >> The actual patch which is in the kernel is different from
    >> the patch in the mail: only the signal mask handling has been disabled.
    >> This is why this is only a cleanup patch.

    >
    > And that does not explain to the world at large what this
    > to-be-enabled system call does, or why it's needed.


    And one further point: what is the userspace interface going to look
    like for this system call?

    Are we to see a repeat of the unsafe userspace implementation of
    pselect() that was present in glibc before the kernel finally added a
    pselect() system call?

    In other words: is the userspace interface for accept4() going to be
    the thinnest of wrappers, or will glibc add an unsafe
    paccept()-with-signal-mask in userspace?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread