* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Boris Ostrovsky @ 2017-10-17 13:58 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Juergen Gross, Mike Galbraith, Ingo Molnar, Peter Zijlstra,
Andrew Cooper, Rusty Russell, virtualization, x86, linux-kernel,
Chris Wright, live-patching, Linus Torvalds, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <20171017052413.nzbqniurzw7eim4b@treble>
On 10/17/2017 01:24 AM, Josh Poimboeuf wrote:
> On Mon, Oct 16, 2017 at 02:18:48PM -0400, Boris Ostrovsky wrote:
>> On 10/12/2017 03:53 PM, Boris Ostrovsky wrote:
>>> On 10/12/2017 03:27 PM, Andrew Cooper wrote:
>>>> On 12/10/17 20:11, Boris Ostrovsky wrote:
>>>>> There is also another problem:
>>>>>
>>>>> [ 1.312425] general protection fault: 0000 [#1] SMP
>>>>> [ 1.312901] Modules linked in:
>>>>> [ 1.313389] CPU: 0 PID: 1 Comm: init Not tainted 4.14.0-rc4+ #6
>>>>> [ 1.313878] task: ffff88003e2c0000 task.stack: ffffc9000038c000
>>>>> [ 1.314360] RIP: 10000e030:entry_SYSCALL_64_fastpath+0x1/0xa5
>>>>> [ 1.314854] RSP: e02b:ffffc9000038ff50 EFLAGS: 00010046
>>>>> [ 1.315336] RAX: 000000000000000c RBX: 000055f550168040 RCX:
>>>>> 00007fcfc959f59a
>>>>> [ 1.315827] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
>>>>> 0000000000000000
>>>>> [ 1.316315] RBP: 000000000000000a R08: 000000000000037f R09:
>>>>> 0000000000000064
>>>>> [ 1.316805] R10: 000000001f89cbf5 R11: ffff88003e2c0000 R12:
>>>>> 00007fcfc958ad60
>>>>> [ 1.317300] R13: 0000000000000000 R14: 000055f550185954 R15:
>>>>> 0000000000001000
>>>>> [ 1.317801] FS: 0000000000000000(0000) GS:ffff88003f800000(0000)
>>>>> knlGS:0000000000000000
>>>>> [ 1.318267] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> [ 1.318750] CR2: 00007fcfc97ab218 CR3: 000000003c88e000 CR4:
>>>>> 0000000000042660
>>>>> [ 1.319235] Call Trace:
>>>>> [ 1.319700] Code: 51 50 57 56 52 51 6a da 41 50 41 51 41 52 41 53 48
>>>>> 83 ec 30 65 4c 8b 1c 25 c0 d2 00 00 41 f7 03 df 39 08 90 0f 85 a5 00 00
>>>>> 00 50 <ff> 15 9c 95 d0 ff 58 48 3d 4c 01 00 00 77 0f 4c 89 d1 ff 14 c5
>>>>> [ 1.321161] RIP: entry_SYSCALL_64_fastpath+0x1/0xa5 RSP: ffffc9000038ff50
>>>>> [ 1.344255] ---[ end trace d7cb8cd6cd7c294c ]---
>>>>> [ 1.345009] Kernel panic - not syncing: Attempted to kill init!
>>>>> exitcode=0x0000000b
>>>>>
>>>>>
>>>>> All code
>>>>> ========
>>>>> 0: 51 push %rcx
>>>>> 1: 50 push %rax
>>>>> 2: 57 push %rdi
>>>>> 3: 56 push %rsi
>>>>> 4: 52 push %rdx
>>>>> 5: 51 push %rcx
>>>>> 6: 6a da pushq $0xffffffffffffffda
>>>>> 8: 41 50 push %r8
>>>>> a: 41 51 push %r9
>>>>> c: 41 52 push %r10
>>>>> e: 41 53 push %r11
>>>>> 10: 48 83 ec 30 sub $0x30,%rsp
>>>>> 14: 65 4c 8b 1c 25 c0 d2 mov %gs:0xd2c0,%r11
>>>>> 1b: 00 00
>>>>> 1d: 41 f7 03 df 39 08 90 testl $0x900839df,(%r11)
>>>>> 24: 0f 85 a5 00 00 00 jne 0xcf
>>>>> 2a: 50 push %rax
>>>>> 2b:* ff 15 9c 95 d0 ff callq *-0x2f6a64(%rip) #
>>>>> 0xffffffffffd095cd <-- trapping instruction
>>>>> 31: 58 pop %rax
>>>>> 32: 48 3d 4c 01 00 00 cmp $0x14c,%rax
>>>>> 38: 77 0f ja 0x49
>>>>> 3a: 4c 89 d1 mov %r10,%rcx
>>>>> 3d: ff .byte 0xff
>>>>> 3e: 14 c5 adc $0xc5,%al
>>>>>
>>>>>
>>>>> so the original 'cli' was replaced with the pv call but to me the offset
>>>>> looks a bit off, no? Shouldn't it always be positive?
>>>> callq takes a 32bit signed displacement, so jumping back by up to 2G is
>>>> perfectly legitimate.
>>> Yes, but
>>>
>>> ostr@workbase> nm vmlinux | grep entry_SYSCALL_64_fastpath
>>> ffffffff817365dd t entry_SYSCALL_64_fastpath
>>> ostr@workbase> nm vmlinux | grep " pv_irq_ops"
>>> ffffffff81c2dbc0 D pv_irq_ops
>>> ostr@workbase>
>>>
>>> so pv_irq_ops.irq_disable is about 5MB ahead of where we are now. (I
>>> didn't mean that x86 instruction set doesn't allow negative
>>> displacement, I was trying to say that pv_irq_ops always live further down)
>> I believe the problem is this:
>>
>> #define PV_INDIRECT(addr) *addr(%rip)
>>
>> The displacement that the linker computes will be relative to the where
>> this instruction is placed at the time of linking, which is in
>> .pv_altinstructions (and not .text). So when we copy it into .text the
>> displacement becomes bogus.
> apply_alternatives() is supposed to adjust that displacement based on
> the new IP, though it could be messing that up somehow. (See patch
> 10/13.)
>
That patch doesn't take into account the fact that replacement
instructions may have to save/restore registers. So, for example,
- if (a->replacementlen && is_jmp(replacement[0]))
+ } else if (a->replacementlen == 6 && *insnbuf == 0xff &&
+ *(insnbuf+1) == 0x15) {
+ /* indirect call */
+ *(s32 *)(insnbuf + 2) += replacement - instr;
+ DPRINTK("Fix indirect CALL offset: 0x%x, CALL *0x%lx",
+ *(s32 *)(insnbuf + 2),
+ (unsigned long)instr + *(s32 *)(insnbuf + 2) + 6);
+
doesn't do the adjustment of
2a: 50 push %rax
2b:* ff 15 9c 95 d0 ff callq *-0x2f6a64(%rip)
31: 58 pop %rax
because instbuf points to 'push' and not to 'call'.
-boris
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Boris Ostrovsky @ 2017-10-17 14:05 UTC (permalink / raw)
To: Brian Gerst
Cc: Juergen Gross, Mike Galbraith, H. Peter Anvin, Peter Zijlstra,
Andrew Cooper, Rusty Russell, Linux Virtualization,
the arch/x86 maintainers, Linux Kernel Mailing List, Chris Wright,
live-patching, Linus Torvalds, Borislav Petkov, Andy Lutomirski,
Josh Poimboeuf, Ingo Molnar, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <CAMzpN2j_6KzT22nLmYMM3RDxF0UkSQXCisb6umBka0T2ZHwDpQ@mail.gmail.com>
On 10/17/2017 09:10 AM, Brian Gerst wrote:
> On Mon, Oct 16, 2017 at 2:18 PM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>>
>> Replacing the macro with
>>
>> #define PV_INDIRECT(addr) *addr // well, it's not so much
>> indirect anymore
>>
>> makes things work. Or maybe it can be adjusted top be kept truly indirect.
> That is still an indirect call, just using absolute addressing for the
> pointer instead of RIP-relative.
Oh, right, I've got my terminology all wrong.
-boris
> Alternatives has very limited
> relocation capabilities. It will only handle a single call or jmp
> replacement. Using absolute addressing is slightly less efficient
> (takes one extra byte to encode, and needs a relocation for KASLR),
> but it works just as well. You could also relocate the instruction
> manually by adding the delta between the original and replacement code
> to the displacement.
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Josh Poimboeuf @ 2017-10-17 14:36 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Mike Galbraith, Ingo Molnar, Peter Zijlstra,
Andrew Cooper, Rusty Russell, virtualization, x86, linux-kernel,
Chris Wright, live-patching, Linus Torvalds, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <de621b0b-f767-222a-e1de-aabd0e9a0bf9@oracle.com>
On Tue, Oct 17, 2017 at 09:58:59AM -0400, Boris Ostrovsky wrote:
> On 10/17/2017 01:24 AM, Josh Poimboeuf wrote:
> > On Mon, Oct 16, 2017 at 02:18:48PM -0400, Boris Ostrovsky wrote:
> >> On 10/12/2017 03:53 PM, Boris Ostrovsky wrote:
> >>> On 10/12/2017 03:27 PM, Andrew Cooper wrote:
> >>>> On 12/10/17 20:11, Boris Ostrovsky wrote:
> >>>>> There is also another problem:
> >>>>>
> >>>>> [ 1.312425] general protection fault: 0000 [#1] SMP
> >>>>> [ 1.312901] Modules linked in:
> >>>>> [ 1.313389] CPU: 0 PID: 1 Comm: init Not tainted 4.14.0-rc4+ #6
> >>>>> [ 1.313878] task: ffff88003e2c0000 task.stack: ffffc9000038c000
> >>>>> [ 1.314360] RIP: 10000e030:entry_SYSCALL_64_fastpath+0x1/0xa5
> >>>>> [ 1.314854] RSP: e02b:ffffc9000038ff50 EFLAGS: 00010046
> >>>>> [ 1.315336] RAX: 000000000000000c RBX: 000055f550168040 RCX:
> >>>>> 00007fcfc959f59a
> >>>>> [ 1.315827] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> >>>>> 0000000000000000
> >>>>> [ 1.316315] RBP: 000000000000000a R08: 000000000000037f R09:
> >>>>> 0000000000000064
> >>>>> [ 1.316805] R10: 000000001f89cbf5 R11: ffff88003e2c0000 R12:
> >>>>> 00007fcfc958ad60
> >>>>> [ 1.317300] R13: 0000000000000000 R14: 000055f550185954 R15:
> >>>>> 0000000000001000
> >>>>> [ 1.317801] FS: 0000000000000000(0000) GS:ffff88003f800000(0000)
> >>>>> knlGS:0000000000000000
> >>>>> [ 1.318267] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>>>> [ 1.318750] CR2: 00007fcfc97ab218 CR3: 000000003c88e000 CR4:
> >>>>> 0000000000042660
> >>>>> [ 1.319235] Call Trace:
> >>>>> [ 1.319700] Code: 51 50 57 56 52 51 6a da 41 50 41 51 41 52 41 53 48
> >>>>> 83 ec 30 65 4c 8b 1c 25 c0 d2 00 00 41 f7 03 df 39 08 90 0f 85 a5 00 00
> >>>>> 00 50 <ff> 15 9c 95 d0 ff 58 48 3d 4c 01 00 00 77 0f 4c 89 d1 ff 14 c5
> >>>>> [ 1.321161] RIP: entry_SYSCALL_64_fastpath+0x1/0xa5 RSP: ffffc9000038ff50
> >>>>> [ 1.344255] ---[ end trace d7cb8cd6cd7c294c ]---
> >>>>> [ 1.345009] Kernel panic - not syncing: Attempted to kill init!
> >>>>> exitcode=0x0000000b
> >>>>>
> >>>>>
> >>>>> All code
> >>>>> ========
> >>>>> 0: 51 push %rcx
> >>>>> 1: 50 push %rax
> >>>>> 2: 57 push %rdi
> >>>>> 3: 56 push %rsi
> >>>>> 4: 52 push %rdx
> >>>>> 5: 51 push %rcx
> >>>>> 6: 6a da pushq $0xffffffffffffffda
> >>>>> 8: 41 50 push %r8
> >>>>> a: 41 51 push %r9
> >>>>> c: 41 52 push %r10
> >>>>> e: 41 53 push %r11
> >>>>> 10: 48 83 ec 30 sub $0x30,%rsp
> >>>>> 14: 65 4c 8b 1c 25 c0 d2 mov %gs:0xd2c0,%r11
> >>>>> 1b: 00 00
> >>>>> 1d: 41 f7 03 df 39 08 90 testl $0x900839df,(%r11)
> >>>>> 24: 0f 85 a5 00 00 00 jne 0xcf
> >>>>> 2a: 50 push %rax
> >>>>> 2b:* ff 15 9c 95 d0 ff callq *-0x2f6a64(%rip) #
> >>>>> 0xffffffffffd095cd <-- trapping instruction
> >>>>> 31: 58 pop %rax
> >>>>> 32: 48 3d 4c 01 00 00 cmp $0x14c,%rax
> >>>>> 38: 77 0f ja 0x49
> >>>>> 3a: 4c 89 d1 mov %r10,%rcx
> >>>>> 3d: ff .byte 0xff
> >>>>> 3e: 14 c5 adc $0xc5,%al
> >>>>>
> >>>>>
> >>>>> so the original 'cli' was replaced with the pv call but to me the offset
> >>>>> looks a bit off, no? Shouldn't it always be positive?
> >>>> callq takes a 32bit signed displacement, so jumping back by up to 2G is
> >>>> perfectly legitimate.
> >>> Yes, but
> >>>
> >>> ostr@workbase> nm vmlinux | grep entry_SYSCALL_64_fastpath
> >>> ffffffff817365dd t entry_SYSCALL_64_fastpath
> >>> ostr@workbase> nm vmlinux | grep " pv_irq_ops"
> >>> ffffffff81c2dbc0 D pv_irq_ops
> >>> ostr@workbase>
> >>>
> >>> so pv_irq_ops.irq_disable is about 5MB ahead of where we are now. (I
> >>> didn't mean that x86 instruction set doesn't allow negative
> >>> displacement, I was trying to say that pv_irq_ops always live further down)
> >> I believe the problem is this:
> >>
> >> #define PV_INDIRECT(addr) *addr(%rip)
> >>
> >> The displacement that the linker computes will be relative to the where
> >> this instruction is placed at the time of linking, which is in
> >> .pv_altinstructions (and not .text). So when we copy it into .text the
> >> displacement becomes bogus.
> > apply_alternatives() is supposed to adjust that displacement based on
> > the new IP, though it could be messing that up somehow. (See patch
> > 10/13.)
> >
>
> That patch doesn't take into account the fact that replacement
> instructions may have to save/restore registers. So, for example,
>
>
> - if (a->replacementlen && is_jmp(replacement[0]))
> + } else if (a->replacementlen == 6 && *insnbuf == 0xff &&
> + *(insnbuf+1) == 0x15) {
> + /* indirect call */
> + *(s32 *)(insnbuf + 2) += replacement - instr;
> + DPRINTK("Fix indirect CALL offset: 0x%x, CALL *0x%lx",
> + *(s32 *)(insnbuf + 2),
> + (unsigned long)instr + *(s32 *)(insnbuf + 2) + 6);
> +
>
> doesn't do the adjustment of
>
> 2a: 50 push %rax
> 2b:* ff 15 9c 95 d0 ff callq *-0x2f6a64(%rip)
> 31: 58 pop %rax
>
> because instbuf points to 'push' and not to 'call'.
Ah. I forgot that asm paravirt patches put the saves/restores _in_ the
replacement, whereas in C code they're _outside_ the replacement.
Changing PV_INDIRECT to use absolute addressing would be a decent fix,
but I think that would break the PIE support Thomas Garnier has been
working on.
Maybe we can add a new field to the alternatives entry struct which
specifies the offset to the CALL instruction, so apply_alternatives()
can find it.
--
Josh
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Boris Ostrovsky @ 2017-10-17 15:36 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Juergen Gross, Mike Galbraith, Ingo Molnar, Peter Zijlstra,
Andrew Cooper, Rusty Russell, virtualization, x86, linux-kernel,
Chris Wright, live-patching, Linus Torvalds, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <20171017143613.6i7auk3mqcqayx3o@treble>
On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
>
> Maybe we can add a new field to the alternatives entry struct which
> specifies the offset to the CALL instruction, so apply_alternatives()
> can find it.
We'd also have to assume that the restore part of an alternative entry
is the same size as the save part. Which is true now.
-boris
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Josh Poimboeuf @ 2017-10-17 20:17 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Mike Galbraith, Ingo Molnar, Peter Zijlstra,
Andrew Cooper, Rusty Russell, virtualization, x86, linux-kernel,
Chris Wright, live-patching, Linus Torvalds, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <2e7890fb-3634-0f3b-003f-fc1772504b9a@oracle.com>
On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky wrote:
> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
> >
> > Maybe we can add a new field to the alternatives entry struct which
> > specifies the offset to the CALL instruction, so apply_alternatives()
> > can find it.
>
> We'd also have to assume that the restore part of an alternative entry
> is the same size as the save part. Which is true now.
Why?
--
Josh
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Boris Ostrovsky @ 2017-10-17 20:36 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Juergen Gross, Mike Galbraith, Ingo Molnar, Peter Zijlstra,
Andrew Cooper, Rusty Russell, virtualization, x86, linux-kernel,
Chris Wright, live-patching, Linus Torvalds, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <20171017201743.tnw6wulu4gjvkqli@treble>
On 10/17/2017 04:17 PM, Josh Poimboeuf wrote:
> On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky wrote:
>> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
>>> Maybe we can add a new field to the alternatives entry struct which
>>> specifies the offset to the CALL instruction, so apply_alternatives()
>>> can find it.
>> We'd also have to assume that the restore part of an alternative entry
>> is the same size as the save part. Which is true now.
> Why?
>
Don't you need to know the size of the instruction without save and
restore part?
+ if (a->replacementlen == 6 && *insnbuf == 0xff && *(insnbuf+1) == 0x15)
Otherwise you'd need another field for the actual instruction length.
-boris
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Josh Poimboeuf @ 2017-10-17 20:50 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Mike Galbraith, Ingo Molnar, Peter Zijlstra,
Andrew Cooper, Rusty Russell, virtualization, x86, linux-kernel,
Chris Wright, live-patching, Linus Torvalds, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <44a2cbeb-7424-0a2a-7f9c-b669e049bded@oracle.com>
On Tue, Oct 17, 2017 at 04:36:00PM -0400, Boris Ostrovsky wrote:
> On 10/17/2017 04:17 PM, Josh Poimboeuf wrote:
> > On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky wrote:
> >> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
> >>> Maybe we can add a new field to the alternatives entry struct which
> >>> specifies the offset to the CALL instruction, so apply_alternatives()
> >>> can find it.
> >> We'd also have to assume that the restore part of an alternative entry
> >> is the same size as the save part. Which is true now.
> > Why?
> >
>
> Don't you need to know the size of the instruction without save and
> restore part?
>
> + if (a->replacementlen == 6 && *insnbuf == 0xff && *(insnbuf+1) == 0x15)
>
> Otherwise you'd need another field for the actual instruction length.
If we know where the CALL instruction starts, and can verify that it
starts with "ff 15", then we know the instruction length: 6 bytes.
Right?
--
Josh
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Boris Ostrovsky @ 2017-10-17 20:59 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Juergen Gross, Mike Galbraith, Ingo Molnar, Peter Zijlstra,
Andrew Cooper, Rusty Russell, virtualization, x86, linux-kernel,
Chris Wright, live-patching, Linus Torvalds, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <20171017205037.rnqto2i2hul4q2s4@treble>
On 10/17/2017 04:50 PM, Josh Poimboeuf wrote:
> On Tue, Oct 17, 2017 at 04:36:00PM -0400, Boris Ostrovsky wrote:
>> On 10/17/2017 04:17 PM, Josh Poimboeuf wrote:
>>> On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky wrote:
>>>> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
>>>>> Maybe we can add a new field to the alternatives entry struct which
>>>>> specifies the offset to the CALL instruction, so apply_alternatives()
>>>>> can find it.
>>>> We'd also have to assume that the restore part of an alternative entry
>>>> is the same size as the save part. Which is true now.
>>> Why?
>>>
>> Don't you need to know the size of the instruction without save and
>> restore part?
>>
>> + if (a->replacementlen == 6 && *insnbuf == 0xff && *(insnbuf+1) == 0x15)
>>
>> Otherwise you'd need another field for the actual instruction length.
> If we know where the CALL instruction starts, and can verify that it
> starts with "ff 15", then we know the instruction length: 6 bytes.
> Right?
>
Oh, OK. Then you shouldn't need a->replacementlen test(s?) in
apply_alternatives()?
-boris
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Josh Poimboeuf @ 2017-10-17 21:03 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Mike Galbraith, Ingo Molnar, Peter Zijlstra,
Andrew Cooper, Rusty Russell, virtualization, x86, linux-kernel,
Chris Wright, live-patching, Linus Torvalds, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <5f62f099-c773-ae64-d685-f6db77406020@oracle.com>
On Tue, Oct 17, 2017 at 04:59:41PM -0400, Boris Ostrovsky wrote:
> On 10/17/2017 04:50 PM, Josh Poimboeuf wrote:
> > On Tue, Oct 17, 2017 at 04:36:00PM -0400, Boris Ostrovsky wrote:
> >> On 10/17/2017 04:17 PM, Josh Poimboeuf wrote:
> >>> On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky wrote:
> >>>> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
> >>>>> Maybe we can add a new field to the alternatives entry struct which
> >>>>> specifies the offset to the CALL instruction, so apply_alternatives()
> >>>>> can find it.
> >>>> We'd also have to assume that the restore part of an alternative entry
> >>>> is the same size as the save part. Which is true now.
> >>> Why?
> >>>
> >> Don't you need to know the size of the instruction without save and
> >> restore part?
> >>
> >> + if (a->replacementlen == 6 && *insnbuf == 0xff && *(insnbuf+1) == 0x15)
> >>
> >> Otherwise you'd need another field for the actual instruction length.
> > If we know where the CALL instruction starts, and can verify that it
> > starts with "ff 15", then we know the instruction length: 6 bytes.
> > Right?
> >
>
> Oh, OK. Then you shouldn't need a->replacementlen test(s?) in
> apply_alternatives()?
Right. Though in the above code it was needed for a different reason,
to make sure it wasn't reading past the end of the buffer.
--
Josh
^ permalink raw reply
* CFP ICEIS 2018 - 20th Int.l Conf. on Enterprise Information Systems (Funchal, Madeira/Portugal)
From: iceis @ 2017-10-18 9:24 UTC (permalink / raw)
To: virtualization
SUBMISSION DEADLINE
20th International Conference on Enterprise Information Systems
Submission Deadline: October 30, 2017
http://www.iceis.org/
March 21 - 24, 2018
Funchal, Madeira, Portugal.
ICEIS is organized in 6 major tracks:
- Databases and Information Systems Integration
- Artificial Intelligence and Decision Support Systems
- Information Systems Analysis and Specification
- Software Agents and Internet Computing
- Human-Computer Interaction
- Enterprise Architecture
In Cooperation with SWIM, AAAI, ACM SIGAI, ACM SIGCHI, ACM SIGMIS.
With the presence of internationally distinguished keynote speakers:
Alexander Brodsky, George Mason University, United States
Plamen Angelov, Lancaster University, United Kingdom
Salvatore Distefano, Università degli Studi di Messina, Italy
David Aveiro, University of Madeira / Madeira-ITI, Portugal
A short list of presented papers will be selected so that revised and extended versions of these papers will be published by Springer.
All papers presented at the congress venue will also be available at the SCITEPRESS Digital Library (http://www.scitepress.org/DigitalLibrary/).
Should you have any question please don’t hesitate contacting me.
Kind regards,
ICEIS Secretariat
Address: Av. D. Manuel I, 27A, 2º esq.
2910-595 Setubal, Portugal
Tel: +351 265 520 184
Fax: +351 265 520 186
Web: http://www.iceis.org/
e-mail: iceis.secretariat@insticc.org
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
From: Tetsuo Handa @ 2017-10-18 10:59 UTC (permalink / raw)
To: mst; +Cc: virtualization, linux-mm, rmaksudova, den, mhocko
In-Reply-To: <20171016195317-mutt-send-email-mst@kernel.org>
Tetsuo Handa wrote:
> 20171016-deflate.log.xz continued printing "puff" messages without any OOM
> killer messages, for fill_balloon() always inflates faster than leak_balloon()
> deflates.
>
> Since the OOM killer cannot be invoked unless leak_balloon() completely
> deflates faster than fill_balloon() inflates, the guest remained unusable
> (e.g. unable to login via ssh) other than printing "puff" messages.
> This result was worse than 20171016-default.log.xz , for the system was
> not able to make any forward progress (i.e. complete OOM lockup).
I tested further and found that it is not complete OOM lockup.
It turned out that the reason of being unable to login via ssh was that fork()
was failing because __vm_enough_memory() was failing because
/proc/sys/vm/overcommit_memory was set to 0. Although virtio_balloon driver
was ready to release pages if asked via virtballoon_oom_notify() from
out_of_memory(), __vm_enough_memory() was not able to take such pages into
account. As a result, operations which need to use fork() were failing without
calling out_of_memory().
( http://lkml.kernel.org/r/201710181954.FHH51594.MtFOFLOQFSOHVJ@I-love.SAKURA.ne.jp )
Do you see anything wrong with the patch I used for emulating
VIRTIO_BALLOON_F_DEFLATE_ON_OOM path (shown below) ?
----------------------------------------
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f0b3a0b..a679ac2 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -164,7 +164,7 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
}
set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
- if (!virtio_has_feature(vb->vdev,
+ if (virtio_has_feature(vb->vdev,
VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
adjust_managed_page_count(page, -1);
}
@@ -184,7 +184,7 @@ static void release_pages_balloon(struct virtio_balloon *vb,
struct page *page, *next;
list_for_each_entry_safe(page, next, pages, lru) {
- if (!virtio_has_feature(vb->vdev,
+ if (virtio_has_feature(vb->vdev,
VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
adjust_managed_page_count(page, 1);
list_del(&page->lru);
@@ -363,7 +363,7 @@ static int virtballoon_oom_notify(struct notifier_block *self,
unsigned num_freed_pages;
vb = container_of(self, struct virtio_balloon, nb);
- if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
return NOTIFY_OK;
freed = parm;
----------------------------------------
> As I demonstrated above, VIRTIO_BALLOON_F_DEFLATE_ON_OOM can lead to complete
> OOM lockup because out_of_memory() => fill_balloon() => out_of_memory() =>
> fill_balloon() sequence can effectively disable the OOM killer when the host
> assumed that it's safe to inflate the balloon to a large portion of guest
> memory and this won't cause an OOM situation.
The other problem is that, although it is not complete OOM lockup, it is too
slow to wait if we hit out_of_memory() => fill_balloon() => out_of_memory() =>
fill_balloon() sequence.
> If leak_balloon() from out_of_memory() should be stronger than
> fill_balloon() from update_balloon_size_func(), we need to make
> sure that update_balloon_size_func() stops calling fill_balloon()
> when leak_balloon() was called from out_of_memory().
I tried below patch to reduce the possibility of hitting out_of_memory() =>
fill_balloon() => out_of_memory() => fill_balloon() sequence.
----------------------------------------
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index a679ac2..9037fee 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -57,7 +57,7 @@ struct virtio_balloon {
/* The balloon servicing is delegated to a freezable workqueue. */
struct work_struct update_balloon_stats_work;
- struct work_struct update_balloon_size_work;
+ struct delayed_work update_balloon_size_work;
/* Prevent updating balloon when it is being canceled. */
spinlock_t stop_update_lock;
@@ -88,6 +88,7 @@ struct virtio_balloon {
/* To register callback in oom notifier call chain */
struct notifier_block nb;
+ struct timer_list deflate_on_oom_timer;
};
static struct virtio_device_id id_table[] = {
@@ -141,7 +142,8 @@ static void set_page_pfns(struct virtio_balloon *vb,
page_to_balloon_pfn(page) + i);
}
-static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
+static unsigned fill_balloon(struct virtio_balloon *vb, size_t num,
+ unsigned long *delay)
{
struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
unsigned num_allocated_pages;
@@ -152,14 +154,21 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
mutex_lock(&vb->balloon_lock);
for (vb->num_pfns = 0; vb->num_pfns < num;
vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
- struct page *page = balloon_page_enqueue(vb_dev_info);
+ struct page *page;
+
+ if (timer_pending(&vb->deflate_on_oom_timer)) {
+ /* Wait for hold off timer expiracy. */
+ *delay = HZ;
+ break;
+ }
+ page = balloon_page_enqueue(vb_dev_info);
if (!page) {
dev_info_ratelimited(&vb->vdev->dev,
"Out of puff! Can't get %u pages\n",
VIRTIO_BALLOON_PAGES_PER_PAGE);
/* Sleep for at least 1/5 of a second before retry. */
- msleep(200);
+ *delay = HZ / 5;
break;
}
set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
@@ -310,7 +319,8 @@ static void virtballoon_changed(struct virtio_device *vdev)
spin_lock_irqsave(&vb->stop_update_lock, flags);
if (!vb->stop_update)
- queue_work(system_freezable_wq, &vb->update_balloon_size_work);
+ queue_delayed_work(system_freezable_wq,
+ &vb->update_balloon_size_work, 0);
spin_unlock_irqrestore(&vb->stop_update_lock, flags);
}
@@ -366,9 +376,13 @@ static int virtballoon_oom_notify(struct notifier_block *self,
if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
return NOTIFY_OK;
+ /* Hold off fill_balloon() for 60 seconds. */
+ mod_timer(&vb->deflate_on_oom_timer, jiffies + 60 * HZ);
freed = parm;
num_freed_pages = leak_balloon(vb, oom_pages);
update_balloon_size(vb);
+ dev_info_ratelimited(&vb->vdev->dev, "Released %u pages. Remains %u pages.\n",
+ num_freed_pages, vb->num_pages);
*freed += num_freed_pages;
return NOTIFY_OK;
@@ -387,19 +401,21 @@ static void update_balloon_size_func(struct work_struct *work)
{
struct virtio_balloon *vb;
s64 diff;
+ unsigned long delay = 0;
- vb = container_of(work, struct virtio_balloon,
+ vb = container_of(to_delayed_work(work), struct virtio_balloon,
update_balloon_size_work);
diff = towards_target(vb);
if (diff > 0)
- diff -= fill_balloon(vb, diff);
+ diff -= fill_balloon(vb, diff, &delay);
else if (diff < 0)
diff += leak_balloon(vb, -diff);
update_balloon_size(vb);
if (diff)
- queue_work(system_freezable_wq, work);
+ queue_delayed_work(system_freezable_wq, to_delayed_work(work),
+ delay);
}
static int init_vqs(struct virtio_balloon *vb)
@@ -521,6 +537,10 @@ static struct dentry *balloon_mount(struct file_system_type *fs_type,
#endif /* CONFIG_BALLOON_COMPACTION */
+static void timer_expired(unsigned long unused)
+{
+}
+
static int virtballoon_probe(struct virtio_device *vdev)
{
struct virtio_balloon *vb;
@@ -539,7 +559,8 @@ static int virtballoon_probe(struct virtio_device *vdev)
}
INIT_WORK(&vb->update_balloon_stats_work, update_balloon_stats_func);
- INIT_WORK(&vb->update_balloon_size_work, update_balloon_size_func);
+ INIT_DELAYED_WORK(&vb->update_balloon_size_work,
+ update_balloon_size_func);
spin_lock_init(&vb->stop_update_lock);
vb->stop_update = false;
vb->num_pages = 0;
@@ -553,6 +574,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
if (err)
goto out_free_vb;
+ setup_timer(&vb->deflate_on_oom_timer, timer_expired, 0);
vb->nb.notifier_call = virtballoon_oom_notify;
vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
err = register_oom_notifier(&vb->nb);
@@ -564,6 +586,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
if (IS_ERR(balloon_mnt)) {
err = PTR_ERR(balloon_mnt);
unregister_oom_notifier(&vb->nb);
+ del_timer_sync(&vb->deflate_on_oom_timer);
goto out_del_vqs;
}
@@ -573,6 +596,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
err = PTR_ERR(vb->vb_dev_info.inode);
kern_unmount(balloon_mnt);
unregister_oom_notifier(&vb->nb);
+ del_timer_sync(&vb->deflate_on_oom_timer);
vb->vb_dev_info.inode = NULL;
goto out_del_vqs;
}
@@ -611,11 +635,12 @@ static void virtballoon_remove(struct virtio_device *vdev)
struct virtio_balloon *vb = vdev->priv;
unregister_oom_notifier(&vb->nb);
+ del_timer_sync(&vb->deflate_on_oom_timer);
spin_lock_irq(&vb->stop_update_lock);
vb->stop_update = true;
spin_unlock_irq(&vb->stop_update_lock);
- cancel_work_sync(&vb->update_balloon_size_work);
+ cancel_delayed_work_sync(&vb->update_balloon_size_work);
cancel_work_sync(&vb->update_balloon_stats_work);
remove_common(vb);
----------------------------------------
While response was better than now, inflating again spoiled the effort.
Retrying to inflate until allocation fails is already too painful.
Complete log is at http://I-love.SAKURA.ne.jp/tmp/20171018-deflate.log.xz .
----------------------------------------
[ 19.529096] kworker/0:2: page allocation failure: order:0, mode:0x14310ca(GFP_HIGHUSER_MOVABLE|__GFP_NORETRY|__GFP_NOMEMALLOC), nodemask=(null)
[ 19.530721] kworker/0:2 cpuset=/ mems_allowed=0
[ 19.531581] CPU: 0 PID: 111 Comm: kworker/0:2 Not tainted 4.14.0-rc5+ #302
[ 19.532397] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
[ 19.533285] Workqueue: events_freezable update_balloon_size_func [virtio_balloon]
[ 19.534143] Call Trace:
[ 19.535015] dump_stack+0x63/0x87
[ 19.535844] warn_alloc+0x114/0x1c0
[ 19.536667] __alloc_pages_slowpath+0x9a6/0xba7
[ 19.537491] ? sched_clock_cpu+0x11/0xb0
[ 19.538311] __alloc_pages_nodemask+0x26a/0x290
[ 19.539188] alloc_pages_current+0x6a/0xb0
[ 19.540004] balloon_page_enqueue+0x25/0xf0
[ 19.540818] update_balloon_size_func+0xe1/0x260 [virtio_balloon]
[ 19.541626] process_one_work+0x149/0x360
[ 19.542417] worker_thread+0x4d/0x3c0
[ 19.543186] kthread+0x109/0x140
[ 19.543930] ? rescuer_thread+0x380/0x380
[ 19.544716] ? kthread_park+0x60/0x60
[ 19.545426] ret_from_fork+0x25/0x30
[ 19.546141] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 19.547903] virtio_balloon virtio3: Released 256 pages. Remains 1984834 pages.
[ 19.659660] virtio_balloon virtio3: Released 256 pages. Remains 1984578 pages.
[ 21.891392] virtio_balloon virtio3: Released 256 pages. Remains 1984322 pages.
[ 21.894719] virtio_balloon virtio3: Released 256 pages. Remains 1984066 pages.
[ 22.490131] virtio_balloon virtio3: Released 256 pages. Remains 1983810 pages.
[ 31.939666] virtio_balloon virtio3: Released 256 pages. Remains 1983554 pages.
[ 95.524753] kworker/0:2: page allocation failure: order:0, mode:0x14310ca(GFP_HIGHUSER_MOVABLE|__GFP_NORETRY|__GFP_NOMEMALLOC), nodemask=(null)
[ 95.525641] kworker/0:2 cpuset=/ mems_allowed=0
[ 95.526110] CPU: 0 PID: 111 Comm: kworker/0:2 Not tainted 4.14.0-rc5+ #302
[ 95.526552] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
[ 95.527018] Workqueue: events_freezable update_balloon_size_func [virtio_balloon]
[ 95.527492] Call Trace:
[ 95.527969] dump_stack+0x63/0x87
[ 95.528469] warn_alloc+0x114/0x1c0
[ 95.528922] __alloc_pages_slowpath+0x9a6/0xba7
[ 95.529388] ? qxl_image_free_objects+0x56/0x60 [qxl]
[ 95.529849] ? qxl_draw_opaque_fb+0x102/0x3a0 [qxl]
[ 95.530315] __alloc_pages_nodemask+0x26a/0x290
[ 95.530777] alloc_pages_current+0x6a/0xb0
[ 95.531243] balloon_page_enqueue+0x25/0xf0
[ 95.531703] update_balloon_size_func+0xe1/0x260 [virtio_balloon]
[ 95.532180] process_one_work+0x149/0x360
[ 95.532645] worker_thread+0x4d/0x3c0
[ 95.533143] kthread+0x109/0x140
[ 95.533622] ? rescuer_thread+0x380/0x380
[ 95.534100] ? kthread_park+0x60/0x60
[ 95.534568] ret_from_fork+0x25/0x30
[ 95.535093] warn_alloc_show_mem: 1 callbacks suppressed
[ 95.535093] Mem-Info:
[ 95.536072] active_anon:11171 inactive_anon:2084 isolated_anon:0
[ 95.536072] active_file:8 inactive_file:70 isolated_file:0
[ 95.536072] unevictable:0 dirty:0 writeback:0 unstable:0
[ 95.536072] slab_reclaimable:3554 slab_unreclaimable:6848
[ 95.536072] mapped:588 shmem:2144 pagetables:749 bounce:0
[ 95.536072] free:25859 free_pcp:72 free_cma:0
[ 95.538922] Node 0 active_anon:44684kB inactive_anon:8336kB active_file:32kB inactive_file:280kB unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:2352kB dirty:0kB writeback:0kB shmem:8576kB shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 10240kB writeback_tmp:0kB unstable:0kB all_unreclaimable? no
[ 95.540516] Node 0 DMA free:15900kB min:132kB low:164kB high:196kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB present:15992kB managed:15908kB mlocked:0kB kernel_stack:0kB pagetables:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
[ 95.542325] lowmem_reserve[]: 0 2954 7925 7925 7925
[ 95.543020] Node 0 DMA32 free:44748kB min:25144kB low:31428kB high:37712kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB present:3129308kB managed:3063740kB mlocked:0kB kernel_stack:0kB pagetables:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
[ 95.544949] lowmem_reserve[]: 0 0 4970 4970 4970
[ 95.545624] Node 0 Normal free:42788kB min:42304kB low:52880kB high:63456kB active_anon:44684kB inactive_anon:8336kB active_file:32kB inactive_file:108kB unevictable:0kB writepending:0kB present:5242880kB managed:5093540kB mlocked:0kB kernel_stack:1984kB pagetables:2996kB bounce:0kB free_pcp:372kB local_pcp:220kB free_cma:0kB
[ 95.547739] lowmem_reserve[]: 0 0 0 0 0
[ 95.548464] Node 0 DMA: 1*4kB (U) 1*8kB (U) 1*16kB (U) 0*32kB 2*64kB (U) 1*128kB (U) 1*256kB (U) 0*512kB 1*1024kB (U) 1*2048kB (M) 3*4096kB (M) = 15900kB
[ 95.549988] Node 0 DMA32: 3*4kB (UM) 4*8kB (UM) 2*16kB (U) 2*32kB (U) 1*64kB (U) 0*128kB 2*256kB (UM) 2*512kB (UM) 2*1024kB (UM) 2*2048kB (UM) 9*4096kB (M) = 44748kB
[ 95.551551] Node 0 Normal: 925*4kB (UME) 455*8kB (UME) 349*16kB (UME) 137*32kB (UME) 37*64kB (UME) 23*128kB (UME) 8*256kB (UME) 5*512kB (UM) 3*1024kB (UM) 6*2048kB (U) 0*4096kB = 42588kB
[ 95.553147] Node 0 hugepages_total=0 hugepages_free=0 hugepages_surp=0 hugepages_size=2048kB
[ 95.554030] 2224 total pagecache pages
[ 95.554874] 0 pages in swap cache
[ 95.555667] Swap cache stats: add 0, delete 0, find 0/0
[ 95.556469] Free swap = 0kB
[ 95.557280] Total swap = 0kB
[ 95.558079] 2097045 pages RAM
[ 95.558856] 0 pages HighMem/MovableOnly
[ 95.559652] 53748 pages reserved
[ 95.560444] 0 pages cma reserved
[ 95.561262] 0 pages hwpoisoned
[ 95.562086] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 95.565779] virtio_balloon virtio3: Released 256 pages. Remains 1984947 pages.
[ 96.265255] virtio_balloon virtio3: Released 256 pages. Remains 1984691 pages.
[ 105.498910] virtio_balloon virtio3: Released 256 pages. Remains 1984435 pages.
[ 105.500518] virtio_balloon virtio3: Released 256 pages. Remains 1984179 pages.
[ 105.520034] virtio_balloon virtio3: Released 256 pages. Remains 1983923 pages.
----------------------------------------
Michael S. Tsirkin wrote:
> I think that's the case. Question is, when can we inflate again?
I think that it is when the host explicitly asked again, for
VIRTIO_BALLOON_F_DEFLATE_ON_OOM path does not schedule for later inflation.
^ permalink raw reply related
* Re: [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
From: Michael S. Tsirkin @ 2017-10-18 17:16 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: virtualization, linux-mm, rmaksudova, den, mhocko
In-Reply-To: <201710181959.ACI05296.JLMVQOOFtHSOFF@I-love.SAKURA.ne.jp>
On Wed, Oct 18, 2017 at 07:59:23PM +0900, Tetsuo Handa wrote:
> Tetsuo Handa wrote:
> > 20171016-deflate.log.xz continued printing "puff" messages without any OOM
> > killer messages, for fill_balloon() always inflates faster than leak_balloon()
> > deflates.
> >
> > Since the OOM killer cannot be invoked unless leak_balloon() completely
> > deflates faster than fill_balloon() inflates, the guest remained unusable
> > (e.g. unable to login via ssh) other than printing "puff" messages.
> > This result was worse than 20171016-default.log.xz , for the system was
> > not able to make any forward progress (i.e. complete OOM lockup).
>
> I tested further and found that it is not complete OOM lockup.
>
> It turned out that the reason of being unable to login via ssh was that fork()
> was failing because __vm_enough_memory() was failing because
> /proc/sys/vm/overcommit_memory was set to 0. Although virtio_balloon driver
> was ready to release pages if asked via virtballoon_oom_notify() from
> out_of_memory(), __vm_enough_memory() was not able to take such pages into
> account. As a result, operations which need to use fork() were failing without
> calling out_of_memory().
> ( http://lkml.kernel.org/r/201710181954.FHH51594.MtFOFLOQFSOHVJ@I-love.SAKURA.ne.jp )
>
> Do you see anything wrong with the patch I used for emulating
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM path (shown below) ?
>
> ----------------------------------------
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f0b3a0b..a679ac2 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -164,7 +164,7 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> }
> set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> - if (!virtio_has_feature(vb->vdev,
> + if (virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> adjust_managed_page_count(page, -1);
> }
> @@ -184,7 +184,7 @@ static void release_pages_balloon(struct virtio_balloon *vb,
> struct page *page, *next;
>
> list_for_each_entry_safe(page, next, pages, lru) {
> - if (!virtio_has_feature(vb->vdev,
> + if (virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> adjust_managed_page_count(page, 1);
> list_del(&page->lru);
> @@ -363,7 +363,7 @@ static int virtballoon_oom_notify(struct notifier_block *self,
> unsigned num_freed_pages;
>
> vb = container_of(self, struct virtio_balloon, nb);
> - if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> return NOTIFY_OK;
>
> freed = parm;
> ----------------------------------------
Looks right but it's probably easier to configure qemu to set that
feature bit. Basically you just add deflate-on-oom=on to the
balloon device.
> > As I demonstrated above, VIRTIO_BALLOON_F_DEFLATE_ON_OOM can lead to complete
> > OOM lockup because out_of_memory() => fill_balloon() => out_of_memory() =>
> > fill_balloon() sequence can effectively disable the OOM killer when the host
> > assumed that it's safe to inflate the balloon to a large portion of guest
> > memory and this won't cause an OOM situation.
>
> The other problem is that, although it is not complete OOM lockup, it is too
> slow to wait if we hit out_of_memory() => fill_balloon() => out_of_memory() =>
> fill_balloon() sequence.
>
> > If leak_balloon() from out_of_memory() should be stronger than
> > fill_balloon() from update_balloon_size_func(), we need to make
> > sure that update_balloon_size_func() stops calling fill_balloon()
> > when leak_balloon() was called from out_of_memory().
>
> I tried below patch to reduce the possibility of hitting out_of_memory() =>
> fill_balloon() => out_of_memory() => fill_balloon() sequence.
>
> ----------------------------------------
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index a679ac2..9037fee 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -57,7 +57,7 @@ struct virtio_balloon {
>
> /* The balloon servicing is delegated to a freezable workqueue. */
> struct work_struct update_balloon_stats_work;
> - struct work_struct update_balloon_size_work;
> + struct delayed_work update_balloon_size_work;
>
> /* Prevent updating balloon when it is being canceled. */
> spinlock_t stop_update_lock;
> @@ -88,6 +88,7 @@ struct virtio_balloon {
>
> /* To register callback in oom notifier call chain */
> struct notifier_block nb;
> + struct timer_list deflate_on_oom_timer;
> };
>
> static struct virtio_device_id id_table[] = {
> @@ -141,7 +142,8 @@ static void set_page_pfns(struct virtio_balloon *vb,
> page_to_balloon_pfn(page) + i);
> }
>
> -static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> +static unsigned fill_balloon(struct virtio_balloon *vb, size_t num,
> + unsigned long *delay)
> {
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> unsigned num_allocated_pages;
> @@ -152,14 +154,21 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - struct page *page = balloon_page_enqueue(vb_dev_info);
> + struct page *page;
> +
> + if (timer_pending(&vb->deflate_on_oom_timer)) {
> + /* Wait for hold off timer expiracy. */
> + *delay = HZ;
> + break;
> + }
> + page = balloon_page_enqueue(vb_dev_info);
>
> if (!page) {
> dev_info_ratelimited(&vb->vdev->dev,
> "Out of puff! Can't get %u pages\n",
> VIRTIO_BALLOON_PAGES_PER_PAGE);
> /* Sleep for at least 1/5 of a second before retry. */
> - msleep(200);
> + *delay = HZ / 5;
> break;
> }
> set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> @@ -310,7 +319,8 @@ static void virtballoon_changed(struct virtio_device *vdev)
>
> spin_lock_irqsave(&vb->stop_update_lock, flags);
> if (!vb->stop_update)
> - queue_work(system_freezable_wq, &vb->update_balloon_size_work);
> + queue_delayed_work(system_freezable_wq,
> + &vb->update_balloon_size_work, 0);
> spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> }
>
> @@ -366,9 +376,13 @@ static int virtballoon_oom_notify(struct notifier_block *self,
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> return NOTIFY_OK;
>
> + /* Hold off fill_balloon() for 60 seconds. */
> + mod_timer(&vb->deflate_on_oom_timer, jiffies + 60 * HZ);
> freed = parm;
> num_freed_pages = leak_balloon(vb, oom_pages);
> update_balloon_size(vb);
> + dev_info_ratelimited(&vb->vdev->dev, "Released %u pages. Remains %u pages.\n",
> + num_freed_pages, vb->num_pages);
> *freed += num_freed_pages;
>
> return NOTIFY_OK;
> @@ -387,19 +401,21 @@ static void update_balloon_size_func(struct work_struct *work)
> {
> struct virtio_balloon *vb;
> s64 diff;
> + unsigned long delay = 0;
>
> - vb = container_of(work, struct virtio_balloon,
> + vb = container_of(to_delayed_work(work), struct virtio_balloon,
> update_balloon_size_work);
> diff = towards_target(vb);
>
> if (diff > 0)
> - diff -= fill_balloon(vb, diff);
> + diff -= fill_balloon(vb, diff, &delay);
> else if (diff < 0)
> diff += leak_balloon(vb, -diff);
> update_balloon_size(vb);
>
> if (diff)
> - queue_work(system_freezable_wq, work);
> + queue_delayed_work(system_freezable_wq, to_delayed_work(work),
> + delay);
> }
>
> static int init_vqs(struct virtio_balloon *vb)
> @@ -521,6 +537,10 @@ static struct dentry *balloon_mount(struct file_system_type *fs_type,
>
> #endif /* CONFIG_BALLOON_COMPACTION */
>
> +static void timer_expired(unsigned long unused)
> +{
> +}
> +
> static int virtballoon_probe(struct virtio_device *vdev)
> {
> struct virtio_balloon *vb;
> @@ -539,7 +559,8 @@ static int virtballoon_probe(struct virtio_device *vdev)
> }
>
> INIT_WORK(&vb->update_balloon_stats_work, update_balloon_stats_func);
> - INIT_WORK(&vb->update_balloon_size_work, update_balloon_size_func);
> + INIT_DELAYED_WORK(&vb->update_balloon_size_work,
> + update_balloon_size_func);
> spin_lock_init(&vb->stop_update_lock);
> vb->stop_update = false;
> vb->num_pages = 0;
> @@ -553,6 +574,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (err)
> goto out_free_vb;
>
> + setup_timer(&vb->deflate_on_oom_timer, timer_expired, 0);
> vb->nb.notifier_call = virtballoon_oom_notify;
> vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
> err = register_oom_notifier(&vb->nb);
> @@ -564,6 +586,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (IS_ERR(balloon_mnt)) {
> err = PTR_ERR(balloon_mnt);
> unregister_oom_notifier(&vb->nb);
> + del_timer_sync(&vb->deflate_on_oom_timer);
> goto out_del_vqs;
> }
>
> @@ -573,6 +596,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> err = PTR_ERR(vb->vb_dev_info.inode);
> kern_unmount(balloon_mnt);
> unregister_oom_notifier(&vb->nb);
> + del_timer_sync(&vb->deflate_on_oom_timer);
> vb->vb_dev_info.inode = NULL;
> goto out_del_vqs;
> }
> @@ -611,11 +635,12 @@ static void virtballoon_remove(struct virtio_device *vdev)
> struct virtio_balloon *vb = vdev->priv;
>
> unregister_oom_notifier(&vb->nb);
> + del_timer_sync(&vb->deflate_on_oom_timer);
>
> spin_lock_irq(&vb->stop_update_lock);
> vb->stop_update = true;
> spin_unlock_irq(&vb->stop_update_lock);
> - cancel_work_sync(&vb->update_balloon_size_work);
> + cancel_delayed_work_sync(&vb->update_balloon_size_work);
> cancel_work_sync(&vb->update_balloon_stats_work);
>
> remove_common(vb);
> ----------------------------------------
OK. Or if you use my patch, you can just set a flag and go
if (vb->oom)
msleep(1000);
at beginning of fill_balloon.
> While response was better than now, inflating again spoiled the effort.
> Retrying to inflate until allocation fails is already too painful.
>
> Complete log is at http://I-love.SAKURA.ne.jp/tmp/20171018-deflate.log.xz .
> ----------------------------------------
> [ 19.529096] kworker/0:2: page allocation failure: order:0, mode:0x14310ca(GFP_HIGHUSER_MOVABLE|__GFP_NORETRY|__GFP_NOMEMALLOC), nodemask=(null)
> [ 19.530721] kworker/0:2 cpuset=/ mems_allowed=0
> [ 19.531581] CPU: 0 PID: 111 Comm: kworker/0:2 Not tainted 4.14.0-rc5+ #302
> [ 19.532397] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> [ 19.533285] Workqueue: events_freezable update_balloon_size_func [virtio_balloon]
> [ 19.534143] Call Trace:
> [ 19.535015] dump_stack+0x63/0x87
> [ 19.535844] warn_alloc+0x114/0x1c0
> [ 19.536667] __alloc_pages_slowpath+0x9a6/0xba7
> [ 19.537491] ? sched_clock_cpu+0x11/0xb0
> [ 19.538311] __alloc_pages_nodemask+0x26a/0x290
> [ 19.539188] alloc_pages_current+0x6a/0xb0
> [ 19.540004] balloon_page_enqueue+0x25/0xf0
> [ 19.540818] update_balloon_size_func+0xe1/0x260 [virtio_balloon]
> [ 19.541626] process_one_work+0x149/0x360
> [ 19.542417] worker_thread+0x4d/0x3c0
> [ 19.543186] kthread+0x109/0x140
> [ 19.543930] ? rescuer_thread+0x380/0x380
> [ 19.544716] ? kthread_park+0x60/0x60
> [ 19.545426] ret_from_fork+0x25/0x30
> [ 19.546141] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 19.547903] virtio_balloon virtio3: Released 256 pages. Remains 1984834 pages.
> [ 19.659660] virtio_balloon virtio3: Released 256 pages. Remains 1984578 pages.
> [ 21.891392] virtio_balloon virtio3: Released 256 pages. Remains 1984322 pages.
> [ 21.894719] virtio_balloon virtio3: Released 256 pages. Remains 1984066 pages.
> [ 22.490131] virtio_balloon virtio3: Released 256 pages. Remains 1983810 pages.
> [ 31.939666] virtio_balloon virtio3: Released 256 pages. Remains 1983554 pages.
> [ 95.524753] kworker/0:2: page allocation failure: order:0, mode:0x14310ca(GFP_HIGHUSER_MOVABLE|__GFP_NORETRY|__GFP_NOMEMALLOC), nodemask=(null)
> [ 95.525641] kworker/0:2 cpuset=/ mems_allowed=0
> [ 95.526110] CPU: 0 PID: 111 Comm: kworker/0:2 Not tainted 4.14.0-rc5+ #302
> [ 95.526552] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> [ 95.527018] Workqueue: events_freezable update_balloon_size_func [virtio_balloon]
> [ 95.527492] Call Trace:
> [ 95.527969] dump_stack+0x63/0x87
> [ 95.528469] warn_alloc+0x114/0x1c0
> [ 95.528922] __alloc_pages_slowpath+0x9a6/0xba7
> [ 95.529388] ? qxl_image_free_objects+0x56/0x60 [qxl]
> [ 95.529849] ? qxl_draw_opaque_fb+0x102/0x3a0 [qxl]
> [ 95.530315] __alloc_pages_nodemask+0x26a/0x290
> [ 95.530777] alloc_pages_current+0x6a/0xb0
> [ 95.531243] balloon_page_enqueue+0x25/0xf0
> [ 95.531703] update_balloon_size_func+0xe1/0x260 [virtio_balloon]
> [ 95.532180] process_one_work+0x149/0x360
> [ 95.532645] worker_thread+0x4d/0x3c0
> [ 95.533143] kthread+0x109/0x140
> [ 95.533622] ? rescuer_thread+0x380/0x380
> [ 95.534100] ? kthread_park+0x60/0x60
> [ 95.534568] ret_from_fork+0x25/0x30
> [ 95.535093] warn_alloc_show_mem: 1 callbacks suppressed
> [ 95.535093] Mem-Info:
> [ 95.536072] active_anon:11171 inactive_anon:2084 isolated_anon:0
> [ 95.536072] active_file:8 inactive_file:70 isolated_file:0
> [ 95.536072] unevictable:0 dirty:0 writeback:0 unstable:0
> [ 95.536072] slab_reclaimable:3554 slab_unreclaimable:6848
> [ 95.536072] mapped:588 shmem:2144 pagetables:749 bounce:0
> [ 95.536072] free:25859 free_pcp:72 free_cma:0
> [ 95.538922] Node 0 active_anon:44684kB inactive_anon:8336kB active_file:32kB inactive_file:280kB unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:2352kB dirty:0kB writeback:0kB shmem:8576kB shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 10240kB writeback_tmp:0kB unstable:0kB all_unreclaimable? no
> [ 95.540516] Node 0 DMA free:15900kB min:132kB low:164kB high:196kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB present:15992kB managed:15908kB mlocked:0kB kernel_stack:0kB pagetables:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
> [ 95.542325] lowmem_reserve[]: 0 2954 7925 7925 7925
> [ 95.543020] Node 0 DMA32 free:44748kB min:25144kB low:31428kB high:37712kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB present:3129308kB managed:3063740kB mlocked:0kB kernel_stack:0kB pagetables:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
> [ 95.544949] lowmem_reserve[]: 0 0 4970 4970 4970
> [ 95.545624] Node 0 Normal free:42788kB min:42304kB low:52880kB high:63456kB active_anon:44684kB inactive_anon:8336kB active_file:32kB inactive_file:108kB unevictable:0kB writepending:0kB present:5242880kB managed:5093540kB mlocked:0kB kernel_stack:1984kB pagetables:2996kB bounce:0kB free_pcp:372kB local_pcp:220kB free_cma:0kB
> [ 95.547739] lowmem_reserve[]: 0 0 0 0 0
> [ 95.548464] Node 0 DMA: 1*4kB (U) 1*8kB (U) 1*16kB (U) 0*32kB 2*64kB (U) 1*128kB (U) 1*256kB (U) 0*512kB 1*1024kB (U) 1*2048kB (M) 3*4096kB (M) = 15900kB
> [ 95.549988] Node 0 DMA32: 3*4kB (UM) 4*8kB (UM) 2*16kB (U) 2*32kB (U) 1*64kB (U) 0*128kB 2*256kB (UM) 2*512kB (UM) 2*1024kB (UM) 2*2048kB (UM) 9*4096kB (M) = 44748kB
> [ 95.551551] Node 0 Normal: 925*4kB (UME) 455*8kB (UME) 349*16kB (UME) 137*32kB (UME) 37*64kB (UME) 23*128kB (UME) 8*256kB (UME) 5*512kB (UM) 3*1024kB (UM) 6*2048kB (U) 0*4096kB = 42588kB
> [ 95.553147] Node 0 hugepages_total=0 hugepages_free=0 hugepages_surp=0 hugepages_size=2048kB
> [ 95.554030] 2224 total pagecache pages
> [ 95.554874] 0 pages in swap cache
> [ 95.555667] Swap cache stats: add 0, delete 0, find 0/0
> [ 95.556469] Free swap = 0kB
> [ 95.557280] Total swap = 0kB
> [ 95.558079] 2097045 pages RAM
> [ 95.558856] 0 pages HighMem/MovableOnly
> [ 95.559652] 53748 pages reserved
> [ 95.560444] 0 pages cma reserved
> [ 95.561262] 0 pages hwpoisoned
> [ 95.562086] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 95.565779] virtio_balloon virtio3: Released 256 pages. Remains 1984947 pages.
> [ 96.265255] virtio_balloon virtio3: Released 256 pages. Remains 1984691 pages.
> [ 105.498910] virtio_balloon virtio3: Released 256 pages. Remains 1984435 pages.
> [ 105.500518] virtio_balloon virtio3: Released 256 pages. Remains 1984179 pages.
> [ 105.520034] virtio_balloon virtio3: Released 256 pages. Remains 1983923 pages.
> ----------------------------------------
>
> Michael S. Tsirkin wrote:
> > I think that's the case. Question is, when can we inflate again?
>
> I think that it is when the host explicitly asked again, for
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM path does not schedule for later inflation.
Problem is host has no idea when it's safe.
If we expect host to ask again after X seconds we
might just as well do it in the guest.
^ permalink raw reply
* Re: [PATCH] virtio_balloon: fix deadlock on OOM
From: Michael S. Tsirkin @ 2017-10-18 17:19 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: mhocko, linux-kernel, virtualization, linux-mm
In-Reply-To: <201710132306.FBC78628.OJLHFVQSFOtOMF@I-love.SAKURA.ne.jp>
On Fri, Oct 13, 2017 at 11:06:23PM +0900, Tetsuo Handa wrote:
> Michael S. Tsirkin wrote:
> > This is a replacement for
> > [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
> > but unlike that patch it actually deflates on oom even in presence of
> > lock contention.
>
> But Wei Wang is proposing VIRTIO_BALLOON_F_SG which will try to allocate
> memory, isn't he?
Hopefully that can be fixed by allocating outside the lock.
> >
> > drivers/virtio/virtio_balloon.c | 30 ++++++++++++++++++++++--------
> > include/linux/balloon_compaction.h | 38 +++++++++++++++++++++++++++++++++++++-
> > mm/balloon_compaction.c | 27 +++++++++++++++++++++------
> > 3 files changed, 80 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index f0b3a0b..725e366 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -143,16 +143,14 @@ static void set_page_pfns(struct virtio_balloon *vb,
> >
> > static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> > {
> > - struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> > unsigned num_allocated_pages;
> > + unsigned num_pfns;
> > + struct page *page;
> > + LIST_HEAD(pages);
> >
> > - /* We can only do one array worth at a time. */
> > - num = min(num, ARRAY_SIZE(vb->pfns));
> > -
>
> I don't think moving this min() to later is correct, for
> "num" can be e.g. 1048576, can't it?
Good catch, will fix. Thanks!
> > - mutex_lock(&vb->balloon_lock);
> > - for (vb->num_pfns = 0; vb->num_pfns < num;
> > - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> > - struct page *page = balloon_page_enqueue(vb_dev_info);
> > + for (num_pfns = 0; num_pfns < num;
> > + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> > + struct page *page = balloon_page_alloc();
> >
> > if (!page) {
> > dev_info_ratelimited(&vb->vdev->dev,
> > @@ -162,6 +160,22 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> > msleep(200);
> > break;
> > }
> > +
> > + balloon_page_push(&pages, page);
> > + }
>
> If balloon_page_alloc() did not fail, it will queue "num"
> (e.g. 1048576) pages into pages list, won't it?
>
> > +
> > + /* We can only do one array worth at a time. */
> > + num = min(num, ARRAY_SIZE(vb->pfns));
> > +
>
> Now we cap "num" to VIRTIO_BALLOON_ARRAY_PFNS_MAX (which is 256), but
>
> > + mutex_lock(&vb->balloon_lock);
> > +
> > + vb->num_pfns = 0;
> > +
> > + while ((page = balloon_page_pop(&pages))) {
>
> this loop will repeat for e.g. 1048576 times, and
>
> > + balloon_page_enqueue(&vb->vb_dev_info, page);
> > +
> > + vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > +
>
> we increment vb->num_pfns for e.g. 1048576 times which will go
> beyond VIRTIO_BALLOON_ARRAY_PFNS_MAX array index.
>
> > set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > if (!virtio_has_feature(vb->vdev,
^ permalink raw reply
* Re: [PATCH v1 15/27] compiler: Option to default to hidden symbols
From: Thomas Garnier via Virtualization @ 2017-10-18 23:15 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Nicolas Pitre, Michal Hocko, Radim Krčmář,
linux-doc, kvm list, Len Brown, Peter Zijlstra, Christopher Li,
Jan H . Schönherr, Alexei Starovoitov, virtualization,
David Howells, Paul Gortmaker, Waiman Long, Pavel Machek,
H . Peter Anvin, Kernel Hardening, Christoph Lameter,
Alok Kataria, the arch/x86 maintainers, Herbert Xu,
Daniel Borkmann
In-Reply-To: <20171012200201.GW11645@wotan.suse.de>
On Thu, Oct 12, 2017 at 1:02 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Wed, Oct 11, 2017 at 01:30:15PM -0700, Thomas Garnier wrote:
>> Provide an option to default visibility to hidden except for key
>> symbols. This option is disabled by default and will be used by x86_64
>> PIE support to remove errors between compilation units.
>>
>> The default visibility is also enabled for external symbols that are
>> compared as they maybe equals (start/end of sections). In this case,
>> older versions of GCC will remove the comparison if the symbols are
>> hidden. This issue exists at least on gcc 4.9 and before.
>>
>> Signed-off-by: Thomas Garnier <thgarnie@google.com>
>
> <-- snip -->
>
>> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
>> index 86e8f0b2537b..8f021783a929 100644
>> --- a/arch/x86/kernel/cpu/microcode/core.c
>> +++ b/arch/x86/kernel/cpu/microcode/core.c
>> @@ -144,8 +144,8 @@ static bool __init check_loader_disabled_bsp(void)
>> return *res;
>> }
>>
>> -extern struct builtin_fw __start_builtin_fw[];
>> -extern struct builtin_fw __end_builtin_fw[];
>> +extern struct builtin_fw __start_builtin_fw[] __default_visibility;
>> +extern struct builtin_fw __end_builtin_fw[] __default_visibility;
>>
>> bool get_builtin_firmware(struct cpio_data *cd, const char *name)
>> {
>
> <-- snip -->
>
>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
>> index e5da44eddd2f..1aa5d6dac9e1 100644
>> --- a/include/asm-generic/sections.h
>> +++ b/include/asm-generic/sections.h
>> @@ -30,6 +30,9 @@
>> * __irqentry_text_start, __irqentry_text_end
>> * __softirqentry_text_start, __softirqentry_text_end
>> */
>> +#ifdef CONFIG_DEFAULT_HIDDEN
>> +#pragma GCC visibility push(default)
>> +#endif
>> extern char _text[], _stext[], _etext[];
>> extern char _data[], _sdata[], _edata[];
>> extern char __bss_start[], __bss_stop[];
>> @@ -46,6 +49,9 @@ extern char __softirqentry_text_start[], __softirqentry_text_end[];
>>
>> /* Start and end of .ctors section - used for constructor calls. */
>> extern char __ctors_start[], __ctors_end[];
>> +#ifdef CONFIG_DEFAULT_HIDDEN
>> +#pragma GCC visibility pop
>> +#endif
>>
>> extern __visible const void __nosave_begin, __nosave_end;
>>
>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> index e95a2631e545..6997716f73bf 100644
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -78,6 +78,14 @@ extern void __chk_io_ptr(const volatile void __iomem *);
>> #include <linux/compiler-clang.h>
>> #endif
>>
>> +/* Useful for Position Independent Code to reduce global references */
>> +#ifdef CONFIG_DEFAULT_HIDDEN
>> +#pragma GCC visibility push(hidden)
>> +#define __default_visibility __attribute__((visibility ("default")))
>
> Does this still work with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION ?
I cannot make it work with or without this change. How is it supposed
to be used?
For me with, it crashes with a bad consdev at:
http://elixir.free-electrons.com/linux/latest/source/drivers/tty/tty_io.c#L3194
>
>> +#else
>> +#define __default_visibility
>> +#endif
>> +
>> /*
>> * Generic compiler-dependent macros required for kernel
>> * build go below this comment. Actual compiler/compiler version
>> diff --git a/init/Kconfig b/init/Kconfig
>> index ccb1d8daf241..b640201fcff7 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1649,6 +1649,13 @@ config PROFILING
>> config TRACEPOINTS
>> bool
>>
>> +#
>> +# Default to hidden visibility for all symbols.
>> +# Useful for Position Independent Code to reduce global references.
>> +#
>> +config DEFAULT_HIDDEN
>> + bool
>
> Note it is default.
>
> Has 0-day ran through this git tree? It should be easy to get it added for
> testing. Also, even though most changes are x86 based there are some generic
> changes and I'd love a warm fuzzy this won't break odd / random builds.
> Although 0-day does cover a lot of test cases, it only has limited run time
> tests. There are some other test beds which also cover some more obscure
> architectures. Having a test pass on Guenter's test bed would be nice to
> see. For that please coordinate with Guenter if he's willing to run this
> a test for you.
Not yet, plan to give a v1.5 to Kees Cook to keep in one of his tree
for couple weeks. I expect it will identify interesting issues.
>
> Luis
--
Thomas
^ permalink raw reply
* Re: [PATCH v1 00/27] x86: PIE support and option to extend KASLR randomization
From: Thomas Garnier via Virtualization @ 2017-10-18 23:17 UTC (permalink / raw)
To: Tom Lendacky
Cc: Nicolas Pitre, Michal Hocko, linux-doc, Daniel Micay,
Radim Krčmář, Peter Zijlstra, Christopher Li,
Jan H . Schönherr, Alexei Starovoitov, virtualization,
David Howells, Paul Gortmaker, Waiman Long, Pavel Machek,
H . Peter Anvin, Kernel Hardening, Christoph Lameter,
Thomas Gleixner, the arch/x86 maintainers, Herbert Xu,
Daniel Borkmann, Jonathan Corbet
In-Reply-To: <18a89941-c80d-7007-574b-584f39f90f49@amd.com>
On Thu, Oct 12, 2017 at 9:28 AM, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> On 10/12/2017 10:34 AM, Thomas Garnier wrote:
>>
>> On Wed, Oct 11, 2017 at 2:34 PM, Tom Lendacky <thomas.lendacky@amd.com>
>> wrote:
>>>
>>> On 10/11/2017 3:30 PM, Thomas Garnier wrote:
>>>>
>>>> Changes:
>>>> - patch v1:
>>>> - Simplify ftrace implementation.
>>>> - Use gcc mstack-protector-guard-reg=%gs with PIE when possible.
>>>> - rfc v3:
>>>> - Use --emit-relocs instead of -pie to reduce dynamic relocation
>>>> space on
>>>> mapped memory. It also simplifies the relocation process.
>>>> - Move the start the module section next to the kernel. Remove the
>>>> need for
>>>> -mcmodel=large on modules. Extends module space from 1 to 2G
>>>> maximum.
>>>> - Support for XEN PVH as 32-bit relocations can be ignored with
>>>> --emit-relocs.
>>>> - Support for GOT relocations previously done automatically with
>>>> -pie.
>>>> - Remove need for dynamic PLT in modules.
>>>> - Support dymamic GOT for modules.
>>>> - rfc v2:
>>>> - Add support for global stack cookie while compiler default to fs
>>>> without
>>>> mcmodel=kernel
>>>> - Change patch 7 to correctly jump out of the identity mapping on
>>>> kexec load
>>>> preserve.
>>>>
>>>> These patches make the changes necessary to build the kernel as Position
>>>> Independent Executable (PIE) on x86_64. A PIE kernel can be relocated
>>>> below
>>>> the top 2G of the virtual address space. It allows to optionally extend
>>>> the
>>>> KASLR randomization range from 1G to 3G.
>>>
>>>
>>> Hi Thomas,
>>>
>>> I've applied your patches so that I can verify that SME works with PIE.
>>> Unfortunately, I'm running into build warnings and errors when I enable
>>> PIE.
>>>
>>> With CONFIG_STACK_VALIDATION=y I receive lots of messages like this:
>>>
>>> drivers/scsi/libfc/fc_exch.o: warning: objtool:
>>> fc_destroy_exch_mgr()+0x0: call without frame pointer save/setup
>>>
>>> Disabling CONFIG_STACK_VALIDATION suppresses those.
>>
>>
>> I ran into that, I plan to fix it in the next iteration.
>>
>>>
>>> But near the end of the build, I receive errors like this:
>>>
>>> arch/x86/kernel/setup.o: In function `dump_kernel_offset':
>>> .../arch/x86/kernel/setup.c:801:(.text+0x32): relocation truncated to
>>> fit: R_X86_64_32S against symbol `_text' defined in .text section in
>>> .tmp_vmlinux1
>>> .
>>> . about 10 more of the above type messages
>>> .
>>> make: *** [vmlinux] Error 1
>>> Error building kernel, exiting
>>>
>>> Are there any config options that should or should not be enabled when
>>> building with PIE enabled? Is there a compiler requirement for PIE (I'm
>>> using gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.5))?
>>
>>
>> I never ran into these ones and I tested compilers older and newer.
>> What was your exact configuration?
>
>
> I'll send you the config in a separate email.
>
> Thanks,
> Tom
Thanks for your feedback (Tom and Markus). The issue was linked to
using a modern gcc with a modern linker, I managed to repro and fix it
on my current version.
I will create a v1.5 for Kees Cook to keep on one of his branch for
few weeks so I can collect as much feedback from 0day. After that I
will send v2.
>
>
>>
>>>
>>> Thanks,
>>> Tom
>>>
>>>>
>>>> Thanks a lot to Ard Biesheuvel & Kees Cook on their feedback on compiler
>>>> changes, PIE support and KASLR in general. Thanks to Roland McGrath on
>>>> his
>>>> feedback for using -pie versus --emit-relocs and details on compiler
>>>> code
>>>> generation.
>>>>
>>>> The patches:
>>>> - 1-3, 5-1#, 17-18: Change in assembly code to be PIE compliant.
>>>> - 4: Add a new _ASM_GET_PTR macro to fetch a symbol address
>>>> generically.
>>>> - 14: Adapt percpu design to work correctly when PIE is enabled.
>>>> - 15: Provide an option to default visibility to hidden except for
>>>> key symbols.
>>>> It removes errors between compilation units.
>>>> - 16: Adapt relocation tool to handle PIE binary correctly.
>>>> - 19: Add support for global cookie.
>>>> - 20: Support ftrace with PIE (used on Ubuntu config).
>>>> - 21: Fix incorrect address marker on dump_pagetables.
>>>> - 22: Add option to move the module section just after the kernel.
>>>> - 23: Adapt module loading to support PIE with dynamic GOT.
>>>> - 24: Make the GOT read-only.
>>>> - 25: Add the CONFIG_X86_PIE option (off by default).
>>>> - 26: Adapt relocation tool to generate a 64-bit relocation table.
>>>> - 27: Add the CONFIG_RANDOMIZE_BASE_LARGE option to increase
>>>> relocation range
>>>> from 1G to 3G (off by default).
>>>>
>>>> Performance/Size impact:
>>>>
>>>> Size of vmlinux (Default configuration):
>>>> File size:
>>>> - PIE disabled: +0.000031%
>>>> - PIE enabled: -3.210% (less relocations)
>>>> .text section:
>>>> - PIE disabled: +0.000644%
>>>> - PIE enabled: +0.837%
>>>>
>>>> Size of vmlinux (Ubuntu configuration):
>>>> File size:
>>>> - PIE disabled: -0.201%
>>>> - PIE enabled: -0.082%
>>>> .text section:
>>>> - PIE disabled: same
>>>> - PIE enabled: +1.319%
>>>>
>>>> Size of vmlinux (Default configuration + ORC):
>>>> File size:
>>>> - PIE enabled: -3.167%
>>>> .text section:
>>>> - PIE enabled: +0.814%
>>>>
>>>> Size of vmlinux (Ubuntu configuration + ORC):
>>>> File size:
>>>> - PIE enabled: -3.167%
>>>> .text section:
>>>> - PIE enabled: +1.26%
>>>>
>>>> The size increase is mainly due to not having access to the 32-bit
>>>> signed
>>>> relocation that can be used with mcmodel=kernel. A small part is due to
>>>> reduced
>>>> optimization for PIE code. This bug [1] was opened with gcc to provide a
>>>> better
>>>> code generation for kernel PIE.
>>>>
>>>> Hackbench (50% and 1600% on thread/process for pipe/sockets):
>>>> - PIE disabled: no significant change (avg +0.1% on latest test).
>>>> - PIE enabled: between -0.50% to +0.86% in average (default and
>>>> Ubuntu config).
>>>>
>>>> slab_test (average of 10 runs):
>>>> - PIE disabled: no significant change (-2% on latest run, likely
>>>> noise).
>>>> - PIE enabled: between -1% and +0.8% on latest runs.
>>>>
>>>> Kernbench (average of 10 Half and Optimal runs):
>>>> Elapsed Time:
>>>> - PIE disabled: no significant change (avg -0.239%)
>>>> - PIE enabled: average +0.07%
>>>> System Time:
>>>> - PIE disabled: no significant change (avg -0.277%)
>>>> - PIE enabled: average +0.7%
>>>>
>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82303
>>>>
>>>> diffstat:
>>>> Documentation/x86/x86_64/mm.txt | 3
>>>> arch/x86/Kconfig | 43 ++++++
>>>> arch/x86/Makefile | 40 +++++
>>>> arch/x86/boot/boot.h | 2
>>>> arch/x86/boot/compressed/Makefile | 5
>>>> arch/x86/boot/compressed/misc.c | 10 +
>>>> arch/x86/crypto/aes-x86_64-asm_64.S | 45 ++++--
>>>> arch/x86/crypto/aesni-intel_asm.S | 14 +-
>>>> arch/x86/crypto/aesni-intel_avx-x86_64.S | 6
>>>> arch/x86/crypto/camellia-aesni-avx-asm_64.S | 42 +++---
>>>> arch/x86/crypto/camellia-aesni-avx2-asm_64.S | 44 +++---
>>>> arch/x86/crypto/camellia-x86_64-asm_64.S | 8 -
>>>> arch/x86/crypto/cast5-avx-x86_64-asm_64.S | 50 ++++---
>>>> arch/x86/crypto/cast6-avx-x86_64-asm_64.S | 44 +++---
>>>> arch/x86/crypto/des3_ede-asm_64.S | 96 +++++++++-----
>>>> arch/x86/crypto/ghash-clmulni-intel_asm.S | 4
>>>> arch/x86/crypto/glue_helper-asm-avx.S | 4
>>>> arch/x86/crypto/glue_helper-asm-avx2.S | 6
>>>> arch/x86/entry/entry_32.S | 3
>>>> arch/x86/entry/entry_64.S | 29 ++--
>>>> arch/x86/include/asm/asm.h | 13 +
>>>> arch/x86/include/asm/bug.h | 2
>>>> arch/x86/include/asm/ftrace.h | 6
>>>> arch/x86/include/asm/jump_label.h | 8 -
>>>> arch/x86/include/asm/kvm_host.h | 6
>>>> arch/x86/include/asm/module.h | 11 +
>>>> arch/x86/include/asm/page_64_types.h | 9 +
>>>> arch/x86/include/asm/paravirt_types.h | 12 +
>>>> arch/x86/include/asm/percpu.h | 25 ++-
>>>> arch/x86/include/asm/pgtable_64_types.h | 6
>>>> arch/x86/include/asm/pm-trace.h | 2
>>>> arch/x86/include/asm/processor.h | 12 +
>>>> arch/x86/include/asm/sections.h | 8 +
>>>> arch/x86/include/asm/setup.h | 2
>>>> arch/x86/include/asm/stackprotector.h | 19 ++
>>>> arch/x86/kernel/acpi/wakeup_64.S | 31 ++--
>>>> arch/x86/kernel/asm-offsets.c | 3
>>>> arch/x86/kernel/asm-offsets_32.c | 3
>>>> arch/x86/kernel/asm-offsets_64.c | 3
>>>> arch/x86/kernel/cpu/common.c | 7 -
>>>> arch/x86/kernel/cpu/microcode/core.c | 4
>>>> arch/x86/kernel/ftrace.c | 42 +++++-
>>>> arch/x86/kernel/head64.c | 32 +++-
>>>> arch/x86/kernel/head_32.S | 3
>>>> arch/x86/kernel/head_64.S | 41 +++++-
>>>> arch/x86/kernel/kvm.c | 6
>>>> arch/x86/kernel/module.c | 182
>>>> ++++++++++++++++++++++++++-
>>>> arch/x86/kernel/module.lds | 3
>>>> arch/x86/kernel/process.c | 5
>>>> arch/x86/kernel/relocate_kernel_64.S | 8 -
>>>> arch/x86/kernel/setup_percpu.c | 2
>>>> arch/x86/kernel/vmlinux.lds.S | 13 +
>>>> arch/x86/kvm/svm.c | 4
>>>> arch/x86/lib/cmpxchg16b_emu.S | 8 -
>>>> arch/x86/mm/dump_pagetables.c | 11 +
>>>> arch/x86/power/hibernate_asm_64.S | 4
>>>> arch/x86/tools/relocs.c | 170
>>>> +++++++++++++++++++++++--
>>>> arch/x86/tools/relocs.h | 4
>>>> arch/x86/tools/relocs_common.c | 15 +-
>>>> arch/x86/xen/xen-asm.S | 12 -
>>>> arch/x86/xen/xen-head.S | 9 -
>>>> arch/x86/xen/xen-pvh.S | 13 +
>>>> drivers/base/firmware_class.c | 4
>>>> include/asm-generic/sections.h | 6
>>>> include/asm-generic/vmlinux.lds.h | 12 +
>>>> include/linux/compiler.h | 8 +
>>>> init/Kconfig | 9 +
>>>> kernel/kallsyms.c | 16 +-
>>>> kernel/trace/trace.h | 4
>>>> lib/dynamic_debug.c | 4
>>>> 70 files changed, 1032 insertions(+), 308 deletions(-)
>>>>
>>
>>
>>
>
--
Thomas
^ permalink raw reply
* Re: [PATCH] virtio_balloon: fix deadlock on OOM
From: Wei Wang @ 2017-10-19 7:59 UTC (permalink / raw)
To: Michael S. Tsirkin, Tetsuo Handa
Cc: linux-mm, mhocko, linux-kernel, virtualization
In-Reply-To: <20171018201700-mutt-send-email-mst@kernel.org>
On 10/19/2017 01:19 AM, Michael S. Tsirkin wrote:
> On Fri, Oct 13, 2017 at 11:06:23PM +0900, Tetsuo Handa wrote:
>> Michael S. Tsirkin wrote:
>>> This is a replacement for
>>> [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
>>> but unlike that patch it actually deflates on oom even in presence of
>>> lock contention.
>> But Wei Wang is proposing VIRTIO_BALLOON_F_SG which will try to allocate
>> memory, isn't he?
> Hopefully that can be fixed by allocating outside the lock.
>
I think that would still have an issue even without the lock, because we
can't do
any memory allocation in the OOM code path.
Probably, we could write a separate function, leak_balloon_oom() for the
oom notifier,
which puts the oom deflating pages to the vq one by one, and kick when
the vq is full.
In this case, we would need to stop the normal leak_balloon while oom
deflating starts.
However, a better optimization I think would be to do some kind of
consolidation, since
leak_balloon is already deflating, leak_ballon_oom can just count the
number of pages
that have been deflated by leak_balloon and return when it reaches
oom_pages.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v16 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
From: Wei Wang @ 2017-10-19 8:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mawilcox@microsoft.com,
qemu-devel@nongnu.org, amit.shah@redhat.com,
liliang.opensource@gmail.com, linux-kernel@vger.kernel.org,
willy@infradead.org, virtualization@lists.linux-foundation.org,
linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mhocko
In-Reply-To: <20171013163503-mutt-send-email-mst@kernel.org>
On 10/13/2017 09:38 PM, Michael S. Tsirkin wrote:
> On Thu, Oct 12, 2017 at 11:54:56AM +0800, Wei Wang wrote:
>>> But I think flushing is very fragile. You will easily run into races
>>> if one of the actors gets out of sync and keeps adding data.
>>> I think adding an ID in the free vq stream is a more robust
>>> approach.
>>>
>> Adding ID to the free vq would need the device to distinguish whether it
>> receives an ID or a free page hint,
> Not really. It's pretty simple: a 64 bit buffer is an ID. A 4K and bigger one
> is a page.
I think we can also use the previous method, free page via in_buf, and
id via out_buf.
Best,
Wei
^ permalink raw reply
* Re: [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
From: Tetsuo Handa @ 2017-10-19 11:52 UTC (permalink / raw)
To: mst; +Cc: virtualization, linux-mm, rmaksudova, den, mhocko
In-Reply-To: <20171018201013-mutt-send-email-mst@kernel.org>
Michael S. Tsirkin wrote:
> On Wed, Oct 18, 2017 at 07:59:23PM +0900, Tetsuo Handa wrote:
> > Do you see anything wrong with the patch I used for emulating
> > VIRTIO_BALLOON_F_DEFLATE_ON_OOM path (shown below) ?
> >
> > ----------------------------------------
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index f0b3a0b..a679ac2 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -164,7 +164,7 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> > }
> > set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > - if (!virtio_has_feature(vb->vdev,
> > + if (virtio_has_feature(vb->vdev,
> > VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > adjust_managed_page_count(page, -1);
> > }
> > @@ -184,7 +184,7 @@ static void release_pages_balloon(struct virtio_balloon *vb,
> > struct page *page, *next;
> >
> > list_for_each_entry_safe(page, next, pages, lru) {
> > - if (!virtio_has_feature(vb->vdev,
> > + if (virtio_has_feature(vb->vdev,
> > VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > adjust_managed_page_count(page, 1);
> > list_del(&page->lru);
> > @@ -363,7 +363,7 @@ static int virtballoon_oom_notify(struct notifier_block *self,
> > unsigned num_freed_pages;
> >
> > vb = container_of(self, struct virtio_balloon, nb);
> > - if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > return NOTIFY_OK;
> >
> > freed = parm;
> > ----------------------------------------
>
> Looks right but it's probably easier to configure qemu to set that
> feature bit. Basically you just add deflate-on-oom=on to the
> balloon device.
I'm using CentOS 7 where qemu does not recognize deflate-on-oom option. ;-)
> OK. Or if you use my patch, you can just set a flag and go
> if (vb->oom)
> msleep(1000);
> at beginning of fill_balloon.
I don't think it is a good manner to sleep for long from the point of view of
system_freezable_wq, for system_freezable_wq is expected to flush shortly
according to include/linux/workqueue.h . I think that using delayed_work is better.
> > While response was better than now, inflating again spoiled the effort.
> > Retrying to inflate until allocation fails is already too painful.
> >
> > Michael S. Tsirkin wrote:
> > > I think that's the case. Question is, when can we inflate again?
> >
> > I think that it is when the host explicitly asked again, for
> > VIRTIO_BALLOON_F_DEFLATE_ON_OOM path does not schedule for later inflation.
>
> Problem is host has no idea when it's safe.
> If we expect host to ask again after X seconds we
> might just as well do it in the guest.
To me, fill_balloon() with VIRTIO_BALLOON_F_DEFLATE_ON_OOM sounds like
doing
echo 3 > /proc/sys/vm/drop_caches
where nobody knows whether it won't impact the system.
Thus, I don't think it is a problem. It will be up to administrator
who enters that command.
^ permalink raw reply
* Re: [PATCH v2 0/1] linux: Buffers/caches in VirtIO Balloon driver stats
From: Tomáš Golembiovský @ 2017-10-19 12:33 UTC (permalink / raw)
To: linux-mm, virtualization, qemu-devel, kvm, virtio-dev
Cc: Michael S. Tsirkin, Shaohua Li, Huang Ying
In-Reply-To: <20171005155118.51a5bea3@fiorina>
On Thu, 5 Oct 2017 15:51:18 +0200
Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> On Thu, 21 Sep 2017 14:55:40 +0200
> Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> > Linux driver part
> >
> > v2:
> > - fixed typos
> >
> > Tomáš Golembiovský (1):
> > virtio_balloon: include buffers and cached memory statistics
> >
> > drivers/virtio/virtio_balloon.c | 11 +++++++++++
> > include/uapi/linux/virtio_balloon.h | 4 +++-
> > mm/swap_state.c | 1 +
> > 3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > --
> > 2.14.1
> >
>
> ping
ping
--
Tomáš Golembiovský <tgolembi@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
From: Michael S. Tsirkin @ 2017-10-19 13:00 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: virtualization, linux-mm, rmaksudova, den, mhocko
In-Reply-To: <201710192052.JCE26064.OFtOLSFJVFQOMH@I-love.SAKURA.ne.jp>
On Thu, Oct 19, 2017 at 08:52:20PM +0900, Tetsuo Handa wrote:
> Michael S. Tsirkin wrote:
> > On Wed, Oct 18, 2017 at 07:59:23PM +0900, Tetsuo Handa wrote:
> > > Do you see anything wrong with the patch I used for emulating
> > > VIRTIO_BALLOON_F_DEFLATE_ON_OOM path (shown below) ?
> > >
> > > ----------------------------------------
> > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > index f0b3a0b..a679ac2 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -164,7 +164,7 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> > > }
> > > set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> > > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > > - if (!virtio_has_feature(vb->vdev,
> > > + if (virtio_has_feature(vb->vdev,
> > > VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > > adjust_managed_page_count(page, -1);
> > > }
> > > @@ -184,7 +184,7 @@ static void release_pages_balloon(struct virtio_balloon *vb,
> > > struct page *page, *next;
> > >
> > > list_for_each_entry_safe(page, next, pages, lru) {
> > > - if (!virtio_has_feature(vb->vdev,
> > > + if (virtio_has_feature(vb->vdev,
> > > VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > > adjust_managed_page_count(page, 1);
> > > list_del(&page->lru);
> > > @@ -363,7 +363,7 @@ static int virtballoon_oom_notify(struct notifier_block *self,
> > > unsigned num_freed_pages;
> > >
> > > vb = container_of(self, struct virtio_balloon, nb);
> > > - if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > > return NOTIFY_OK;
> > >
> > > freed = parm;
> > > ----------------------------------------
> >
> > Looks right but it's probably easier to configure qemu to set that
> > feature bit. Basically you just add deflate-on-oom=on to the
> > balloon device.
>
> I'm using CentOS 7 where qemu does not recognize deflate-on-oom option. ;-)
>
> > OK. Or if you use my patch, you can just set a flag and go
> > if (vb->oom)
> > msleep(1000);
> > at beginning of fill_balloon.
>
> I don't think it is a good manner to sleep for long from the point of view of
> system_freezable_wq, for system_freezable_wq is expected to flush shortly
> according to include/linux/workqueue.h . I think that using delayed_work is better.
Well it's already using msleep, I'm fine with reworking it all to use
delayed_work. That's unrelated to the OOM issues though.
>
> > > While response was better than now, inflating again spoiled the effort.
> > > Retrying to inflate until allocation fails is already too painful.
> > >
> > > Michael S. Tsirkin wrote:
> > > > I think that's the case. Question is, when can we inflate again?
> > >
> > > I think that it is when the host explicitly asked again, for
> > > VIRTIO_BALLOON_F_DEFLATE_ON_OOM path does not schedule for later inflation.
> >
> > Problem is host has no idea when it's safe.
> > If we expect host to ask again after X seconds we
> > might just as well do it in the guest.
>
> To me, fill_balloon() with VIRTIO_BALLOON_F_DEFLATE_ON_OOM sounds like
> doing
>
> echo 3 > /proc/sys/vm/drop_caches
>
> where nobody knows whether it won't impact the system.
> Thus, I don't think it is a problem. It will be up to administrator
> who enters that command.
Right now existing hypervisors do not send that interrupt.
If you suggest a new feature where hypervisors send an interrupt,
that might work but will need a new feature bit. Please send an email
to virtio-dev@lists.oasis-open.org (subscriber only, sorry about that)
so the bit can be reserved.
--
MST
^ permalink raw reply
* Re: [PATCH v2 1/1] virtio_balloon: include buffers and cached memory statistics
From: Michael S. Tsirkin @ 2017-10-19 13:12 UTC (permalink / raw)
To: Tomáš Golembiovský
Cc: virtio-dev, kvm, qemu-devel, virtualization, linux-mm, Shaohua Li,
Huang Ying
In-Reply-To: <b13f11c03ed394bd8ad367dc90996ed134ea98da.1505998455.git.tgolembi@redhat.com>
On Thu, Sep 21, 2017 at 02:55:41PM +0200, Tomáš Golembiovský wrote:
> Add a new fields, VIRTIO_BALLOON_S_BUFFERS and VIRTIO_BALLOON_S_CACHED,
> to virtio_balloon memory statistics protocol. The values correspond to
> 'Buffers' and 'Cached' in /proc/meminfo.
>
> To be able to compute the value of 'Cached' memory it is necessary to
> export total_swapcache_pages() to modules.
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
Does 'Buffers' actually make sense? It's a temporary storage -
wouldn't it be significantly out of date by the time
host receives it?
> ---
> drivers/virtio/virtio_balloon.c | 11 +++++++++++
> include/uapi/linux/virtio_balloon.h | 4 +++-
> mm/swap_state.c | 1 +
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f0b3a0b9d42f..c2558ec47a62 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -244,12 +244,19 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> struct sysinfo i;
> unsigned int idx = 0;
> long available;
> + long cached;
>
> all_vm_events(events);
> si_meminfo(&i);
>
> available = si_mem_available();
>
> + cached = global_node_page_state(NR_FILE_PAGES) -
> + total_swapcache_pages() - i.bufferram;
> + if (cached < 0)
> + cached = 0;
> +
> +
> #ifdef CONFIG_VM_EVENT_COUNTERS
> update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> pages_to_bytes(events[PSWPIN]));
> @@ -264,6 +271,10 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> pages_to_bytes(i.totalram));
> update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
> pages_to_bytes(available));
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_BUFFERS,
> + pages_to_bytes(i.bufferram));
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_CACHED,
> + pages_to_bytes(cached));
>
> return idx;
> }
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7ddefe04..d5dc8a56a497 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -52,7 +52,9 @@ struct virtio_balloon_config {
> #define VIRTIO_BALLOON_S_MEMFREE 4 /* Total amount of free memory */
> #define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
> #define VIRTIO_BALLOON_S_AVAIL 6 /* Available memory as in /proc */
> -#define VIRTIO_BALLOON_S_NR 7
> +#define VIRTIO_BALLOON_S_BUFFERS 7 /* Buffers memory as in /proc */
> +#define VIRTIO_BALLOON_S_CACHED 8 /* Cached memory as in /proc */
> +#define VIRTIO_BALLOON_S_NR 9
>
> /*
> * Memory statistics structure.
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 71ce2d1ccbf7..f3a4ff7d6c52 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -95,6 +95,7 @@ unsigned long total_swapcache_pages(void)
> rcu_read_unlock();
> return ret;
> }
> +EXPORT_SYMBOL_GPL(total_swapcache_pages);
>
> static atomic_t swapin_readahead_hits = ATOMIC_INIT(4);
Need an ack from MM crowd on that.
> --
> 2.14.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v1 15/27] compiler: Option to default to hidden symbols
From: Luis R. Rodriguez @ 2017-10-19 19:38 UTC (permalink / raw)
To: Thomas Garnier
Cc: Nicolas Pitre, Michal Hocko, Radim Krčmář,
linux-doc, Daniel Micay, Len Brown, Peter Zijlstra,
Christopher Li, Jan H . Schönherr, Alexei Starovoitov,
virtualization, David Howells, Paul Gortmaker, Waiman Long,
Pavel Machek, H . Peter Anvin, Kernel Hardening,
Christoph Lameter, Alok Kataria, the arch/x86 maintainers,
Herbert Xu, Daniel Borkmann
In-Reply-To: <CAJcbSZEJ-kjcRQD3uHm0QZuOvbHpg9FV=wn7v4-RweDG7J3uqg@mail.gmail.com>
On Wed, Oct 18, 2017 at 04:15:10PM -0700, Thomas Garnier wrote:
> On Thu, Oct 12, 2017 at 1:02 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Wed, Oct 11, 2017 at 01:30:15PM -0700, Thomas Garnier wrote:
> >> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> >> index e95a2631e545..6997716f73bf 100644
> >> --- a/include/linux/compiler.h
> >> +++ b/include/linux/compiler.h
> >> @@ -78,6 +78,14 @@ extern void __chk_io_ptr(const volatile void __iomem *);
> >> #include <linux/compiler-clang.h>
> >> #endif
> >>
> >> +/* Useful for Position Independent Code to reduce global references */
> >> +#ifdef CONFIG_DEFAULT_HIDDEN
> >> +#pragma GCC visibility push(hidden)
> >> +#define __default_visibility __attribute__((visibility ("default")))
> >
> > Does this still work with CONFIG_LD_DEAD_CODE_DATA_ELIMINATION ?
>
> I cannot make it work with or without this change. How is it supposed
> to be used?
Sadly I don't think much documentation was really added as part of the Nick's
commits about feature, even though commit b67067f1176 ("kbuild: allow archs to
select link dead code/data elimination") *does* say this was documented.
Side rant: the whole CONFIG_LTO removal was merged in the same commit without
this having gone in as a separate atomic patch.
Nick can you provide a bit more guidance about how to get this feature going or
tested on an architecture? Or are you just sticking to assuming folks using the
linker / compiler flags will know what to do? *Some* guidance could help.
> For me with, it crashes with a bad consdev at:
> http://elixir.free-electrons.com/linux/latest/source/drivers/tty/tty_io.c#L3194
From my reading of the commit log he only had tested it with with powerpc64le,
each other architecture would have to do work to get as far as even booting.
It would require someone then testing Nick's patches against a working
powerpc setup to ensure we don't regress there.
> >> diff --git a/init/Kconfig b/init/Kconfig
> >> index ccb1d8daf241..b640201fcff7 100644
> >> --- a/init/Kconfig
> >> +++ b/init/Kconfig
> >> @@ -1649,6 +1649,13 @@ config PROFILING
> >> config TRACEPOINTS
> >> bool
> >>
> >> +#
> >> +# Default to hidden visibility for all symbols.
> >> +# Useful for Position Independent Code to reduce global references.
> >> +#
> >> +config DEFAULT_HIDDEN
> >> + bool
> >
> > Note it is default.
> >
> > Has 0-day ran through this git tree? It should be easy to get it added for
> > testing. Also, even though most changes are x86 based there are some generic
> > changes and I'd love a warm fuzzy this won't break odd / random builds.
> > Although 0-day does cover a lot of test cases, it only has limited run time
> > tests. There are some other test beds which also cover some more obscure
> > architectures. Having a test pass on Guenter's test bed would be nice to
> > see. For that please coordinate with Guenter if he's willing to run this
> > a test for you.
>
> Not yet, plan to give a v1.5 to Kees Cook to keep in one of his tree
> for couple weeks. I expect it will identify interesting issues.
I bet :)
Luis
^ permalink raw reply
* Re: [PATCH v1 01/27] x86/crypto: Adapt assembly for PIE support
From: Ingo Molnar @ 2017-10-20 8:24 UTC (permalink / raw)
To: Thomas Garnier
Cc: Nicolas Pitre, Michal Hocko, Radim Krčmář,
linux-doc, Daniel Micay, Len Brown, Peter Zijlstra,
Christopher Li, Jan H . Schönherr, Alexei Starovoitov,
virtualization, David Howells, Paul Gortmaker, Waiman Long,
Pavel Machek, H . Peter Anvin, kernel-hardening,
Christoph Lameter, Thomas Gleixner, x86, Herbert Xu,
Daniel Borkmann, Jonathan Corbet
In-Reply-To: <20171011203027.11248-2-thgarnie@google.com>
* Thomas Garnier <thgarnie@google.com> wrote:
> Change the assembly code to use only relative references of symbols for the
> kernel to be PIE compatible.
>
> Position Independent Executable (PIE) support will allow to extended the
> KASLR randomization range below the -2G memory limit.
> diff --git a/arch/x86/crypto/aes-x86_64-asm_64.S b/arch/x86/crypto/aes-x86_64-asm_64.S
> index 8739cf7795de..86fa068e5e81 100644
> --- a/arch/x86/crypto/aes-x86_64-asm_64.S
> +++ b/arch/x86/crypto/aes-x86_64-asm_64.S
> @@ -48,8 +48,12 @@
> #define R10 %r10
> #define R11 %r11
>
> +/* Hold global for PIE suport */
> +#define RBASE %r12
> +
> #define prologue(FUNC,KEY,B128,B192,r1,r2,r5,r6,r7,r8,r9,r10,r11) \
> ENTRY(FUNC); \
> + pushq RBASE; \
> movq r1,r2; \
> leaq KEY+48(r8),r9; \
> movq r10,r11; \
> @@ -74,54 +78,63 @@
> movl r6 ## E,4(r9); \
> movl r7 ## E,8(r9); \
> movl r8 ## E,12(r9); \
> + popq RBASE; \
> ret; \
> ENDPROC(FUNC);
>
> +#define round_mov(tab_off, reg_i, reg_o) \
> + leaq tab_off(%rip), RBASE; \
> + movl (RBASE,reg_i,4), reg_o;
> +
> +#define round_xor(tab_off, reg_i, reg_o) \
> + leaq tab_off(%rip), RBASE; \
> + xorl (RBASE,reg_i,4), reg_o;
> +
> #define round(TAB,OFFSET,r1,r2,r3,r4,r5,r6,r7,r8,ra,rb,rc,rd) \
> movzbl r2 ## H,r5 ## E; \
> movzbl r2 ## L,r6 ## E; \
> - movl TAB+1024(,r5,4),r5 ## E;\
> + round_mov(TAB+1024, r5, r5 ## E)\
> movw r4 ## X,r2 ## X; \
> - movl TAB(,r6,4),r6 ## E; \
> + round_mov(TAB, r6, r6 ## E) \
> roll $16,r2 ## E; \
> shrl $16,r4 ## E; \
> movzbl r4 ## L,r7 ## E; \
> movzbl r4 ## H,r4 ## E; \
> xorl OFFSET(r8),ra ## E; \
> xorl OFFSET+4(r8),rb ## E; \
> - xorl TAB+3072(,r4,4),r5 ## E;\
> - xorl TAB+2048(,r7,4),r6 ## E;\
> + round_xor(TAB+3072, r4, r5 ## E)\
> + round_xor(TAB+2048, r7, r6 ## E)\
> movzbl r1 ## L,r7 ## E; \
> movzbl r1 ## H,r4 ## E; \
> - movl TAB+1024(,r4,4),r4 ## E;\
> + round_mov(TAB+1024, r4, r4 ## E)\
> movw r3 ## X,r1 ## X; \
> roll $16,r1 ## E; \
> shrl $16,r3 ## E; \
> - xorl TAB(,r7,4),r5 ## E; \
> + round_xor(TAB, r7, r5 ## E) \
> movzbl r3 ## L,r7 ## E; \
> movzbl r3 ## H,r3 ## E; \
> - xorl TAB+3072(,r3,4),r4 ## E;\
> - xorl TAB+2048(,r7,4),r5 ## E;\
> + round_xor(TAB+3072, r3, r4 ## E)\
> + round_xor(TAB+2048, r7, r5 ## E)\
> movzbl r1 ## L,r7 ## E; \
> movzbl r1 ## H,r3 ## E; \
> shrl $16,r1 ## E; \
> - xorl TAB+3072(,r3,4),r6 ## E;\
> - movl TAB+2048(,r7,4),r3 ## E;\
> + round_xor(TAB+3072, r3, r6 ## E)\
> + round_mov(TAB+2048, r7, r3 ## E)\
> movzbl r1 ## L,r7 ## E; \
> movzbl r1 ## H,r1 ## E; \
> - xorl TAB+1024(,r1,4),r6 ## E;\
> - xorl TAB(,r7,4),r3 ## E; \
> + round_xor(TAB+1024, r1, r6 ## E)\
> + round_xor(TAB, r7, r3 ## E) \
> movzbl r2 ## H,r1 ## E; \
> movzbl r2 ## L,r7 ## E; \
> shrl $16,r2 ## E; \
> - xorl TAB+3072(,r1,4),r3 ## E;\
> - xorl TAB+2048(,r7,4),r4 ## E;\
> + round_xor(TAB+3072, r1, r3 ## E)\
> + round_xor(TAB+2048, r7, r4 ## E)\
> movzbl r2 ## H,r1 ## E; \
> movzbl r2 ## L,r2 ## E; \
> xorl OFFSET+8(r8),rc ## E; \
> xorl OFFSET+12(r8),rd ## E; \
> - xorl TAB+1024(,r1,4),r3 ## E;\
> - xorl TAB(,r2,4),r4 ## E;
> + round_xor(TAB+1024, r1, r3 ## E)\
> + round_xor(TAB, r2, r4 ## E)
This appears to be adding unconditional overhead to a function that was moved to
assembly to improve its performance.
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH v1 06/27] x86/entry/64: Adapt assembly for PIE support
From: Ingo Molnar @ 2017-10-20 8:26 UTC (permalink / raw)
To: Thomas Garnier
Cc: Nicolas Pitre, Michal Hocko, Radim Krčmář,
linux-doc, Daniel Micay, Len Brown, Peter Zijlstra,
Christopher Li, Jan H . Schönherr, Alexei Starovoitov,
virtualization, David Howells, Paul Gortmaker, Waiman Long,
Pavel Machek, H . Peter Anvin, kernel-hardening,
Christoph Lameter, Thomas Gleixner, x86, Herbert Xu,
Daniel Borkmann, Jonathan Corbet
In-Reply-To: <20171011203027.11248-7-thgarnie@google.com>
* Thomas Garnier <thgarnie@google.com> wrote:
> Change the assembly code to use only relative references of symbols for the
> kernel to be PIE compatible.
>
> Position Independent Executable (PIE) support will allow to extended the
> KASLR randomization range below the -2G memory limit.
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> ---
> arch/x86/entry/entry_64.S | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 49167258d587..15bd5530d2ae 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -194,12 +194,15 @@ entry_SYSCALL_64_fastpath:
> ja 1f /* return -ENOSYS (already in pt_regs->ax) */
> movq %r10, %rcx
>
> + /* Ensures the call is position independent */
> + leaq sys_call_table(%rip), %r11
> +
> /*
> * This call instruction is handled specially in stub_ptregs_64.
> * It might end up jumping to the slow path. If it jumps, RAX
> * and all argument registers are clobbered.
> */
> - call *sys_call_table(, %rax, 8)
> + call *(%r11, %rax, 8)
> .Lentry_SYSCALL_64_after_fastpath_call:
>
> movq %rax, RAX(%rsp)
> @@ -334,7 +337,8 @@ ENTRY(stub_ptregs_64)
> * RAX stores a pointer to the C function implementing the syscall.
> * IRQs are on.
> */
> - cmpq $.Lentry_SYSCALL_64_after_fastpath_call, (%rsp)
> + leaq .Lentry_SYSCALL_64_after_fastpath_call(%rip), %r11
> + cmpq %r11, (%rsp)
> jne 1f
>
> /*
> @@ -1172,7 +1176,8 @@ ENTRY(error_entry)
> movl %ecx, %eax /* zero extend */
> cmpq %rax, RIP+8(%rsp)
> je .Lbstep_iret
> - cmpq $.Lgs_change, RIP+8(%rsp)
> + leaq .Lgs_change(%rip), %rcx
> + cmpq %rcx, RIP+8(%rsp)
> jne .Lerror_entry_done
>
> /*
> @@ -1383,10 +1388,10 @@ ENTRY(nmi)
> * resume the outer NMI.
> */
>
> - movq $repeat_nmi, %rdx
> + leaq repeat_nmi(%rip), %rdx
> cmpq 8(%rsp), %rdx
> ja 1f
> - movq $end_repeat_nmi, %rdx
> + leaq end_repeat_nmi(%rip), %rdx
> cmpq 8(%rsp), %rdx
> ja nested_nmi_out
> 1:
> @@ -1440,7 +1445,8 @@ nested_nmi:
> pushq %rdx
> pushfq
> pushq $__KERNEL_CS
> - pushq $repeat_nmi
> + leaq repeat_nmi(%rip), %rdx
> + pushq %rdx
>
> /* Put stack back */
> addq $(6*8), %rsp
> @@ -1479,7 +1485,9 @@ first_nmi:
> addq $8, (%rsp) /* Fix up RSP */
> pushfq /* RFLAGS */
> pushq $__KERNEL_CS /* CS */
> - pushq $1f /* RIP */
> + pushq %rax /* Support Position Independent Code */
> + leaq 1f(%rip), %rax /* RIP */
> + xchgq %rax, (%rsp) /* Restore RAX, put 1f */
> INTERRUPT_RETURN /* continues at repeat_nmi below */
> UNWIND_HINT_IRET_REGS
This patch seems to add extra overhead to the syscall fast-path even when PIE is
disabled, right?
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH v1 01/27] x86/crypto: Adapt assembly for PIE support
From: Ard Biesheuvel @ 2017-10-20 8:28 UTC (permalink / raw)
To: Ingo Molnar
Cc: Nicolas Pitre, Michal Hocko, Radim Krčmář,
Linux Doc Mailing List, Daniel Micay, Len Brown, Peter Zijlstra,
Christopher Li, Jan H . Schönherr, Alexei Starovoitov,
virtualization, David Howells, Paul Gortmaker, Waiman Long,
Pavel Machek, H . Peter Anvin, Kernel Hardening,
Christoph Lameter, Thomas Gleixner, x86@kernel.org, Herbert Xu,
Daniel Borkmann
In-Reply-To: <20171020082420.lsvu7mqjrgnahm5t@gmail.com>
On 20 October 2017 at 09:24, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Thomas Garnier <thgarnie@google.com> wrote:
>
>> Change the assembly code to use only relative references of symbols for the
>> kernel to be PIE compatible.
>>
>> Position Independent Executable (PIE) support will allow to extended the
>> KASLR randomization range below the -2G memory limit.
>
>> diff --git a/arch/x86/crypto/aes-x86_64-asm_64.S b/arch/x86/crypto/aes-x86_64-asm_64.S
>> index 8739cf7795de..86fa068e5e81 100644
>> --- a/arch/x86/crypto/aes-x86_64-asm_64.S
>> +++ b/arch/x86/crypto/aes-x86_64-asm_64.S
>> @@ -48,8 +48,12 @@
>> #define R10 %r10
>> #define R11 %r11
>>
>> +/* Hold global for PIE suport */
>> +#define RBASE %r12
>> +
>> #define prologue(FUNC,KEY,B128,B192,r1,r2,r5,r6,r7,r8,r9,r10,r11) \
>> ENTRY(FUNC); \
>> + pushq RBASE; \
>> movq r1,r2; \
>> leaq KEY+48(r8),r9; \
>> movq r10,r11; \
>> @@ -74,54 +78,63 @@
>> movl r6 ## E,4(r9); \
>> movl r7 ## E,8(r9); \
>> movl r8 ## E,12(r9); \
>> + popq RBASE; \
>> ret; \
>> ENDPROC(FUNC);
>>
>> +#define round_mov(tab_off, reg_i, reg_o) \
>> + leaq tab_off(%rip), RBASE; \
>> + movl (RBASE,reg_i,4), reg_o;
>> +
>> +#define round_xor(tab_off, reg_i, reg_o) \
>> + leaq tab_off(%rip), RBASE; \
>> + xorl (RBASE,reg_i,4), reg_o;
>> +
>> #define round(TAB,OFFSET,r1,r2,r3,r4,r5,r6,r7,r8,ra,rb,rc,rd) \
>> movzbl r2 ## H,r5 ## E; \
>> movzbl r2 ## L,r6 ## E; \
>> - movl TAB+1024(,r5,4),r5 ## E;\
>> + round_mov(TAB+1024, r5, r5 ## E)\
>> movw r4 ## X,r2 ## X; \
>> - movl TAB(,r6,4),r6 ## E; \
>> + round_mov(TAB, r6, r6 ## E) \
>> roll $16,r2 ## E; \
>> shrl $16,r4 ## E; \
>> movzbl r4 ## L,r7 ## E; \
>> movzbl r4 ## H,r4 ## E; \
>> xorl OFFSET(r8),ra ## E; \
>> xorl OFFSET+4(r8),rb ## E; \
>> - xorl TAB+3072(,r4,4),r5 ## E;\
>> - xorl TAB+2048(,r7,4),r6 ## E;\
>> + round_xor(TAB+3072, r4, r5 ## E)\
>> + round_xor(TAB+2048, r7, r6 ## E)\
>> movzbl r1 ## L,r7 ## E; \
>> movzbl r1 ## H,r4 ## E; \
>> - movl TAB+1024(,r4,4),r4 ## E;\
>> + round_mov(TAB+1024, r4, r4 ## E)\
>> movw r3 ## X,r1 ## X; \
>> roll $16,r1 ## E; \
>> shrl $16,r3 ## E; \
>> - xorl TAB(,r7,4),r5 ## E; \
>> + round_xor(TAB, r7, r5 ## E) \
>> movzbl r3 ## L,r7 ## E; \
>> movzbl r3 ## H,r3 ## E; \
>> - xorl TAB+3072(,r3,4),r4 ## E;\
>> - xorl TAB+2048(,r7,4),r5 ## E;\
>> + round_xor(TAB+3072, r3, r4 ## E)\
>> + round_xor(TAB+2048, r7, r5 ## E)\
>> movzbl r1 ## L,r7 ## E; \
>> movzbl r1 ## H,r3 ## E; \
>> shrl $16,r1 ## E; \
>> - xorl TAB+3072(,r3,4),r6 ## E;\
>> - movl TAB+2048(,r7,4),r3 ## E;\
>> + round_xor(TAB+3072, r3, r6 ## E)\
>> + round_mov(TAB+2048, r7, r3 ## E)\
>> movzbl r1 ## L,r7 ## E; \
>> movzbl r1 ## H,r1 ## E; \
>> - xorl TAB+1024(,r1,4),r6 ## E;\
>> - xorl TAB(,r7,4),r3 ## E; \
>> + round_xor(TAB+1024, r1, r6 ## E)\
>> + round_xor(TAB, r7, r3 ## E) \
>> movzbl r2 ## H,r1 ## E; \
>> movzbl r2 ## L,r7 ## E; \
>> shrl $16,r2 ## E; \
>> - xorl TAB+3072(,r1,4),r3 ## E;\
>> - xorl TAB+2048(,r7,4),r4 ## E;\
>> + round_xor(TAB+3072, r1, r3 ## E)\
>> + round_xor(TAB+2048, r7, r4 ## E)\
>> movzbl r2 ## H,r1 ## E; \
>> movzbl r2 ## L,r2 ## E; \
>> xorl OFFSET+8(r8),rc ## E; \
>> xorl OFFSET+12(r8),rd ## E; \
>> - xorl TAB+1024(,r1,4),r3 ## E;\
>> - xorl TAB(,r2,4),r4 ## E;
>> + round_xor(TAB+1024, r1, r3 ## E)\
>> + round_xor(TAB, r2, r4 ## E)
>
> This appears to be adding unconditional overhead to a function that was moved to
> assembly to improve its performance.
>
I did some benchmarking on this code a while ago and, interestingly,
it was slower than the generic C implementation (on a Pentium E2200),
so we may want to consider whether we still need this driver in the
first place.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox