x86_64: Sigcontext improvements

Patch 1 is IMO a bug fix.

From: Andy Lutomirski <[email protected]>

The comment in the signal code says that apps can save/restore other segments on their own. It's true that apps can *save* SS on their own, but there's no way for apps to restore it: SYSCALL effectively resets SS to __USER_DS, so any value that user code tries to load into SS gets lost on entry to sigreturn.

This recycles two padding bytes in the segment selector area for SS.

While we're at it, we need a second change to make this useful. If the signal we're delivering is caused by a bad SS value, saving that value isn't enough. We need to remove that bad value from the regs before we try to deliver the signal. Oddly, x32 already got this right.

I suspect that 64-bit programs that try to run 16-bit code and use signals will have a lot of trouble without this.

Signed-off-by Andy Lutomirski <[email protected]>

From: Andy Lutomirski <[email protected]>

The comment in the signal code says that apps can save/restore other segments on their own. It's true that apps can *save* SS on their own, but there's no way for apps to restore it: SYSCALL effectively resets SS to __USER_DS, so any value that user code tries to load into SS gets lost on entry to sigreturn.

This recycles two padding bytes in the segment selector area for SS.

While we're at it, we need a second change to make this useful. If the signal we're delivering is caused by a bad SS value, saving that value isn't enough. We need to remove that bad value from the regs before we try to deliver the signal. Oddly, x32 already got this right.

From: Andy Lutomirski <[email protected]>

The comment in the signal code says that apps can save/restore other segments on their own. It's true that apps can *save* SS on their own, but there's no way for apps to restore it: SYSCALL effectively resets SS to __USER_DS, so any value that user code tries to load into SS gets lost on entry to sigreturn.

This recycles two padding bytes in the segment selector area for SS.

While we're at it, we need a second change to make this useful. If the signal we're delivering is caused by a bad SS value, saving that value isn't enough. We need to remove that bad value from the regs before we try to deliver the signal. Oddly, x32 already got this right.

I suspect that 64-bit programs that try to run 16-bit code and use signals will have a lot of trouble without this.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/sigcontext.h      |  2 +-
 arch/x86/include/uapi/asm/sigcontext.h |  2 +-
 arch/x86/kernel/signal.c               | 22 +++++++++++++---------
 3 files changed, 15 insertions(+), 11 deletions(-)

arch/x86/include/asm/sigcontext.h +1 -1

index 9dfce4e0417d..f910cdcb71fd 100644
@@ -59,7 +59,7 @@ struct sigcontext {
        unsigned short cs;
        unsigned short gs;
        unsigned short fs;
-       unsigned short __pad0;
+       unsigned short ss;
        unsigned long err;
        unsigned long trapno;
        unsigned long oldmask;

arch/x86/include/uapi/asm/sigcontext.h +1 -1

index d8b9f9081e86..076b11fd6fa1 100644
@@ -179,7 +179,7 @@ struct sigcontext {
        __u16 cs;
        __u16 gs;
        __u16 fs;
-       __u16 __pad0;
+       __u16 ss;
        __u64 err;
        __u64 trapno;
        __u64 oldmask;

arch/x86/kernel/signal.c +13 -10

index ed37a768d0fc..9511eb7f17b0 100644
@@ -94,15 +94,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
                COPY(r15);
 #endif /* CONFIG_X86_64 */
 
-#ifdef CONFIG_X86_32
                COPY_SEG_CPL3(cs);
                COPY_SEG_CPL3(ss);
-#else /* !CONFIG_X86_32 */
-               /* Kernel saves and restores only the CS segment register on signals,
-                * which is the bare minimum needed to allow mixed 32/64-bit code.
-                * App's signal handler can save/restore other segments if needed. */
-               COPY_SEG_CPL3(cs);
-#endif /* CONFIG_X86_32 */
 
                get_user_ex(tmpflags, &sc->flags);
                regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);

@@ -164,6 +157,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
                put_user_ex(regs->cs, &sc->cs);
                put_user_ex(0, &sc->gs);
                put_user_ex(0, &sc->fs);
+               put_user_ex(regs->ss, &sc->ss);
 #endif /* CONFIG_X86_32 */
 
                put_user_ex(fpstate, &sc->fpstate);

@@ -457,9 +451,19 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
 
        regs->sp = (unsigned long)frame;
 
-       /* Set up the CS register to run signal handlers in 64-bit mode,
-          even if the handler happens to be interrupting 32-bit code. */
+       /*
+        * Set up the CS and SS registers to run signal handlers in
+        * 64-bit mode, even if the handler happens to be interrupting
+        * 32-bit or 16-bit code.
+        *
+        * SS is subtle.  In 64-bit mode, we don't need any particular
+        * SS descriptor, but we do need SS to be valid.  It's possible
+        * that the old SS is entirely bogus -- this can happen if the
+        * signal we're trying to deliver is #GP or #SS caused by a bad
+        * SS value.
+        */
        regs->cs = __USER_CS;
+       regs->ss = __USER_DS;
 
        return 0;
 }
