* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-03 18:15 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, Andi Kleen, heiko carstens, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will
In-Reply-To: <20180703181143.GB2494@hirez.programming.kicks-ass.net>
----- On Jul 3, 2018, at 2:11 PM, Peter Zijlstra peterz@infradead.org wrote:
> On Tue, Jul 03, 2018 at 01:58:37PM -0400, Mathieu Desnoyers wrote:
>> I can modify the ABI to put the cpu_id_start and cpu_id fields inside
>> a union, and update it with a single store.
>>
>> Thoughts ?
>
> Let's keep them for now, we can always frob this later, they are aligned
> and proper, no need to expose that union to userspace.
Isn't it weird to change the API of an exposed public uapi header ? What
if userspace chooses to do sizeof(__rseq_abi.cpu_id) ? We would break
this unless we use a transparent union, which puts constraints I would
hope not to have on compilers supporting transparent unions (I recall
C++ had issues with this).
I'd prefer to expose the union right away if it's fine with you.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Peter Zijlstra @ 2018-07-03 18:11 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Linus Torvalds, Andi Kleen, heiko carstens, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will
In-Reply-To: <1048940999.11846.1530640717837.JavaMail.zimbra@efficios.com>
On Tue, Jul 03, 2018 at 01:58:37PM -0400, Mathieu Desnoyers wrote:
> I can modify the ABI to put the cpu_id_start and cpu_id fields inside
> a union, and update it with a single store.
>
> Thoughts ?
Let's keep them for now, we can always frob this later, they are aligned
and proper, no need to expose that union to userspace.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Peter Zijlstra @ 2018-07-03 18:10 UTC (permalink / raw)
To: Linus Torvalds
Cc: Mathieu Desnoyers, Andi Kleen, Heiko Carstens, Andy Lutomirski,
Thomas Gleixner, Linux Kernel Mailing List, Linux API,
Paul McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King - ARM Linux, Ingo Molnar, Peter Anvin,
Christoph Lameter, Ben Maurer, Steven Rostedt, Josh Triplett,
Catalin Marinas <catalin.m>
In-Reply-To: <CA+55aFxcVbG2DMmmrW-HLTXZE+rcBWc5F48UYoL2heEsEpyO=g@mail.gmail.com>
On Tue, Jul 03, 2018 at 10:59:45AM -0700, Linus Torvalds wrote:
> On Tue, Jul 3, 2018 at 10:49 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > > I can simply document that loads/stores from/to all struct rseq fields
> > > should be thread-local then ?
> >
> > I'm not sure that covers things sufficiently. You really want the
> > userspace load/stores to be single instructions.
>
> Actually, I think we should try very hard to limit even that to _just_
> the rseq pointer itself.
> So I'd suggest that the only part we aim to have any "atomic" behavior
> at all is for the individual fields in "struct rseq" itself. So the
> cpu id and the base pointer and the flags. And even they are
> thread-local, so the atomicity is not about the kernel, but about user
> space needing to read and update them in word-sized chunks.
>
> End result: absolutely nothing is atomic for the kernel.
Yes, agreed, that is what I meant but very poorly expressed. Only the
rseq bits themselves need this single-copy atomic stuff -- for
userspace.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-03 18:09 UTC (permalink / raw)
To: Linus Torvalds
Cc: Peter Zijlstra, Andi Kleen, heiko carstens, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas
In-Reply-To: <CA+55aFxcVbG2DMmmrW-HLTXZE+rcBWc5F48UYoL2heEsEpyO=g@mail.gmail.com>
----- On Jul 3, 2018, at 1:59 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Tue, Jul 3, 2018 at 10:49 AM Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> > I can simply document that loads/stores from/to all struct rseq fields
>> > should be thread-local then ?
>>
>> I'm not sure that covers things sufficiently. You really want the
>> userspace load/stores to be single instructions.
>
> Actually, I think we should try very hard to limit even that to _just_
> the rseq pointer itself.
>
> Everything else can be filled in ahead of time with non-atomic stores,
> and then the last thing that happens - and the only thing that wants
> that final "one last atomic write" is the rseq pointer write.
>
> No?
Well a small nit here: the rseq->rseq_cs pointer store is performed
at the _very beginning_ of the rseq critical section. We indeed want
that store to be performed as a single instruction by user-space.
What I think you have in mind as "one last atomic write" is the commit
instruction at the end of the critical section, which does not touch
any field in struct rseq.
>
> So I'd suggest that the only part we aim to have any "atomic" behavior
> at all is for the individual fields in "struct rseq" itself. So the
> cpu id and the base pointer and the flags. And even they are
> thread-local, so the atomicity is not about the kernel, but about user
> space needing to read and update them in word-sized chunks.
>
> End result: absolutely nothing is atomic for the kernel.
Yes, +1. If everyone is OK with that I'll go and implement the changes
within the coming day.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Linus Torvalds @ 2018-07-03 17:59 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mathieu Desnoyers, Andi Kleen, Heiko Carstens, Andy Lutomirski,
Thomas Gleixner, Linux Kernel Mailing List, Linux API,
Paul McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King - ARM Linux, Ingo Molnar, Peter Anvin,
Christoph Lameter, Ben Maurer, Steven Rostedt, Josh Triplett,
Catalin Marinas <catalin.m>
In-Reply-To: <20180703174833.GZ2494@hirez.programming.kicks-ass.net>
On Tue, Jul 3, 2018 at 10:49 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> > I can simply document that loads/stores from/to all struct rseq fields
> > should be thread-local then ?
>
> I'm not sure that covers things sufficiently. You really want the
> userspace load/stores to be single instructions.
Actually, I think we should try very hard to limit even that to _just_
the rseq pointer itself.
Everything else can be filled in ahead of time with non-atomic stores,
and then the last thing that happens - and the only thing that wants
that final "one last atomic write" is the rseq pointer write.
No?
So I'd suggest that the only part we aim to have any "atomic" behavior
at all is for the individual fields in "struct rseq" itself. So the
cpu id and the base pointer and the flags. And even they are
thread-local, so the atomicity is not about the kernel, but about user
space needing to read and update them in word-sized chunks.
End result: absolutely nothing is atomic for the kernel.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-03 17:58 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, Andi Kleen, heiko carstens, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will
In-Reply-To: <20180703174833.GZ2494@hirez.programming.kicks-ass.net>
----- On Jul 3, 2018, at 1:48 PM, Peter Zijlstra peterz@infradead.org wrote:
> On Tue, Jul 03, 2018 at 01:38:59PM -0400, Mathieu Desnoyers wrote:
>> ----- On Jul 3, 2018, at 1:34 PM, Peter Zijlstra peterz@infradead.org wrote:
>>
>> > On Tue, Jul 03, 2018 at 10:10:37AM -0700, Linus Torvalds wrote:
>> >> On Tue, Jul 3, 2018 at 9:40 AM Andi Kleen <andi@firstfloor.org> wrote:
>> >> >
>> >> > So it sounds like architectures that don't have an instruction atomic u64
>> >> > *_user need to disable interrupts during the access, and somehow handle that
>> >> > case when a page fault happens?
>> >>
>> >> No. It's actually the store by *user* space that is the critical one.
>> >> Not the whole 64-bit value, just the low pointer part.
>> >>
>> >> The kernel could do it as a byte-by-byte load, really. It's
>> >> per-thread, and once the kernel is running, it's not going to change.
>> >> The kernel never changes the value, it just loads it from user space.
>> >
>> > The kernel doesn't change _this_ value, but the kernel does change other
>> > values, like for instance rseq->cpu_id. But even there, it could use
>> > byte stores and it is again the userspace load of that field that is
>> > critical again and needs to be a single op.
>>
>> I can simply document that loads/stores from/to all struct rseq fields
>> should be thread-local then ?
>
> I'm not sure that covers things sufficiently. You really want the
> userspace load/stores to be single instructions.
Yes, of course. More specifically, I would document that those need to
be single-copy atomicity load/store performed by the local thread.
> Also, I think it was rseq_update_cpu_id() where we wanted to use a
> single u64 store if possible but you worried about the stores.
With this added bit of restriction on thread-local loads, indeed we
can then update them without caring about atomicity at kernel level.
I can modify the ABI to put the cpu_id_start and cpu_id fields inside
a union, and update it with a single store.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Peter Zijlstra @ 2018-07-03 17:48 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Linus Torvalds, Andi Kleen, heiko carstens, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will
In-Reply-To: <399697782.11820.1530639539750.JavaMail.zimbra@efficios.com>
On Tue, Jul 03, 2018 at 01:38:59PM -0400, Mathieu Desnoyers wrote:
> ----- On Jul 3, 2018, at 1:34 PM, Peter Zijlstra peterz@infradead.org wrote:
>
> > On Tue, Jul 03, 2018 at 10:10:37AM -0700, Linus Torvalds wrote:
> >> On Tue, Jul 3, 2018 at 9:40 AM Andi Kleen <andi@firstfloor.org> wrote:
> >> >
> >> > So it sounds like architectures that don't have an instruction atomic u64
> >> > *_user need to disable interrupts during the access, and somehow handle that
> >> > case when a page fault happens?
> >>
> >> No. It's actually the store by *user* space that is the critical one.
> >> Not the whole 64-bit value, just the low pointer part.
> >>
> >> The kernel could do it as a byte-by-byte load, really. It's
> >> per-thread, and once the kernel is running, it's not going to change.
> >> The kernel never changes the value, it just loads it from user space.
> >
> > The kernel doesn't change _this_ value, but the kernel does change other
> > values, like for instance rseq->cpu_id. But even there, it could use
> > byte stores and it is again the userspace load of that field that is
> > critical again and needs to be a single op.
>
> I can simply document that loads/stores from/to all struct rseq fields
> should be thread-local then ?
I'm not sure that covers things sufficiently. You really want the
userspace load/stores to be single instructions.
Also, I think it was rseq_update_cpu_id() where we wanted to use a
single u64 store if possible but you worried about the stores.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-03 17:38 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, Andi Kleen, heiko carstens, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will
In-Reply-To: <20180703173451.GX2494@hirez.programming.kicks-ass.net>
----- On Jul 3, 2018, at 1:34 PM, Peter Zijlstra peterz@infradead.org wrote:
> On Tue, Jul 03, 2018 at 10:10:37AM -0700, Linus Torvalds wrote:
>> On Tue, Jul 3, 2018 at 9:40 AM Andi Kleen <andi@firstfloor.org> wrote:
>> >
>> > So it sounds like architectures that don't have an instruction atomic u64
>> > *_user need to disable interrupts during the access, and somehow handle that
>> > case when a page fault happens?
>>
>> No. It's actually the store by *user* space that is the critical one.
>> Not the whole 64-bit value, just the low pointer part.
>>
>> The kernel could do it as a byte-by-byte load, really. It's
>> per-thread, and once the kernel is running, it's not going to change.
>> The kernel never changes the value, it just loads it from user space.
>
> The kernel doesn't change _this_ value, but the kernel does change other
> values, like for instance rseq->cpu_id. But even there, it could use
> byte stores and it is again the userspace load of that field that is
> critical again and needs to be a single op.
I can simply document that loads/stores from/to all struct rseq fields
should be thread-local then ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Peter Zijlstra @ 2018-07-03 17:34 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andi Kleen, Heiko Carstens, Mathieu Desnoyers, Andy Lutomirski,
Thomas Gleixner, Linux Kernel Mailing List, Linux API,
Paul McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King - ARM Linux, Ingo Molnar, Peter Anvin,
Christoph Lameter, Ben Maurer, Steven Rostedt, Josh Triplett,
Catalin Marinas <catalin.m>
In-Reply-To: <CA+55aFzYEQgEhCnrwtbGuCy36Wh+aiw0_9rgMcjUgbn1JtZAPQ@mail.gmail.com>
On Tue, Jul 03, 2018 at 10:10:37AM -0700, Linus Torvalds wrote:
> On Tue, Jul 3, 2018 at 9:40 AM Andi Kleen <andi@firstfloor.org> wrote:
> >
> > So it sounds like architectures that don't have an instruction atomic u64
> > *_user need to disable interrupts during the access, and somehow handle that
> > case when a page fault happens?
>
> No. It's actually the store by *user* space that is the critical one.
> Not the whole 64-bit value, just the low pointer part.
>
> The kernel could do it as a byte-by-byte load, really. It's
> per-thread, and once the kernel is running, it's not going to change.
> The kernel never changes the value, it just loads it from user space.
The kernel doesn't change _this_ value, but the kernel does change other
values, like for instance rseq->cpu_id. But even there, it could use
byte stores and it is again the userspace load of that field that is
critical again and needs to be a single op.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Mathieu Desnoyers @ 2018-07-03 17:26 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andi Kleen, Peter Zijlstra, heiko carstens, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas
In-Reply-To: <CA+55aFzYEQgEhCnrwtbGuCy36Wh+aiw0_9rgMcjUgbn1JtZAPQ@mail.gmail.com>
----- On Jul 3, 2018, at 1:10 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Tue, Jul 3, 2018 at 9:40 AM Andi Kleen <andi@firstfloor.org> wrote:
>>
>> So it sounds like architectures that don't have an instruction atomic u64
>> *_user need to disable interrupts during the access, and somehow handle that
>> case when a page fault happens?
>
> No. It's actually the store by *user* space that is the critical one.
> Not the whole 64-bit value, just the low pointer part.
>
> The kernel could do it as a byte-by-byte load, really. It's
> per-thread, and once the kernel is running, it's not going to change.
> The kernel never changes the value, it just loads it from user space.
>
> So all the atomicity worries for the kernel are a red herring. They'd
> arguably be nice to have - but only for an insane case that makes
> absolutely no sense (a different thread trying to change the value).
>
> Can we please stop the idiocy already? The kernel could read the rseq
> pointer one bit at a time, and do a little dance with "yield()" in
> between, and take interrupts and page faults, and it wouldn't matter
> AT ALL.
>
> It's not even that we read the value from an interrupt context, it's
> that as we return to user space (which can be the result of an
> interrupt) we can read the value.
>
> This whole thread has been filled with crazy "what if" things that don't matter.
Sorry to come back in the thread late, looks like I've missed all the
fun.
I agree with Linus: we can simply document that updates to rseq->rseq_cs
should be thread-local in the rseq uapi and be done with it. This would
allow using get_user(u64) even on 32-bit architectures, because we cannot
care less if an architecture chooses to read the u64 byte-wise while
standing on its feet.
With this added requirement, Andy's idea of using a union between __u64
and upper/lower __u32 would fit very nicely.
If everyone is OK with that approach, I can prepare an updated patch.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Linus Torvalds @ 2018-07-03 17:10 UTC (permalink / raw)
To: Andi Kleen
Cc: Peter Zijlstra, Heiko Carstens, Mathieu Desnoyers,
Andy Lutomirski, Thomas Gleixner, Linux Kernel Mailing List,
Linux API, Paul McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King - ARM Linux, Ingo Molnar, Peter Anvin,
Christoph Lameter, Ben Maurer, Steven Rostedt, Josh Triplett,
Catalin Marinas <cata>
In-Reply-To: <20180703164048.i2te5gjemcafqzwf@two.firstfloor.org>
On Tue, Jul 3, 2018 at 9:40 AM Andi Kleen <andi@firstfloor.org> wrote:
>
> So it sounds like architectures that don't have an instruction atomic u64
> *_user need to disable interrupts during the access, and somehow handle that
> case when a page fault happens?
No. It's actually the store by *user* space that is the critical one.
Not the whole 64-bit value, just the low pointer part.
The kernel could do it as a byte-by-byte load, really. It's
per-thread, and once the kernel is running, it's not going to change.
The kernel never changes the value, it just loads it from user space.
So all the atomicity worries for the kernel are a red herring. They'd
arguably be nice to have - but only for an insane case that makes
absolutely no sense (a different thread trying to change the value).
Can we please stop the idiocy already? The kernel could read the rseq
pointer one bit at a time, and do a little dance with "yield()" in
between, and take interrupts and page faults, and it wouldn't matter
AT ALL.
It's not even that we read the value from an interrupt context, it's
that as we return to user space (which can be the result of an
interrupt) we can read the value.
This whole thread has been filled with crazy "what if" things that don't matter.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Andy Lutomirski @ 2018-07-03 17:06 UTC (permalink / raw)
To: Andi Kleen
Cc: Peter Zijlstra, Heiko Carstens, Mathieu Desnoyers, Linus Torvalds,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas
In-Reply-To: <20180703164048.i2te5gjemcafqzwf@two.firstfloor.org>
On Jul 3, 2018, at 9:40 AM, Andi Kleen <andi@firstfloor.org> wrote:
>>
>> So I think you're good... But yes, you raise an interresting point.
>
> So it sounds like architectures that don't have an instruction atomic u64
> *_user need to disable interrupts during the access, and somehow handle that
> case when a page fault happens?
I think all this discussion of “atomic” is a huge distraction. The properties we need are:
- User code can change rseq_cs from one valid user pointer to another with a single instruction (or equivalent) such that we can’t end up in the kernel with the write only partially done as seen in that thread.
- The kernel needs to be able to read the value consistently with the above requirement.
I don’t think it’s possible to have a valid implementation of get_user() on any architecture that’s so weak that this doesn’t work.
If user code writes rseq_cs from the wrong thread, I think the user code is buggy and we simply don’t care what happens. The kernel should be allowed to use an arbitrarily weak read with respect to other threads.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Peter Zijlstra @ 2018-07-03 17:02 UTC (permalink / raw)
To: Andi Kleen
Cc: Heiko Carstens, Mathieu Desnoyers, Linus Torvalds,
Andy Lutomirski, Thomas Gleixner, linux-kernel, linux-api,
Paul E. McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Ingo Molnar, H. Peter Anvin,
Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Catalin Marinas
In-Reply-To: <20180703164048.i2te5gjemcafqzwf@two.firstfloor.org>
On Tue, Jul 03, 2018 at 09:40:48AM -0700, Andi Kleen wrote:
> >
> > So I think you're good... But yes, you raise an interresting point.
>
> So it sounds like architectures that don't have an instruction atomic u64
> *_user need to disable interrupts during the access, and somehow handle that
> case when a page fault happens?
So for 32bit, as Linus already said, a split store is _fine_, because
the top word is always going to be 0 anyway.
So all we really need is native word sized loads / stores. s390 is just
a little weird here (it wouldn't be s390 if it wasn't I suppose) for not
actually using regular loads / stores because of the split address space
stuff.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Andi Kleen @ 2018-07-03 16:40 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Heiko Carstens, Mathieu Desnoyers, Linus Torvalds,
Andy Lutomirski, Thomas Gleixner, linux-kernel, linux-api,
Paul E. McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Ingo Molnar, H. Peter Anvin,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett
In-Reply-To: <20180703092113.GV2494@hirez.programming.kicks-ass.net>
>
> So I think you're good... But yes, you raise an interresting point.
So it sounds like architectures that don't have an instruction atomic u64
*_user need to disable interrupts during the access, and somehow handle that
case when a page fault happens?
-Andi
^ permalink raw reply
* Re: [REGRESSION] "Locked" and "Pss" in /proc/*/smaps are the same
From: Daniel Colascione @ 2018-07-03 16:20 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Thomas Lindroth, Andrew Morton, linux-api, linux-kernel, linux-mm
In-Reply-To: <ebf6c7fb-fec3-6a26-544f-710ed193c154@suse.cz>
On Tue, Jul 3, 2018 at 12:36 AM, Vlastimil Babka <vbabka@suse.cz> wrote:
> +CC
>
> On 07/01/2018 08:31 PM, Thomas Lindroth wrote:
>> While looking around in /proc on my v4.14.52 system I noticed that
>> all processes got a lot of "Locked" memory in /proc/*/smaps. A lot
>> more memory than a regular user can usually lock with mlock().
>>
>> commit 493b0e9d945fa9dfe96be93ae41b4ca4b6fdb317 (v4.14-rc1) seems
>> to have changed the behavior of "Locked".
Thanks for fixing that. I submitted a patch [1] for this bug and some
others a while ago, but the patch didn't make it into the tree because
or wasn't split up correctly or something, and I had to do other work.
[1] https://marc.info/?l=linux-mm&m=151927723128134&w=2
^ permalink raw reply
* Re: [PATCH v6] pidns: introduce syscall translate_pid
From: Nagarathnam Muthusamy @ 2018-07-03 14:33 UTC (permalink / raw)
To: Konstantin Khlebnikov, linux-api, linux-kernel, Jann Horn,
Serge Hallyn, Prakash Sangappa, Oleg Nesterov, Eric W. Biederman,
Andrew Morton, Andy Lutomirski, Michael Kerrisk (man-pages)
In-Reply-To: <152788068212.768348.15192457501079586650.stgit@buzz>
On 6/1/2018 12:18 PM, Konstantin Khlebnikov wrote:
> Each process have different pids, one for each pid namespace it belongs.
> When interaction happens within single pid-ns translation isn't required.
> More complicated scenarios needs special handling.
>
> For example:
> - reading pid-files or logs written inside container with pid namespace
> - writing logs with internal pids outside container for pushing them into
> - attaching with ptrace to tasks from different pid namespace
>
> Generally speaking, any cross pid-ns API with pids needs translation.
>
> Currently there are several interfaces that could be used here:
>
> Pid namespaces are identified by device and inode of /proc/[pid]/ns/pid.
>
> Pids for nested pid namespaces are shown in file /proc/[pid]/status.
> In some cases pid translation could be easily done using this information.
> Backward translation requires scanning all tasks and becomes really
> complicated for deeper namespace nesting.
>
> Unix socket automatically translates pid attached to SCM_CREDENTIALS.
> This requires CAP_SYS_ADMIN for sending arbitrary pids and entering
> into pid namespace, this expose process and could be insecure.
>
> This patch adds new syscall for converting pids between pid namespaces:
>
> pid_t translate_pid(pid_t pid, int source, int target);
>
> Pid-namespaces are referred file descriptors opened to proc files
> /proc/[pid]/ns/pid or /proc/[pid]/ns/pid_for_children.
> Negative argument points to current pid namespace.
>
> Syscall returns pid in target pid-ns or zero if task have no pid there.
>
> Error codes:
> EBADF - file descriptor is closed
> EINVAL - file descriptor isn't pid namespace
> ESRCH - task not found in @source namespace
>
> Translation could breach pid-ns isolation and return pids from outer pid
> namespaces iff process already has file descriptor for these namespaces.
>
> 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?
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Reanimated-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: https://lkml.org/lkml/2017/10/13/177
> * rename from getvpid() 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
> v5: https://lkml.org/lkml/2018/4/4/677
> * rewrite commit message
> * resolve pidns by task pid or by pidns fd
> * add arguments source_type and target_type
> v6:
> * revert back minimized v4 design
> * rebase to next-20180601
> * fix COND_SYSCALL stub
> * use next syscall number, old used for io_pgetevents
>
> --- sample tool ---
>
> #define _GNU_SOURCE
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdlib.h>
> #include <stdio.h>
> #include <err.h>
>
> #ifndef SYS_translate_pid
> #ifdef __x86_64__
> #define SYS_translate_pid 334
> #elif defined __i386__
> #define SYS_translate_pid 386
> #endif
> #endif
>
> pid_t translate_pid(pid_t pid, int source, int target) {
> return syscall(SYS_translate_pid, pid, source, target);
> }
>
> int main(int argc, char **argv) {
> int pid, source, target;
> char buf[64];
>
> if (argc != 4)
> errx(1, "usage: %s <pid> <source> <target>", argv[0]);
>
> pid = atoi(argv[1]);
> source = atoi(argv[2]);
> target = atoi(argv[3]);
>
> if (source > 0) {
> snprintf(buf, sizeof(buf), "/proc/%d/ns/pid", source);
> source = open(buf, O_RDONLY);
> if (source < 0)
> err(2, "open source %s", buf);
> }
>
> if (target > 0) {
> snprintf(buf, sizeof(buf), "/proc/%d/ns/pid", target);
> target = open(buf, O_RDONLY);
> if (target < 0)
> err(2, "open target %s", buf);
> }
>
> pid = translate_pid(pid, source, target);
> if (pid < 0)
> err(2, "translate_pid");
>
> printf("%d\n", pid);
> return 0;
> }
>
> ---
> ---
> 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 14a2f996e543..e70685750d43 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -397,3 +397,4 @@
> 383 i386 statx sys_statx __ia32_sys_statx
> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> +386 i386 translate_pid sys_translate_pid __ia32_sys_translate_pid
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index cd36232ab62f..ebfd89055424 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -342,6 +342,7 @@
> 331 common pkey_free __x64_sys_pkey_free
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> +334 common translate_pid __x64_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 390e814fdc8d..3f33971cf1c8 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -843,6 +843,7 @@ asmlinkage long sys_clock_adjtime(clockid_t which_clock,
> struct timex __user *tx);
> asmlinkage long sys_syncfs(int fd);
> asmlinkage long sys_setns(int fd, int nstype);
> +asmlinkage long sys_translate_pid(pid_t pid, int source, int target);
> asmlinkage long sys_sendmmsg(int fd, struct mmsghdr __user *msg,
> unsigned int vlen, unsigned flags);
> asmlinkage long sys_process_vm_readv(pid_t pid,
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index 2a2ac53d8b8b..3b872cbbe264 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 06b4ccee0047..bf276e9ace9a 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -153,6 +153,9 @@ COND_SYSCALL_COMPAT(kexec_load);
> COND_SYSCALL(init_module);
> COND_SYSCALL(delete_module);
>
> +/* kernel/pid_namespace.c */
> +COND_SYSCALL(translate_pid);
> +
> /* kernel/posix-timers.c */
>
> /* kernel/printk.c */
Ping? I believe this patch has been in a limbo for almost a month. Any
comments or suggestions or Ack?
Thanks,
Nagarathnam.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Peter Zijlstra @ 2018-07-03 9:24 UTC (permalink / raw)
To: Heiko Carstens
Cc: Mathieu Desnoyers, Linus Torvalds, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas
In-Reply-To: <20180703091717.GK3704@osiris>
On Tue, Jul 03, 2018 at 11:17:17AM +0200, Heiko Carstens wrote:
> And to answer also your question: we don't use a regular load, since we
> would have to use 'sacf' construct surrounding the load instruction which
> would be much slower.
> We have something like that implemented for the futex atomic ops, and we
> could also implement something like that for this use case
> (e.g. get_user_atomic()), if really needed.
/me digs out the s390-PoO PDF and understands.. this is because of the
split user/kernel address space stuff.
Fair enough.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Peter Zijlstra @ 2018-07-03 9:21 UTC (permalink / raw)
To: Heiko Carstens
Cc: Mathieu Desnoyers, Linus Torvalds, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas
In-Reply-To: <20180703085546.GJ3704@osiris>
On Tue, Jul 03, 2018 at 10:55:46AM +0200, Heiko Carstens wrote:
> >
> > The problem is interrupts; we need interrupts on the CPU doing the store
> > to observe either the old or the new value, not a mix.
> >
> > If mvcos does not guarantee that, we're having problems. Is there a
> > reason get_user() cannot use a 'regular' load?
>
> Well, that's single instruction semantics. This is something we actually
> can guarantee, since the mvcos instruction itself won't be interrupted and
> copies all 1/2/4/8 bytes in a row.
>
> So we are talking about that single instructions are required and not
> atomic accesses?
rseq is strictly task local. So from that pov single-copy atomic and
single instruction semantics end up being very similar.
The most complicated scenario would be where we interrupt the task,
schedule it out, migrate it and resume execution on another CPU. In that
case the second CPU also needs to observe a 'whole' value.
But note that in that example there's a fair bit of ordering provided by
the scheduler to ensure all the state from the old CPU is observed by
the new CPU (on s390 just the rq->lock fiddling would imply a bunch of
general memory barriers).
So I think you're good... But yes, you raise an interresting point.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Heiko Carstens @ 2018-07-03 9:17 UTC (permalink / raw)
To: Peter Zijlstra, Mathieu Desnoyers, Linus Torvalds,
Andy Lutomirski, Thomas Gleixner, linux-kernel, linux-api,
Paul E. McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Ingo Molnar, H. Peter Anvin,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett
In-Reply-To: <20180703085546.GJ3704@osiris>
On Tue, Jul 03, 2018 at 10:55:46AM +0200, Heiko Carstens wrote:
> > > > We're piece-wise enabling rseq across architectures anyway, and when the
> > > > relevant maintains do this, they can have a look at their
> > > > {get,put}_user() implementations and fix them.
> > > >
> > > > If you rely on get_user(u64) working, that means microblaze is already
> > > > broken, but I suppose it already was, since their rseq enablement patch
> > > > is extremely dodgy. Michal?
> > >
> > > s390 uses the mvcos instruction to implement get_user(). That instruction
> > > is not defined to be atomic, but may copy bytes piecemeal.. I had the
> > > impression that the rseq fields are supposed to be updated within the
> > > context of a single thread (user + kernel space).
> > >
> > > However if another user space thread is allowed to do this as well, then
> > > the get_user() approach won't fly on s390.
> > >
> > > That leaves the question: does it even make sense for a thread to update
> > > the rseq structure of a different thread?
> >
> > The problem is interrupts; we need interrupts on the CPU doing the store
> > to observe either the old or the new value, not a mix.
> >
> > If mvcos does not guarantee that, we're having problems. Is there a
> > reason get_user() cannot use a 'regular' load?
>
> Well, that's single instruction semantics. This is something we actually
> can guarantee, since the mvcos instruction itself won't be interrupted and
> copies all 1/2/4/8 bytes in a row.
>
> So we are talking about that single instructions are required and not
> atomic accesses?
And to answer also your question: we don't use a regular load, since we
would have to use 'sacf' construct surrounding the load instruction which
would be much slower.
We have something like that implemented for the futex atomic ops, and we
could also implement something like that for this use case
(e.g. get_user_atomic()), if really needed.
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Heiko Carstens @ 2018-07-03 8:55 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mathieu Desnoyers, Linus Torvalds, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas
In-Reply-To: <20180703084312.GU2494@hirez.programming.kicks-ass.net>
> > > We're piece-wise enabling rseq across architectures anyway, and when the
> > > relevant maintains do this, they can have a look at their
> > > {get,put}_user() implementations and fix them.
> > >
> > > If you rely on get_user(u64) working, that means microblaze is already
> > > broken, but I suppose it already was, since their rseq enablement patch
> > > is extremely dodgy. Michal?
> >
> > s390 uses the mvcos instruction to implement get_user(). That instruction
> > is not defined to be atomic, but may copy bytes piecemeal.. I had the
> > impression that the rseq fields are supposed to be updated within the
> > context of a single thread (user + kernel space).
> >
> > However if another user space thread is allowed to do this as well, then
> > the get_user() approach won't fly on s390.
> >
> > That leaves the question: does it even make sense for a thread to update
> > the rseq structure of a different thread?
>
> The problem is interrupts; we need interrupts on the CPU doing the store
> to observe either the old or the new value, not a mix.
>
> If mvcos does not guarantee that, we're having problems. Is there a
> reason get_user() cannot use a 'regular' load?
Well, that's single instruction semantics. This is something we actually
can guarantee, since the mvcos instruction itself won't be interrupted and
copies all 1/2/4/8 bytes in a row.
So we are talking about that single instructions are required and not
atomic accesses?
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Peter Zijlstra @ 2018-07-03 8:43 UTC (permalink / raw)
To: Heiko Carstens
Cc: Mathieu Desnoyers, Linus Torvalds, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas
In-Reply-To: <20180703082955.GH3704@osiris>
On Tue, Jul 03, 2018 at 10:29:55AM +0200, Heiko Carstens wrote:
> On Tue, Jul 03, 2018 at 10:14:49AM +0200, Peter Zijlstra wrote:
> > On Mon, Jul 02, 2018 at 10:30:09PM -0400, Mathieu Desnoyers wrote:
> > > > Use "get_user()". It works for 64-bit objects too, and it will be
> > > > atomic in the 32-bit sub-parts on a 32-bit architecture.
> > >
> > > Is it really ? Last time we had this discussion, not all architectures
> > > guaranteed that reading a 64-bit integer would happen in two atomic
> > > 32-bit sub-parts. This was the main motivation for the LINUX_FIELD_u32_u64()
> > > macro as it stands today (rather than using a union).
> >
> > Just state, as a requirement for supporting rseq, that the arch
> > {get,put}_user(u64) on 32bit targets must be exactly 2 u32 loads/stores.
> >
> > We're piece-wise enabling rseq across architectures anyway, and when the
> > relevant maintains do this, they can have a look at their
> > {get,put}_user() implementations and fix them.
> >
> > If you rely on get_user(u64) working, that means microblaze is already
> > broken, but I suppose it already was, since their rseq enablement patch
> > is extremely dodgy. Michal?
>
> s390 uses the mvcos instruction to implement get_user(). That instruction
> is not defined to be atomic, but may copy bytes piecemeal.. I had the
> impression that the rseq fields are supposed to be updated within the
> context of a single thread (user + kernel space).
>
> However if another user space thread is allowed to do this as well, then
> the get_user() approach won't fly on s390.
>
> That leaves the question: does it even make sense for a thread to update
> the rseq structure of a different thread?
The problem is interrupts; we need interrupts on the CPU doing the store
to observe either the old or the new value, not a mix.
If mvcos does not guarantee that, we're having problems. Is there a
reason get_user() cannot use a 'regular' load?
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Heiko Carstens @ 2018-07-03 8:29 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mathieu Desnoyers, Linus Torvalds, Andy Lutomirski,
Thomas Gleixner, linux-kernel, linux-api, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas
In-Reply-To: <20180703081449.GT2494@hirez.programming.kicks-ass.net>
On Tue, Jul 03, 2018 at 10:14:49AM +0200, Peter Zijlstra wrote:
> On Mon, Jul 02, 2018 at 10:30:09PM -0400, Mathieu Desnoyers wrote:
> > > Use "get_user()". It works for 64-bit objects too, and it will be
> > > atomic in the 32-bit sub-parts on a 32-bit architecture.
> >
> > Is it really ? Last time we had this discussion, not all architectures
> > guaranteed that reading a 64-bit integer would happen in two atomic
> > 32-bit sub-parts. This was the main motivation for the LINUX_FIELD_u32_u64()
> > macro as it stands today (rather than using a union).
>
> Just state, as a requirement for supporting rseq, that the arch
> {get,put}_user(u64) on 32bit targets must be exactly 2 u32 loads/stores.
>
> We're piece-wise enabling rseq across architectures anyway, and when the
> relevant maintains do this, they can have a look at their
> {get,put}_user() implementations and fix them.
>
> If you rely on get_user(u64) working, that means microblaze is already
> broken, but I suppose it already was, since their rseq enablement patch
> is extremely dodgy. Michal?
s390 uses the mvcos instruction to implement get_user(). That instruction
is not defined to be atomic, but may copy bytes piecemeal.. I had the
impression that the rseq fields are supposed to be updated within the
context of a single thread (user + kernel space).
However if another user space thread is allowed to do this as well, then
the get_user() approach won't fly on s390.
That leaves the question: does it even make sense for a thread to update
the rseq structure of a different thread?
^ permalink raw reply
* Re: [RFC PATCH for 4.18] rseq: use __u64 for rseq_cs fields, validate user inputs
From: Peter Zijlstra @ 2018-07-03 8:14 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Linus Torvalds, Andy Lutomirski, Thomas Gleixner, linux-kernel,
linux-api, Paul E. McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Ingo Molnar, H. Peter Anvin,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Catalin Marinas, Will Deacon, Michael
In-Reply-To: <1776351430.10902.1530585009519.JavaMail.zimbra@efficios.com>
On Mon, Jul 02, 2018 at 10:30:09PM -0400, Mathieu Desnoyers wrote:
> > Use "get_user()". It works for 64-bit objects too, and it will be
> > atomic in the 32-bit sub-parts on a 32-bit architecture.
>
> Is it really ? Last time we had this discussion, not all architectures
> guaranteed that reading a 64-bit integer would happen in two atomic
> 32-bit sub-parts. This was the main motivation for the LINUX_FIELD_u32_u64()
> macro as it stands today (rather than using a union).
Just state, as a requirement for supporting rseq, that the arch
{get,put}_user(u64) on 32bit targets must be exactly 2 u32 loads/stores.
We're piece-wise enabling rseq across architectures anyway, and when the
relevant maintains do this, they can have a look at their
{get,put}_user() implementations and fix them.
If you rely on get_user(u64) working, that means microblaze is already
broken, but I suppose it already was, since their rseq enablement patch
is extremely dodgy. Michal?
^ permalink raw reply
* Re: [REGRESSION] "Locked" and "Pss" in /proc/*/smaps are the same
From: Vlastimil Babka @ 2018-07-03 7:45 UTC (permalink / raw)
To: Thomas Lindroth, dancol, Andrew Morton; +Cc: linux-api, linux-kernel, linux-mm
In-Reply-To: <ebf6c7fb-fec3-6a26-544f-710ed193c154@suse.cz>
On 07/03/2018 09:36 AM, Vlastimil Babka wrote:
> On 07/01/2018 08:31 PM, Thomas Lindroth wrote:
>> While looking around in /proc on my v4.14.52 system I noticed that
>> all processes got a lot of "Locked" memory in /proc/*/smaps. A lot
>> more memory than a regular user can usually lock with mlock().
>>
>> commit 493b0e9d945fa9dfe96be93ae41b4ca4b6fdb317 (v4.14-rc1) seems
>> to have changed the behavior of "Locked".
Oops, I forgot, thanks for the nice report :)
Vlastimil
^ permalink raw reply
* Re: [REGRESSION] "Locked" and "Pss" in /proc/*/smaps are the same
From: Vlastimil Babka @ 2018-07-03 7:36 UTC (permalink / raw)
To: Thomas Lindroth, dancol, Andrew Morton; +Cc: linux-api, linux-kernel, linux-mm
In-Reply-To: <69eb77f7-c8cc-fdee-b44f-ad7e522b8467@gmail.com>
+CC
On 07/01/2018 08:31 PM, Thomas Lindroth wrote:
> While looking around in /proc on my v4.14.52 system I noticed that
> all processes got a lot of "Locked" memory in /proc/*/smaps. A lot
> more memory than a regular user can usually lock with mlock().
>
> commit 493b0e9d945fa9dfe96be93ae41b4ca4b6fdb317 (v4.14-rc1) seems
> to have changed the behavior of "Locked".
>
> commit 493b0e9d945fa9dfe96be93ae41b4ca4b6fdb317
> Author: Daniel Colascione <dancol@google.com>
> Date: Wed Sep 6 16:25:08 2017 -0700
>
> mm: add /proc/pid/smaps_rollup
>
> Before that commit the code was like this. Notice the VM_LOCKED
> check.
>
> seq_printf(m,
> "Size: %8lu kB\n"
> "Rss: %8lu kB\n"
> "Pss: %8lu kB\n"
> "Shared_Clean: %8lu kB\n"
> "Shared_Dirty: %8lu kB\n"
> "Private_Clean: %8lu kB\n"
> "Private_Dirty: %8lu kB\n"
> "Referenced: %8lu kB\n"
> "Anonymous: %8lu kB\n"
> "LazyFree: %8lu kB\n"
> "AnonHugePages: %8lu kB\n"
> "ShmemPmdMapped: %8lu kB\n"
> "Shared_Hugetlb: %8lu kB\n"
> "Private_Hugetlb: %7lu kB\n"
> "Swap: %8lu kB\n"
> "SwapPss: %8lu kB\n"
> "KernelPageSize: %8lu kB\n"
> "MMUPageSize: %8lu kB\n"
> "Locked: %8lu kB\n",
> (vma->vm_end - vma->vm_start) >> 10,
> mss.resident >> 10,
> (unsigned long)(mss.pss >> (10 + PSS_SHIFT)),
> mss.shared_clean >> 10,
> mss.shared_dirty >> 10,
> mss.private_clean >> 10,
> mss.private_dirty >> 10,
> mss.referenced >> 10,
> mss.anonymous >> 10,
> mss.lazyfree >> 10,
> mss.anonymous_thp >> 10,
> mss.shmem_thp >> 10,
> mss.shared_hugetlb >> 10,
> mss.private_hugetlb >> 10,
> mss.swap >> 10,
> (unsigned long)(mss.swap_pss >> (10 + PSS_SHIFT)),
> vma_kernel_pagesize(vma) >> 10,
> vma_mmu_pagesize(vma) >> 10,
> (vma->vm_flags & VM_LOCKED) ?
> (unsigned long)(mss.pss >> (10 + PSS_SHIFT)) : 0);
>
> After that commit Locked is now the same as Pss. This looks like a
> mistake.
>
> seq_printf(m,
> "Rss: %8lu kB\n"
> "Pss: %8lu kB\n"
> "Shared_Clean: %8lu kB\n"
> "Shared_Dirty: %8lu kB\n"
> "Private_Clean: %8lu kB\n"
> "Private_Dirty: %8lu kB\n"
> "Referenced: %8lu kB\n"
> "Anonymous: %8lu kB\n"
> "LazyFree: %8lu kB\n"
> "AnonHugePages: %8lu kB\n"
> "ShmemPmdMapped: %8lu kB\n"
> "Shared_Hugetlb: %8lu kB\n"
> "Private_Hugetlb: %7lu kB\n"
> "Swap: %8lu kB\n"
> "SwapPss: %8lu kB\n"
> "Locked: %8lu kB\n",
> mss->resident >> 10,
> (unsigned long)(mss->pss >> (10 + PSS_SHIFT)),
> mss->shared_clean >> 10,
> mss->shared_dirty >> 10,
> mss->private_clean >> 10,
> mss->private_dirty >> 10,
> mss->referenced >> 10,
> mss->anonymous >> 10,
> mss->lazyfree >> 10,
> mss->anonymous_thp >> 10,
> mss->shmem_thp >> 10,
> mss->shared_hugetlb >> 10,
> mss->private_hugetlb >> 10,
> mss->swap >> 10,
> (unsigned long)(mss->swap_pss >> (10 + PSS_SHIFT)),
> (unsigned long)(mss->pss >> (10 + PSS_SHIFT)));
>
> The latest git has changed a bit but the functionality is the
> same.
----8<----
>From fa721521c981167c24ac8f4be446443d293d741e Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Tue, 3 Jul 2018 09:24:27 +0200
Subject: [PATCH] mm: fix Locked field in /proc/pid/smaps*
Thomas reports:
: While looking around in /proc on my v4.14.52 system I noticed that
: all processes got a lot of "Locked" memory in /proc/*/smaps. A lot
: more memory than a regular user can usually lock with mlock().
:
: commit 493b0e9d945fa9dfe96be93ae41b4ca4b6fdb317 (v4.14-rc1) seems
: to have changed the behavior of "Locked".
:
: Before that commit the code was like this. Notice the VM_LOCKED
: check.
:
: (vma->vm_flags & VM_LOCKED) ?
: (unsigned long)(mss.pss >> (10 + PSS_SHIFT)) : 0);
:
: After that commit Locked is now the same as Pss. This looks like a
: mistake.
:
: (unsigned long)(mss->pss >> (10 + PSS_SHIFT)));
Indeed, the commit has added mss->pss_locked with the correct value that
depends on VM_LOCKED, but forgot to actually use it. Fix it.
Fixes: 493b0e9d945f ("mm: add /proc/pid/smaps_rollup")
Reported-by: Thomas Lindroth <thomas.lindroth@gmail.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: stable@vger.kernel.org
---
fs/proc/task_mmu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e9679016271f..dfd73a4616ce 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -831,7 +831,8 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
SEQ_PUT_DEC(" kB\nSwap: ", mss->swap);
SEQ_PUT_DEC(" kB\nSwapPss: ",
mss->swap_pss >> PSS_SHIFT);
- SEQ_PUT_DEC(" kB\nLocked: ", mss->pss >> PSS_SHIFT);
+ SEQ_PUT_DEC(" kB\nLocked: ",
+ mss->pss_locked >> PSS_SHIFT);
seq_puts(m, " kB\n");
}
if (!rollup_mode) {
--
2.18.0
^ permalink raw reply related
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