[PATCH] CRIS v10: Correct do_signal to fix oops and clean up signal handling in general. - Kernel

This is a discussion on [PATCH] CRIS v10: Correct do_signal to fix oops and clean up signal handling in general. - Kernel ; CRIS v10: Correct do_signal to fix oops and clean up signal handling in general. This fixes a kernel panic on boot due to do_signal not being compatible with it's callers. - do_signal now returns void, and does not have the ...

+ Reply to Thread
Results 1 to 3 of 3

Thread: [PATCH] CRIS v10: Correct do_signal to fix oops and clean up signal handling in general.

  1. [PATCH] CRIS v10: Correct do_signal to fix oops and clean up signal handling in general.

    CRIS v10: Correct do_signal to fix oops and clean up signal handling in general.

    This fixes a kernel panic on boot due to do_signal not being compatible
    with it's callers.

    - do_signal now returns void, and does not have the previous signal set
    as a parameter.
    - Remove sys_rt_sigsuspend, we can use the common one instead.
    - Change sys_sigsuspend to be more like x86, don't call do_signal here.
    - handle_signal, setup_frame and setup_rt_frame now return -EFAULT
    if we've delivered a segfault, which is used by callers to perform
    necessary cleanup.
    - Break long lines, correct whitespace and formatting errors.

    Signed-off-by: Jesper Nilsson
    ---
    arch/cris/arch-v10/kernel/signal.c | 251 ++++++++++++++++--------------------
    1 files changed, 112 insertions(+), 139 deletions(-)

    diff --git a/arch/cris/arch-v10/kernel/signal.c b/arch/cris/arch-v10/kernel/signal.c
    index 41d4a5f..b6be705 100644
    --- a/arch/cris/arch-v10/kernel/signal.c
    +++ b/arch/cris/arch-v10/kernel/signal.c
    @@ -7,7 +7,7 @@
    *
    * Ideas also taken from arch/arm.
    *
    - * Copyright (C) 2000, 2001 Axis Communications AB
    + * Copyright (C) 2000-2007 Axis Communications AB
    *
    * Authors: Bjorn Wesen (bjornw@axis.com)
    *
    @@ -40,84 +40,30 @@
    */
    #define RESTART_CRIS_SYS(regs) regs->r10 = regs->orig_r10; regs->irp -= 2;

    -int do_signal(int canrestart, sigset_t *oldset, struct pt_regs *regs);
    +void do_signal(int canrestart, struct pt_regs *regs);

    /*
    - * Atomically swap in the new signal mask, and wait for a signal. Define
    + * Atomically swap in the new signal mask, and wait for a signal. Define
    * dummy arguments to be able to reach the regs argument. (Note that this
    * arrangement relies on old_sigset_t occupying one register.)
    */
    -int
    -sys_sigsuspend(old_sigset_t mask, long r11, long r12, long r13, long mof,
    - long srp, struct pt_regs *regs)
    +int sys_sigsuspend(old_sigset_t mask, long r11, long r12, long r13, long mof,
    + long srp, struct pt_regs *regs)
    {
    - sigset_t saveset;
    -
    mask &= _BLOCKABLE;
    spin_lock_irq(&current->sighand->siglock);
    - saveset = current->blocked;
    + current->saved_sigmask = current->blocked;
    siginitset(&current->blocked, mask);
    recalc_sigpending();
    spin_unlock_irq(&current->sighand->siglock);
    -
    - regs->r10 = -EINTR;
    - while (1) {
    - current->state = TASK_INTERRUPTIBLE;
    - schedule();
    - if (do_signal(0, &saveset, regs))
    - /* We will get here twice: once to call the signal
    - handler, then again to return from the
    - sigsuspend system call. When calling the
    - signal handler, R10 holds the signal number as
    - set through do_signal. The sigsuspend call
    - will return with the restored value set above;
    - always -EINTR. */
    - return regs->r10;
    - }
    + current->state = TASK_INTERRUPTIBLE;
    + schedule();
    + set_thread_flag(TIF_RESTORE_SIGMASK);
    + return -ERESTARTNOHAND;
    }

    -/* Define dummy arguments to be able to reach the regs argument. (Note that
    - * this arrangement relies on size_t occupying one register.)
    - */
    -int
    -sys_rt_sigsuspend(sigset_t *unewset, size_t sigsetsize, long r12, long r13,
    - long mof, long srp, struct pt_regs *regs)
    -{
    - sigset_t saveset, newset;
    -
    - /* XXX: Don't preclude handling different sized sigset_t's. */
    - if (sigsetsize != sizeof(sigset_t))
    - return -EINVAL;
    -
    - if (copy_from_user(&newset, unewset, sizeof(newset)))
    - return -EFAULT;
    - sigdelsetmask(&newset, ~_BLOCKABLE);
    -
    - spin_lock_irq(&current->sighand->siglock);
    - saveset = current->blocked;
    - current->blocked = newset;
    - recalc_sigpending();
    - spin_unlock_irq(&current->sighand->siglock);
    -
    - regs->r10 = -EINTR;
    - while (1) {
    - current->state = TASK_INTERRUPTIBLE;
    - schedule();
    - if (do_signal(0, &saveset, regs))
    - /* We will get here twice: once to call the signal
    - handler, then again to return from the
    - sigsuspend system call. When calling the
    - signal handler, R10 holds the signal number as
    - set through do_signal. The sigsuspend call
    - will return with the restored value set above;
    - always -EINTR. */
    - return regs->r10;
    - }
    -}
    -
    -int
    -sys_sigaction(int sig, const struct old_sigaction __user *act,
    - struct old_sigaction *oact)
    +int sys_sigaction(int sig, const struct old_sigaction __user *act,
    + struct old_sigaction *oact)
    {
    struct k_sigaction new_ka, old_ka;
    int ret;
    @@ -147,8 +93,7 @@ sys_sigaction(int sig, const struct old_sigaction __user *act,
    return ret;
    }

    -int
    -sys_sigaltstack(const stack_t *uss, stack_t __user *uoss)
    +int sys_sigaltstack(const stack_t *uss, stack_t __user *uoss)
    {
    return do_sigaltstack(uss, uoss, rdusp());
    }
    @@ -205,7 +150,7 @@ restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc)

    /* TODO: the other ports use regs->orig_XX to disable syscall checks
    * after this completes, but we don't use that mechanism. maybe we can
    - * use it now ?
    + * use it now ?
    */

    return err;
    @@ -216,7 +161,7 @@ badframe:

    /* Define dummy arguments to be able to reach the regs argument. */

    -asmlinkage int sys_sigreturn(long r10, long r11, long r12, long r13, long mof,
    +asmlinkage int sys_sigreturn(long r10, long r11, long r12, long r13, long mof,
    long srp, struct pt_regs *regs)
    {
    struct sigframe __user *frame = (struct sigframe *)rdusp();
    @@ -243,7 +188,7 @@ asmlinkage int sys_sigreturn(long r10, long r11, long r12, long r13, long mof,
    current->blocked = set;
    recalc_sigpending();
    spin_unlock_irq(&current->sighand->siglock);
    -
    +
    if (restore_sigcontext(regs, &frame->sc))
    goto badframe;

    @@ -254,11 +199,11 @@ asmlinkage int sys_sigreturn(long r10, long r11, long r12, long r13, long mof,
    badframe:
    force_sig(SIGSEGV, current);
    return 0;
    -}
    +}

    /* Define dummy arguments to be able to reach the regs argument. */

    -asmlinkage int sys_rt_sigreturn(long r10, long r11, long r12, long r13,
    +asmlinkage int sys_rt_sigreturn(long r10, long r11, long r12, long r13,
    long mof, long srp, struct pt_regs *regs)
    {
    struct rt_sigframe __user *frame = (struct rt_sigframe *)rdusp();
    @@ -282,7 +227,7 @@ asmlinkage int sys_rt_sigreturn(long r10, long r11, long r12, long r13,
    current->blocked = set;
    recalc_sigpending();
    spin_unlock_irq(&current->sighand->siglock);
    -
    +
    if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
    goto badframe;

    @@ -294,14 +239,14 @@ asmlinkage int sys_rt_sigreturn(long r10, long r11, long r12, long r13,
    badframe:
    force_sig(SIGSEGV, current);
    return 0;
    -}
    +}

    /*
    * Set up a signal frame.
    */

    -static int
    -setup_sigcontext(struct sigcontext __user *sc, struct pt_regs *regs, unsigned long mask)
    +static int setup_sigcontext(struct sigcontext __user *sc,
    + struct pt_regs *regs, unsigned long mask)
    {
    int err = 0;
    unsigned long usp = rdusp();
    @@ -324,10 +269,11 @@ setup_sigcontext(struct sigcontext __user *sc, struct pt_regs *regs, unsigned lo
    return err;
    }

    -/* figure out where we want to put the new signal frame - usually on the stack */
    +/* Figure out where we want to put the new signal frame
    + * - usually on the stack. */

    static inline void __user *
    -get_sigframe(struct k_sigaction *ka, struct pt_regs * regs, size_t frame_size)
    +get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size)
    {
    unsigned long sp = rdusp();

    @@ -345,15 +291,15 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs * regs, size_t frame_size)
    }

    /* grab and setup a signal frame.
    - *
    + *
    * basically we stack a lot of state info, and arrange for the
    * user-mode program to return to the kernel using either a
    * trampoline which performs the syscall sigreturn, or a provided
    * user-mode trampoline.
    */

    -static void setup_frame(int sig, struct k_sigaction *ka,
    - sigset_t *set, struct pt_regs * regs)
    +static int setup_frame(int sig, struct k_sigaction *ka,
    + sigset_t *set, struct pt_regs *regs)
    {
    struct sigframe __user *frame;
    unsigned long return_ip;
    @@ -401,14 +347,15 @@ static void setup_frame(int sig, struct k_sigaction *ka,

    wrusp((unsigned long)frame);

    - return;
    + return 0;

    give_sigsegv:
    force_sigsegv(sig, current);
    + return -EFAULT;
    }

    -static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
    - sigset_t *set, struct pt_regs * regs)
    +static int setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
    + sigset_t *set, struct pt_regs *regs)
    {
    struct rt_sigframe __user *frame;
    unsigned long return_ip;
    @@ -443,9 +390,10 @@ static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
    /* trampoline - the desired return ip is the retcode itself */
    return_ip = (unsigned long)&frame->retcode;
    /* This is movu.w __NR_rt_sigreturn, r9; break 13; */
    - err |= __put_user(0x9c5f, (short __user*)(frame->retcode+0));
    - err |= __put_user(__NR_rt_sigreturn, (short __user*)(frame->retcode+2));
    - err |= __put_user(0xe93d, (short __user*)(frame->retcode+4));
    + err |= __put_user(0x9c5f, (short __user *)(frame->retcode+0));
    + err |= __put_user(__NR_rt_sigreturn,
    + (short __user *)(frame->retcode+2));
    + err |= __put_user(0xe93d, (short __user *)(frame->retcode+4));
    }

    if (err)
    @@ -455,73 +403,81 @@ static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,

    /* Set up registers for signal handler */

    - regs->irp = (unsigned long) ka->sa.sa_handler; /* what we enter NOW */
    - regs->srp = return_ip; /* what we enter LATER */
    - regs->r10 = sig; /* first argument is signo */
    - regs->r11 = (unsigned long) &frame->info; /* second argument is (siginfo_t *) */
    - regs->r12 = 0; /* third argument is unused */
    -
    - /* actually move the usp to reflect the stacked frame */
    -
    + /* What we enter NOW */
    + regs->irp = (unsigned long) ka->sa.sa_handler;
    + /* What we enter LATER */
    + regs->srp = return_ip;
    + /* First argument is signo */
    + regs->r10 = sig;
    + /* Second argument is (siginfo_t *) */
    + regs->r11 = (unsigned long)&frame->info;
    + /* Third argument is unused */
    + regs->r12 = 0;
    +
    + /* Actually move the usp to reflect the stacked frame */
    wrusp((unsigned long)frame);

    - return;
    + return 0;

    give_sigsegv:
    force_sigsegv(sig, current);
    + return -EFAULT;
    }

    /*
    * OK, we're invoking a handler
    - */
    + */

    -static inline void
    -handle_signal(int canrestart, unsigned long sig,
    - siginfo_t *info, struct k_sigaction *ka,
    - sigset_t *oldset, struct pt_regs * regs)
    +static inline int handle_signal(int canrestart, unsigned long sig,
    + siginfo_t *info, struct k_sigaction *ka,
    + sigset_t *oldset, struct pt_regs *regs)
    {
    + int ret;
    +
    /* Are we from a system call? */
    if (canrestart) {
    /* If so, check system call restarting.. */
    switch (regs->r10) {
    - case -ERESTART_RESTARTBLOCK:
    - case -ERESTARTNOHAND:
    - /* ERESTARTNOHAND means that the syscall should only be
    - restarted if there was no handler for the signal, and since
    - we only get here if there is a handler, we don't restart */
    + case -ERESTART_RESTARTBLOCK:
    + case -ERESTARTNOHAND:
    + /* ERESTARTNOHAND means that the syscall should
    + * only be restarted if there was no handler for
    + * the signal, and since we only get here if there
    + * is a handler, we don't restart */
    + regs->r10 = -EINTR;
    + break;
    + case -ERESTARTSYS:
    + /* ERESTARTSYS means to restart the syscall if
    + * there is no handler or the handler was
    + * registered with SA_RESTART */
    + if (!(ka->sa.sa_flags & SA_RESTART)) {
    regs->r10 = -EINTR;
    break;
    -
    - case -ERESTARTSYS:
    - /* ERESTARTSYS means to restart the syscall if there is no
    - handler or the handler was registered with SA_RESTART */
    - if (!(ka->sa.sa_flags & SA_RESTART)) {
    - regs->r10 = -EINTR;
    - break;
    - }
    - /* fallthrough */
    - case -ERESTARTNOINTR:
    - /* ERESTARTNOINTR means that the syscall should be called again
    - after the signal handler returns. */
    - RESTART_CRIS_SYS(regs);
    + }
    + /* fallthrough */
    + case -ERESTARTNOINTR:
    + /* ERESTARTNOINTR means that the syscall should
    + * be called again after the signal handler returns. */
    + RESTART_CRIS_SYS(regs);
    }
    }

    /* Set up the stack frame */
    if (ka->sa.sa_flags & SA_SIGINFO)
    - setup_rt_frame(sig, ka, info, oldset, regs);
    + ret = setup_rt_frame(sig, ka, info, oldset, regs);
    else
    - setup_frame(sig, ka, oldset, regs);
    -
    - if (ka->sa.sa_flags & SA_ONESHOT)
    - ka->sa.sa_handler = SIG_DFL;
    -
    - spin_lock_irq(&current->sighand->siglock);
    - sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
    - if (!(ka->sa.sa_flags & SA_NODEFER))
    - sigaddset(&current->blocked,sig);
    - recalc_sigpending();
    - spin_unlock_irq(&current->sighand->siglock);
    + ret = setup_frame(sig, ka, oldset, regs);
    +
    + if (ret == 0) {
    + spin_lock_irq(&current->sighand->siglock);
    + sigorsets(&current->blocked, &current->blocked,
    + &ka->sa.sa_mask);
    + if (!(ka->sa.sa_flags & SA_NODEFER))
    + sigaddset(&current->blocked, sig);
    + recalc_sigpending();
    + spin_unlock_irq(&current->sighand->siglock);
    + }
    + return ret;
    }

    /*
    @@ -536,11 +492,12 @@ handle_signal(int canrestart, unsigned long sig,
    * mode below.
    */

    -int do_signal(int canrestart, sigset_t *oldset, struct pt_regs *regs)
    +void do_signal(int canrestart, struct pt_regs *regs)
    {
    siginfo_t info;
    int signr;
    struct k_sigaction ka;
    + sigset_t *oldset;

    /*
    * We want the common case to go fast, which
    @@ -549,16 +506,26 @@ int do_signal(int canrestart, sigset_t *oldset, struct pt_regs *regs)
    * if so.
    */
    if (!user_mode(regs))
    - return 1;
    + return;

    - if (!oldset)
    + if (test_thread_flag(TIF_RESTORE_SIGMASK))
    + oldset = &current->saved_sigmask;
    + else
    oldset = &current->blocked;

    signr = get_signal_to_deliver(&info, &ka, regs, NULL);
    if (signr > 0) {
    /* Whee! Actually deliver the signal. */
    - handle_signal(canrestart, signr, &info, &ka, oldset, regs);
    - return 1;
    + if (handle_signal(canrestart, signr, &info, &ka,
    + oldset, regs)) {
    + /* a signal was successfully delivered; the saved
    + * sigmask will have been stored in the signal frame,
    + * and will be restored by sigreturn, so we can simply
    + * clear the TIF_RESTORE_SIGMASK flag */
    + if (test_thread_flag(TIF_RESTORE_SIGMASK))
    + clear_thread_flag(TIF_RESTORE_SIGMASK);
    + }
    + return;
    }

    /* Did we come from a system call? */
    @@ -569,10 +536,16 @@ int do_signal(int canrestart, sigset_t *oldset, struct pt_regs *regs)
    regs->r10 == -ERESTARTNOINTR) {
    RESTART_CRIS_SYS(regs);
    }
    - if (regs->r10 == -ERESTART_RESTARTBLOCK){
    + if (regs->r10 == -ERESTART_RESTARTBLOCK) {
    regs->r10 = __NR_restart_syscall;
    regs->irp -= 2;
    }
    }
    - return 0;
    +
    + /* if there's no signal to deliver, we just put the saved sigmask
    + * back */
    + if (test_thread_flag(TIF_RESTORE_SIGMASK)) {
    + clear_thread_flag(TIF_RESTORE_SIGMASK);
    + sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
    + }
    }
    --
    1.5.3.6.970.gd25430


    /^JN - Jesper Nilsson
    --
    Jesper Nilsson -- jesper.nilsson@axis.com
    --
    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] CRIS v10: Correct do_signal to fix oops and clean up signal handling in general.

    On Fri, 11 Jan 2008 19:59:24 +0100
    Jesper Nilsson wrote:

    > CRIS v10: Correct do_signal to fix oops and clean up signal handling in general.
    >
    > This fixes a kernel panic on boot due to do_signal not being compatible
    > with it's callers.
    >


    Please sequence-number patches even if they are unrelated. That will make
    emails like this one easier.

    1: CRIS v10: Correct do_signal to fix oops and clean up signal handling in general.
    2: CRIS: Define __ARCH_WANT_SYS_RT_SIGSUSPEND in unistd.h for CRIS
    3: CRIS v10: kernel/time.c needs to include linux/vmstat.h to compile.
    4: CRIS v10: Driver for ds1302 needs to include cris-specific i2c.h

    Patches 3 and 4 were missing your signed-off-by:. I added it.

    I queued patches 1, 3 and 4 for 2.6.24 and patch 2 for 2.6.25. Do you agree?
    --
    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] CRIS v10: Correct do_signal to fix oops and clean up signal handling in general.

    On Fri, Jan 11, 2008 at 03:45:13PM -0800, Andrew Morton wrote:
    > On Fri, 11 Jan 2008 19:59:24 +0100
    > Jesper Nilsson wrote:
    >
    > > CRIS v10: Correct do_signal to fix oops and clean up signal handling in general.
    > >
    > > This fixes a kernel panic on boot due to do_signal not being compatible
    > > with it's callers.

    >
    > Please sequence-number patches even if they are unrelated. That will make
    > emails like this one easier.


    Yes, sorry about that, they should have been sequenced.

    > 1: CRIS v10: Correct do_signal to fix oops and clean up signal handling in general.
    > 2: CRIS: Define __ARCH_WANT_SYS_RT_SIGSUSPEND in unistd.h for CRIS
    > 3: CRIS v10: kernel/time.c needs to include linux/vmstat.h to compile.
    > 4: CRIS v10: Driver for ds1302 needs to include cris-specific i2c.h
    >
    > Patches 3 and 4 were missing your signed-off-by:. I added it.
    >
    > I queued patches 1, 3 and 4 for 2.6.24 and patch 2 for 2.6.25. Do you agree?


    All four should be queued for 2.6.24 since patch 2 is needed if patch 1
    is included, otherwise sys_rt_sigsuspend will be undefined causing a
    link error.

    Thanks!

    /^JN - Jesper Nilsson
    --
    Jesper Nilsson -- jesper.nilsson@axis.com
    --
    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