--

2.3.0

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

This commit breaks CRIU. I don't have any details yet. I'm going to investigate this issue and provide more details tomorrow.

[root@avagin-fc19-cr criu]# setsid sleep 1000 &
[1] 1225
[root@avagin-fc19-cr criu]# ps -C sleep
PID TTY TIME CMD
1226 ? 00:00:00 sleep
[root@avagin-fc19-cr criu]# ./criu dump -t 1226 -D dump --shell-job [root@avagin-fc19-cr criu]# ./criu restore -D dump --shell-job Error (parasite-syscall.c:923): Task is in unexpected state: b7f (SIGSEGV)

Quoted 4 lines · Andrey Wagin

This commit breaks CRIU. I don't have any details yet. I'm going to investigate this issue and provide more details tomorrow.

Quoted 8 lines

[root@avagin-fc19-cr criu]# setsid sleep 1000 &
[1] 1225
[root@avagin-fc19-cr criu]# ps -C sleep
PID TTY TIME CMD
1226 ? 00:00:00 sleep
[root@avagin-fc19-cr criu]# ./criu dump -t 1226 -D dump --shell-job [root@avagin-fc19-cr criu]# ./criu restore -D dump --shell-job Error (parasite-syscall.c:923): Task is in unexpected state: b7f (SIGSEGV)

This is funny. Because currenty I am looking into criu sources for quite different reason (and I HATE this reason ;)

Shot in a dark afer a quick grep: restore_gpregs() should initialize ->ss?

perhaps something like below... obviously uncompiled/untested.

And my grep can't find the definition of UserX86RegsEntry in crtools... Perhaps the change below needs CPREG1(ss, anothername).

Seriously, where is UserX86RegsEntry?

Oleg.

Quoted 4 lines · Andrey Wagin

This commit breaks CRIU. I don't have any details yet. I'm going to investigate this issue and provide more details tomorrow.

Quoted 8 lines

[root@avagin-fc19-cr criu]# setsid sleep 1000 &
[1] 1225
[root@avagin-fc19-cr criu]# ps -C sleep
PID TTY TIME CMD
1226 ? 00:00:00 sleep
[root@avagin-fc19-cr criu]# ./criu dump -t 1226 -D dump --shell-job [root@avagin-fc19-cr criu]# ./criu restore -D dump --shell-job Error (parasite-syscall.c:923): Task is in unexpected state: b7f (SIGSEGV)

This is funny. Because currenty I am looking into criu sources for quite different reason (and I HATE this reason ;)

Shot in a dark afer a quick grep: restore_gpregs() should initialize ->ss?

perhaps something like below... obviously uncompiled/untested.

And my grep can't find the definition of UserX86RegsEntry in crtools... Perhaps the change below needs CPREG1(ss, anothername).

Seriously, where is UserX86RegsEntry?

Oleg.

arch/x86/crtools.c +1 -0

@@ -475,6 +475,7 @@ int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r)
 	CPREG2(rip, ip);
 	CPREG2(eflags, flags);
 	CPREG1(cs);
+	CPREG1(ss);
 	CPREG1(gs);
 	CPREG1(fs);
 

arch/x86/include/asm/restorer.h +0 -0

