* Re: [GIT PULL] Kernel lockdown for secure boot
From: Linus Torvalds @ 2018-04-03 21:01 UTC (permalink / raw)
To: Matthew Garrett
Cc: Andrew Lutomirski, David Howells, Ard Biesheuvel, James Morris,
Alan Cox, Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes,
linux-man, jlee, LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <CACdnJuuXOP5T-BRAwMvc7XF8S49wyvp4ZYgcU=EZkaGNgPAQig@mail.gmail.com>
On Tue, Apr 3, 2018 at 1:54 PM, Matthew Garrett <mjg59@google.com> wrote:
>
>> .. maybe you don't *want* secure boot, but it's been pushed in your
>> face by people with an agenda?
>
> Then turn it off, or build a self-signed kernel that doesn't do this?
Umm. So you asked a question, and then when you got an answer you said
"don't do that then".
The fact is, some hardware pushes secure boot pretty hard. That has
*nothing* to do with some "lockdown" mode.
Why do you conflate the two? That was the original question. You
replied with another question. People answered yours.
NOW ANSWER THE ORIGINAL QUESTION, DAMMIT.
Linus
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Matthew Garrett @ 2018-04-03 20:54 UTC (permalink / raw)
To: Linus Torvalds
Cc: luto, David Howells, Ard Biesheuvel, jmorris, Alan Cox,
Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes, linux-man,
jlee, LSM List, linux-api, Kees Cook, linux-efi
In-Reply-To: <CA+55aFy5EPxAyqyGGaYUUyakuLbjXHiseWCkz-oNdo=Y-878tg@mail.gmail.com>
On Tue, Apr 3, 2018 at 1:53 PM Linus Torvalds
<torvalds@linux-foundation.org>
wrote:
> On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett <mjg59@google.com> wrote:
> > On Tue, Apr 3, 2018 at 8:11 AM Andy Lutomirski <luto@kernel.org> wrote:
> >> Can you explain that much more clearly? I'm asking why booting via
> >> UEFI Secure Boot should enable lockdown, and I don't see what this has
> >> to do with kexec. And "someone blacklist[ing] your key in the
> >> bootloader" sounds like a political issue, not a technical issue.
> >
> > A kernel that allows users arbitrary access to ring 0 is just an
> > overfeatured bootloader. Why would you want secure boot in that case?
> .. maybe you don't *want* secure boot, but it's been pushed in your
> face by people with an agenda?
Then turn it off, or build a self-signed kernel that doesn't do this?
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Linus Torvalds @ 2018-04-03 20:53 UTC (permalink / raw)
To: Matthew Garrett
Cc: Andrew Lutomirski, David Howells, Ard Biesheuvel, James Morris,
Alan Cox, Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes,
linux-man, jlee, LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <CACdnJut31+ZtiR40nkOjm89cfUxS0EM=w-oG69PtWHbXYJQxPg@mail.gmail.com>
On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett <mjg59@google.com> wrote:
> On Tue, Apr 3, 2018 at 8:11 AM Andy Lutomirski <luto@kernel.org> wrote:
>> Can you explain that much more clearly? I'm asking why booting via
>> UEFI Secure Boot should enable lockdown, and I don't see what this has
>> to do with kexec. And "someone blacklist[ing] your key in the
>> bootloader" sounds like a political issue, not a technical issue.
>
> A kernel that allows users arbitrary access to ring 0 is just an
> overfeatured bootloader. Why would you want secure boot in that case?
.. maybe you don't *want* secure boot, but it's been pushed in your
face by people with an agenda?
Seriously.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-04-03 20:32 UTC (permalink / raw)
To: One Thousand Gnomes
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas <cata>
In-Reply-To: <17439540.2334.1522773387555.JavaMail.zimbra@efficios.com>
----- On Apr 3, 2018, at 12:36 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> ----- On Apr 2, 2018, at 11:33 AM, Mathieu Desnoyers
> mathieu.desnoyers@efficios.com wrote:
>
>> ----- On Apr 1, 2018, at 12:13 PM, One Thousand Gnomes
>> gnomes@lxorguk.ukuu.org.uk wrote:
>>
[...]
>>> I still like the idea it's just the latencies concern me.
>>
[...]
>
> Looking into this a bit more, I notice the following: The pgprot_noncached
> (_PAGE_NOCACHE on x86) pgprot is part of the vma->vm_page_prot. Therefore,
> in order to have userspace provide pointers to noncached pages as input
> to cpu_opv, they need to be part of a userspace vma which has a
> pgprot_noncached vm_page_prot.
>
> The cpu_opv system call uses get_user_pages_fast() to grab the struct page
> from the userspace addresses, and then passes those pages to vm_map_ram(),
> with a PAGE_KERNEL pgprot. This creates a temporary kernel mapping to those
> pages, which is then used to read/write from/to those pages with preemption
> disabled.
>
> Therefore, with the proposed cpu_opv implementation, the kernel is not
> touching noncached mappings with preemption disabled, which should take
> care of your latency concern.
[...]
The following extra check should let userspace know it's trying to
provide a pointer to noncached memory by returning -1, errno=EFAULT.
Is the approach acceptable ?
Thanks,
Mathieu
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ad06d42..0245481 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2425,6 +2425,18 @@ static inline struct page *follow_page(struct vm_area_struct *vma,
return follow_page_mask(vma, address, foll_flags, &unused_page_mask);
}
+static inline bool is_vma_noncached(struct vm_area_struct *vma)
+{
+ pgprot_t pgprot = vma->vm_page_prot;
+
+ /* Check whether architecture implements noncached pages. */
+ if (pgprot_val(pgprot_noncached(PAGE_KERNEL)) == pgprot_val(PAGE_KERNEL))
+ return false;
+ if (pgprot_val(pgprot) != pgprot_val(pgprot_noncached(pgprot)))
+ return false;
+ return true;
+}
+
#define FOLL_WRITE 0x01 /* check pte is writable */
#define FOLL_TOUCH 0x02 /* mark page accessed */
#define FOLL_GET 0x04 /* do get_page on page */
diff --git a/kernel/cpu_opv.c b/kernel/cpu_opv.c
index 197339e..e4395b4 100644
--- a/kernel/cpu_opv.c
+++ b/kernel/cpu_opv.c
@@ -362,7 +362,19 @@ static int cpu_op_pin_pages(unsigned long addr, unsigned long len,
int ret, nr_pages, nr_put_pages, n;
unsigned long _vaddr;
struct vaddr *va;
+ struct vm_area_struct *vma;
+ vma = find_vma_intersection(current->mm, addr, addr + len);
+ if (!vma)
+ return -EFAULT;
+ /*
+ * cpu_opv() accesses its own cached mapping of the userspace pages.
+ * Considering that concurrent noncached and cached accesses may yield
+ * to unexpected results in terms of memory consistency, explicitly
+ * disallow cpu_opv on noncached memory.
+ */
+ if (is_vma_noncached(vma))
+ return -EFAULT;
nr_pages = cpu_op_count_pages(addr, len);
if (!nr_pages)
return 0;
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply related
* Re: [GIT PULL] Kernel lockdown for secure boot
From: David Howells @ 2018-04-03 19:49 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Matthew Garrett, Ard Biesheuvel, James Morris, Alan Cox,
Linus Torvalds, Greg Kroah-Hartman, Linux Kernel Mailing List,
Justin Forbes, linux-man, joeyli, LSM List, Linux API, Kees Cook,
linux-efi
In-Reply-To: <CALCETrWHS9p1My=j07=V=OP7v4SwQkW-kJ3JOx6EbPU8fb_XEQ@mail.gmail.com>
Andy Lutomirski <luto@kernel.org> wrote:
> >>> A kernel that allows users arbitrary access to ring 0 is just an
> >>> overfeatured bootloader. Why would you want secure boot in that case?
> >>
> >> To get a chain of trust.
> >
> > You don't have a chain of trust that you can trust in that case.
> >
> Please elaborate on why I can’t trust it.
If the user can arbitrarily modify the running kernel image, you cannot trust
anything. You cannot determine the trustworthiness of something because your
basis for determining that trust can be compromised.
> Please also elaborate on how lockdown helps at all.
Stopping the kernel from being arbitrarily modified allows you to preserve
your trust.
Stopping the kernel from being arbitrarily read stops any encryption keys it
may be using from being retrieved.
And, if you can't guarantee the trustworthiness of your own image, you can't
pass the trust onto the next image that you kexec.
Now, I can't guarantee that my patches close every hole, they just close all
the holes I know about - including some obscure ones like using DMA-capable
ISA devices to hack/access the kernel image.
David
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Matthew Garrett @ 2018-04-03 19:29 UTC (permalink / raw)
To: luto
Cc: David Howells, Ard Biesheuvel, jmorris, Alan Cox, Linus Torvalds,
Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes, linux-man,
jlee, LSM List, linux-api, Kees Cook, linux-efi
In-Reply-To: <CALCETrXeVod=kNpG-M7yKAMM0-n+PMg_OakN6ecWrTPmKXgMLg@mail.gmail.com>
On Tue, Apr 3, 2018 at 9:46 AM Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett <mjg59@google.com> wrote:
> > A kernel that allows users arbitrary access to ring 0 is just an
> > overfeatured bootloader. Why would you want secure boot in that case?
> To get a chain of trust. I can provision a system with some public
> keys, stored in UEFI authenticated variables, such that the system
> will only boot a signed image. That signed image, can, in turn, load
> a signed (or hashed or otherwise verfified) kernel and a verified
> initramfs. The initramfs can run a full system from a verified (using
> dm-verity or similar) filesystem, for example. Now it's very hard to
> persistently attack this system. Chromium OS does something very much
> like this, except that it doesn't use UEFI as far as I know. So does
> iOS, and so do some Android versions. None of this requires lockdown,
> or even a separation between usermode and kernelmode, to work
> correctly. One could even do this on an MMU-less system if one really
> cared to. More usefully, someone probably has done this using a
> unikernel.
That's only viable if you're the only person with the ability to sign stuff
for your machine - the moment there are generic distributions that your
machine trusts, an attacker can use one as a bootloader to compromise your
trust chain. Since most UEFI secure boot systems have to trust generic
distributions (if you don't trust the third party signing key then your GPU
won't post), the ecosystem depends on it not being possible for people to
use generic distributions as bootloaders.
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Kees Cook @ 2018-04-03 19:07 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Matthew Garrett, David Howells, Ard Biesheuvel, James Morris,
Alan Cox, Linus Torvalds, Greg Kroah-Hartman,
Linux Kernel Mailing List, Justin Forbes, linux-man, joeyli,
LSM List, Linux API, linux-efi
In-Reply-To: <CALCETrW=Tjd2K9Ka0y_9es3XWiWHpKySDBwCq9vG=pbgkd2nFQ@mail.gmail.com>
On Tue, Apr 3, 2018 at 12:01 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Apr 3, 2018 at 11:45 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Tue, Apr 3, 2018 at 9:45 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>> On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett <mjg59@google.com> wrote:
>>>> On Tue, Apr 3, 2018 at 8:11 AM Andy Lutomirski <luto@kernel.org> wrote:
>>>>> Can you explain that much more clearly? I'm asking why booting via
>>>>> UEFI Secure Boot should enable lockdown, and I don't see what this has
>>>>> to do with kexec. And "someone blacklist[ing] your key in the
>>>>> bootloader" sounds like a political issue, not a technical issue.
>>>>
>>>> A kernel that allows users arbitrary access to ring 0 is just an
>>>> overfeatured bootloader. Why would you want secure boot in that case?
>>>
>>> To get a chain of trust. I can provision a system with some public
>>> keys, stored in UEFI authenticated variables, such that the system
>>> will only boot a signed image. That signed image, can, in turn, load
>>> a signed (or hashed or otherwise verfified) kernel and a verified
>>> initramfs. The initramfs can run a full system from a verified (using
>>> dm-verity or similar) filesystem, for example. Now it's very hard to
>>> persistently attack this system. Chromium OS does something very much
>>> like this, except that it doesn't use UEFI as far as I know. So does
>>> iOS, and so do some Android versions.
>>
>> Correct, Chrome OS does not use UEFI, and we still want this patch
>> series, as it plugs all the known "intentional" escalation paths from
>> uid-0 to ring-0. Happily, that means all the politics around the UEFI
>> and Secure Boot case can be ignored, because those issues are specific
>> to Secure Boot, not the lockdown series. (They are _related_, sure,
>> but lockdown isn't only about Secure Boot -- it's just that SB is one
>> of the widely deployed implementations of this kind of
>> trust-chain-booting-thing. Chrome OS and Android's Verified Boot do
>> similar things and have the same expectations about the uid-0/ring-0
>> separation.)
>>
>> The goal for that bright line on Chrome OS and Android is to stop
>> attack persistence. We want to know that a reboot onto a new kernel
>> and OS image will actually result in getting the desired system state,
>> and that any attack on persistent system data (even for things running
>> with full root privileges) can't result in using kernel interfaces to
>> gain kernel control. This isn't expected to be _perfect_, since
>> nothing is. But it creates a place to work from. The idea that uid-0
>> is NOT ring-0 is still relatively new, so the existing designs in the
>> kernel aren't well suited to building that distinction. I view this
>> series as a solid first step to getting there, though.
>
> But wouldn't Chrome OS possibly want to lock down kernel memory write
> vectors but not read vectors? After all, debugging is useful even on
> Chrome OS.
Chrome OS absolutely wants to block writing. We also want to block
reading as much as we possibly can, though yes we bump up against
debugging in that quest. But those cases are manageable and specific,
IMO.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Andy Lutomirski @ 2018-04-03 19:01 UTC (permalink / raw)
To: David Howells
Cc: Andy Lutomirski, Matthew Garrett, Ard Biesheuvel, James Morris,
Alan Cox, Linus Torvalds, Greg Kroah-Hartman,
Linux Kernel Mailing List, Justin Forbes, linux-man, joeyli,
LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <9758.1522775763@warthog.procyon.org.uk>
> On Apr 3, 2018, at 10:16 AM, David Howells <dhowells@redhat.com> wrote:
>
> Andy Lutomirski <luto@kernel.org> wrote:
>
>>> A kernel that allows users arbitrary access to ring 0 is just an
>>> overfeatured bootloader. Why would you want secure boot in that case?
>>
>> To get a chain of trust.
>
> You don't have a chain of trust that you can trust in that case.
>
Please elaborate on why I can’t trust it. Please also elaborate on how
lockdown helps at all.
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Andy Lutomirski @ 2018-04-03 19:01 UTC (permalink / raw)
To: Kees Cook
Cc: Andy Lutomirski, Matthew Garrett, David Howells, Ard Biesheuvel,
James Morris, Alan Cox, Linus Torvalds, Greg Kroah-Hartman,
Linux Kernel Mailing List, Justin Forbes, linux-man, joeyli,
LSM List, Linux API, linux-efi
In-Reply-To: <CAGXu5j+CyVXEvsMarJjBwaNh7poVZtmit5PGmQM9rKKqZPqVXg@mail.gmail.com>
On Tue, Apr 3, 2018 at 11:45 AM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Apr 3, 2018 at 9:45 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett <mjg59@google.com> wrote:
>>> On Tue, Apr 3, 2018 at 8:11 AM Andy Lutomirski <luto@kernel.org> wrote:
>>>> Can you explain that much more clearly? I'm asking why booting via
>>>> UEFI Secure Boot should enable lockdown, and I don't see what this has
>>>> to do with kexec. And "someone blacklist[ing] your key in the
>>>> bootloader" sounds like a political issue, not a technical issue.
>>>
>>> A kernel that allows users arbitrary access to ring 0 is just an
>>> overfeatured bootloader. Why would you want secure boot in that case?
>>
>> To get a chain of trust. I can provision a system with some public
>> keys, stored in UEFI authenticated variables, such that the system
>> will only boot a signed image. That signed image, can, in turn, load
>> a signed (or hashed or otherwise verfified) kernel and a verified
>> initramfs. The initramfs can run a full system from a verified (using
>> dm-verity or similar) filesystem, for example. Now it's very hard to
>> persistently attack this system. Chromium OS does something very much
>> like this, except that it doesn't use UEFI as far as I know. So does
>> iOS, and so do some Android versions.
>
> Correct, Chrome OS does not use UEFI, and we still want this patch
> series, as it plugs all the known "intentional" escalation paths from
> uid-0 to ring-0. Happily, that means all the politics around the UEFI
> and Secure Boot case can be ignored, because those issues are specific
> to Secure Boot, not the lockdown series. (They are _related_, sure,
> but lockdown isn't only about Secure Boot -- it's just that SB is one
> of the widely deployed implementations of this kind of
> trust-chain-booting-thing. Chrome OS and Android's Verified Boot do
> similar things and have the same expectations about the uid-0/ring-0
> separation.)
>
> The goal for that bright line on Chrome OS and Android is to stop
> attack persistence. We want to know that a reboot onto a new kernel
> and OS image will actually result in getting the desired system state,
> and that any attack on persistent system data (even for things running
> with full root privileges) can't result in using kernel interfaces to
> gain kernel control. This isn't expected to be _perfect_, since
> nothing is. But it creates a place to work from. The idea that uid-0
> is NOT ring-0 is still relatively new, so the existing designs in the
> kernel aren't well suited to building that distinction. I view this
> series as a solid first step to getting there, though.
>
But wouldn't Chrome OS possibly want to lock down kernel memory write
vectors but not read vectors? After all, debugging is useful even on
Chrome OS.
--Andy
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: David Howells @ 2018-04-03 17:16 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Matthew Garrett, Ard Biesheuvel, James Morris, Alan Cox,
Linus Torvalds, Greg Kroah-Hartman, Linux Kernel Mailing List,
Justin Forbes, linux-man, joeyli, LSM List, Linux API, Kees Cook,
linux-efi
In-Reply-To: <CALCETrXeVod=kNpG-M7yKAMM0-n+PMg_OakN6ecWrTPmKXgMLg@mail.gmail.com>
Andy Lutomirski <luto@kernel.org> wrote:
> > A kernel that allows users arbitrary access to ring 0 is just an
> > overfeatured bootloader. Why would you want secure boot in that case?
>
> To get a chain of trust.
You don't have a chain of trust that you can trust in that case.
David
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Andy Lutomirski @ 2018-04-03 16:45 UTC (permalink / raw)
To: Matthew Garrett
Cc: Andrew Lutomirski, David Howells, Ard Biesheuvel, James Morris,
Alan Cox, Linus Torvalds, Greg Kroah-Hartman,
Linux Kernel Mailing List, Justin Forbes, linux-man, joeyli,
LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <CACdnJut31+ZtiR40nkOjm89cfUxS0EM=w-oG69PtWHbXYJQxPg@mail.gmail.com>
On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett <mjg59@google.com> wrote:
> On Tue, Apr 3, 2018 at 8:11 AM Andy Lutomirski <luto@kernel.org> wrote:
>> Can you explain that much more clearly? I'm asking why booting via
>> UEFI Secure Boot should enable lockdown, and I don't see what this has
>> to do with kexec. And "someone blacklist[ing] your key in the
>> bootloader" sounds like a political issue, not a technical issue.
>
> A kernel that allows users arbitrary access to ring 0 is just an
> overfeatured bootloader. Why would you want secure boot in that case?
To get a chain of trust. I can provision a system with some public
keys, stored in UEFI authenticated variables, such that the system
will only boot a signed image. That signed image, can, in turn, load
a signed (or hashed or otherwise verfified) kernel and a verified
initramfs. The initramfs can run a full system from a verified (using
dm-verity or similar) filesystem, for example. Now it's very hard to
persistently attack this system. Chromium OS does something very much
like this, except that it doesn't use UEFI as far as I know. So does
iOS, and so do some Android versions. None of this requires lockdown,
or even a separation between usermode and kernelmode, to work
correctly. One could even do this on an MMU-less system if one really
cared to. More usefully, someone probably has done this using a
unikernel.
If I had to guess at a motivation that makes this patchset work, it
would be that there is an uneasy truce between Microsoft and the
various vendors of signed Linux bootloaders. That truce could
conceivably require that the signed bootloaders not knowingly ship a
system that allows a non-physically-present user to chainload Windows.
If so, the patchset should say that loud and clear in its description
and the parts that block bpf should go away.
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-04-03 16:36 UTC (permalink / raw)
To: One Thousand Gnomes
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas <cata>
In-Reply-To: <1890356924.1736.1522683188833.JavaMail.zimbra@efficios.com>
----- On Apr 2, 2018, at 11:33 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> ----- On Apr 1, 2018, at 12:13 PM, One Thousand Gnomes
> gnomes@lxorguk.ukuu.org.uk wrote:
>
>> On Tue, 27 Mar 2018 12:05:23 -0400
>> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>>
>>> Expose a new system call allowing each thread to register one userspace
>>> memory area to be used as an ABI between kernel and user-space for two
>>> purposes: user-space restartable sequences and quick access to read the
>>> current CPU number value from user-space.
>>
>> What is the *worst* case timing achievable by using the atomics ? What
>> does it do to real time performance requirements ?
>
> Given that there are two system calls introduced in this series (rseq and
> cpu_opv), can you clarify which system call you refer to in the two questions
> above ?
>
> For rseq, given that its userspace works pretty much like a read seqlock
> (it retries on failure), it has no impact whatsoever on scheduler behavior.
> So characterizing its worst case timing does not appear to be relevant.
>
>> For cpu_opv you now
>> give an answer but your answer is assuming there isn't another thread
>> actively thrashing the cache or store buffers, and that the user didn't
>> sneakily pass in a page of uncacheable memory (eg framebuffer, or GPU
>> space).
>
> Are those considered as device pages ?
>
>>
>> I don't see anything that restricts it to cached pages. With that check
>> in place for x86 at least it would probably be ok and I think the sneaky
>> attacks to make it uncacheable would fail becuase you've got the pages
>> locked so trying to give them to an accelerator will block until you are
>> done.
>>
>> I still like the idea it's just the latencies concern me.
>
> Indeed, cpu_opv touches pages that are shared with user-space with
> preemption off, so this one affects the scheduler latency. The worse-case
> timings I measured for cpu_opv were with cache-cold memory. So I expect that
> another thread actively trashing the cache would be in the same ballpark
> figure. It does not account for a concurrent thread thrashing the store
> buffers though.
>
> The checks enforcing which pages can be touched by cpu_opv operations are
> done within cpu_op_check_page(). is_zone_device_page() is used to ensure no
> device page is touched with preempt disabled. I understand that you would
> prefer to disallow pages of uncacheable memory as well, which I'm fine with.
> Is there an API similar to is_zone_device_page() to check whether a page is
> uncacheable ?
Looking into this a bit more, I notice the following: The pgprot_noncached
(_PAGE_NOCACHE on x86) pgprot is part of the vma->vm_page_prot. Therefore,
in order to have userspace provide pointers to noncached pages as input
to cpu_opv, they need to be part of a userspace vma which has a
pgprot_noncached vm_page_prot.
The cpu_opv system call uses get_user_pages_fast() to grab the struct page
from the userspace addresses, and then passes those pages to vm_map_ram(),
with a PAGE_KERNEL pgprot. This creates a temporary kernel mapping to those
pages, which is then used to read/write from/to those pages with preemption
disabled.
Therefore, with the proposed cpu_opv implementation, the kernel is not
touching noncached mappings with preemption disabled, which should take
care of your latency concern.
Am I missing something ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Matthew Garrett @ 2018-04-03 16:29 UTC (permalink / raw)
To: luto
Cc: David Howells, Ard Biesheuvel, jmorris, Alan Cox, Linus Torvalds,
Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes, linux-man,
jlee, LSM List, linux-api, Kees Cook, linux-efi
In-Reply-To: <CALCETrUFOdrH5ShCCd8yguGjC2NtG8fJY7AW26HH467-02UVGA@mail.gmail.com>
On Tue, Apr 3, 2018 at 8:11 AM Andy Lutomirski <luto@kernel.org> wrote:
> Can you explain that much more clearly? I'm asking why booting via
> UEFI Secure Boot should enable lockdown, and I don't see what this has
> to do with kexec. And "someone blacklist[ing] your key in the
> bootloader" sounds like a political issue, not a technical issue.
A kernel that allows users arbitrary access to ring 0 is just an
overfeatured bootloader. Why would you want secure boot in that case?
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Andy Lutomirski @ 2018-04-03 16:26 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andy Lutomirski, David Howells, Ard Biesheuvel, James Morris,
One Thousand Gnomes, Linus Torvalds, Matthew Garrett, Greg KH,
LKML, Justin Forbes, linux-man, joeyli, LSM List, Linux API,
Kees Cook, linux-efi
In-Reply-To: <20180403154156.f37wrzjhuvijzi2i@ast-mbp.dhcp.thefacebook.com>
On Tue, Apr 3, 2018 at 8:41 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Tue, Apr 03, 2018 at 08:11:07AM -0700, Andy Lutomirski wrote:
>> >
>> >> "bpf: Restrict kernel image access functions when the kernel is locked down":
>> >> This patch just sucks in general.
>> >
>> > Yes - but that's what Alexei Starovoitov specified. bpf kind of sucks since
>> > it gives you unrestricted access to the kernel.
>>
>> bpf, in certain contexts, gives you unrestricted access to *reading*
>> kernel memory. bpf should, under no circumstances, let you write to
>> the kernel unless you're using fault injection or similar.
>>
>> I'm surprised that Alexei acked this patch. If something like XDP or
>> bpfilter starts becoming widely used, this patch will require a lot of
>> reworking to avoid breaking standard distros.
>
> my understanding was that this lockdown set attemps to disallow _reads_
> of kernel memory from anything, so first version of patch was adding
> run-time checks for bpf_probe_read() which is no-go
> and without this helper the bpf for tracing is losing a lot of its power,
> so the easiest is to disable it all.
Fair enough.
> I think lockdown suppose to disable xdp, bpfilter, nflog, raw sockets + pcap too
> otherwise even cap_net_admin can see traffic coming into host.
> Similarly kprobe, perf_event, ftrace should be off as well?
>
I'm reasonably sure that lockdown is not intended to be this far
reaching. cap_net_admin can see traffic coming into the host, and I
don't think lockdown is intended to change that.
David, I think this is exactly why you need to define what "lockdown"
means. As it stands, the best argument I've seen involves
"blacklisting", but that's a political thing and almost no one
involved has any ability to evaluate it. Right now there's a series
of patches that check for "lockdown" and seem to disable things that
make someone uncomfortable. That's not a good way to design a
security feature.
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Alexei Starovoitov @ 2018-04-03 15:41 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Howells, Ard Biesheuvel, James Morris, One Thousand Gnomes,
Linus Torvalds, Matthew Garrett, Greg KH, LKML, Justin Forbes,
linux-man, joeyli, LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <CALCETrUFOdrH5ShCCd8yguGjC2NtG8fJY7AW26HH467-02UVGA@mail.gmail.com>
On Tue, Apr 03, 2018 at 08:11:07AM -0700, Andy Lutomirski wrote:
> >
> >> "bpf: Restrict kernel image access functions when the kernel is locked down":
> >> This patch just sucks in general.
> >
> > Yes - but that's what Alexei Starovoitov specified. bpf kind of sucks since
> > it gives you unrestricted access to the kernel.
>
> bpf, in certain contexts, gives you unrestricted access to *reading*
> kernel memory. bpf should, under no circumstances, let you write to
> the kernel unless you're using fault injection or similar.
>
> I'm surprised that Alexei acked this patch. If something like XDP or
> bpfilter starts becoming widely used, this patch will require a lot of
> reworking to avoid breaking standard distros.
my understanding was that this lockdown set attemps to disallow _reads_
of kernel memory from anything, so first version of patch was adding
run-time checks for bpf_probe_read() which is no-go
and without this helper the bpf for tracing is losing a lot of its power,
so the easiest is to disable it all.
I think lockdown suppose to disable xdp, bpfilter, nflog, raw sockets + pcap too
otherwise even cap_net_admin can see traffic coming into host.
Similarly kprobe, perf_event, ftrace should be off as well?
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Andy Lutomirski @ 2018-04-03 15:11 UTC (permalink / raw)
To: David Howells, Ard Biesheuvel
Cc: Andy Lutomirski, James Morris, One Thousand Gnomes,
Linus Torvalds, Matthew Garrett, Greg KH, LKML, Justin Forbes,
linux-man, joeyli, LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <30459.1522739219@warthog.procyon.org.uk>
[re-added cc's, I think. Sorry, I think I failed to use the gmane
gateway correctly there.]
On Tue, Apr 3, 2018 at 12:06 AM, David Howells <dhowells@redhat.com> wrote:
> Andy Lutomirski <luto@kernel.org> wrote:
>
>> This is an attempt at a review. I'm replying here because I can't find the
>> actual relevant patch emails.
>
> This was the latest post:
>
> https://lkml.org/lkml/2017/11/9/660
>
> and they were posted multiple times before that, plus distributions, such as
> Fedora, have been carrying them for a long while.
>
>> For the rest of this review, I'm going to pretend that you actually want two
>> features: "try-prevent-root-from-corrupting-the-kernel" and
>> "try-to-prevent-root-from-reading-kernel-memory".
>
> It theoretically boils down into those two, but the line is blurrier than you
> think.
>
> Further, some of the vectors that can be used to do one can potentially do the
> other also and it starts getting to be a lot of extra work to distinguish the
> two.
>
>> I do *not* see why the mere act of using Secure Boot should have this
>> effect.
>
> To be able to pass secure boot mode over kexec, you have to make sure that the
> kernel image doesn't get corrupted, lest someone blacklist your signing key in
> the bootloader.
Can you explain that much more clearly? I'm asking why booting via
UEFI Secure Boot should enable lockdown, and I don't see what this has
to do with kexec. And "someone blacklist[ing] your key in the
bootloader" sounds like a political issue, not a technical issue.
What is the actual purpose of these patches?
>
>> In particular, UEFI Secure Boot should *not* enable
>> "try-to-prevent-root-from-reading-kernel-memory", which means that, unless
>> you actually implement the split, you should drop a bunch of the patches.
>
> Yes it should. If someone can read your kernel image, they can steal the
> crypto keys you use to encrypt your filesystem.
Can you please explain the actual attack that is avoided by doing this?
Suppose I'm a bad guy attacking someone's laptop. If I just have
normal uid!=0 access, then these patches have no effect. Instead,
we're talking about an attacker who is somehow able to become global
root and bypass all LSM restrictions but has not gained kernel code
execution. It is indeed the case that your patches make it harder to
simply read the dm-crypt encryption key out of main memory. But root
can attack the disk encryption in many other ways. They can
persistently compromise the machine by adding services or user
accounts or intentionally misconfiguring something. They can directly
read the entire contents of the disk. They can modify the initrd so
that the next time the machine reboots and the user types the
password, the attacker gets the key (unless the TPM is involved, but
getting *that* right on a standard distro is difficult or impossible).
And I'm not even sure why an attacker who manages to become root wants
your disk encryption key. That key is worth nothing unless the
attacker makes its attack persistent, but, if the attacker can install
a persistent user-level backdoor, then they can read the cleartext off
your disk just as easily as they can read the ciphertext.
>
>> "Restrict /dev/{mem,kmem,port} when the kernel is locked down": this should
>> probably split into one restriction for read and one for write.
>
> Not so for /dev/port. Read & Write here are _not_ the same as Read & Write
> on, say, /dev/mem. In fact, if /dev/mem gives you access to mmio ports, then
> the same applies there. Btw, Fedora hasn't even provided /dev/kmem for a
> while.
Then split /dev/mem and turn off /dev/port for all locked-down modes.
>
>> "bpf: Restrict kernel image access functions when the kernel is locked down":
>> This patch just sucks in general.
>
> Yes - but that's what Alexei Starovoitov specified. bpf kind of sucks since
> it gives you unrestricted access to the kernel.
bpf, in certain contexts, gives you unrestricted access to *reading*
kernel memory. bpf should, under no circumstances, let you write to
the kernel unless you're using fault injection or similar.
I'm surprised that Alexei acked this patch. If something like XDP or
bpfilter starts becoming widely used, this patch will require a lot of
reworking to avoid breaking standard distros.
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: David Howells @ 2018-04-03 7:06 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, James Morris, gnomes, Linus Torvalds, mjg59, gregkh,
linux-kernel, jforbes, linux-man, jlee, linux-security-module,
Linux API, Kees Cook
In-Reply-To: <186aeb7e-1225-4bb8-3ff5-863a1cde86de@kernel.org>
Andy Lutomirski <luto@kernel.org> wrote:
> This is an attempt at a review. I'm replying here because I can't find the
> actual relevant patch emails.
This was the latest post:
https://lkml.org/lkml/2017/11/9/660
and they were posted multiple times before that, plus distributions, such as
Fedora, have been carrying them for a long while.
> For the rest of this review, I'm going to pretend that you actually want two
> features: "try-prevent-root-from-corrupting-the-kernel" and
> "try-to-prevent-root-from-reading-kernel-memory".
It theoretically boils down into those two, but the line is blurrier than you
think.
Further, some of the vectors that can be used to do one can potentially do the
other also and it starts getting to be a lot of extra work to distinguish the
two.
> I do *not* see why the mere act of using Secure Boot should have this
> effect.
To be able to pass secure boot mode over kexec, you have to make sure that the
kernel image doesn't get corrupted, lest someone blacklist your signing key in
the bootloader.
> In particular, UEFI Secure Boot should *not* enable
> "try-to-prevent-root-from-reading-kernel-memory", which means that, unless
> you actually implement the split, you should drop a bunch of the patches.
Yes it should. If someone can read your kernel image, they can steal the
crypto keys you use to encrypt your filesystem.
> "Restrict /dev/{mem,kmem,port} when the kernel is locked down": this should
> probably split into one restriction for read and one for write.
Not so for /dev/port. Read & Write here are _not_ the same as Read & Write
on, say, /dev/mem. In fact, if /dev/mem gives you access to mmio ports, then
the same applies there. Btw, Fedora hasn't even provided /dev/kmem for a
while.
> "bpf: Restrict kernel image access functions when the kernel is locked down":
> This patch just sucks in general.
Yes - but that's what Alexei Starovoitov specified. bpf kind of sucks since
it gives you unrestricted access to the kernel.
> "debugfs: Restrict debugfs when the kernel is locked down": The logic is IMO
> nutty. Why the 0444 restriction? I see no reason that reading a 0644 file
> should be treated any differently from reading a 0444 file.
Yes. IMO it should be locked down entirely. However, it's been abused and
there are things in there that are apparently needed (ie. it's not
debugging-only now); unfortunately, it *also* contains files that directly map
hardware.
> "efi: Lock down the kernel if booted in secure boot mode": you have a stray
> change in fs/debugfs/inode.c in here.
Good catch, thanks.
> Also, as above, I really dislike this patch.
You dislike everything, but you didn't say so any of the times these patches
were posted...
> "lockdown: Print current->comm in restriction messages": Shouldn't this be
> folded in with whatever patch added that code in the first place?
Perhaps, but at the time I added it, I didn't want to go back and change the
existing patches again. If I have to do so, I'll fold it in then.
David
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Andy Lutomirski @ 2018-04-03 1:47 UTC (permalink / raw)
To: Kees Cook
Cc: Andy Lutomirski, James Morris, David Howells, Alan Cox,
Linus Torvalds, Matthew Garrett, Greg KH, LKML, Justin Forbes,
linux-man, joeyli, linux-security-module, Linux API
In-Reply-To: <CAGXu5j+UWQWDacMvvRCke3xUOb7uTkxn=WaHzG4kJTKWh-6tAA@mail.gmail.com>
> On Apr 2, 2018, at 5:59 PM, Kees Cook <keescook@chromium.org> wrote:
>
>> On Mon, Apr 2, 2018 at 5:37 PM, Andy Lutomirski <luto@kernel.org> wrote:
>>> On 03/30/2018 05:46 PM, James Morris wrote:
>>>
>>>> On Sat, 31 Mar 2018, David Howells wrote:
>>>>
>>>> Date: Thu, 26 Oct 2017 17:37:38 +0100
>>>>
>>>> Hi James,
>>>>
>>>> Can you pull this patchset into security/next please? It has been in
>>>> linux-next since the beginning of March.
>>>>
>>>> It adds kernel lockdown support for EFI secure boot.
>>>
>>>
>>> Applied to
>>> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
>>> next-lockdown and next-testing
>>>
>>> Are there any known coverage gaps now?
>>>
>>>
>>>
>>
>> This is an attempt at a review. I'm replying here because I can't find the
>> actual relevant patch emails.
>>
>> Cover letter:
>>
>>> Here's a set of patches to institute a "locked-down mode" in the
>>> kernel and to trigger that mode if the kernel is booted in secure-boot >
>>> mode or through the command line.
>>
>> I think this is seriously problematic in that it's not well defined. It
>> sounds like "locked-down mode" means "make me feel good about something".
>
> Naming of this feature has been multi-year bikeshedding, so if we
> could just leave the name, that'd be nice.
Fair enough. How about enum kernel_lockdown_level with three modes?
>
>
>> "Restrict /dev/{mem,kmem,port} when the kernel is locked down": this should
>> probably split into one restriction for read and one for write.
>
> I think splitting read and write is only useful if there is a use-case
> for only blocking one of them. I struggle to imagine allowing write
> and blocking read, so really it's the case of wanting to allow read
> and disallow write. Is there actually a use-case for this? In all the
> "locked down" cases I've seen, both are desired.
>
Let’s suppose for the sake of argument that UEFI really has a good reason to block writes. Blocking reads (kprobes, perf, etc) sounds extremely annoying, especially if running a stock distro, and I’d much rather not do it unless there’s a specific use case that needs it.
^ permalink raw reply
* Re: [PATCH v4 04/24] fpga: add device feature list support
From: Wu Hao @ 2018-04-03 1:36 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <CANk1AXRt9Tn6HbYtL3xpYPY=CgUrTgCuvCrYcBVJUObBTX=N9w@mail.gmail.com>
On Mon, Apr 02, 2018 at 02:06:56PM -0500, Alan Tull wrote:
> On Sun, Apr 1, 2018 at 11:22 PM, Wu Hao <hao.wu@intel.com> wrote:
> > On Thu, Mar 29, 2018 at 04:57:22PM -0500, Alan Tull wrote:
> >> On Mon, Mar 26, 2018 at 9:35 PM, Wu Hao <hao.wu@intel.com> wrote:
> >>
> >> Hi Hao,
> >>
> >> Currently there is one set of functions that handles port enable,
> >> disable, and reset and it's in dfl.c and dfl.h, so that's not in any
> >> driver module that can be switched out if necessary for a different
> >> implementation of the port. Finding a way for this patchset to be
> >> structured for DFL to control what low level manager/port drivers are
> >> used is the current challenge that I've got a lot of my attention on.
> >>
> >> Thanks for the explanations on how virtualization affects how this can
> >> be implemented.
> >>
> >> > On Mon, Mar 26, 2018 at 12:21:23PM -0500, Alan Tull wrote:
> >> >> On Thu, Mar 22, 2018 at 11:33 PM, Wu Hao <hao.wu@intel.com> wrote:
> >> >>
> >> >> >> > +
> >> >> >> > +/*
> >> >> >> > + * This function resets the FPGA Port and its accelerator (AFU) by function
> >> >> >> > + * __fpga_port_disable and __fpga_port_enable (set port soft reset bit and
> >> >> >> > + * then clear it). Userspace can do Port reset at any time, e.g during DMA
> >> >> >> > + * or Partial Reconfiguration. But it should never cause any system level
> >> >> >> > + * issue, only functional failure (e.g DMA or PR operation failure) and be
> >> >> >> > + * recoverable from the failure.
> >> >> >> > + *
> >> >> >> > + * Note: the accelerator (AFU) is not accessible when its port is in reset
> >> >> >> > + * (disabled). Any attempts on MMIO access to AFU while in reset, will
> >> >> >> > + * result errors reported via port error reporting sub feature (if present).
> >> >> >> > + */
> >> >> >> > +static inline int __fpga_port_reset(struct platform_device *pdev)
> >> >> >> > +{
> >> >> >> > + int ret;
> >> >> >> > +
> >> >> >> > + ret = __fpga_port_disable(pdev);
> >> >> >> > + if (ret)
> >> >> >> > + return ret;
> >> >> >> > +
> >> >> >> > + __fpga_port_enable(pdev);
> >> >> >> > +
> >> >> >> > + return 0;
> >> >> >> > +}
> >> >> >> > +
> >> >> >> > +static inline int fpga_port_reset(struct platform_device *pdev)
> >> >> >> > +{
> >> >> >> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >> >> >> > + int ret;
> >> >> >> > +
> >> >> >> > + mutex_lock(&pdata->lock);
> >> >> >> > + ret = __fpga_port_reset(pdev);
> >> >> >> > + mutex_unlock(&pdata->lock);
> >> >> >> > +
> >> >> >> > + return ret;
> >> >> >> > +}
> >> >> >>
> >> >> >> I'm still scratching my head about how the enumeration code also has
> >> >> >> code that handles resetting the PL in a FPGA region and
> >> >> >> enabling/disabling the bridge. We've discussed this before [1] and I
> >> >> >> know you've looked into it, I'm still trying to figure out how this
> >> >> >> can be made modular, so when someone needs to support a different port
> >> >> >> in the future, it isn't a complete rewrite.
> >> >> >>
> >> >> >> Speaking of resets, one way forward would be to create a reset
> >> >> >> controller for the port (and if possible move the port code to the
> >> >> >> bridge platform driver). The current linux-next repo adds support for
> >> >> >> reset lookups, so that reset controllers are supported for non-DT
> >> >> >> platforms [2].
> >> >> >>
> >> >> >> So the bridge driver would implement the enable/disable functions and
> >> >> >> create a reset controller, the fpga-region (or whoever else needs it)
> >> >> >> could look the reset controller and use the reset. By using the
> >> >> >> kernel reset framework, we don't have to have that piece of code
> >> >> >> shared around by having a reset function in a .h file. And it avoids
> >> >> >> adding extra dependencies between modules. Also, where necessary, I'd
> >> >> >> rather add functionality to the existing bridge/mgr/region frameworks,
> >> >> >> adding common interfaces at that level to allow reuse (like adding
> >> >> >> status to fpga-mgr). Ideally, this DFL framework would sit on top of
> >> >> >> mgr and bridge and allow those to be swapped out for reuse of the DFL
> >> >> >> framework on other devices. Also it will save future headaches as mgr
> >> >> >> or port implementations evolve.
> >> >> >
> >> >> > Thanks a lot for the suggestion. I really really appreciate this.
> >> >>
> >> >> Yes, this is a good discussion, thanks.
> >> >>
> >> >> >
> >> >> > Actually if we consider the virutalization case as I mentioned in [1] below,
> >> >> > that means AFU and its Port will be turned into a PCI VF and assigned (passed
> >> >> > through) to a virtual machine. There is no FME block on that PCI VF device,
> >> >> > (the FME is always kept in PCI PF device in the host) and currently the bridge
> >> >> > is created by FME module for PR functionatily. So in the guest virtual machine,
> >> >> > nobody creates the reset controller actually.
> >> >> >
> >> >> > As I mentioned in [1], one possible method is, put these port reset functions to
> >> >> > AFU (Port) module, and share those functions with FME bridge module.
> >> >>
> >> >> Yes, the port reset functions could move into an AFU driver, and then
> >> >> also the AFU driver could also create a reset controller and register
> >> >> a lookup [2] for the reset. That would be just a few lines of code.
> >> >> The reset controller would control enabling/disabling the port. The
> >> >> bridge driver could get the reset controller to use during FPGA
> >> >> programming. That is instead of sharing a reset function with the
> >> >> bridge driver. It decouples the FPGA bridge driver and simplifies it
> >> >> to be something that just needs to control a reset instead of needing
> >> >> to include a specific .h file that makes a port reset function
> >> >> available.
> >> >
> >> > Hi Alan
> >> >
> >> > Thanks a lot for the feedback. :)
> >> >
> >> > The major concern here is, for virtualization case, after we enable the SRIOV
> >> > to create VFs, AFUs(and ports) are turned into VFs from PF. Once AFUs are moved
> >> > from PF to VFs, then we should remove all related user interfaces exported by
> >> > the afu platform device under PF by unregistering these platform devices from
> >> > the system. So in this case the reset controller created by the AFU platform
> >> > driver, should be removed when the AFU platform devices are deleted from the
> >> > system in this case, but we still have FME and FME bridge present on PF, then
> >> > FME bridge can't find the reset controller any longer to do port enable/disable.
> >>
> >> OK
> >>
> >> >
> >> > Sorry, I found my previous description is not accurate.
> >> >
> >> > VFs could be passed through to a virtual machine, if we let AFU/Port create
> >> > reset controller, then the reset controllers are created in the virtual machine.
> >> > And FME is always in PF in the host, so FME bridge in host have no access to the
> >> > reset controllers in the virtual machine.
> >>
> >> Thanks for the explanation. Does the current implementation allows
> >> the port's PORT_CTRL_SFTRST reset bit to be controlled by PF and VF at
> >> the same time?
> >
> > Yes, it allows it to be accessed by PF and VF at the same time, only for Port
> > Registers, not for AFU registers (any access will cause errors reported by HW).
>
> OK that explains a lot.
>
> >
> >> Or is the idea that the VF has to be given up in order to allow the FME PF to
> >> be able to reprogram?
> >
> > Without any notification mechanism between PF and VFs, the safe way of doing the
> > PR to AFUs (accelerators) on VFs is, 1) hot-unplug the AFU (VF) from the VM,
> > 2) turn the AFU back to PF from VF, 3) PR to the AFU on PF, 4) turn that AFU to
> > VF again, 5) hot-plug the AFU (VF) to target VM again. We tested this flow, it
> > works and doesn't need to shutdown the VM.
>
> That sounds like a lot of trouble.
Without notification mechanism, this is the safe way of doing PR. We can't PR a
AFU (assigned to VM) directly without any notification to VM, as VM owns this
device (AFU). SW works on guest may get troubles if PR is done at unexpected
time. For above flow, it's just the pcie hot plug function, and we tested it
works fine as we expected.
>
> > But once we have implemented some
> > methods to notification between PF and VFs, we don't have to do these steps.
>
> See below. Let's get aligned with what we're trying to architect first.
Sure.
>
> >
> >> After the AFU and port is turned into a VF, is the port's memory range is
> >> mapped in both the PF and the VF?
> >
> > Yes.
> >
> >>
> >> >
> >> >>
> >> >> > I think
> >> >> > that will make the code in the common DFL framework a little more clean,
> >> >>
> >> >> Yes, IIUC that may also make it easier as the port/AFU gets added
> >> >> functionality that is intended to be controlled by the VF anyway
> >> >> (while the only port-related thing that is needed by the FME is port
> >> >> enable/disable).
> >> >>
> >> >> > but it
> >> >> > will introduce some module dependency here for sure, (e.g FME modules can't
> >> >> > finish PR without AFU (Port) Module loaded).
> >> >>
> >> >> That sounds like an OK type of dependency, i.e. if the modules are not
> >> >> all loaded, it doesn't work. :-)
> >> >
> >> > Find a reset controller by lookup, if not found, return error code. It seems
> >> > not a really hard module dependency between port/afu and FME bridge modules.
> >>
> >> That was what I was hoping would work here. But if the module isn't
> >> loaded because it failed due to the reset controller in the AFU driver
> >> went away, then, yes, that won't work.
> >>
> >> > But if in FME bridge, it uses functions exposed by port/afu module, that's a
> >> > hard dependency. : )
> >>
> >> Yes I'm trying to find ways to get away from that kind of hard
> >> dependency. So when someone uses this with a different port, it won't
> >> be a huge rewrite of dfl.c and dfl.h. I understand that the port is
> >> used by both the AFU and the PR code, that's why it's in a file that
> >> is included by both of them. That's going to be a problem as soon as
> >> this is used with a different port.
> >
> > or we could add some callbacks, and let port driver register its own function
> > for enable/disable operation? But then dfl.c / dfl.h will still see some common
> > port code there.
> >
> >>
> >> >
> >> > I can try to move related code to afu/port driver instead in the next version
> >> > for sure, but I can't create the reset controller per the reason above. Please
> >> > let me know if more thoughts on this. : )
> >>
> >> Maybe that is the way forward. I'm still thinking about this. So the
> >> DFL will create a AFU driver that includes the port. If someone
> >> implements a different port, there would be a different id to cause
> >> that AFU driver to be loaded instead. It seems a shame that more of
> >> the AFU code couldn't be reused. That was the original idea of
> >> fpga-bridge. Unfortunately it seems that the bridge is needed by both
> >> the VF and PF so it's complicated by that.
> >>
> >> >
> >> >>
> >> >> > But anyway it may be still
> >> >> > acceptable for users as all these modules could be loaded automatically. How do
> >> >> > you think? :)
> >> >>
> >> >> The other thing I want to get right now is if there is a different
> >> >> AFU/port that needs a different driver. Can the DFL be changed to
> >> >> specify what AFU/port to load? I really really want to avoid large
> >> >> code rewrites in the future that we can anticipate now. Such as
> >> >> someone implements their own static image, it has DFL, but the port is
> >> >> somewhat different. Instead of seeing features as just something that
> >> >> gets added, the DFL also specifies what port driver and mgr driver to
> >> >> load. The stuff we discussed above is a good step towards that, but
> >> >> not all of it.
> >> >
> >> > I'm not sure if any vendor
> >>
> >> Since this is open source, it's important to remember that vendors
> >> aren't the only ones driving development of Linux. Any user of FPGA
> >> under Linux can (and has) come along and add to this subsystem. This
> >> code should not discourage that.
> >
> > Agree.
> >
> >>
> >> > wants to create a totally different port here, if
> >> > yes, then it could have a different feature id in Device Feature Header (DFH).
> >> > I think it's possible to use that feature id to decide which driver to load
> >> > (or which platform device to create).
> >>
> >> I think it's what we need.
> >
> > Yes.
> >
> >>
> >> > But vendors don't have to do that, as it
> >> > could reuse current port driver and private features added already, or even
> >> > add some new vendor specific private feature under the port to save cost.
> >>
> >> They would have to implement a static image with port registers that
> >> function the same way for at least port enable/disable/reset. If they
> >> need to tweak the driver implementation for their hardware then that's
> >> not possible or it's ugly at least.
> >
> > Agree, in that case, it's better to use a new feature id with a different
> > port implementation.
> >
> >> This is also the case if you have some newer version of you port while
> >> keeping legacy support for your original port.
> >
> > In Device Feature Header, there is a field to indicate the revision of
> > this Port (private feature has revision bit in DFH too). But we should
> > not use this field to indicate a totally different implementation.
> >
> >>
> >> I understand that virtualization is making this hard. Thanks for
> >> thinking about how this can move forward on this issue.
> >
> > Yes, I will try to move the actual port related code into AFU/port driver
> > in the next version, thanks for the comments and suggestions.
>
> I have some serious doubts that's the direction to go in. Before you
> do a lot of work in that direction, let me explain again the larger
> context and what's motivating my comments.
>
> The point of having a FPGA framework (fpga-bridge.c, fpga-manager.c,
> and fpga-region.c) is to separate the layers above the framework
> (enumeration and interfaces) from the layer below the framework (low
> level FPGA bridge/manager/region drivers). The layer above and the
> layer below shouldn't share code or talk directly to each other. That
> kind of workaround defeats the purpose of having a framework and
> prevents reuse. If you need a workaround like that, it's probably a
> case where the framework needs some added functionality that's
> generally usable. For example, we've run into that before, in v1,
> your FPGA manager driver was returning status via its private data.
> We discussed it and added status to the fpga-mgr framework so you
> wouldn't need to do that.
>
> So implementing fpga_port_enable in the enumeration code and then
> accessing that code in both the AFU code (upper layer) and the FME
> bridge driver (lower layer) beaks the model. When an implementation
> works around a framework to do what it wants, that mean that none of
> that implementation is reusable. A lot of this review has been me
> trying to understand and untangle that. I'm trying to guide the
> development of the FPGA framework to have reusibility.
>
> The 'port' is really what the existing FPGA framework calls an FPGA
> bridge (with added functionality). The port code should go into the
> bridge driver dfl-fme-br.c. There will need to be some new
> functionality needed for fpga-bridge.c to be able to do what you want
> - some way of making a reset function available for VF while
> enable/disable is still available as PF for the fpga-region.c to
> control.
Understand, actually I have considered to move bridge creation from FME
to Port, but it's facing the same problem as reset controller, as port
platform device should be unregisted from system in virtualization case.
Let me consider further to see if any better approach on this implementation.
May be back to this thread later for discussion.
Thanks
Hao
>
> Thanks,
> Alan
>
> >
> > Thanks
> > Hao
> >
> >>
> >> Alan
> >>
> >> >
> >> > Thanks
> >> > Hao
> >> >
> >> >>
> >> >> Alan
> >> >>
> >> >> >
> >> >> > Thanks
> >> >> > Hao
> >> >> >
> >> >> >
> >> >> >>
> >> >> >> Alan
> >> >> >>
> >> >> >> [1] https://lkml.org/lkml/2017/12/22/398
> >> >> >> [2] https://patchwork.kernel.org/patch/10247475/
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Kees Cook @ 2018-04-03 0:59 UTC (permalink / raw)
To: Andy Lutomirski
Cc: James Morris, David Howells, Alan Cox, Linus Torvalds,
Matthew Garrett, Greg KH, LKML, Justin Forbes, linux-man, joeyli,
linux-security-module, Linux API
In-Reply-To: <186aeb7e-1225-4bb8-3ff5-863a1cde86de@kernel.org>
On Mon, Apr 2, 2018 at 5:37 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On 03/30/2018 05:46 PM, James Morris wrote:
>>
>> On Sat, 31 Mar 2018, David Howells wrote:
>>
>>> Date: Thu, 26 Oct 2017 17:37:38 +0100
>>>
>>> Hi James,
>>>
>>> Can you pull this patchset into security/next please? It has been in
>>> linux-next since the beginning of March.
>>>
>>> It adds kernel lockdown support for EFI secure boot.
>>
>>
>> Applied to
>> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
>> next-lockdown and next-testing
>>
>> Are there any known coverage gaps now?
>>
>>
>>
>
> This is an attempt at a review. I'm replying here because I can't find the
> actual relevant patch emails.
>
> Cover letter:
>
>> Here's a set of patches to institute a "locked-down mode" in the
>> kernel and to trigger that mode if the kernel is booted in secure-boot >
>> mode or through the command line.
>
> I think this is seriously problematic in that it's not well defined. It
> sounds like "locked-down mode" means "make me feel good about something".
Naming of this feature has been multi-year bikeshedding, so if we
could just leave the name, that'd be nice.
> For the rest of this review, I'm going to pretend that you actually want two
> features: "try-prevent-root-from-corrupting-the-kernel" and
> "try-to-prevent-root-from-reading-kernel-memory".
That is how I view it, yes. It's about creating a bright line between
uid-0 and ring-0. The most powerful of these distinctions was made
long ago with signed modules. It hasn't been enough, though, since
there have been many ways for uid-0 to read or write kernel memory. My
expectation for this was to reasonably fill all the remaining gaps.
> Also, there should be a justification that allows normal people (i.e. those
> who are not involved in the UEFI signing process) to understand *why* this
> should have anything to do with UEFI. I can very easily see why it would
> make sense for a UEFI authenticated variable to tell the kernel to enable
> one or both of these modes or for there to be an authenticated mechanism for
> the bootloader to tell the kernel to enable it. I do *not* see why the mere
> act of using Secure Boot should have this effect.
>
> In particular, UEFI Secure Boot should *not* enable
> "try-to-prevent-root-from-reading-kernel-memory", which means that, unless
> you actually implement the split, you should drop a bunch of the patches.
>
> In fact, I think the kernel should try to get away from the idea that UEFI
> Secure Boot should imply annoying restrictions. It's really annoying and
> it's never been clear to me that it has a benefit.
FWIW, I've never been a fan of this being UEFI-centric: more than
Secure Boot needs this. For example, Chrome OS's static root of trust
and boot firmware isn't UEFI, but it wants this feature enabled.
Chrome OS would set it on the command line, since the command line is
part of the signed boot image along with the kernel, etc.
> "Restrict /dev/{mem,kmem,port} when the kernel is locked down": this should
> probably split into one restriction for read and one for write.
I think splitting read and write is only useful if there is a use-case
for only blocking one of them. I struggle to imagine allowing write
and blocking read, so really it's the case of wanting to allow read
and disallow write. Is there actually a use-case for this? In all the
"locked down" cases I've seen, both are desired.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Andy Lutomirski @ 2018-04-03 0:37 UTC (permalink / raw)
To: James Morris, David Howells
Cc: gnomes, Linus Torvalds, mjg59, gregkh, linux-kernel, jforbes,
linux-man, jlee, linux-security-module, Linux API, Kees Cook
In-Reply-To: <alpine.LRH.2.21.1803311145180.7769@namei.org>
On 03/30/2018 05:46 PM, James Morris wrote:
> On Sat, 31 Mar 2018, David Howells wrote:
>
>> Date: Thu, 26 Oct 2017 17:37:38 +0100
>>
>> Hi James,
>>
>> Can you pull this patchset into security/next please? It has been in
>> linux-next since the beginning of March.
>>
>> It adds kernel lockdown support for EFI secure boot.
>
> Applied to
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
> next-lockdown and next-testing
>
> Are there any known coverage gaps now?
>
>
>
This is an attempt at a review. I'm replying here because I can't find
the actual relevant patch emails.
Cover letter:
> Here's a set of patches to institute a "locked-down mode" in the
> kernel and to trigger that mode if the kernel is booted in
secure-boot > mode or through the command line.
I think this is seriously problematic in that it's not well defined. It
sounds like "locked-down mode" means "make me feel good about
something". For the rest of this review, I'm going to pretend that you
actually want two features:
"try-prevent-root-from-corrupting-the-kernel" and
"try-to-prevent-root-from-reading-kernel-memory".
Also, there should be a justification that allows normal people (i.e.
those who are not involved in the UEFI signing process) to understand
*why* this should have anything to do with UEFI. I can very easily see
why it would make sense for a UEFI authenticated variable to tell the
kernel to enable one or both of these modes or for there to be an
authenticated mechanism for the bootloader to tell the kernel to enable
it. I do *not* see why the mere act of using Secure Boot should have
this effect.
In particular, UEFI Secure Boot should *not* enable
"try-to-prevent-root-from-reading-kernel-memory", which means that,
unless you actually implement the split, you should drop a bunch of the
patches.
In fact, I think the kernel should try to get away from the idea that
UEFI Secure Boot should imply annoying restrictions. It's really
annoying and it's never been clear to me that it has a benefit.
"Restrict /dev/{mem,kmem,port} when the kernel is locked down": this
should probably split into one restriction for read and one for write.
"Lock down /proc/kcore": should only apply to
"try-to-prevent-root-from-reading-kernel-memory"
"Lock down kprobes": ditto
"bpf: Restrict kernel image access functions when the kernel is locked
down": This patch just sucks in general. At the very least, it should
only apply to "bpf: Restrict kernel image access functions when the
kernel is locked down". But you should probably just force all eBPF
users through the unprivileged path when locked down instead, since eBPF
is really quite useful even with the stricter verification mode.
"Lock down perf": how about preventing using perf on the kernel when
"try-to-prevent-root-from-reading-kernel-memory" is set and not
restricting it otherwise?
"debugfs: Restrict debugfs when the kernel is locked down": The logic is
IMO nutty. Why the 0444 restriction? I see no reason that reading a
0644 file should be treated any differently from reading a 0444 file.
Regardless, I think you should prevent writing or reading depending on
lockdown mode and add an API so that individual debugfs files can
override this.
"efi: Lock down the kernel if booted in secure boot mode": you have a
stray change in fs/debugfs/inode.c in here. Also, as above, I really
dislike this patch.
"lockdown: Print current->comm in restriction messages": Shouldn't this
be folded in with whatever patch added that code in the first place?
^ permalink raw reply
* [RESEND PATCH V4] pidns: introduce syscall translate_pid
From: nagarathnam.muthusamy @ 2018-04-02 21:57 UTC (permalink / raw)
To: linux-api, linux-kernel, ebiederm, khlebnikov
Cc: akpm, serge.hallyn, oleg, luto, jannh, nagarathnam.muthusamy,
prakash.sangappa
pid_t translate_pid(pid_t pid, int source, int target);
This syscall converts pid from source pid-ns into pid in target pid-ns.
If pid is unreachable from target pid-ns it returns zero.
Pid-namespaces are referred file descriptors opened to proc files
/proc/[pid]/ns/pid or /proc/[pid]/ns/pid_for_children. Negative argument
refers to current pid namespace, same as file /proc/self/ns/pid.
Kernel expose virtual pids in /proc/[pid]/status:NSpid, but backward
translation requires scanning all tasks. Also pids could be translated
by sending them through unix socket between namespaces, this method is
slow and insecure because other side is exposed inside pid namespace.
Examples:
translate_pid(pid, ns, -1) - get pid in our pid namespace
translate_pid(pid, -1, ns) - get pid in other pid namespace
translate_pid(1, ns, -1) - get pid of init task for namespace
translate_pid(pid, -1, ns) > 0 - is pid is reachable from ns?
translate_pid(1, ns1, ns2) > 0 - is ns1 inside ns2?
translate_pid(1, ns1, ns2) == 0 - is ns1 outside ns2?
translate_pid(1, ns1, ns2) == 1 - is ns1 equal ns2?
Error codes:
EBADF - file descriptor is closed
EINVAL - file descriptor isn't pid-namespace
ESRCH - task not found in @source namespace
Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Signed-off-by: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
---
v1: https://lkml.org/lkml/2015/9/15/411
v2: https://lkml.org/lkml/2015/9/24/278
* use namespace-fd as second/third argument
* add -pid for getting parent pid
* move code into kernel/sys.c next to getppid
* drop ifdef CONFIG_PID_NS
* add generic syscall
v3: https://lkml.org/lkml/2015/9/28/3
* use proc_ns_fdget()
* update description
* rebase to next-20150925
* fix conflict with mlock2
v4:
* rename into translate_pid()
* remove syscall if CONFIG_PID_NS=n
* drop -pid for parent task
* drop fget-fdget optimizations
* add helper get_pid_ns_by_fd()
* wire only into x86
---
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 1 +
kernel/pid_namespace.c | 66 ++++++++++++++++++++++++++++++++++
kernel/sys_ni.c | 3 ++
5 files changed, 72 insertions(+)
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 448ac21..257d839 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -391,3 +391,4 @@
382 i386 pkey_free sys_pkey_free
383 i386 statx sys_statx
384 i386 arch_prctl sys_arch_prctl compat_sys_arch_prctl
+385 i386 translate_pid sys_translate_pid
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 5aef183..1ebdab8 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -339,6 +339,7 @@
330 common pkey_alloc sys_pkey_alloc
331 common pkey_free sys_pkey_free
332 common statx sys_statx
+333 common translate_pid sys_translate_pid
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index a78186d..6467ebc 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -901,6 +901,7 @@ asmlinkage long sys_open_by_handle_at(int mountdirfd,
struct file_handle __user *handle,
int flags);
asmlinkage long sys_setns(int fd, int nstype);
+asmlinkage long sys_translate_pid(pid_t pid, int source, int target);
asmlinkage long sys_process_vm_readv(pid_t pid,
const struct iovec __user *lvec,
unsigned long liovcnt,
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 773b2b3..bb56a78 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -13,6 +13,7 @@
#include <linux/user_namespace.h>
#include <linux/syscalls.h>
#include <linux/cred.h>
+#include <linux/file.h>
#include <linux/err.h>
#include <linux/acct.h>
#include <linux/slab.h>
@@ -380,6 +381,71 @@ static void pidns_put(struct ns_common *ns)
put_pid_ns(to_pid_ns(ns));
}
+static struct pid_namespace *get_pid_ns_by_fd(int fd)
+{
+ struct pid_namespace *pidns;
+ struct ns_common *ns;
+ struct file *file;
+
+ file = proc_ns_fget(fd);
+ if (IS_ERR(file))
+ return ERR_CAST(file);
+
+ ns = get_proc_ns(file_inode(file));
+ if (ns->ops->type == CLONE_NEWPID)
+ pidns = get_pid_ns(to_pid_ns(ns));
+ else
+ pidns = ERR_PTR(-EINVAL);
+
+ fput(file);
+ return pidns;
+}
+
+/*
+ * translate_pid - convert pid in source pid-ns into target pid-ns.
+ * @pid: pid for translation
+ * @source: pid-ns file descriptor or -1 for active namespace
+ * @target: pid-ns file descriptor or -1 for active namesapce
+ *
+ * Returns pid in @target pid-ns, zero if task have no pid there,
+ * or -ESRCH if task with @pid does not found in @source pid-ns.
+ */
+SYSCALL_DEFINE3(translate_pid, pid_t, pid, int, source, int, target)
+{
+ struct pid_namespace *source_ns, *target_ns;
+ struct pid *struct_pid;
+ pid_t result;
+
+ if (source >= 0) {
+ source_ns = get_pid_ns_by_fd(source);
+ result = PTR_ERR(source_ns);
+ if (IS_ERR(source_ns))
+ goto err_source;
+ } else
+ source_ns = task_active_pid_ns(current);
+
+ if (target >= 0) {
+ target_ns = get_pid_ns_by_fd(target);
+ result = PTR_ERR(target_ns);
+ if (IS_ERR(target_ns))
+ goto err_target;
+ } else
+ target_ns = task_active_pid_ns(current);
+
+ rcu_read_lock();
+ struct_pid = find_pid_ns(pid, source_ns);
+ result = struct_pid ? pid_nr_ns(struct_pid, target_ns) : -ESRCH;
+ rcu_read_unlock();
+
+ if (target >= 0)
+ put_pid_ns(target_ns);
+err_target:
+ if (source >= 0)
+ put_pid_ns(source_ns);
+err_source:
+ return result;
+}
+
static int pidns_install(struct nsproxy *nsproxy, struct ns_common *ns)
{
struct pid_namespace *active = task_active_pid_ns(current);
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index b518976..bf6ef46 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -259,3 +259,6 @@ asmlinkage long sys_ni_syscall(void)
cond_syscall(sys_pkey_mprotect);
cond_syscall(sys_pkey_alloc);
cond_syscall(sys_pkey_free);
+
+/* pid namespace */
+cond_syscall(sys_translate_pid);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v4 04/24] fpga: add device feature list support
From: Alan Tull @ 2018-04-02 19:06 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <20180402042233.GA27618@hao-dev>
On Sun, Apr 1, 2018 at 11:22 PM, Wu Hao <hao.wu@intel.com> wrote:
> On Thu, Mar 29, 2018 at 04:57:22PM -0500, Alan Tull wrote:
>> On Mon, Mar 26, 2018 at 9:35 PM, Wu Hao <hao.wu@intel.com> wrote:
>>
>> Hi Hao,
>>
>> Currently there is one set of functions that handles port enable,
>> disable, and reset and it's in dfl.c and dfl.h, so that's not in any
>> driver module that can be switched out if necessary for a different
>> implementation of the port. Finding a way for this patchset to be
>> structured for DFL to control what low level manager/port drivers are
>> used is the current challenge that I've got a lot of my attention on.
>>
>> Thanks for the explanations on how virtualization affects how this can
>> be implemented.
>>
>> > On Mon, Mar 26, 2018 at 12:21:23PM -0500, Alan Tull wrote:
>> >> On Thu, Mar 22, 2018 at 11:33 PM, Wu Hao <hao.wu@intel.com> wrote:
>> >>
>> >> >> > +
>> >> >> > +/*
>> >> >> > + * This function resets the FPGA Port and its accelerator (AFU) by function
>> >> >> > + * __fpga_port_disable and __fpga_port_enable (set port soft reset bit and
>> >> >> > + * then clear it). Userspace can do Port reset at any time, e.g during DMA
>> >> >> > + * or Partial Reconfiguration. But it should never cause any system level
>> >> >> > + * issue, only functional failure (e.g DMA or PR operation failure) and be
>> >> >> > + * recoverable from the failure.
>> >> >> > + *
>> >> >> > + * Note: the accelerator (AFU) is not accessible when its port is in reset
>> >> >> > + * (disabled). Any attempts on MMIO access to AFU while in reset, will
>> >> >> > + * result errors reported via port error reporting sub feature (if present).
>> >> >> > + */
>> >> >> > +static inline int __fpga_port_reset(struct platform_device *pdev)
>> >> >> > +{
>> >> >> > + int ret;
>> >> >> > +
>> >> >> > + ret = __fpga_port_disable(pdev);
>> >> >> > + if (ret)
>> >> >> > + return ret;
>> >> >> > +
>> >> >> > + __fpga_port_enable(pdev);
>> >> >> > +
>> >> >> > + return 0;
>> >> >> > +}
>> >> >> > +
>> >> >> > +static inline int fpga_port_reset(struct platform_device *pdev)
>> >> >> > +{
>> >> >> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> >> >> > + int ret;
>> >> >> > +
>> >> >> > + mutex_lock(&pdata->lock);
>> >> >> > + ret = __fpga_port_reset(pdev);
>> >> >> > + mutex_unlock(&pdata->lock);
>> >> >> > +
>> >> >> > + return ret;
>> >> >> > +}
>> >> >>
>> >> >> I'm still scratching my head about how the enumeration code also has
>> >> >> code that handles resetting the PL in a FPGA region and
>> >> >> enabling/disabling the bridge. We've discussed this before [1] and I
>> >> >> know you've looked into it, I'm still trying to figure out how this
>> >> >> can be made modular, so when someone needs to support a different port
>> >> >> in the future, it isn't a complete rewrite.
>> >> >>
>> >> >> Speaking of resets, one way forward would be to create a reset
>> >> >> controller for the port (and if possible move the port code to the
>> >> >> bridge platform driver). The current linux-next repo adds support for
>> >> >> reset lookups, so that reset controllers are supported for non-DT
>> >> >> platforms [2].
>> >> >>
>> >> >> So the bridge driver would implement the enable/disable functions and
>> >> >> create a reset controller, the fpga-region (or whoever else needs it)
>> >> >> could look the reset controller and use the reset. By using the
>> >> >> kernel reset framework, we don't have to have that piece of code
>> >> >> shared around by having a reset function in a .h file. And it avoids
>> >> >> adding extra dependencies between modules. Also, where necessary, I'd
>> >> >> rather add functionality to the existing bridge/mgr/region frameworks,
>> >> >> adding common interfaces at that level to allow reuse (like adding
>> >> >> status to fpga-mgr). Ideally, this DFL framework would sit on top of
>> >> >> mgr and bridge and allow those to be swapped out for reuse of the DFL
>> >> >> framework on other devices. Also it will save future headaches as mgr
>> >> >> or port implementations evolve.
>> >> >
>> >> > Thanks a lot for the suggestion. I really really appreciate this.
>> >>
>> >> Yes, this is a good discussion, thanks.
>> >>
>> >> >
>> >> > Actually if we consider the virutalization case as I mentioned in [1] below,
>> >> > that means AFU and its Port will be turned into a PCI VF and assigned (passed
>> >> > through) to a virtual machine. There is no FME block on that PCI VF device,
>> >> > (the FME is always kept in PCI PF device in the host) and currently the bridge
>> >> > is created by FME module for PR functionatily. So in the guest virtual machine,
>> >> > nobody creates the reset controller actually.
>> >> >
>> >> > As I mentioned in [1], one possible method is, put these port reset functions to
>> >> > AFU (Port) module, and share those functions with FME bridge module.
>> >>
>> >> Yes, the port reset functions could move into an AFU driver, and then
>> >> also the AFU driver could also create a reset controller and register
>> >> a lookup [2] for the reset. That would be just a few lines of code.
>> >> The reset controller would control enabling/disabling the port. The
>> >> bridge driver could get the reset controller to use during FPGA
>> >> programming. That is instead of sharing a reset function with the
>> >> bridge driver. It decouples the FPGA bridge driver and simplifies it
>> >> to be something that just needs to control a reset instead of needing
>> >> to include a specific .h file that makes a port reset function
>> >> available.
>> >
>> > Hi Alan
>> >
>> > Thanks a lot for the feedback. :)
>> >
>> > The major concern here is, for virtualization case, after we enable the SRIOV
>> > to create VFs, AFUs(and ports) are turned into VFs from PF. Once AFUs are moved
>> > from PF to VFs, then we should remove all related user interfaces exported by
>> > the afu platform device under PF by unregistering these platform devices from
>> > the system. So in this case the reset controller created by the AFU platform
>> > driver, should be removed when the AFU platform devices are deleted from the
>> > system in this case, but we still have FME and FME bridge present on PF, then
>> > FME bridge can't find the reset controller any longer to do port enable/disable.
>>
>> OK
>>
>> >
>> > Sorry, I found my previous description is not accurate.
>> >
>> > VFs could be passed through to a virtual machine, if we let AFU/Port create
>> > reset controller, then the reset controllers are created in the virtual machine.
>> > And FME is always in PF in the host, so FME bridge in host have no access to the
>> > reset controllers in the virtual machine.
>>
>> Thanks for the explanation. Does the current implementation allows
>> the port's PORT_CTRL_SFTRST reset bit to be controlled by PF and VF at
>> the same time?
>
> Yes, it allows it to be accessed by PF and VF at the same time, only for Port
> Registers, not for AFU registers (any access will cause errors reported by HW).
OK that explains a lot.
>
>> Or is the idea that the VF has to be given up in order to allow the FME PF to
>> be able to reprogram?
>
> Without any notification mechanism between PF and VFs, the safe way of doing the
> PR to AFUs (accelerators) on VFs is, 1) hot-unplug the AFU (VF) from the VM,
> 2) turn the AFU back to PF from VF, 3) PR to the AFU on PF, 4) turn that AFU to
> VF again, 5) hot-plug the AFU (VF) to target VM again. We tested this flow, it
> works and doesn't need to shutdown the VM.
That sounds like a lot of trouble.
> But once we have implemented some
> methods to notification between PF and VFs, we don't have to do these steps.
See below. Let's get aligned with what we're trying to architect first.
>
>> After the AFU and port is turned into a VF, is the port's memory range is
>> mapped in both the PF and the VF?
>
> Yes.
>
>>
>> >
>> >>
>> >> > I think
>> >> > that will make the code in the common DFL framework a little more clean,
>> >>
>> >> Yes, IIUC that may also make it easier as the port/AFU gets added
>> >> functionality that is intended to be controlled by the VF anyway
>> >> (while the only port-related thing that is needed by the FME is port
>> >> enable/disable).
>> >>
>> >> > but it
>> >> > will introduce some module dependency here for sure, (e.g FME modules can't
>> >> > finish PR without AFU (Port) Module loaded).
>> >>
>> >> That sounds like an OK type of dependency, i.e. if the modules are not
>> >> all loaded, it doesn't work. :-)
>> >
>> > Find a reset controller by lookup, if not found, return error code. It seems
>> > not a really hard module dependency between port/afu and FME bridge modules.
>>
>> That was what I was hoping would work here. But if the module isn't
>> loaded because it failed due to the reset controller in the AFU driver
>> went away, then, yes, that won't work.
>>
>> > But if in FME bridge, it uses functions exposed by port/afu module, that's a
>> > hard dependency. : )
>>
>> Yes I'm trying to find ways to get away from that kind of hard
>> dependency. So when someone uses this with a different port, it won't
>> be a huge rewrite of dfl.c and dfl.h. I understand that the port is
>> used by both the AFU and the PR code, that's why it's in a file that
>> is included by both of them. That's going to be a problem as soon as
>> this is used with a different port.
>
> or we could add some callbacks, and let port driver register its own function
> for enable/disable operation? But then dfl.c / dfl.h will still see some common
> port code there.
>
>>
>> >
>> > I can try to move related code to afu/port driver instead in the next version
>> > for sure, but I can't create the reset controller per the reason above. Please
>> > let me know if more thoughts on this. : )
>>
>> Maybe that is the way forward. I'm still thinking about this. So the
>> DFL will create a AFU driver that includes the port. If someone
>> implements a different port, there would be a different id to cause
>> that AFU driver to be loaded instead. It seems a shame that more of
>> the AFU code couldn't be reused. That was the original idea of
>> fpga-bridge. Unfortunately it seems that the bridge is needed by both
>> the VF and PF so it's complicated by that.
>>
>> >
>> >>
>> >> > But anyway it may be still
>> >> > acceptable for users as all these modules could be loaded automatically. How do
>> >> > you think? :)
>> >>
>> >> The other thing I want to get right now is if there is a different
>> >> AFU/port that needs a different driver. Can the DFL be changed to
>> >> specify what AFU/port to load? I really really want to avoid large
>> >> code rewrites in the future that we can anticipate now. Such as
>> >> someone implements their own static image, it has DFL, but the port is
>> >> somewhat different. Instead of seeing features as just something that
>> >> gets added, the DFL also specifies what port driver and mgr driver to
>> >> load. The stuff we discussed above is a good step towards that, but
>> >> not all of it.
>> >
>> > I'm not sure if any vendor
>>
>> Since this is open source, it's important to remember that vendors
>> aren't the only ones driving development of Linux. Any user of FPGA
>> under Linux can (and has) come along and add to this subsystem. This
>> code should not discourage that.
>
> Agree.
>
>>
>> > wants to create a totally different port here, if
>> > yes, then it could have a different feature id in Device Feature Header (DFH).
>> > I think it's possible to use that feature id to decide which driver to load
>> > (or which platform device to create).
>>
>> I think it's what we need.
>
> Yes.
>
>>
>> > But vendors don't have to do that, as it
>> > could reuse current port driver and private features added already, or even
>> > add some new vendor specific private feature under the port to save cost.
>>
>> They would have to implement a static image with port registers that
>> function the same way for at least port enable/disable/reset. If they
>> need to tweak the driver implementation for their hardware then that's
>> not possible or it's ugly at least.
>
> Agree, in that case, it's better to use a new feature id with a different
> port implementation.
>
>> This is also the case if you have some newer version of you port while
>> keeping legacy support for your original port.
>
> In Device Feature Header, there is a field to indicate the revision of
> this Port (private feature has revision bit in DFH too). But we should
> not use this field to indicate a totally different implementation.
>
>>
>> I understand that virtualization is making this hard. Thanks for
>> thinking about how this can move forward on this issue.
>
> Yes, I will try to move the actual port related code into AFU/port driver
> in the next version, thanks for the comments and suggestions.
I have some serious doubts that's the direction to go in. Before you
do a lot of work in that direction, let me explain again the larger
context and what's motivating my comments.
The point of having a FPGA framework (fpga-bridge.c, fpga-manager.c,
and fpga-region.c) is to separate the layers above the framework
(enumeration and interfaces) from the layer below the framework (low
level FPGA bridge/manager/region drivers). The layer above and the
layer below shouldn't share code or talk directly to each other. That
kind of workaround defeats the purpose of having a framework and
prevents reuse. If you need a workaround like that, it's probably a
case where the framework needs some added functionality that's
generally usable. For example, we've run into that before, in v1,
your FPGA manager driver was returning status via its private data.
We discussed it and added status to the fpga-mgr framework so you
wouldn't need to do that.
So implementing fpga_port_enable in the enumeration code and then
accessing that code in both the AFU code (upper layer) and the FME
bridge driver (lower layer) beaks the model. When an implementation
works around a framework to do what it wants, that mean that none of
that implementation is reusable. A lot of this review has been me
trying to understand and untangle that. I'm trying to guide the
development of the FPGA framework to have reusibility.
The 'port' is really what the existing FPGA framework calls an FPGA
bridge (with added functionality). The port code should go into the
bridge driver dfl-fme-br.c. There will need to be some new
functionality needed for fpga-bridge.c to be able to do what you want
- some way of making a reset function available for VF while
enable/disable is still available as PF for the fpga-region.c to
control.
Thanks,
Alan
>
> Thanks
> Hao
>
>>
>> Alan
>>
>> >
>> > Thanks
>> > Hao
>> >
>> >>
>> >> Alan
>> >>
>> >> >
>> >> > Thanks
>> >> > Hao
>> >> >
>> >> >
>> >> >>
>> >> >> Alan
>> >> >>
>> >> >> [1] https://lkml.org/lkml/2017/12/22/398
>> >> >> [2] https://patchwork.kernel.org/patch/10247475/
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-04-02 15:33 UTC (permalink / raw)
To: One Thousand Gnomes
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas <cata>
In-Reply-To: <20180401171356.085a2a33@alans-desktop>
----- On Apr 1, 2018, at 12:13 PM, One Thousand Gnomes gnomes@lxorguk.ukuu.org.uk wrote:
> On Tue, 27 Mar 2018 12:05:23 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
>> Expose a new system call allowing each thread to register one userspace
>> memory area to be used as an ABI between kernel and user-space for two
>> purposes: user-space restartable sequences and quick access to read the
>> current CPU number value from user-space.
>
> What is the *worst* case timing achievable by using the atomics ? What
> does it do to real time performance requirements ?
Given that there are two system calls introduced in this series (rseq and
cpu_opv), can you clarify which system call you refer to in the two questions
above ?
For rseq, given that its userspace works pretty much like a read seqlock
(it retries on failure), it has no impact whatsoever on scheduler behavior.
So characterizing its worst case timing does not appear to be relevant.
> For cpu_opv you now
> give an answer but your answer is assuming there isn't another thread
> actively thrashing the cache or store buffers, and that the user didn't
> sneakily pass in a page of uncacheable memory (eg framebuffer, or GPU
> space).
Are those considered as device pages ?
>
> I don't see anything that restricts it to cached pages. With that check
> in place for x86 at least it would probably be ok and I think the sneaky
> attacks to make it uncacheable would fail becuase you've got the pages
> locked so trying to give them to an accelerator will block until you are
> done.
>
> I still like the idea it's just the latencies concern me.
Indeed, cpu_opv touches pages that are shared with user-space with
preemption off, so this one affects the scheduler latency. The worse-case
timings I measured for cpu_opv were with cache-cold memory. So I expect that
another thread actively trashing the cache would be in the same ballpark
figure. It does not account for a concurrent thread thrashing the store
buffers though.
The checks enforcing which pages can be touched by cpu_opv operations are
done within cpu_op_check_page(). is_zone_device_page() is used to ensure no
device page is touched with preempt disabled. I understand that you would
prefer to disallow pages of uncacheable memory as well, which I'm fine with.
Is there an API similar to is_zone_device_page() to check whether a page is
uncacheable ?
>
>> Restartable sequences are atomic with respect to preemption
>> (making it atomic with respect to other threads running on the
>> same CPU), as well as signal delivery (user-space execution
>> contexts nested over the same thread).
>
> CPU generally means 'big lump with legs on it'. You are not atomic to the
> same CPU, because that CPU may have 30+ cores with 8 threads per core.
>
> It could do with some better terminology (hardware thread, CPU context ?)
Would you be OK with Christoph's terminology of "Hardware Execution Context" ?
>
>> In a typical usage scenario, the thread registering the rseq
>> structure will be performing loads and stores from/to that
>> structure. It is however also allowed to read that structure
>> from other threads. The rseq field updates performed by the
>> kernel provide relaxed atomicity semantics, which guarantee
>> that other threads performing relaxed atomic reads of the cpu
>> number cache will always observe a consistent value.
>
> So what happens to your API if the kernel atomics get improved ? You are
> effectively exporting rseq behaviour from private to public.
Relaxed atomics is pretty much the loosest kind of consistency we can
provide before we start allowing the compiler to do load/store tearing
(it's basically a volatile store of a word-aligned word). It does not
involve any kind of memory barrier whatsoever. I expect that the atomics
that may evolve in the future will be those with release/acquire and
implicit barriers semantics. The relaxed atomicity does not cover any of
these.
Thanks,
Mathieu
>
> Alan
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Paul E. McKenney @ 2018-04-02 15:27 UTC (permalink / raw)
To: Christopher Lameter
Cc: Alan Cox, Mathieu Desnoyers, Peter Zijlstra, Boqun Feng,
Andy Lutomirski, Dave Watson, linux-kernel, linux-api,
Paul Turner, Andrew Morton, Russell King, Thomas Gleixner,
Ingo Molnar, H . Peter Anvin, Andrew Hunter, Andi Kleen,
Ben Maurer, Steven Rostedt, Josh Triplett, Linus Torvalds,
Catalin Marinas
In-Reply-To: <alpine.DEB.2.20.1804021000290.23911@nuc-kabylake>
On Mon, Apr 02, 2018 at 10:03:58AM -0500, Christopher Lameter wrote:
> On Sun, 1 Apr 2018, Alan Cox wrote:
>
> > > Restartable sequences are atomic with respect to preemption
> > > (making it atomic with respect to other threads running on the
> > > same CPU), as well as signal delivery (user-space execution
> > > contexts nested over the same thread).
> >
> > CPU generally means 'big lump with legs on it'. You are not atomic to the
> > same CPU, because that CPU may have 30+ cores with 8 threads per core.
> >
> > It could do with some better terminology (hardware thread, CPU context ?)
>
> Well we call it a "CPU" in the scheduler context I think. We could use
> better terminology throughout the kernel tools and source.
Agreed, it has been "CPU" for "single hardware thread" for a very long
time. People tend to use "core" for "group of hardware threads" and
"socket" for "big lump with legs on it".
> Hardware Execution Context?
Should be even more fun when non-CPU hardware execution contexts show
up in force within each core. ;-)
But the terminology in place for non-CPU hardware execution contexts
should be able to survive that event.
> > > In a typical usage scenario, the thread registering the rseq
> > > structure will be performing loads and stores from/to that
> > > structure. It is however also allowed to read that structure
> > > from other threads. The rseq field updates performed by the
> > > kernel provide relaxed atomicity semantics, which guarantee
> > > that other threads performing relaxed atomic reads of the cpu
> > > number cache will always observe a consistent value.
> >
> > So what happens to your API if the kernel atomics get improved ? You are
> > effectively exporting rseq behaviour from private to public.
>
> There is already a pretty complex coherency model guiding kernel atomics.
> Improvements/changes to that are difficult and the effect will ripple
> throughout the kernel. So I would suggest that these areas of the kernel
> are pretty "petrified" (or written in stone).
I suspect that there are much more pressing areas of confusion in any
case!
Thanx, Paul
^ 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