Skip to contentSkip to conversation

checkpoint: handle more siginfo->si_code values

RFC In review Nathan Lynch 2 participants 2 messages Jun 17, 2010 – Jun 18, 2010

lore
Share & tools
lore

Nathan Lynch

author
#0 lore raw

RFC 1/1 checkpoint: handle more siginfo->si_code values

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.

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(-)
kernel/signal.c +6 -2
index e4ca9a6..cd25592 100644
@@ -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;
+
 	/* 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;
 	}
 }
 

@@ -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;
 	}
 
 	return 0;
Signature · 1 line
1.6.0.2
#1 lore raw
Quoted 3 lines · Nathan Lynch

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.

Quoted 12 lines

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(-)
kernel/signal.c +0 -0
index e4ca9a6..cd25592 100644

kernel/signal.c static void fill_siginfo(struct ckpt_siginfo *si, siginfo_t *info) · line 2924

Quoted 6 lines
kernel/signal.c +3 -0
@@ -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).

kernel/signal.c static void fill_siginfo(struct ckpt_siginfo *si, siginfo_t *info) · line 2960

Quoted 10 lines
kernel/signal.c +2 -1
 	/* 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 :)

kernel/signal.c static int load_siginfo(siginfo_t *info, struct ckpt_siginfo *si) · line 3009

Quoted 9 lines
kernel/signal.c +1 -1
 	}
 }
 

@@ -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.