index 70199fb..c04fb94 100644

Quoted 5 lines

arch/x86/crtools.c +0 -0

@@ -475,6 +475,7 @@ int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r)
        CPREG2(rip, ip);
        CPREG2(eflags, flags);

arch/x86/crtools.c int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r) · line 480

Quoted 4 lines

arch/x86/crtools.c +1 -0

        CPREG1(cs);
+       CPREG1(ss);
        CPREG1(gs);
        CPREG1(fs);

Huh? Is CRIU actually trying to build an entire sigcontext from scratch here? I don't see how this can reliably work across kernel versions or CPU versions.

Also, what's up with CPREG1(gs) and CPREG1(fs)? I assume that's redundant, because that hasn't worked for many years, but CRIU works, so there must be correct code somewhere to restore those regs.

Quoted 5 lines · Andy Lutomirski

Quoted 5 lines

arch/x86/crtools.c +0 -0

@@ -475,6 +475,7 @@ int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r)
        CPREG2(rip, ip);
        CPREG2(eflags, flags);

Quoted 8 lines

arch/x86/crtools.c int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r) · line 480

Quoted 4 lines

arch/x86/crtools.c +1 -0

        CPREG1(cs);
+       CPREG1(ss);
        CPREG1(gs);
        CPREG1(fs);

Huh? Is CRIU actually trying to build an entire sigcontext from scratch here? I don't see how this can reliably work across kernel versions or CPU versions.

Yes we are. And why it can't work reliably? As to CPU -- we're testing that cpu features saved in image should match ones provided by the kernel runtime, ie on the machine where we're restoring.

Quoted 3 lines

Also, what's up with CPREG1(gs) and CPREG1(fs)? I assume that's redundant, because that hasn't worked for many years, but CRIU works, so there must be correct code somewhere to restore those regs.

Basically the initial registers values are fetched with ptrace when program is been dumped, then on restore we copy them back into sigcontext. That said we don't mangle fs/gs anyhow simply restore the values back from dump.

Quoted 5 lines · Andy Lutomirski

Quoted 5 lines

arch/x86/crtools.c +0 -0

@@ -475,6 +475,7 @@ int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r)
        CPREG2(rip, ip);
        CPREG2(eflags, flags);

Quoted 8 lines

arch/x86/crtools.c int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r) · line 480

Quoted 4 lines

arch/x86/crtools.c +1 -0

        CPREG1(cs);
+       CPREG1(ss);
        CPREG1(gs);
        CPREG1(fs);

Huh? Is CRIU actually trying to build an entire sigcontext from scratch here? I don't see how this can reliably work across kernel versions or CPU versions.

Yes we are. And why it can't work reliably? As to CPU -- we're testing that cpu features saved in image should match ones provided by the kernel runtime, ie on the machine where we're restoring.

Quoted 16 lines · Cyrill Gorcunov

Quoted 13 lines · Andy Lutomirski

arch/x86/crtools.c int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r) · line 480

Quoted 9 lines

arch/x86/crtools.c +1 -0

@@ -475,6 +475,7 @@ int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r)
        CPREG2(rip, ip);
        CPREG2(eflags, flags);
        CPREG1(cs);
+       CPREG1(ss);
        CPREG1(gs);
        CPREG1(fs);

Huh? Is CRIU actually trying to build an entire sigcontext from scratch here? I don't see how this can reliably work across kernel versions or CPU versions.

Quoted 4 lines

Yes we are. And why it can't work reliably? As to CPU -- we're testing that cpu features saved in image should match ones provided by the kernel runtime, ie on the machine where we're restoring.

But, say, __USER_CS can be changed in kernel, and nobody should notice this.

Quoted 2 lines · Oleg Nesterov

Shot in a dark afer a quick grep: restore_gpregs() should initialize ->ss?

It hasn't been needed earlier, if this would help it means abi is broken, no? :) Otherwise I don't understand what's happening.

Quoted 6 lines

perhaps something like below... obviously uncompiled/untested.

And my grep can't find the definition of UserX86RegsEntry in crtools... Perhaps the change below needs CPREG1(ss, anothername).

