On 06/17/2010 06:37 PM, Nathan Lynch wrote:
> I have a multithreaded testcase that easily hits the BUG() in
> fill_siginfo() by raise()'ing a signal it has blocked before
> checkpoint. The value of si_code in this case is SI_TKILL.
Thanks for pointing out - this was indeed overlooked.
>
> This gets my testcase to pass, but I'm not sure it's a complete
> solution... comments?
> ---
> kernel/signal.c | 8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index e4ca9a6..cd25592 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2919,6 +2919,9 @@ static void fill_siginfo(struct ckpt_siginfo *si, siginfo_t *info)
> si->_errno = info->si_errno;
> si->code = info->si_code;
>
> + if (info->si_code < 0)
> + return;
> +
This means that we do not save (nor restore) additional state
related to signals from tkill() syscall, or from other kernel
generated signals.
For example -
---
static int do_tkill(pid_t tgid, pid_t pid, int sig)
{
struct siginfo info;
info.si_signo = sig;
info.si_errno = 0;
info.si_code = SI_TKILL;
info.si_pid = task_tgid_vnr(current);
info.si_uid = current_uid();
return do_send_specific(tgid, pid, sig, &info);
}
---
In this case, we lose the values of @si_pid and @si_uid fields.
The correct solution would be to examine each si_code < 0 and
add to the switch() statements (both checkpoint and restart)
to save/restart the additional information. (If it does not
carry more info, then add to the switch and do nothing).
> /* TODO: convert info->si_uid to uid_objref */
>
> switch (info->si_code & __SI_MASK) {
> @@ -2953,7 +2956,8 @@ static void fill_siginfo(struct ckpt_siginfo *si, siginfo_t *info)
> si->sigval_ptr = (unsigned long) info->si_ptr;
> break;
> default:
> - BUG();
> + ckpt_debug("unknown si_code 0x%x\n", info->si_code);
> + break;
The reason I put a BUG there, is to make a kernel bug visible...
and it surely worked :)
> }
> }
>
> @@ -3002,7 +3006,7 @@ static int load_siginfo(siginfo_t *info, struct ckpt_siginfo *si)
> info->si_ptr = (void __user *) (unsigned long) si->sigval_ptr;
> break;
> default:
> - return -EINVAL;
> + break;
Nack. This will allow users to inject arbitrary values here (at
the moment it's probably harmless, I think, but the kernel could
rely on values there).
Instead, let's add the missing si_code values explicitly to the
switch statement.
Thanks,
Oren.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
