* Re: Setting monotonic time?
From: Thomas Gleixner @ 2018-10-02 20:06 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Eric W . Biederman, avagin, dima, Linux Kernel Mailing List,
0x7f454c46, adrian, Andy Lutomirski, Christian Brauner, gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov, xemul,
Shuah Khan, containers, criu, Linux API, the arch/x86 maintainers,
Alexey Dobriyan, linux-kselftest
In-Reply-To: <CAK8P3a2zTqrCGURE3XVpn3_w6C4R5O+cq8TdtbyKCMGyRk9_jw@mail.gmail.com>
On Tue, 2 Oct 2018, Arnd Bergmann wrote:
> On Mon, Oct 1, 2018 at 8:53 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Mon, 1 Oct 2018, Eric W. Biederman wrote:
> > > In the context of process migration there is a simpler subproblem that I
> > > think it is worth exploring if we can do something about.
> > >
> > > For a cluster of machines all running with synchronized
> > > clocks. CLOCK_REALTIME matches. CLOCK_MONOTNIC does not match between
> > > machines. Not having a matching CLOCK_MONOTONIC prevents successful
> > > process migration between nodes in that cluster.
> > >
> > > Would it be possible to allow setting CLOCK_MONOTONIC at the very
> > > beginning of time? So that all of the nodes in a cluster can be in
> > > sync?
> > >
> > > No change in skew just in offset for CLOCK_MONOTONIC.
> > >
> > > There are also dragons involved in coordinating things so that
> > > CLOCK_MONOTONIC gets set before CLOCK_MONOTONIC gets used. So I don't
> > > know if allowing CLOCK_MONOTONIC to be set would be practical but it
> > > seems work exploring all on it's own.
> >
> > It's used very early on in the kernel, so that would be a major surprise
> > for many things including user space which has expectations on clock
> > monotonic.
> >
> > It would be reasonably easy to add CLOCK_MONONOTIC_SYNC which can be set in
> > the way you described and then in name spaces make it possible to magically
> > map CLOCK_MONOTONIC to CLOCK_MONOTONIC_SYNC.
> >
> > It still wouldn't allow to have different NTP/PTP time domains, but might
> > be a good start to address the main migration headaches.
>
> If we make CLOCK_MONOTONIC settable this way in a namespace,
> do you think that should include device drivers that report timestamps
> in CLOCK_MONOTONIC base, or only the timekeeping clock and timer
> interfaces?
Uurgh. That gets messy very fast.
> Examples for drivers that can report timestamps are input, sound, v4l,
> and drm. I think most of these can report stamps in either monotonic
> or realtime base, while socket timestamps notably are always in
> realtime.
>
> We can probably get away with not setting the timebase for those
> device drivers as long as the checkpoint/restart and migration features
> are not expected to restore the state of an open character device
> in that way. I don't know if that is a reasonable assumption to make
> for the examples I listed.
No idea. I'm not a container migration wizard.
Thanks,
tglx
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Dan Williams @ 2018-10-02 20:18 UTC (permalink / raw)
To: Jan Kara
Cc: linux-xfs, linux-nvdimm, Linux API, Christoph Hellwig, Linux MM,
linux-fsdevel, linux-ext4
In-Reply-To: <20181002153100.GG9127-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org>
On Tue, Oct 2, 2018 at 8:32 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
>
> On Tue 02-10-18 07:52:06, Christoph Hellwig wrote:
> > On Tue, Oct 02, 2018 at 04:44:13PM +0200, Johannes Thumshirn wrote:
> > > On Tue, Oct 02, 2018 at 07:37:13AM -0700, Christoph Hellwig wrote:
> > > > No, it should not. DAX is an implementation detail thay may change
> > > > or go away at any time.
> > >
> > > Well we had an issue with an application checking for dax, this is how
> > > we landed here in the first place.
> >
> > So what exacty is that "DAX" they are querying about (and no, I'm not
> > joking, nor being philosophical).
>
> I believe the application we are speaking about is mostly concerned about
> the memory overhead of the page cache. Think of a machine that has ~ 1TB of
> DRAM, the database running on it is about that size as well and they want
> database state stored somewhere persistently - which they may want to do by
> modifying mmaped database files if they do small updates... So they really
> want to be able to use close to all DRAM for the DB and not leave slack
> space for the kernel page cache to cache 1TB of database files.
VM_MIXEDMAP was never a reliable indication of DAX because it could be
set for random other device-drivers that use vm_insert_mixed(). The
MAP_SYNC flag positively indicates that page cache is disabled for a
given mapping, although whether that property is due to "dax" or some
other kernel mechanics is purely an internal detail.
I'm not opposed to faking out VM_MIXEDMAP if this broken check has
made it into production, but again, it's unreliable.
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: David Rientjes @ 2018-10-02 20:29 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20181002112851.GP18290@dhcp22.suse.cz>
On Tue, 2 Oct 2018, Michal Hocko wrote:
> On Wed 26-09-18 08:06:24, Michal Hocko wrote:
> > On Tue 25-09-18 15:04:06, Andrew Morton wrote:
> > > On Tue, 25 Sep 2018 14:45:19 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:
> > >
> > > > > > It is also used in
> > > > > > automated testing to ensure that vmas get disabled for thp appropriately
> > > > > > and we used "nh" since that is how PR_SET_THP_DISABLE previously enforced
> > > > > > this, and those tests now break.
> > > > >
> > > > > This sounds like a bit of an abuse to me. It shows how an internal
> > > > > implementation detail leaks out to the userspace which is something we
> > > > > should try to avoid.
> > > > >
> > > >
> > > > Well, it's already how this has worked for years before commit
> > > > 1860033237d4 broke it. Changing the implementation in the kernel is fine
> > > > as long as you don't break userspace who relies on what is exported to it
> > > > and is the only way to determine if MADV_NOHUGEPAGE is preventing it from
> > > > being backed by hugepages.
> > >
> > > 1860033237d4 was over a year ago so perhaps we don't need to be
> > > too worried about restoring the old interface. In which case
> > > we have an opportunity to make improvements such as that suggested
> > > by Michal?
> >
> > Yeah, can we add a way to export PR_SET_THP_DISABLE to userspace
> > somehow? E.g. /proc/<pid>/status. It is a process wide thing so
> > reporting it per VMA sounds strange at best.
>
> So how about this? (not tested yet but it should be pretty
> straightforward)
Umm, prctl(PR_GET_THP_DISABLE)?
^ permalink raw reply
* Re: [RFC 00/20] ns: Introduce Time Namespace
From: Dmitry Safonov @ 2018-10-02 21:05 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrei Vagin, Eric W. Biederman, Dmitry Safonov, open list,
Adrian Reber, Andy Lutomirski, Christian Brauner, Cyrill Gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov,
Pavel Emelyanov, Shuah Khan, containers, crml, Linux API, X86 ML,
Alexey Dobriyan, linux-kselftest
In-Reply-To: <alpine.DEB.2.21.1810020811290.32062@nanos.tec.linutronix.de>
Hi Thomas, Andrei, Eric,
On Tue, 2 Oct 2018 at 07:15, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, 1 Oct 2018, Andrey Vagin wrote:
>
> > On Thu, Sep 27, 2018 at 11:41:49PM +0200, Thomas Gleixner wrote:
> > > On Thu, 27 Sep 2018, Thomas Gleixner wrote:
> > > > Add time skew via NTP/PTP into the picture and you might have to adjust
> > > > timers as well, because you need to guarantee that they are not expiring
> > > > early.
> > > >
> > > > I haven't looked through Dimitry's patches yet, but I don't see how this
> > > > can work at all without introducing subtle issues all over the place.
> > >
> > > And just a quick scan tells me that this is broken. Timers will expire
> > > early or late. The latter is acceptible to some extent, but larger delays
> > > might come with surprise. Expiring early is an absolute nono.
> >
> > Do you mean that we have to adjust all timers after changing offset for
> > CLOCK_MONOTONIC or CLOCK_BOOTTIME? Our idea is that offsets for
> > monotonic and boot times will be set immediately after creating a time
> > namespace before using any timers.
>
> I explained that in detail in this thread, but it's not about the initial
> setting of clock mono/boot before any timers have been armed.
>
> It's about setting the offset or clock realtime (via settimeofday) when
> timers are already armed. Also having a entirely different time domain,
> e.g. separate NTP adjustments, makes that necessary.
It looks like, there is a bit of misunderstanding each other:
Andrei was talking about the current RFC version, where we haven't
introduced offsets for clock realtime. While Thomas IIUC, is looking
how-to expand time namespace over realtime.
As CLOCK_REALTIME virtualization raises so many complex questions
like a different length of the second or list of realtime timers in ns we
haven't added any realization for it.
It seems like an initial introduction for timens can be expanded after to cover
realtime clocks too. While it may seem incomplete, it solves issues for
restoring/migration of real-world applications like nodejs, Oracle DB server
which fails after being restored if there is a leap in monotonic time.
While solving the mentioned issues, it doesn't bring overhead.
(well, Andy noted that cmp for zero-offsets on vdso can be optimized too,
which will be done in v1).
Thomas, thanks much for your input - now we know that we'll need to
introduce list for timers in namespace when we'll add realtime clocks.
Do you believe that CLOCK_MONOTONIC_SYNC would be an easier
concept than offsets per-namespace?
Thanks,
Dmitry
^ permalink raw reply
* Re: [RFC 00/20] ns: Introduce Time Namespace
From: Thomas Gleixner @ 2018-10-02 21:26 UTC (permalink / raw)
To: Dmitry Safonov
Cc: Andrei Vagin, Eric W. Biederman, Dmitry Safonov, open list,
Adrian Reber, Andy Lutomirski, Christian Brauner, Cyrill Gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov,
Pavel Emelyanov, Shuah Khan, containers, crml, Linux API, X86 ML,
Alexey Dobriyan, linux-kselftest
In-Reply-To: <CAJwJo6a+WWpAxVAUuYt8TrCF+1a=BdoFQjU9vto0iiUN5vygsA@mail.gmail.com>
Dmitry,
On Tue, 2 Oct 2018, Dmitry Safonov wrote:
> On Tue, 2 Oct 2018 at 07:15, Thomas Gleixner <tglx@linutronix.de> wrote:
> > I explained that in detail in this thread, but it's not about the initial
> > setting of clock mono/boot before any timers have been armed.
> >
> > It's about setting the offset or clock realtime (via settimeofday) when
> > timers are already armed. Also having a entirely different time domain,
> > e.g. separate NTP adjustments, makes that necessary.
>
> It looks like, there is a bit of misunderstanding each other:
> Andrei was talking about the current RFC version, where we haven't
> introduced offsets for clock realtime. While Thomas IIUC, is looking
> how-to expand time namespace over realtime.
>
> As CLOCK_REALTIME virtualization raises so many complex questions
> like a different length of the second or list of realtime timers in ns we
> haven't added any realization for it.
>
> It seems like an initial introduction for timens can be expanded after to cover
> realtime clocks too. While it may seem incomplete, it solves issues for
> restoring/migration of real-world applications like nodejs, Oracle DB server
> which fails after being restored if there is a leap in monotonic time.
Well, yes. But you really have to think about the full picture. Just adding
part of the overall solution right now, just because it can be glued into
the code easily, is not the best approach IMO as it might result in
substantial rework of the whole thing sooner than later. I really don't
want to end up with something which is not extensible and has to be
supported forever.
Just for the record, the current approach with name space offsets for
monotonic is also prone to malfunction vs. timers, unless you can prevent
changing the offset _after_ the namespace has been set up and timers have
been armed. I admit, that I did not look close enough to verify that.
> While solving the mentioned issues, it doesn't bring overhead.
> (well, Andy noted that cmp for zero-offsets on vdso can be optimized too,
> which will be done in v1).
>
> Thomas, thanks much for your input - now we know that we'll need to
> introduce list for timers in namespace when we'll add realtime clocks.
> Do you believe that CLOCK_MONOTONIC_SYNC would be an easier
> concept than offsets per-namespace?
Haven't thought it through. This was just an idea in reaction to Eric's
question whether setting clock monotonic might be feasible. But yes, it
might be worth to think about it.
I think you should really define the long term requirements for time
namespaces and perhaps set some limitations in functionality upfront.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH v4 19/27] x86/cet/shstk: Introduce WRUSS instruction
From: Eugene Syromiatnikov @ 2018-10-03 4:15 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-20-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:43AM -0700, Yu-cheng Yu wrote:
> WRUSS is a new kernel-mode instruction but writes directly
> to user shadow stack memory. This is used to construct
> a return address on the shadow stack for the signal
> handler.
>
> This instruction can fault if the user shadow stack is
> invalid shadow stack memory. In that case, the kernel does
> fixup.
"a fixup"
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/asm/special_insns.h | 32 ++++++++++++++++++++++++++++
> arch/x86/mm/fault.c | 9 ++++++++
> 2 files changed, 41 insertions(+)
>
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 317fc59b512c..c04e68ef47da 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -237,6 +237,38 @@ static inline void clwb(volatile void *__p)
> : [pax] "a" (p));
> }
>
> +#ifdef CONFIG_X86_INTEL_CET
> +#if defined(CONFIG_IA32_EMULATION) || defined(CONFIG_X86_X32)
> +static inline int write_user_shstk_32(unsigned long addr, unsigned int val)
> +{
> + asm_volatile_goto("1: wrussd %1, (%0)\n"
> + _ASM_EXTABLE(1b, %l[fail])
> + :: "r" (addr), "r" (val)
> + :: fail);
> + return 0;
> +fail:
> + return -1;
Should it...
> +}
> +#else
> +static inline int write_user_shstk_32(unsigned long addr, unsigned int val)
> +{
> + WARN_ONCE(1, "write_user_shstk_32 used but not supported.\n");
"is/was used"
> + return -EFAULT;
> +}
> +#endif
> +
> +static inline int write_user_shstk_64(unsigned long addr, unsigned long val)
> +{
> + asm_volatile_goto("1: wrussq %1, (%0)\n"
> + _ASM_EXTABLE(1b, %l[fail])
> + :: "r" (addr), "r" (val)
> + :: fail);
> + return 0;
> +fail:
> + return -1;
...and it be -EPERM, if -EFAULT was returned earlier for write_user_shstk_32?
> +}
> +#endif /* CONFIG_X86_INTEL_CET */
> +
> #define nop() asm volatile ("nop")
>
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 7c3877a982f4..4d4ac57a4ba2 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1305,6 +1305,15 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
> error_code |= X86_PF_USER;
> flags |= FAULT_FLAG_USER;
> } else {
> + /*
> + * WRUSS is a kernel instrcution and but writes
"WRUSS is a kernel instruction but writes"
> + * to user shadow stack. When a fault occurs,
> + * both X86_PF_USER and X86_PF_SHSTK are set.
> + * Clear X86_PF_USER here.
> + */
> + if ((error_code & (X86_PF_USER | X86_PF_SHSTK)) ==
> + (X86_PF_USER | X86_PF_SHSTK))
> + error_code &= ~X86_PF_USER;
> if (regs->flags & X86_EFLAGS_IF)
> local_irq_enable();
> }
> --
> 2.17.1
>
^ permalink raw reply
* Re: Setting monotonic time?
From: Eric W. Biederman @ 2018-10-03 4:50 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Arnd Bergmann, avagin, dima, Linux Kernel Mailing List,
0x7f454c46, adrian, Andy Lutomirski, Christian Brauner, gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov, xemul,
Shuah Khan, containers, criu, Linux API, the arch/x86 maintainers,
Alexey Dobriyan, linux-kselftest
In-Reply-To: <alpine.DEB.2.21.1810022205080.1435@nanos.tec.linutronix.de>
Thomas Gleixner <tglx@linutronix.de> writes:
> On Tue, 2 Oct 2018, Arnd Bergmann wrote:
>> On Mon, Oct 1, 2018 at 8:53 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> >
>> > On Mon, 1 Oct 2018, Eric W. Biederman wrote:
>> > > In the context of process migration there is a simpler subproblem that I
>> > > think it is worth exploring if we can do something about.
>> > >
>> > > For a cluster of machines all running with synchronized
>> > > clocks. CLOCK_REALTIME matches. CLOCK_MONOTNIC does not match between
>> > > machines. Not having a matching CLOCK_MONOTONIC prevents successful
>> > > process migration between nodes in that cluster.
>> > >
>> > > Would it be possible to allow setting CLOCK_MONOTONIC at the very
>> > > beginning of time? So that all of the nodes in a cluster can be in
>> > > sync?
>> > >
>> > > No change in skew just in offset for CLOCK_MONOTONIC.
>> > >
>> > > There are also dragons involved in coordinating things so that
>> > > CLOCK_MONOTONIC gets set before CLOCK_MONOTONIC gets used. So I don't
>> > > know if allowing CLOCK_MONOTONIC to be set would be practical but it
>> > > seems work exploring all on it's own.
>> >
>> > It's used very early on in the kernel, so that would be a major surprise
>> > for many things including user space which has expectations on clock
>> > monotonic.
>> >
>> > It would be reasonably easy to add CLOCK_MONONOTIC_SYNC which can be set in
>> > the way you described and then in name spaces make it possible to magically
>> > map CLOCK_MONOTONIC to CLOCK_MONOTONIC_SYNC.
>> >
>> > It still wouldn't allow to have different NTP/PTP time domains, but might
>> > be a good start to address the main migration headaches.
>>
>> If we make CLOCK_MONOTONIC settable this way in a namespace,
>> do you think that should include device drivers that report timestamps
>> in CLOCK_MONOTONIC base, or only the timekeeping clock and timer
>> interfaces?
>
> Uurgh. That gets messy very fast.
>
>> Examples for drivers that can report timestamps are input, sound, v4l,
>> and drm. I think most of these can report stamps in either monotonic
>> or realtime base, while socket timestamps notably are always in
>> realtime.
>>
>> We can probably get away with not setting the timebase for those
>> device drivers as long as the checkpoint/restart and migration features
>> are not expected to restore the state of an open character device
>> in that way. I don't know if that is a reasonable assumption to make
>> for the examples I listed.
>
> No idea. I'm not a container migration wizard.
Direct access to hardware/drivers and not through an abstraction like
the vfs (an abstraction over block devices) can legitimately be handled
by hotplug events. I unplug one keyboard I plug in another.
I don't know if the input layer is more of a general abstraction
or more of a hardware device. I have not dug into it but my guess
is abstraction from what I have heard.
The scary difficulty here is if after restart input is reporting times
in CLOCK_MONOTONIC and the applications in the namespace are talking
about times in CLOCK_MONOTONIC_SYNC. Then there is an issue. As even
with a fixed offset the times don't match up.
So a time namespace absolutely needs to do is figure out how to deal
with all of the kernel interfaces reporting times and figure out how to
report them in the current time namespace.
Eric
^ permalink raw reply
* Re: [RFC PATCH v4 24/27] mm/mmap: Create a guard area between VMAs
From: Eugene Syromiatnikov @ 2018-10-03 4:56 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-25-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
> Create a guard area between VMAs, to detect memory corruption.
Do I understand correctly that with this patch a user space program
no longer be able to place two mappings back to back? If it is so,
it will likely break a lot of things; for example, it's a common ring
buffer implementations technique, to map buffer memory twice back
to back in order to avoid special handling of items wrapping its end.
^ permalink raw reply
* Re: Setting monotonic time?
From: Thomas Gleixner @ 2018-10-03 5:25 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Arnd Bergmann, avagin, dima, Linux Kernel Mailing List,
0x7f454c46, adrian, Andy Lutomirski, Christian Brauner, gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov, xemul,
Shuah Khan, containers, criu, Linux API, the arch/x86 maintainers,
Alexey Dobriyan, linux-kselftest
In-Reply-To: <87in2jskew.fsf@xmission.com>
On Wed, 3 Oct 2018, Eric W. Biederman wrote:
> Direct access to hardware/drivers and not through an abstraction like
> the vfs (an abstraction over block devices) can legitimately be handled
> by hotplug events. I unplug one keyboard I plug in another.
>
> I don't know if the input layer is more of a general abstraction
> or more of a hardware device. I have not dug into it but my guess
> is abstraction from what I have heard.
>
> The scary difficulty here is if after restart input is reporting times
> in CLOCK_MONOTONIC and the applications in the namespace are talking
> about times in CLOCK_MONOTONIC_SYNC. Then there is an issue. As even
> with a fixed offset the times don't match up.
>
> So a time namespace absolutely needs to do is figure out how to deal
> with all of the kernel interfaces reporting times and figure out how to
> report them in the current time namespace.
So you want to talk to Arnd who is leading the y2038 effort. He knowns how
many and which interfaces are involved aside of the obvious core timer
ones. It's quite an amount and the problem is that you really need to do
that at the interface level, because many of those time stamps are taken in
contexts which are completely oblivious of name spaces. Ditto for timeouts
and similar things which are handed in through these interfaces.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH v4 24/27] mm/mmap: Create a guard area between VMAs
From: Andy Lutomirski @ 2018-10-03 5:36 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: Yu-cheng Yu, X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
LKML, linux-doc, Linux-MM, linux-arch, Linux API, Arnd Bergmann,
Balbir Singh, Cyrill Gorcunov, Dave Hansen, Florian Weimer,
H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit
In-Reply-To: <20181003045611.GB22724@asgard.redhat.com>
On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov <esyr@redhat.com> wrote:
>
> On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
> > Create a guard area between VMAs, to detect memory corruption.
>
> Do I understand correctly that with this patch a user space program
> no longer be able to place two mappings back to back? If it is so,
> it will likely break a lot of things; for example, it's a common ring
> buffer implementations technique, to map buffer memory twice back
> to back in order to avoid special handling of items wrapping its end.
I haven't checked what the patch actually does, but it shouldn't have
any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
--Andy
^ permalink raw reply
* Re: [RFC v2 v2 1/1] ns: add binfmt_misc to the mount namespace
From: Eric W. Biederman @ 2018-10-03 6:07 UTC (permalink / raw)
To: Laurent Vivier
Cc: linux-kernel, Dmitry Safonov, Andrei Vagin, Alexander Viro,
James Bottomley, containers, linux-fsdevel, linux-api
In-Reply-To: <20181002102054.13245-2-laurent@vivier.eu>
Laurent Vivier <laurent@vivier.eu> writes:
> This patch allows to have a different binftm_misc configuration
> in each container we mount binfmt_misc filesystem with mount namespace
> enabled.
>
> A container started without the CLONE_NEWNS will use the host binfmt_misc
> configuration, otherwise the container starts with an empty binfmt_misc
> interpreters list.
>
> For instance, using "unshare" we can start a chroot of an another
> architecture and configure the binfmt_misc interpreted without being root
> to run the binaries in this chroot.
A couple of things.
As has already been mentioned on your previous version anything that
comes through the filesystem interface needs to lookup up the local
binfmt context not through current but through file->f_dentry->d_sb.
AKA the superblock of the mounted filesystem.
As you have this coded any time a mount namespace is unshared you get a
new binfmt context. That has a very reasonable chance of breaking
existing userspace.
A mount of binfmt_misc today from within a user namespace is not allowed
which is why I have figured that will be a nice place to trigger
creating a new binfmt context.
It is fundamentally necessary to be able to get a pointer to the binfmt
context from current. Either stored in an existing namespace or
stored in nsproxy. Anything else will risk breaking backwards
compatibility with existing user space for no good reason.
What is fundamentally being changed is the behavior of exec.
Changing the behavior of exec needs to be carefully contained or we risk
confusing privileged applications.
I believe your last email to James Bottomley detailed a very strong use
case for this functionality.
As the key gains over the existing kernel is unprivileged use. As it is
the behavior of exec that is changing. You definitely need a user
namespace involved.
So I think the simplest would be to hang the binfmt context off of a
user namespace. But I am open to other ideas.
My primary concern is that we keep the cognitive and the maintenance
burden as small as is reasonably possible so that the costs don't out
weigh the benefit.
Eric
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> fs/binfmt_misc.c | 50 +++++++++++++++++++++++++-----------------------
> fs/mount.h | 8 ++++++++
> fs/namespace.c | 6 ++++++
> 3 files changed, 40 insertions(+), 24 deletions(-)
>
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index aa4a7a23ff99..ecb14776c759 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -25,6 +25,7 @@
> #include <linux/syscalls.h>
> #include <linux/fs.h>
> #include <linux/uaccess.h>
> +#include <mount.h>
>
> #include "internal.h"
>
> @@ -38,9 +39,6 @@ enum {
> VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
> };
>
> -static LIST_HEAD(entries);
> -static int enabled = 1;
> -
> enum {Enabled, Magic};
> #define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
> #define MISC_FMT_OPEN_BINARY (1 << 30)
> @@ -60,10 +58,7 @@ typedef struct {
> struct file *interp_file;
> } Node;
>
> -static DEFINE_RWLOCK(entries_lock);
> static struct file_system_type bm_fs_type;
> -static struct vfsmount *bm_mnt;
> -static int entry_count;
>
> /*
> * Max length of the register string. Determined by:
> @@ -91,7 +86,7 @@ static Node *check_file(struct linux_binprm *bprm)
> struct list_head *l;
>
> /* Walk all the registered handlers. */
> - list_for_each(l, &entries) {
> + list_for_each(l, &binfmt_ns(entries)) {
> Node *e = list_entry(l, Node, list);
> char *s;
> int j;
> @@ -135,15 +130,15 @@ static int load_misc_binary(struct linux_binprm *bprm)
> int fd_binary = -1;
>
> retval = -ENOEXEC;
> - if (!enabled)
> + if (!binfmt_ns(enabled))
> return retval;
>
> /* to keep locking time low, we copy the interpreter string */
> - read_lock(&entries_lock);
> + read_lock(&binfmt_ns(entries_lock));
> fmt = check_file(bprm);
> if (fmt)
> dget(fmt->dentry);
> - read_unlock(&entries_lock);
> + read_unlock(&binfmt_ns(entries_lock));
> if (!fmt)
> return retval;
>
> @@ -613,15 +608,15 @@ static void kill_node(Node *e)
> {
> struct dentry *dentry;
>
> - write_lock(&entries_lock);
> + write_lock(&binfmt_ns(entries_lock));
> list_del_init(&e->list);
> - write_unlock(&entries_lock);
> + write_unlock(&binfmt_ns(entries_lock));
>
> dentry = e->dentry;
> drop_nlink(d_inode(dentry));
> d_drop(dentry);
> dput(dentry);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&binfmt_ns(bm_mnt), &binfmt_ns(entry_count));
> }
>
> /* /<entry> */
> @@ -716,7 +711,8 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> if (!inode)
> goto out2;
>
> - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
> + err = simple_pin_fs(&bm_fs_type, &binfmt_ns(bm_mnt),
> + &binfmt_ns(entry_count));
> if (err) {
> iput(inode);
> inode = NULL;
> @@ -730,7 +726,8 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> if (IS_ERR(f)) {
> err = PTR_ERR(f);
> pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&binfmt_ns(bm_mnt),
> + &binfmt_ns(entry_count));
> iput(inode);
> inode = NULL;
> goto out2;
> @@ -743,9 +740,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> inode->i_fop = &bm_entry_operations;
>
> d_instantiate(dentry, inode);
> - write_lock(&entries_lock);
> - list_add(&e->list, &entries);
> - write_unlock(&entries_lock);
> + write_lock(&binfmt_ns(entries_lock));
> + list_add(&e->list, &binfmt_ns(entries));
> + write_unlock(&binfmt_ns(entries_lock));
>
> err = 0;
> out2:
> @@ -770,7 +767,7 @@ static const struct file_operations bm_register_operations = {
> static ssize_t
> bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> {
> - char *s = enabled ? "enabled\n" : "disabled\n";
> + char *s = binfmt_ns(enabled) ? "enabled\n" : "disabled\n";
>
> return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
> }
> @@ -784,19 +781,20 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,
> switch (res) {
> case 1:
> /* Disable all handlers. */
> - enabled = 0;
> + binfmt_ns(enabled) = 0;
> break;
> case 2:
> /* Enable all handlers. */
> - enabled = 1;
> + binfmt_ns(enabled) = 1;
> break;
> case 3:
> /* Delete all handlers. */
> root = file_inode(file)->i_sb->s_root;
> inode_lock(d_inode(root));
>
> - while (!list_empty(&entries))
> - kill_node(list_first_entry(&entries, Node, list));
> + while (!list_empty(&binfmt_ns(entries)))
> + kill_node(list_first_entry(&binfmt_ns(entries),
> + Node, list));
>
> inode_unlock(d_inode(root));
> break;
> @@ -838,7 +836,10 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
> static struct dentry *bm_mount(struct file_system_type *fs_type,
> int flags, const char *dev_name, void *data)
> {
> - return mount_single(fs_type, flags, data, bm_fill_super);
> + struct mnt_namespace *mnt_ns = current->nsproxy->mnt_ns;
> +
> + return mount_ns(fs_type, flags, data, mnt_ns, mnt_ns->user_ns,
> + bm_fill_super);
> }
>
> static struct linux_binfmt misc_format = {
> @@ -849,6 +850,7 @@ static struct linux_binfmt misc_format = {
> static struct file_system_type bm_fs_type = {
> .owner = THIS_MODULE,
> .name = "binfmt_misc",
> + .fs_flags = FS_USERNS_MOUNT,
> .mount = bm_mount,
> .kill_sb = kill_litter_super,
> };
> diff --git a/fs/mount.h b/fs/mount.h
> index f39bc9da4d73..f03b35141440 100644
> --- a/fs/mount.h
> +++ b/fs/mount.h
> @@ -17,6 +17,12 @@ struct mnt_namespace {
> u64 event;
> unsigned int mounts; /* # of mounts in the namespace */
> unsigned int pending_mounts;
> + /* binfmt misc */
> + struct list_head entries;
> + rwlock_t entries_lock;
> + int enabled;
> + struct vfsmount *bm_mnt;
> + int entry_count;
> } __randomize_layout;
>
> struct mnt_pcp {
> @@ -72,6 +78,8 @@ struct mount {
> struct dentry *mnt_ex_mountpoint;
> } __randomize_layout;
>
> +#define binfmt_ns(a) (current->nsproxy->mnt_ns->a)
> +
> #define MNT_NS_INTERNAL ERR_PTR(-EINVAL) /* distinct from any mnt_namespace */
>
> static inline struct mount *real_mount(struct vfsmount *mnt)
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 99186556f8d3..f92b8371228d 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2850,6 +2850,12 @@ static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns)
> new_ns->ucounts = ucounts;
> new_ns->mounts = 0;
> new_ns->pending_mounts = 0;
> + /* binfmt_misc */
> + INIT_LIST_HEAD(&new_ns->entries);
> + new_ns->enabled = 1;
> + rwlock_init(&new_ns->entries_lock);
> + new_ns->bm_mnt = NULL;
> + new_ns->entry_count = 0;
> return new_ns;
> }
^ permalink raw reply
* Re: Setting monotonic time?
From: Eric W. Biederman @ 2018-10-03 6:14 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Arnd Bergmann, avagin, dima, Linux Kernel Mailing List,
0x7f454c46, adrian, Andy Lutomirski, Christian Brauner, gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov, xemul,
Shuah Khan, containers, criu, Linux API, the arch/x86 maintainers,
Alexey Dobriyan, linux-kselftest
In-Reply-To: <alpine.DEB.2.21.1810030711520.1435@nanos.tec.linutronix.de>
Thomas Gleixner <tglx@linutronix.de> writes:
> On Wed, 3 Oct 2018, Eric W. Biederman wrote:
>> Direct access to hardware/drivers and not through an abstraction like
>> the vfs (an abstraction over block devices) can legitimately be handled
>> by hotplug events. I unplug one keyboard I plug in another.
>>
>> I don't know if the input layer is more of a general abstraction
>> or more of a hardware device. I have not dug into it but my guess
>> is abstraction from what I have heard.
>>
>> The scary difficulty here is if after restart input is reporting times
>> in CLOCK_MONOTONIC and the applications in the namespace are talking
>> about times in CLOCK_MONOTONIC_SYNC. Then there is an issue. As even
>> with a fixed offset the times don't match up.
>>
>> So a time namespace absolutely needs to do is figure out how to deal
>> with all of the kernel interfaces reporting times and figure out how to
>> report them in the current time namespace.
>
> So you want to talk to Arnd who is leading the y2038 effort. He knowns how
> many and which interfaces are involved aside of the obvious core timer
> ones. It's quite an amount and the problem is that you really need to do
> that at the interface level, because many of those time stamps are taken in
> contexts which are completely oblivious of name spaces. Ditto for timeouts
> and similar things which are handed in through these interfaces.
Yep. That sounds right.
Eric
^ permalink raw reply
* Re: Setting monotonic time?
From: Thomas Gleixner @ 2018-10-03 6:14 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Arnd Bergmann, avagin, dima, Linux Kernel Mailing List,
0x7f454c46, adrian, Andy Lutomirski, Christian Brauner, gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov, xemul,
Shuah Khan, containers, criu, Linux API, the arch/x86 maintainers,
Alexey Dobriyan, linux-kselftest
In-Reply-To: <alpine.DEB.2.21.1810030711520.1435@nanos.tec.linutronix.de>
On Wed, 3 Oct 2018, Thomas Gleixner wrote:
> On Wed, 3 Oct 2018, Eric W. Biederman wrote:
> > Direct access to hardware/drivers and not through an abstraction like
> > the vfs (an abstraction over block devices) can legitimately be handled
> > by hotplug events. I unplug one keyboard I plug in another.
> >
> > I don't know if the input layer is more of a general abstraction
> > or more of a hardware device. I have not dug into it but my guess
> > is abstraction from what I have heard.
> >
> > The scary difficulty here is if after restart input is reporting times
> > in CLOCK_MONOTONIC and the applications in the namespace are talking
> > about times in CLOCK_MONOTONIC_SYNC. Then there is an issue. As even
> > with a fixed offset the times don't match up.
> >
> > So a time namespace absolutely needs to do is figure out how to deal
> > with all of the kernel interfaces reporting times and figure out how to
> > report them in the current time namespace.
>
> So you want to talk to Arnd who is leading the y2038 effort. He knowns how
> many and which interfaces are involved aside of the obvious core timer
> ones. It's quite an amount and the problem is that you really need to do
> that at the interface level, because many of those time stamps are taken in
> contexts which are completely oblivious of name spaces. Ditto for timeouts
> and similar things which are handed in through these interfaces.
Plus you have to make sure, that any new interface will have that
treatment. For y2038 that's easy as we just require to use timespec64 for
new ones. For your problem that's not so trivial.
Thanks,
tglx
^ permalink raw reply
* Re: Setting monotonic time?
From: Arnd Bergmann @ 2018-10-03 7:02 UTC (permalink / raw)
To: Eric W . Biederman
Cc: Thomas Gleixner, avagin, dima, Linux Kernel Mailing List,
0x7f454c46, adrian, Andy Lutomirski, Christian Brauner, gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov, xemul,
Shuah Khan, containers, criu, Linux API, the arch/x86 maintainers,
Alexey Dobriyan, linux-kselftest
In-Reply-To: <87in2jo8u6.fsf@xmission.com>
On Wed, Oct 3, 2018 at 8:14 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Thomas Gleixner <tglx@linutronix.de> writes:
>
> > On Wed, 3 Oct 2018, Eric W. Biederman wrote:
> >> Direct access to hardware/drivers and not through an abstraction like
> >> the vfs (an abstraction over block devices) can legitimately be handled
> >> by hotplug events. I unplug one keyboard I plug in another.
> >>
> >> I don't know if the input layer is more of a general abstraction
> >> or more of a hardware device. I have not dug into it but my guess
> >> is abstraction from what I have heard.
> >>
> >> The scary difficulty here is if after restart input is reporting times
> >> in CLOCK_MONOTONIC and the applications in the namespace are talking
> >> about times in CLOCK_MONOTONIC_SYNC. Then there is an issue. As even
> >> with a fixed offset the times don't match up.
> >>
> >> So a time namespace absolutely needs to do is figure out how to deal
> >> with all of the kernel interfaces reporting times and figure out how to
> >> report them in the current time namespace.
> >
> > So you want to talk to Arnd who is leading the y2038 effort. He knowns how
> > many and which interfaces are involved aside of the obvious core timer
> > ones. It's quite an amount and the problem is that you really need to do
> > that at the interface level, because many of those time stamps are taken in
> > contexts which are completely oblivious of name spaces. Ditto for timeouts
> > and similar things which are handed in through these interfaces.
>
> Yep. That sounds right.
Let's stay with the input event example for the moment: Here, we have a
character device, and a user calls read() to retrieve one or more records
of type 'struct input_event' using the evdev_read() function. The original
timestamp gets put there using this logic:
ktime_t time;
struct timespec64 ts;
time = client->clk_type == EV_CLK_REAL ?
ktime_get_real() :
client->clk_type == EV_CLK_MONO ?
ktime_get() :
ktime_get_boottime();
ts = ktime_to_timespec64(time);
ev.input_event_sec = ts.tv_sec;
ev.input_event_usec = ts.tv_nsec / NSEC_PER_USEC;
clk_type can get set using an ioctl() to real, monotonic or
boottime. We have to stop using EV_CLK_REAL in the
future because that breaks in y2038, but I guess EV_CLK_MONO
and EV_CLK_BOOK should stay.
If we want this to work correctly in a namespace that has a
user defined CLOCK_MONOTONIC timebase, one way to
do it might be to always call ktime_get() when we record
the timestamp in the kernel-internal CLOCK_MONOTONIC
base, but then convert it to the correct base when copying to
user space.
Note that AFAIU practically all users of evdev do /not/ actually
care about the time base, they only care about the elapsed
time between intervals, e.g. to track how fast a pointer should
move based on input from a trackpad. I don't see any reason
why one would compare this timestamp to a clock_gettime()
value, but of course at the moment this has well-defined
behavior that would break if we change clock_gettime(), and
we have a process in the namespace that opens
/dev/input/eventX and relies on meaningful timestamps
relative to a particular base.
Arnd
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-10-03 7:36 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1810021329260.87409@chino.kir.corp.google.com>
On Tue 02-10-18 13:29:42, David Rientjes wrote:
> On Tue, 2 Oct 2018, Michal Hocko wrote:
>
> > On Wed 26-09-18 08:06:24, Michal Hocko wrote:
> > > On Tue 25-09-18 15:04:06, Andrew Morton wrote:
> > > > On Tue, 25 Sep 2018 14:45:19 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:
> > > >
> > > > > > > It is also used in
> > > > > > > automated testing to ensure that vmas get disabled for thp appropriately
> > > > > > > and we used "nh" since that is how PR_SET_THP_DISABLE previously enforced
> > > > > > > this, and those tests now break.
> > > > > >
> > > > > > This sounds like a bit of an abuse to me. It shows how an internal
> > > > > > implementation detail leaks out to the userspace which is something we
> > > > > > should try to avoid.
> > > > > >
> > > > >
> > > > > Well, it's already how this has worked for years before commit
> > > > > 1860033237d4 broke it. Changing the implementation in the kernel is fine
> > > > > as long as you don't break userspace who relies on what is exported to it
> > > > > and is the only way to determine if MADV_NOHUGEPAGE is preventing it from
> > > > > being backed by hugepages.
> > > >
> > > > 1860033237d4 was over a year ago so perhaps we don't need to be
> > > > too worried about restoring the old interface. In which case
> > > > we have an opportunity to make improvements such as that suggested
> > > > by Michal?
> > >
> > > Yeah, can we add a way to export PR_SET_THP_DISABLE to userspace
> > > somehow? E.g. /proc/<pid>/status. It is a process wide thing so
> > > reporting it per VMA sounds strange at best.
> >
> > So how about this? (not tested yet but it should be pretty
> > straightforward)
>
> Umm, prctl(PR_GET_THP_DISABLE)?
/me confused. I thought you want to query for the flag on a
_different_ process.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC v2 v2 0/1] ns: introduce binfmt_misc namespace
From: James Bottomley @ 2018-10-03 10:13 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel
Cc: Andrei Vagin, Dmitry Safonov, linux-api, containers,
Eric Biederman, linux-fsdevel, Alexander Viro
In-Reply-To: <a9997ce3-d63d-44b4-6a2a-09fd207a1550@vivier.eu>
On Tue, 2018-10-02 at 18:47 +0200, Laurent Vivier wrote:
> Le 02/10/2018 à 18:13, James Bottomley a écrit :
> > On Tue, 2018-10-02 at 12:20 +0200, Laurent Vivier wrote:
> > > v2: no new namespace, binfmt_misc data are now part of
> > > the mount namespace
> > > I put this in mount namespace instead of user namespace
> > > because the mount namespace is already needed and
> > > I don't want to force to have the user namespace for that.
> > > As this is a filesystem, it seems logic to have it here.
> > >
> > > This allows to define a new interpreter for each new container.
> > >
> > > But the main goal is to be able to chroot to a directory
> > > using a binfmt_misc interpreter without being root.
> >
> > Reading all this, I don't quite understand why this works for me
> > and
> > not for you (I think I get from your explanation that it doesn't
> > work
> > for you, but I might have missed something):
> >
> > jejb@jarvis:~> uname -m
> > x86_64
> > jejb@jarvis:~> unshare -r -m
> > root@jarvis:~# chroot /home/jejb/containers/aarch64
> > jarvis:/ # uname -m
> > aarch64
> >
> > Of course to get that to work I have an 'F' entry in
> > /etc/binfmt.d/qemu-aarch64.conf
> >
>
> I'd like to configure the interpreter without being root.
>
> As a simple user can run a VM and a full system inside, I'd like to
> be
> able to start a container/chroot without having to configure
> something
> at the host level.
>
> For instance, I'd like to provide to "someone" (with no admin rights)
> a tar file with inside an OS environment for a given target and the
> interpreter, and allow him to run the binaries inside just by running
> a simple command (like qemu-system-XXX -hda my.img)
OK, since trying to persuade the distros to add the 'F' flag has been
challenging, I certainly buy this use case.
There is a security risk to allowing an unprivileged user to supply an
arbitrary interpreter (suid and sgid binaries), but as long as
whatever's agreed requires root in the user namespace, I'm happy we
have the security issue confined.
James
> It's also interesting for a test purpose: I can test concurrently
> different interpreters for the same target without modifying the
> target root filesystem (with the 'F' flag but on a per directory
> basis) or the host configuration.
>
> Another case is we can't configure qemu-mips/qemu-mipsel (old kernel
> API) and qemu-mipsn32/qemu-mipsne32el (new kernel API) interpreters
> on the same system because they share the same ELF signature (to be
> honest qemu should have only one binary for the old and the new
> interface and dynamically change it according to the ELF binary that
> is loaded, as it is done for ARM).
>
> But if no one thinks it's useful, I don't want to push this more than
> that...
>
> Thanks,
> Laurent
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [RFC PATCH v4 06/27] x86/cet: Control protection exception handler
From: Eugene Syromiatnikov @ 2018-10-03 10:39 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-7-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:30AM -0700, Yu-cheng Yu wrote:
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index e6db475164ed..873765adc244 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -578,6 +578,64 @@ do_general_protection(struct pt_regs *regs, long error_code)
> }
> NOKPROBE_SYMBOL(do_general_protection);
>
> +static const char *control_protection_err[] =
> +{
> + "unknown",
> + "near-ret",
> + "far-ret/iret",
> + "endbranch",
> + "rstorssp",
> + "setssbsy",
> +};
> +
> +/*
> + * When a control protection exception occurs, send a signal
> + * to the responsible application. Currently, control
> + * protection is only enabled for the user mode. This
> + * exception should not come from the kernel mode.
> + */
> +dotraplinkage void
> +do_control_protection(struct pt_regs *regs, long error_code)
> +{
> + struct task_struct *tsk;
> +
> + RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> + if (notify_die(DIE_TRAP, "control protection fault", regs,
> + error_code, X86_TRAP_CP, SIGSEGV) == NOTIFY_STOP)
> + return;
> + cond_local_irq_enable(regs);
> +
> + if (!user_mode(regs))
> + die("kernel control protection fault", regs, error_code);
> +
> + if (!static_cpu_has(X86_FEATURE_SHSTK) &&
> + !static_cpu_has(X86_FEATURE_IBT))
> + WARN_ONCE(1, "CET is disabled but got control "
> + "protection fault\n");
> +
> + tsk = current;
> + tsk->thread.error_code = error_code;
> + tsk->thread.trap_nr = X86_TRAP_CP;
> +
> + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
> + printk_ratelimit()) {
> + unsigned int max_err;
> +
> + max_err = ARRAY_SIZE(control_protection_err) - 1;
> + if ((error_code < 0) || (error_code > max_err))
> + error_code = 0;
> + pr_info("%s[%d] control protection ip:%lx sp:%lx error:%lx(%s)",
> + tsk->comm, task_pid_nr(tsk),
> + regs->ip, regs->sp, error_code,
> + control_protection_err[error_code]);
> + print_vma_addr(KERN_CONT " in ", regs->ip);
> + pr_cont("\n");
> + }
> +
> + force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
That way, no information is provided to userspace (both application and
debugger), which is rather unfortunate. It would be nice if a new SEGV_*
code was added at least, and CET error (with error code constant provided
in UAPI) is passed via si_errno. (Having ip/sp/*ssp would be even
better, but I'm not exactly sure about ramifications of providing this
kind of information to user space).
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Jan Kara @ 2018-10-03 12:50 UTC (permalink / raw)
To: Dan Williams
Cc: linux-xfs, linux-nvdimm, Linux API, Christoph Hellwig, Linux MM,
linux-fsdevel, Jan Kara, linux-ext4
In-Reply-To: <CAPcyv4j0tTD+rENqFExA68aw=-MmtCBaOe1qJovyrmJC=yBg-Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue 02-10-18 13:18:54, Dan Williams wrote:
> On Tue, Oct 2, 2018 at 8:32 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> >
> > On Tue 02-10-18 07:52:06, Christoph Hellwig wrote:
> > > On Tue, Oct 02, 2018 at 04:44:13PM +0200, Johannes Thumshirn wrote:
> > > > On Tue, Oct 02, 2018 at 07:37:13AM -0700, Christoph Hellwig wrote:
> > > > > No, it should not. DAX is an implementation detail thay may change
> > > > > or go away at any time.
> > > >
> > > > Well we had an issue with an application checking for dax, this is how
> > > > we landed here in the first place.
> > >
> > > So what exacty is that "DAX" they are querying about (and no, I'm not
> > > joking, nor being philosophical).
> >
> > I believe the application we are speaking about is mostly concerned about
> > the memory overhead of the page cache. Think of a machine that has ~ 1TB of
> > DRAM, the database running on it is about that size as well and they want
> > database state stored somewhere persistently - which they may want to do by
> > modifying mmaped database files if they do small updates... So they really
> > want to be able to use close to all DRAM for the DB and not leave slack
> > space for the kernel page cache to cache 1TB of database files.
>
> VM_MIXEDMAP was never a reliable indication of DAX because it could be
> set for random other device-drivers that use vm_insert_mixed(). The
> MAP_SYNC flag positively indicates that page cache is disabled for a
> given mapping, although whether that property is due to "dax" or some
> other kernel mechanics is purely an internal detail.
>
> I'm not opposed to faking out VM_MIXEDMAP if this broken check has
> made it into production, but again, it's unreliable.
So luckily this particular application wasn't widely deployed yet so we
will likely get away with the vendor asking customers to update to a
version not looking into smaps and parsing /proc/mounts instead.
But I don't find parsing /proc/mounts that beautiful either and I'd prefer
if we had a better interface for applications to query whether they can
avoid page cache for mmaps or not.
Honza
--
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR
^ permalink raw reply
* Re: [RFC PATCH v4 10/27] drm/i915/gvt: Update _PAGE_DIRTY to _PAGE_DIRTY_BITS
From: Eugene Syromiatnikov @ 2018-10-03 13:19 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-11-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:34AM -0700, Yu-cheng Yu wrote:
> Update _PAGE_DIRTY to _PAGE_DIRTY_BITS in split_2MB_gtt_entry().
>
> In order to support Control Flow Enforcement (CET), _PAGE_DIRTY
> is now _PAGE_DIRTY_HW or _PAGE_DIRTY_SW.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 00aad8164dec..2d6ba1462dd8 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -1170,7 +1170,7 @@ static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> }
>
> /* Clear dirty field. */
> - se->val64 &= ~_PAGE_DIRTY;
> + se->val64 &= ~_PAGE_DIRTY_BITS;
_PAGE_DIRTY_BITS is defined only in "[RFC PATCH v4 11/27] x86/mm:
Introduce _PAGE_DIRTY_SW",
^ permalink raw reply
* Re: [RFC PATCH v4 09/27] x86/mm: Change _PAGE_DIRTY to _PAGE_DIRTY_HW
From: Matthew Wilcox @ 2018-10-03 13:38 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-10-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:33AM -0700, Yu-cheng Yu wrote:
> We are going to create _PAGE_DIRTY_SW for non-hardware, memory
> management purposes. Rename _PAGE_DIRTY to _PAGE_DIRTY_HW and
> _PAGE_BIT_DIRTY to _PAGE_BIT_DIRTY_HW to make these PTE dirty
> bits more clear. There are no functional changes in this
> patch.
I would like there to be some documentation in this patchset which
explains the difference between PAGE_SOFT_DIRTY and PAGE_DIRTY_SW.
Also, is it really necessary to rename PAGE_DIRTY? It feels like a
lot of churn.
^ permalink raw reply
* Re: [RFC PATCH v4 09/27] x86/mm: Change _PAGE_DIRTY to _PAGE_DIRTY_HW
From: Dave Hansen @ 2018-10-03 14:05 UTC (permalink / raw)
To: Matthew Wilcox, Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Florian Weimer,
H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit, Oleg Nesterov, Pavel Machek, Peter Zijlstra,
Randy Dunlap <rdu>
In-Reply-To: <20181003133856.GA24782@bombadil.infradead.org>
On 10/03/2018 06:38 AM, Matthew Wilcox wrote:
> On Fri, Sep 21, 2018 at 08:03:33AM -0700, Yu-cheng Yu wrote:
>> We are going to create _PAGE_DIRTY_SW for non-hardware, memory
>> management purposes. Rename _PAGE_DIRTY to _PAGE_DIRTY_HW and
>> _PAGE_BIT_DIRTY to _PAGE_BIT_DIRTY_HW to make these PTE dirty
>> bits more clear. There are no functional changes in this
>> patch.
> I would like there to be some documentation in this patchset which
> explains the difference between PAGE_SOFT_DIRTY and PAGE_DIRTY_SW.
>
> Also, is it really necessary to rename PAGE_DIRTY? It feels like a
> lot of churn.
This is a lot of churn? Are we looking a the same patch? :)
arch/x86/include/asm/pgtable.h | 6 +++---
arch/x86/include/asm/pgtable_types.h | 17 +++++++++--------
arch/x86/kernel/relocate_kernel_64.S | 2 +-
arch/x86/kvm/vmx.c | 2 +-
4 files changed, 14 insertions(+), 13 deletions(-)
But, yeah, I think we need to. While it will take a little adjustment
in the brains of us old-timers and a bit of pain when switching from old
kernels to new, this makes it a lot more clear what is going on.
^ permalink raw reply
* Re: [RFC PATCH v4 20/27] x86/cet/shstk: Signal handling for shadow stack
From: Eugene Syromiatnikov @ 2018-10-03 14:36 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-21-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:44AM -0700, Yu-cheng Yu wrote:
> When setting up a signal, the kernel creates a shadow stack
> restore token at the current SHSTK address and then stores the
> token's address in the signal frame, right after the FPU state.
> Before restoring a signal, the kernel verifies and then uses the
> restore token to set the SHSTK pointer.
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> index ec256ae27a31..5cc4be6e0982 100644
> --- a/arch/x86/kernel/cet.c
> +++ b/arch/x86/kernel/cet.c
> @@ -46,6 +47,69 @@ static unsigned long get_shstk_addr(void)
> return ptr;
> }
>
> +/*
> + * Verify the restore token at the address of 'ssp' is
> + * valid and then set shadow stack pointer according to the
> + * token.
> + */
> +static int verify_rstor_token(bool ia32, unsigned long ssp,
> + unsigned long *new_ssp)
> +{
> + unsigned long token;
> +
> + *new_ssp = 0;
> +
> + if (!IS_ALIGNED(ssp, 8))
> + return -EINVAL;
> +
> + if (get_user(token, (unsigned long __user *)ssp))
> + return -EFAULT;
> +
> + /* Is 64-bit mode flag correct? */
> + if (ia32 && (token & 3) != 0)
> + return -EINVAL;
> + else if ((token & 3) != 1)
> + return -EINVAL;
It is probably worth adding constant names for these flags, example,
there's Section 2.4 in the currently available description[1], and
it took some time before I decided to look into other patches
and find the patch with the documentation (or finally notice section 2.7).
[1] https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf
> + token &= ~(1UL);
> +
> + if ((!ia32 && !IS_ALIGNED(token, 8)) || !IS_ALIGNED(token, 4))
> + return -EINVAL;
> +
> + if ((ALIGN_DOWN(token, 8) - 8) != ssp)
> + return -EINVAL;
> +
> + *new_ssp = token;
> + return 0;
> +}
> +
> +/*
> + * Create a restore token on the shadow stack.
> + * A token is always 8-byte and aligned to 8.
> + */
> +static int create_rstor_token(bool ia32, unsigned long ssp,
> + unsigned long *new_ssp)
> +{
> + unsigned long addr;
> +
> + *new_ssp = 0;
> +
> + if ((!ia32 && !IS_ALIGNED(ssp, 8)) || !IS_ALIGNED(ssp, 4))
> + return -EINVAL;
Maybe refactor this check into a separate function/macro?
> +
> + addr = ALIGN_DOWN(ssp, 8) - 8;
> +
> + /* Is the token for 64-bit? */
> + if (!ia32)
> + ssp |= 1;
Again, usage of a named constant might document it better.
> +
> + if (write_user_shstk_64(addr, ssp))
This function is defined in "[RFC PATCH v4 19/27] x86/cet/shstk:
Introduce WRUSS instruction"
> + return -EFAULT;
> +
> + *new_ssp = addr;
> + return 0;
> +}
> +
> int cet_setup_shstk(void)
> {
> unsigned long addr, size;
> @@ -107,3 +171,54 @@ void cet_disable_free_shstk(struct task_struct *tsk)
>
> tsk->thread.cet.shstk_enabled = 0;
> }
> +
> +int cet_restore_signal(unsigned long ssp)
> +{
> + unsigned long new_ssp;
> + int err;
> +
> + if (!current->thread.cet.shstk_enabled)
> + return 0;
> +
> + err = verify_rstor_token(in_ia32_syscall(), ssp, &new_ssp);
> +
> + if (err)
> + return err;
> +
> + return set_shstk_ptr(new_ssp);
> +}
> +
> +/*
> + * Setup the shadow stack for the signal handler: first,
> + * create a restore token to keep track of the current ssp,
> + * and then the return address of the signal handler.
> + */
> +int cet_setup_signal(bool ia32, unsigned long rstor_addr,
> + unsigned long *new_ssp)
> +{
> + unsigned long ssp;
> + int err;
> +
> + if (!current->thread.cet.shstk_enabled)
> + return 0;
> +
> + ssp = get_shstk_addr();
> + err = create_rstor_token(ia32, ssp, new_ssp);
> +
> + if (err)
> + return err;
> +
> + if (ia32) {
> + ssp = *new_ssp - sizeof(u32);
> + err = write_user_shstk_32(ssp, (unsigned int)rstor_addr);
> + } else {
> + ssp = *new_ssp - sizeof(u64);
> + err = write_user_shstk_64(ssp, rstor_addr);
> + }
> +
> + if (err)
> + return err;
> +
> + set_shstk_ptr(ssp);
> + return 0;
> +}
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index 92a3b312a53c..e9a85689143f 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -46,6 +46,7 @@
>
> #include <asm/sigframe.h>
> #include <asm/signal.h>
> +#include <asm/cet.h>
>
> #define COPY(x) do { \
> get_user_ex(regs->x, &sc->x); \
> @@ -152,6 +153,10 @@ static int restore_sigcontext(struct pt_regs *regs,
>
> err |= fpu__restore_sig(buf, IS_ENABLED(CONFIG_X86_32));
>
> +#ifdef CONFIG_X86_64
> + err |= restore_sigcontext_ext(buf);
> +#endif
> +
> force_iret();
>
> return err;
> @@ -266,6 +271,11 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
> }
>
> if (fpu->initialized) {
> +#ifdef CONFIG_X86_64
> + /* sigcontext extension */
> + if (boot_cpu_has(X86_FEATURE_SHSTK))
> + sp -= sizeof(struct sc_ext) + 8;
> +#endif
> sp = fpu__alloc_mathframe(sp, IS_ENABLED(CONFIG_X86_32),
> &buf_fx, &math_size);
That might be refactored in a separate function.
Also, it looks like that possible padding for 8-byte alignment
(copy_ext_{to,from}_user) is not accounted here.
> *fpstate = (void __user *)sp;
> @@ -493,6 +503,9 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
> err |= setup_sigcontext(&frame->uc.uc_mcontext, fp, regs, set->sig[0]);
> err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
>
> + if (!err)
> + err = setup_sigcontext_ext(ksig, fp);
> +
Why is this not in setup_sigcontext, for example?
> if (err)
> return -EFAULT;
>
> @@ -576,6 +589,9 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
> regs, set->sig[0]);
> err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
>
> + if (!err)
> + err = setup_sigcontext_ext(ksig, fpstate);
> +
> if (err)
> return -EFAULT;
>
> @@ -707,6 +723,86 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> }
> }
>
> +#ifdef CONFIG_X86_64
> +static int copy_ext_from_user(struct sc_ext *ext, void __user *fpu)
> +{
> + void __user *p;
> +
> + if (!fpu)
> + return -EINVAL;
> +
> + p = fpu + fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE;
> + p = (void __user *)ALIGN((unsigned long)p, 8);
> +
> + if (!access_ok(VERIFY_READ, p, sizeof(*ext)))
> + return -EFAULT;
> +
> + if (__copy_from_user(ext, p, sizeof(*ext)))
> + return -EFAULT;
> +
> + if (ext->total_size != sizeof(*ext))
> + return -EINVAL;
> + return 0;
> +}
> +
> +static int copy_ext_to_user(void __user *fpu, struct sc_ext *ext)
> +{
> + void __user *p;
> +
> + if (!fpu)
> + return -EINVAL;
> +
> + if (ext->total_size != sizeof(*ext))
> + return -EINVAL;
> +
> + p = fpu + fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE;
> + p = (void __user *)ALIGN((unsigned long)p, 8);
> +
> + if (!access_ok(VERIFY_WRITE, p, sizeof(*ext)))
> + return -EFAULT;
> +
> + if (__copy_to_user(p, ext, sizeof(*ext)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +int restore_sigcontext_ext(void __user *fp)
> +{
> + int err = 0;
> +
> + if (boot_cpu_has(X86_FEATURE_SHSTK) && fp) {
> + struct sc_ext ext = {0, 0};
> +
> + err = copy_ext_from_user(&ext, fp);
> +
> + if (!err)
> + err = cet_restore_signal(ext.ssp);
> + }
> +
> + return err;
> +}
> +
> +int setup_sigcontext_ext(struct ksignal *ksig, void __user *fp)
> +{
> + int err = 0;
> +
> + if (boot_cpu_has(X86_FEATURE_SHSTK) && fp) {
> + struct sc_ext ext = {0, 0};
> + unsigned long rstor;
> +
> + rstor = (unsigned long)ksig->ka.sa.sa_restorer;
> + err = cet_setup_signal(is_ia32_frame(ksig), rstor, &ext.ssp);
> + if (!err) {
> + ext.total_size = sizeof(ext);
> + err = copy_ext_to_user(fp, &ext);
> + }
> + }
> +
> + return err;
> +}
> +#endif
> +
> static void
> handle_signal(struct ksignal *ksig, struct pt_regs *regs)
> {
> --
> 2.17.1
>
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Dan Williams @ 2018-10-03 14:38 UTC (permalink / raw)
To: Jan Kara
Cc: linux-xfs, linux-nvdimm, Linux API, Christoph Hellwig, Linux MM,
linux-fsdevel, linux-ext4
In-Reply-To: <20181003125056.GA21043-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org>
On Wed, Oct 3, 2018 at 5:51 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
>
> On Tue 02-10-18 13:18:54, Dan Williams wrote:
> > On Tue, Oct 2, 2018 at 8:32 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> > >
> > > On Tue 02-10-18 07:52:06, Christoph Hellwig wrote:
> > > > On Tue, Oct 02, 2018 at 04:44:13PM +0200, Johannes Thumshirn wrote:
> > > > > On Tue, Oct 02, 2018 at 07:37:13AM -0700, Christoph Hellwig wrote:
> > > > > > No, it should not. DAX is an implementation detail thay may change
> > > > > > or go away at any time.
> > > > >
> > > > > Well we had an issue with an application checking for dax, this is how
> > > > > we landed here in the first place.
> > > >
> > > > So what exacty is that "DAX" they are querying about (and no, I'm not
> > > > joking, nor being philosophical).
> > >
> > > I believe the application we are speaking about is mostly concerned about
> > > the memory overhead of the page cache. Think of a machine that has ~ 1TB of
> > > DRAM, the database running on it is about that size as well and they want
> > > database state stored somewhere persistently - which they may want to do by
> > > modifying mmaped database files if they do small updates... So they really
> > > want to be able to use close to all DRAM for the DB and not leave slack
> > > space for the kernel page cache to cache 1TB of database files.
> >
> > VM_MIXEDMAP was never a reliable indication of DAX because it could be
> > set for random other device-drivers that use vm_insert_mixed(). The
> > MAP_SYNC flag positively indicates that page cache is disabled for a
> > given mapping, although whether that property is due to "dax" or some
> > other kernel mechanics is purely an internal detail.
> >
> > I'm not opposed to faking out VM_MIXEDMAP if this broken check has
> > made it into production, but again, it's unreliable.
>
> So luckily this particular application wasn't widely deployed yet so we
> will likely get away with the vendor asking customers to update to a
> version not looking into smaps and parsing /proc/mounts instead.
>
> But I don't find parsing /proc/mounts that beautiful either and I'd prefer
> if we had a better interface for applications to query whether they can
> avoid page cache for mmaps or not.
Yeah, the mount flag is not a good indicator either. I think we need
to follow through on the per-inode property of DAX. Darrick and I
discussed just allowing the property to be inherited from the parent
directory at file creation time. That avoids the dynamic set-up /
teardown races that seem intractable at this point.
What's wrong with MAP_SYNC as a page-cache detector in the meantime?
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Jan Kara @ 2018-10-03 15:06 UTC (permalink / raw)
To: Dan Williams
Cc: linux-xfs, linux-nvdimm, Linux API, Christoph Hellwig, Linux MM,
linux-fsdevel, Jan Kara, linux-ext4
In-Reply-To: <CAPcyv4jfV10yuTiPg6ijsPRRL2-c_48ovfpU5TK1Zu7BWnfk3g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed 03-10-18 07:38:50, Dan Williams wrote:
> On Wed, Oct 3, 2018 at 5:51 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> >
> > On Tue 02-10-18 13:18:54, Dan Williams wrote:
> > > On Tue, Oct 2, 2018 at 8:32 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> > > >
> > > > On Tue 02-10-18 07:52:06, Christoph Hellwig wrote:
> > > > > On Tue, Oct 02, 2018 at 04:44:13PM +0200, Johannes Thumshirn wrote:
> > > > > > On Tue, Oct 02, 2018 at 07:37:13AM -0700, Christoph Hellwig wrote:
> > > > > > > No, it should not. DAX is an implementation detail thay may change
> > > > > > > or go away at any time.
> > > > > >
> > > > > > Well we had an issue with an application checking for dax, this is how
> > > > > > we landed here in the first place.
> > > > >
> > > > > So what exacty is that "DAX" they are querying about (and no, I'm not
> > > > > joking, nor being philosophical).
> > > >
> > > > I believe the application we are speaking about is mostly concerned about
> > > > the memory overhead of the page cache. Think of a machine that has ~ 1TB of
> > > > DRAM, the database running on it is about that size as well and they want
> > > > database state stored somewhere persistently - which they may want to do by
> > > > modifying mmaped database files if they do small updates... So they really
> > > > want to be able to use close to all DRAM for the DB and not leave slack
> > > > space for the kernel page cache to cache 1TB of database files.
> > >
> > > VM_MIXEDMAP was never a reliable indication of DAX because it could be
> > > set for random other device-drivers that use vm_insert_mixed(). The
> > > MAP_SYNC flag positively indicates that page cache is disabled for a
> > > given mapping, although whether that property is due to "dax" or some
> > > other kernel mechanics is purely an internal detail.
> > >
> > > I'm not opposed to faking out VM_MIXEDMAP if this broken check has
> > > made it into production, but again, it's unreliable.
> >
> > So luckily this particular application wasn't widely deployed yet so we
> > will likely get away with the vendor asking customers to update to a
> > version not looking into smaps and parsing /proc/mounts instead.
> >
> > But I don't find parsing /proc/mounts that beautiful either and I'd prefer
> > if we had a better interface for applications to query whether they can
> > avoid page cache for mmaps or not.
>
> Yeah, the mount flag is not a good indicator either. I think we need
> to follow through on the per-inode property of DAX. Darrick and I
> discussed just allowing the property to be inherited from the parent
> directory at file creation time. That avoids the dynamic set-up /
> teardown races that seem intractable at this point.
>
> What's wrong with MAP_SYNC as a page-cache detector in the meantime?
So IMHO checking for MAP_SYNC is about as reliable as checking for 'dax'
mount option. It works now but nobody promises it will reliably detect DAX in
future - e.g. there's nothing that prevents MAP_SYNC to work for mappings
using pagecache if we find a sensible usecase for that.
WRT per-inode DAX property, AFAIU that inode flag is just going to be
advisory thing - i.e., use DAX if possible. If you mount a filesystem with
these inode flags set in a configuration which does not allow DAX to be
used, you will still be able to access such inodes but the access will use
page cache instead. And querying these flags should better show real
on-disk status and not just whether DAX is used as that would result in an
even bigger mess. So this feature seems to be somewhat orthogonal to the
API I'm looking for.
Honza
--
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR
^ permalink raw reply
* Re: [RFC PATCH v4 18/27] x86/cet/shstk: User-mode shadow stack support
From: Eugene Syromiatnikov @ 2018-10-03 15:08 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-19-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:42AM -0700, Yu-cheng Yu wrote:
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 5ea1d64cb0b4..b20450dde5b7 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -652,6 +652,9 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
> [ilog2(VM_PKEY_BIT4)] = "",
> #endif
> #endif /* CONFIG_ARCH_HAS_PKEYS */
> +#ifdef CONFIG_X86_INTEL_SHADOW_STACK_USER
> + [ilog2(VM_SHSTK)] = "ss"
> +#endif
It's probably makes sense to have this hunk as a part of "x86/cet/shstk:
Add Kconfig option for user-mode shadow stack", where VM_SHSTK was
initially introduced.
^ 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