Seriously, where is UserX86RegsEntry?

It's in protobif/core-x86.proto, welcome to protobuf hell.

Quoted 21 lines

Oleg.

arch/x86/crtools.c +1 -0

@@ -475,6 +475,7 @@ int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r)
 	CPREG2(rip, ip);
 	CPREG2(eflags, flags);
 	CPREG1(cs);
+	CPREG1(ss);
 	CPREG1(gs);
 	CPREG1(fs);
 

arch/x86/include/asm/restorer.h +0 -0

index 70199fb..c04fb94 100644

arch/x86/include/asm/restorer.h struct rt_sigcontext { · line 59

Quoted 8 lines

arch/x86/include/asm/restorer.h +1 -1

@@ -53,7 +53,7 @@ struct rt_sigcontext {
 	unsigned short			cs;
 	unsigned short			gs;
 	unsigned short			fs;
-	unsigned short			__pad0;
+	unsigned short			ss;
 	unsigned long			err;
 	unsigned long			trapno;
 	unsigned long			oldmask;

Cyrill

Quoted 5 lines

Basically the initial registers values are fetched with ptrace when program is been dumped, then on restore we copy them back into sigcontext. That said we don't mangle fs/gs anyhow simply restore the values back from dump.

Wouldn't it be a little safer to have the kernel make you a sane sigcontext by raising a signal and catching it rather than writing a new one from scratch? Grr, maybe the kernel should version its sigcontext structures.

Getting fs from ptrace and restoring it to sigcontext is useless -- ptrace handles fs correctly, but that sigcontext field is just padding.

Is CRIU at least reliably putting zero in the ss field? If so, we could add a special case to translate zero to __USER_DS on restore. (And then I'll update my test case, and we'll have to document it, etc.)

Quoted 5 lines · Cyrill Gorcunov

Quoted 2 lines · Oleg Nesterov

Shot in a dark afer a quick grep: restore_gpregs() should initialize ->ss?

Quoted 2 lines

It hasn't been needed earlier, if this would help it means abi is broken, no? :) Otherwise I don't understand what's happening.

until this commit the kernel simply forgot to restore ->ss in sigreturn().

after this commit, if your rt_sigframe has garbage in ->ss then SIGSEGV is clear.

Quoted 4 lines

Quoted 2 lines

Seriously, where is UserX86RegsEntry?

It's in protobif/core-x86.proto, welcome to protobuf hell.

Still can't find UserX86RegsEntry... OK, perhaps it comes from user_x86_regs_entry in protobuf/core-x86.proto. Then I guess the patch I sent can be compiled at least ;)

Quoted 16 lines · Cyrill Gorcunov

Quoted 13 lines · Andy Lutomirski

arch/x86/crtools.c int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r) · line 480

Quoted 9 lines

arch/x86/crtools.c +1 -0

@@ -475,6 +475,7 @@ int restore_gpregs(struct rt_sigframe *f, UserX86RegsEntry *r)
        CPREG2(rip, ip);
        CPREG2(eflags, flags);
        CPREG1(cs);
+       CPREG1(ss);
        CPREG1(gs);
        CPREG1(fs);

Huh? Is CRIU actually trying to build an entire sigcontext from scratch here? I don't see how this can reliably work across kernel versions or CPU versions.

Quoted 4 lines

Yes we are. And why it can't work reliably? As to CPU -- we're testing that cpu features saved in image should match ones provided by the kernel runtime, ie on the machine where we're restoring.

But, say, __USER_CS can be changed in kernel, and nobody should notice this.

But in this case "restore on another machine" or "restore after kernel upgrade" can fail.

So probably restore_gpregs() should only change the general-purpose regs, as its name suggests.

Quoted 6 lines · Cyrill Gorcunov

Quoted 5 lines · Oleg Nesterov

Quoted 2 lines

Shot in a dark afer a quick grep: restore_gpregs() should initialize ->ss?

Quoted 7 lines

Quoted 2 lines

It hasn't been needed earlier, if this would help it means abi is broken, no? :) Otherwise I don't understand what's happening.

until this commit the kernel simply forgot to restore ->ss in sigreturn().

after this commit, if your rt_sigframe has garbage in ->ss then SIGSEGV is clear.

OK, so setting up a proper ss here should fix problem.

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

Seriously, where is UserX86RegsEntry?

Quoted 5 lines

Quoted 1 line

It's in protobif/core-x86.proto, welcome to protobuf hell.

Still can't find UserX86RegsEntry... OK, perhaps it comes from user_x86_regs_entry in protobuf/core-x86.proto. Then I guess the patch I sent can be compiled at least ;)

Yeah, protobuf-c mangles "_" in message name so user_x86_regs_entry -> UserX86RegsEntry. Andrew should be able to test it tomorrow (hopefully it's no urgent right? ;)

This patch fixes the problem. Oleg, could you send this path in the criu maillist?

Quoted 3 lines · Andrey Wagin

This patch fixes the problem. Oleg, could you send this path in the criu maillist?

Sure, will do.

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Andrey Wagin

This patch fixes the problem. Oleg, could you send this path in the criu maillist?

Quoted 1 line

Sure, will do.

We still haven't answered one question: what's the kernel's position on ABI stability wrt CRIU? We clearly shouldn't make changes that break the principle of CRIU, but CRIU encodes so many tricky assumptions about the inner workings of the kernel that it's really tough to avoid breaking old CRIU versions.

So... do we introduce somewhat nasty code into the kernel to keep old CRIU versions working, or do we require that users who want to restore onto new kernels use new CRIU?

(It seems clear to me that CRIU should apply the patch regardless of what the kernel does. It will enable CRIU to work on the same class of programs that are fixed by the kernel change that started this thread.)

--Andy
.

Quoted 6 lines · Andy Lutomirski

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Andrey Wagin

This patch fixes the problem. Oleg, could you send this path in the criu maillist?

Quoted 7 lines

Quoted 1 line

Sure, will do.

We still haven't answered one question: what's the kernel's position on ABI stability wrt CRIU? We clearly shouldn't make changes that break the principle of CRIU, but CRIU encodes so many tricky assumptions about the inner workings of the kernel that it's really tough to avoid breaking old CRIU versions.

Well, we try hard to use only documented kernel API-s. Isn't the sigframe considered to be some sort of "stable API"? I mean -- it's visible by the userspace, nobody prevents glibc or gdb from messing with this stuff just by reading it from memory.

If it's "parse-able" e.g. like VDSO is, but we don't do it in CRIU -- then it's definitely a CRIU BUG to be fixed.

Quoted 4 lines

So... do we introduce somewhat nasty code into the kernel to keep old CRIU versions working, or do we require that users who want to restore onto new kernels use new CRIU?

It's OK (I think) to require newer versions of CRIU, it's easy to update one unlike the kernel ;)

But if "old" version of CRIU just crash the restored processes on "new" kernels and there's no way to detect this properly -- that's the problem.

Quoted 9 lines

(It seems clear to me that CRIU should apply the patch regardless of what the kernel does. It will enable CRIU to work on the same class of programs that are fixed by the kernel change that started this thread.)

--Andy
.

Thanks,
Pavel

Quoted 15 lines · Pavel Emelyanov

Quoted 6 lines · Andy Lutomirski

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Andrey Wagin

This patch fixes the problem. Oleg, could you send this path in the criu maillist?

Quoted 7 lines

Quoted 1 line

Sure, will do.

We still haven't answered one question: what's the kernel's position on ABI stability wrt CRIU? We clearly shouldn't make changes that break the principle of CRIU, but CRIU encodes so many tricky assumptions about the inner workings of the kernel that it's really tough to avoid breaking old CRIU versions.

Quoted 7 lines

Well, we try hard to use only documented kernel API-s. Isn't the sigframe considered to be some sort of "stable API"? I mean -- it's visible by the userspace, nobody prevents glibc or gdb from messing with this stuff just by reading it from memory.

If it's "parse-able" e.g. like VDSO is, but we don't do it in CRIU -- then it's definitely a CRIU BUG to be fixed.

It's certainly parseable by things like gdb. But it's also supposed to be extensible. hpa, any thoughts here?

Quoted 10 lines

Quoted 4 lines

So... do we introduce somewhat nasty code into the kernel to keep old CRIU versions working, or do we require that users who want to restore onto new kernels use new CRIU?

It's OK (I think) to require newer versions of CRIU, it's easy to update one unlike the kernel ;)

But if "old" version of CRIU just crash the restored processes on "new" kernels and there's no way to detect this properly -- that's the problem.

Yeah, that's unfortunate.

I don't have a great idea for how to work around this, unfortunately. Ideally we'd increment some kind of version counter or use an extension mechanism rather than shoving ss into a field that used to be padding.

--Andy

Quoted 6 lines

Quoted 6 lines

(It seems clear to me that CRIU should apply the patch regardless of what the kernel does. It will enable CRIU to work on the same class of programs that are fixed by the kernel change that started this thread.)

Quoted 7 lines

Quoted 3 lines

--Andy
.

Thanks,
Pavel

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Quoted 40 lines · Andy Lutomirski

Quoted 15 lines · Pavel Emelyanov

Quoted 6 lines · Andy Lutomirski

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Andrey Wagin

This patch fixes the problem. Oleg, could you send this path in the criu maillist?

Quoted 7 lines

Quoted 1 line

Sure, will do.

We still haven't answered one question: what's the kernel's position on ABI stability wrt CRIU? We clearly shouldn't make changes that break the principle of CRIU, but CRIU encodes so many tricky assumptions about the inner workings of the kernel that it's really tough to avoid breaking old CRIU versions.

Quoted 7 lines

Well, we try hard to use only documented kernel API-s. Isn't the sigframe considered to be some sort of "stable API"? I mean -- it's visible by the userspace, nobody prevents glibc or gdb from messing with this stuff just by reading it from memory.

If it's "parse-able" e.g. like VDSO is, but we don't do it in CRIU -- then it's definitely a CRIU BUG to be fixed.

It's certainly parseable by things like gdb. But it's also supposed to be extensible. hpa, any thoughts here?

Quoted 10 lines

Quoted 4 lines

So... do we introduce somewhat nasty code into the kernel to keep old CRIU versions working, or do we require that users who want to restore onto new kernels use new CRIU?

It's OK (I think) to require newer versions of CRIU, it's easy to update one unlike the kernel ;)

But if "old" version of CRIU just crash the restored processes on "new" kernels and there's no way to detect this properly -- that's the problem.

Yeah, that's unfortunate.

Quoted 4 lines

I don't have a great idea for how to work around this, unfortunately. Ideally we'd increment some kind of version counter or use an extension mechanism rather than shoving ss into a field that used to be padding.

fwiw currently we're passing zero in this __pad0 (replying to your previous email, so we can workaround in the kernel assuming zero as a special case, not that good but better than nothing).

Quoted 20 lines

--Andy

Quoted 6 lines

Quoted 6 lines

(It seems clear to me that CRIU should apply the patch regardless of what the kernel does. It will enable CRIU to work on the same class of programs that are fixed by the kernel change that started this thread.)

Quoted 7 lines

Quoted 3 lines

--Andy
.

Thanks,
Pavel

Quoted 4 lines

--
Andy Lutomirski
AMA Capital Management, LLC

Cyrill

Quoted 18 lines

Quoted 17 lines

Quoted 10 lines

Quoted 4 lines

So... do we introduce somewhat nasty code into the kernel to keep old CRIU versions working, or do we require that users who want to restore onto new kernels use new CRIU?

It's OK (I think) to require newer versions of CRIU, it's easy to update one unlike the kernel ;)

But if "old" version of CRIU just crash the restored processes on "new" kernels and there's no way to detect this properly -- that's the problem.

Yeah, that's unfortunate.

I don't have a great idea for how to work around this, unfortunately. Ideally we'd increment some kind of version counter or use an extension mechanism rather than shoving ss into a field that used to be padding.

Quoted 3 lines

fwiw currently we're passing zero in this __pad0 (replying to your previous email, so we can workaround in the kernel assuming zero as a special case, not that good but better than nothing).

We could store ss_plus_one instead of ss. Yuck.

The only real down side I can see to special casing zero is that it really is possible to end up with zero in there. For example, the SIGSEGV you get do to the failed sigreturn probably has sigcontext->ss == 0 :)

Quoted 4 lines · Andy Lutomirski

I don't have a great idea for how to work around this, unfortunately. Ideally we'd increment some kind of version counter or use an extension mechanism rather than shoving ss into a field that used to be padding.

fwiw currently we're passing zero in this __pad0 (replying to your previous email, so we can workaround in the kernel assuming zero as a special case, not that good but better than nothing).

Quoted 9 lines · Cyrill Gorcunov

Quoted 4 lines · Andy Lutomirski

I don't have a great idea for how to work around this, unfortunately. Ideally we'd increment some kind of version counter or use an extension mechanism rather than shoving ss into a field that used to be padding.

fwiw currently we're passing zero in this __pad0 (replying to your previous email, so we can workaround in the kernel assuming zero as a special case, not that good but better than nothing).

Special-casing zero sounds not that bad to me.
It can be removed after a few years - just don't forget to document it in a good comment: why we have special case? What software required it?
In which version of that software the need to have this hack was eliminated?

Quoted 11 lines

Quoted 4 lines

fwiw currently we're passing zero in this __pad0 (replying to your previous email, so we can workaround in the kernel assuming zero as a special case, not that good but better than nothing).

Special-casing zero sounds not that bad to me.
It can be removed after a few years - just don't forget to document it in a good comment: why we have special case? What software required it?
In which version of that software the need to have this hack was eliminated?

To be fair, such special case would be ideal for us, so that if noone object against such hack, i would cook a patch.

Quoted 7 lines · Cyrill Gorcunov

Quoted 7 lines

Quoted 4 lines

fwiw currently we're passing zero in this __pad0 (replying to your previous email, so we can workaround in the kernel assuming zero as a special case, not that good but better than nothing).

Special-casing zero sounds not that bad to me.
It can be removed after a few years

Quoted 2 lines

Quoted 2 lines

just don't forget
to document it in a good comment

 | The only real down side I can see to special casing zero is that it
 | really is possible to end up with zero in there.  For example, the
 | SIGSEGV you get do to the failed sigreturn probably has sigcontext->ss
 | == 0 :)

which I don't know how to resolve.

Another downside (to me) is that this special case can help, but only "by accident".

For example, criu has ->ss = 0, but it was not initialized explicitly.

But as I said, I won't argue.

Quoted 4 lines · Oleg Nesterov

Another downside (to me) is that this special case can help, but only "by accident".

Quoted 3 lines

For example, criu has ->ss = 0, but it was not initialized explicitly.

But as I said, I won't argue.

OK, after talking to Pavel -- it's fine to require new criu version for new kernels, thus lets leave kernel pure without this special hack, moreover now we have a fix from Oleg merged into criu so we should be safe. Thanks a lot for all comments!

As long as you're doing that, any chance you could remove CPREG1(gs) and CPREG1(fs) as well? They haven't had any effect since 2.5.something_very_old, and the kernel might want to reuse those sigcontext slots some day.

Quoted 5 lines · Andy Lutomirski

As long as you're doing that, any chance you could remove CPREG1(gs) and CPREG1(fs) as well? They haven't had any effect since 2.5.something_very_old, and the kernel might want to reuse those sigcontext slots some day.

Yeah, absolutely. I'm implementing 32bits support for criu now so I managed to postprone this task but will implement on the week. Thanks for reminder!

Quoted 7 lines · Cyrill Gorcunov

Quoted 5 lines · Andy Lutomirski

As long as you're doing that, any chance you could remove CPREG1(gs) and CPREG1(fs) as well? They haven't had any effect since 2.5.something_very_old, and the kernel might want to reuse those sigcontext slots some day.

Quoted 3 lines

Yeah, absolutely. I'm implementing 32bits support for criu now so I managed to postprone this task but will implement on the week. Thanks for reminder!

Yikes, have fun with that. I think there's a major vdso issue you'll hit. Make sure you test on Intel hardware. I'll try to help if you find stuff in my areas of the kernel.