* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Michal Hocko @ 2018-05-04 11:12 UTC (permalink / raw)
To: prakash.sangappa
Cc: Christopher Lameter, linux-kernel, linux-mm, linux-api, akpm,
kirill.shutemov, n-horiguchi, drepper, rientjes
In-Reply-To: <c80ee329-084b-367f-1937-3175c178e978@oracle.com>
On Thu 03-05-18 15:39:49, prakash.sangappa wrote:
>
>
> On 05/03/2018 11:03 AM, Christopher Lameter wrote:
> > On Tue, 1 May 2018, Prakash Sangappa wrote:
> >
> > > For analysis purpose it is useful to have numa node information
> > > corresponding mapped address ranges of the process. Currently
> > > /proc/<pid>/numa_maps provides list of numa nodes from where pages are
> > > allocated per VMA of the process. This is not useful if an user needs to
> > > determine which numa node the mapped pages are allocated from for a
> > > particular address range. It would have helped if the numa node information
> > > presented in /proc/<pid>/numa_maps was broken down by VA ranges showing the
> > > exact numa node from where the pages have been allocated.
> > Cant you write a small script that scans the information in numa_maps and
> > then displays the total pages per NUMA node and then a list of which
> > ranges have how many pages on a particular node?
>
> Don't think we can determine which numa node a given user process
> address range has pages from, based on the existing 'numa_maps' file.
yes we have. See move_pages...
> > > reading this file will not be restricted(i.e requiring CAP_SYS_ADMIN).
> > So a prime motivator here is security restricted access to numa_maps?
> No it is the opposite. A regular user should be able to determine
> numa node information.
Well, that breaks the layout randomization, doesn't it?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH for 4.18 12/23] cpu_opv: Provide cpu_opv system call (v7)
From: Mathieu Desnoyers @ 2018-05-04 14:32 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon
In-Reply-To: <1248652824.11527.1523912317964.JavaMail.zimbra@efficios.com>
----- On Apr 16, 2018, at 4:58 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> ----- On Apr 16, 2018, at 3:26 PM, Linus Torvalds torvalds@linux-foundation.org
> wrote:
>
>> On Mon, Apr 16, 2018 at 12:21 PM, Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>>>
>>> And I try very hard to avoid being told I'm the one breaking
>>> user-space. ;-)
>>
>> You *can't* be breaking user space. User space doesn't use this yet.
>>
>> That's actually why I'd like to start with the minimal set - to make
>> sure we don't introduce features that will come back to bite us later.
>>
>> The one compelling use case I saw was a memory allocator that used
>> this for getting per-CPU (vs per-thread) memory scaling.
>>
>> That code didn't need the cpu_opv system call at all.
>>
>> And if somebody does a ldload of a malloc library, and then wants to
>> analyze the behavior of a program, maybe they should ldload their own
>> malloc routines first? That's pretty much par for the course for those
>> kinds of projects.
>>
>> So I'd much rather we first merge the non-contentious parts that
>> actually have some numbers for "this improves performance and makes a
>> nice fancy malloc possible".
>>
>> As it is, the cpu_opv seems to be all about theory, not about actual need.
>
> I fully get your point about getting the minimal feature in. So let's focus
> on rseq only.
>
> I will rework the patchset so the rseq selftests don't depend on cpu_opv,
> and remove the cpu_opv stuff. I think it would be a good start for the
> Facebook guys (jemalloc), given that just rseq seems to be enough for them
> for now. It should be enough for the arm64 performance counters as well.
>
> Then we'll figure out what is needed to make other projects use it based on
> their needs (e.g. lttng-ust, liburcu, glibc malloc), and whether jemalloc
> end up requiring cpu_opv for memory migration between per-cpu pools after all.
So, having done this, I find myself in need of advice regarding smoothly
transitioning existing user-space programs/libraries to rseq. Let's consider
a situation where only rseq (without cpu_opv) eventually gets merged into
4.18.
The proposed rseq implementation presents the following constraints:
- Only a single rseq TLS can be registered per thread, therefore rseq needs
to be "owned" by a single library (let's say it's librseq.so),
- User-space rseq critical sections need to be inlined into applications and
libraries for performance reasons (extra branches and calls significantly
degrade performance of those fast-paths).
I have a ring buffer "space reservation" use-case in my user-space tracer
which requires both rseq and cpu_opv.
My original plan to transition this fast-path to rseq was to test the
@cpu_id field value from the rseq TLS and use a fallback based on
atomic instructions if it is negative. rseq is already designed to ensure
we can compare @cpu_id against @cpu_id_start and detect both migration
(cpu id differs) and rseq ENOSYS with a single branch in the fast path.
Once rseq gets merged and deployed into kernels, this means librseq.so
will actually populate the rseq TLS, and this @cpu_id field will be >= 0.
If kernels are released with rseq but without cpu_opv, then I cannot use
this @cpu_id field to detect whether *both* rseq and cpu_opv are available.
I see a few possible ways to handle this, none of which are particularly
great:
1) Duplicate the entire implementation of the user-space functions where
the rseq critical sections are inlined, and dynamically detect whether
cpu_opv is available, and select the right function at runtime. If those
functions are relatively small this could be acceptable,
2) Code patching based on asm goto. There is no user-space library for
this at the moment AFAIK, and patching user-space code triggers COW,
which is bad for TLB and cache locality,
3) Add an extra branch in the rseq fast-path. I would like to avoid this
especially on arm32, where the cost of an extra branch is significant
enough to outweigh the benefit of rseq compared to ll/sc.
So far, only option (1) seems relatively acceptable from my perspective,
but that's only because my functions using rseq are relatively small.
If this code bloat is not seen as acceptable, then we should revisit
merging both rseq and cpu_opv at the same time, and make sure CONFIG_RSEQ
selects CONFIG_CPU_OPV.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Christopher Lameter @ 2018-05-04 14:57 UTC (permalink / raw)
To: prakash.sangappa
Cc: linux-kernel, linux-mm, linux-api, akpm, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes
In-Reply-To: <c80ee329-084b-367f-1937-3175c178e978@oracle.com>
On Thu, 3 May 2018, prakash.sangappa wrote:
> > > exact numa node from where the pages have been allocated.
> > Cant you write a small script that scans the information in numa_maps and
> > then displays the total pages per NUMA node and then a list of which
> > ranges have how many pages on a particular node?
>
> Don't think we can determine which numa node a given user process
> address range has pages from, based on the existing 'numa_maps' file.
Well the information is contained in numa_maps I thought. What is missing?
> > > reading this file will not be restricted(i.e requiring CAP_SYS_ADMIN).
> > So a prime motivator here is security restricted access to numa_maps?
> No it is the opposite. A regular user should be able to determine
> numa node information.
That used to be the case until changes were made to the permissions for
reading numa_maps.
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Prakash Sangappa @ 2018-05-04 16:18 UTC (permalink / raw)
To: Michal Hocko
Cc: Christopher Lameter, linux-kernel, linux-mm, linux-api, akpm,
kirill.shutemov, n-horiguchi, drepper, rientjes
In-Reply-To: <20180504111211.GO4535@dhcp22.suse.cz>
On 5/4/18 4:12 AM, Michal Hocko wrote:
> On Thu 03-05-18 15:39:49, prakash.sangappa wrote:
>>
>> On 05/03/2018 11:03 AM, Christopher Lameter wrote:
>>> On Tue, 1 May 2018, Prakash Sangappa wrote:
>>>
>>>> For analysis purpose it is useful to have numa node information
>>>> corresponding mapped address ranges of the process. Currently
>>>> /proc/<pid>/numa_maps provides list of numa nodes from where pages are
>>>> allocated per VMA of the process. This is not useful if an user needs to
>>>> determine which numa node the mapped pages are allocated from for a
>>>> particular address range. It would have helped if the numa node information
>>>> presented in /proc/<pid>/numa_maps was broken down by VA ranges showing the
>>>> exact numa node from where the pages have been allocated.
>>> Cant you write a small script that scans the information in numa_maps and
>>> then displays the total pages per NUMA node and then a list of which
>>> ranges have how many pages on a particular node?
>> Don't think we can determine which numa node a given user process
>> address range has pages from, based on the existing 'numa_maps' file.
> yes we have. See move_pages...
Sure using move_pages, not based on just 'numa_maps'.
>
>>>> reading this file will not be restricted(i.e requiring CAP_SYS_ADMIN).
>>> So a prime motivator here is security restricted access to numa_maps?
>> No it is the opposite. A regular user should be able to determine
>> numa node information.
> Well, that breaks the layout randomization, doesn't it?
Exposing numa node information itself should not break randomization right?
It would be upto the application. In case of randomization, the application
could generate address range traces of interest for debugging and then
using numa node information one could determine where the memory is laid
out for analysis.
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Prakash Sangappa @ 2018-05-04 16:27 UTC (permalink / raw)
To: Christopher Lameter
Cc: linux-kernel, linux-mm, linux-api, akpm, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes
In-Reply-To: <alpine.DEB.2.21.1805040955550.10847@nuc-kabylake>
On 5/4/18 7:57 AM, Christopher Lameter wrote:
> On Thu, 3 May 2018, prakash.sangappa wrote:
>
>>>> exact numa node from where the pages have been allocated.
>>> Cant you write a small script that scans the information in numa_maps and
>>> then displays the total pages per NUMA node and then a list of which
>>> ranges have how many pages on a particular node?
>> Don't think we can determine which numa node a given user process
>> address range has pages from, based on the existing 'numa_maps' file.
> Well the information is contained in numa_maps I thought. What is missing?
Currently 'numa_maps' gives a list of numa nodes, memory is allocated per
VMA.
Ex. we get something like from numa_maps.
04000 N0=1,N2=2 kernelpagesize_KB=4
First is the start address of a VMA. This VMA could be much larger then
3 4k pages.
It does not say which address in the VMA has the pages mapped.
>
>>>> reading this file will not be restricted(i.e requiring CAP_SYS_ADMIN).
>>> So a prime motivator here is security restricted access to numa_maps?
>> No it is the opposite. A regular user should be able to determine
>> numa node information.
> That used to be the case until changes were made to the permissions for
> reading numa_maps.
>
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/14] Restartable Sequences
From: Ben Maurer @ 2018-05-04 22:17 UTC (permalink / raw)
To: Joel Fernandes, Daniel Colascione
Cc: Mathieu Desnoyers, Peter Zijlstra, Paul McKenney, Boqun Feng,
Andy Lutomirski, Dave Watson, LKML, linux-api@vger.kernel.org,
Paul Turner, Andrew Morton, linux@arm.linux.org.uk,
Thomas Gleixner, Ingo Molnar, hpa@zytor.com, Andrew Hunter,
andi@firstfloor.org, cl@linux.com, Steven Rostedt, Josh Triplett
In-Reply-To: <CAJWu+opkzpmwY5jJ0353JKyaOFCqa95AKGhxUdQZ6pZf_omsaQ@mail.gmail.com>
Hey -
I think the ideas Daniel brings up here are interesting -- specifically the notion that a thread could set a "pre-sleep wish" to signal it's sleeping. As this conversation shows I think there's a fair bit of depth to that. For example, the FUTEX_LOCK is an alternative approach. Another idea might be using the "currently running cpu" area of rseq to tell if the process in question was sleeping (assuming that the kernel would be modified to set this to -1 every time a process was unscheduled)
The idea discussed here seems orthogonal to the core thesis of rseq. I'm wondering if we can focus on getting rseq in, maybe with a eye for making sure this use case could be handled long term. My sense is that this is possible. We could use the flags setting in the per-thread rseq area, or maybe extend the meaning of the structure rseq_cs points to to signal that there was information about how to signal the sleeping of the current process. It seems to me this would be a natural way to add the functionality Daniel talks about if desired in the future.
Daniel - do you think there's anything we should do in the patch set today that would make it easier to implement your idea in the future without expanding the scope of the patch today. i.e. is there anything else we need to do to lay the framework for your idea.
I'd really love to see us get this patch in. There's a whole host of primitives this unlocks (more efficient RCU, better malloc implementations, fast reader-writer locks). I'm sure we'll have more ideas about APIs to provide once we've explored these use cases, but to me this patch is the MVP we need to ship to get that feedback. It's a solid framework that we can build upon, eg with the "opv" syscall or the idea in this thread if user feedback shows those things are necessary.
-b
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/14] Restartable Sequences
From: Thomas Gleixner @ 2018-05-06 10:03 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Daniel Colascione, Mathieu Desnoyers, Paul McKenney, boqun.feng,
luto, davejwatson, linux-kernel, linux-api, Paul Turner,
Andrew Morton, linux, mingo, hpa, Andrew Hunter, andi, cl,
bmaurer, rostedt, josh, torvalds, catalin.marinas, will.deacon,
Michael Kerrisk-manpages, Joel Fernandes
In-Reply-To: <20180502202233.GV12217@hirez.programming.kicks-ass.net>
On Wed, 2 May 2018, Peter Zijlstra wrote:
> On Wed, May 02, 2018 at 06:27:22PM +0000, Daniel Colascione wrote:
> > On Wed, May 2, 2018 at 10:22 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >> On Wed, May 02, 2018 at 03:53:47AM +0000, Daniel Colascione wrote:
> > > > Suppose we make a userspace mutex implemented with a lock word having
> > three
> > > > bits: acquired, sleep_mode, and wait_pending, with the rest of the word
> > not
> > > > being relevant at the moment.
> >
> > > So ideally we'd kill FUTEX_WAIT/FUTEX_WAKE for mutexes entirely, and go
> > > with FUTEX_LOCK/FUTEX_UNLOCK that have the same semantics as the
> > > existing FUTEX_LOCK_PI/FUTEX_UNLOCK_PI, namely, the word contains the
> > > owner TID.
> >
> > That doesn't work if you want to use the rest of the word for something
> > else, like a recursion count. With FUTEX_WAIT and FUTEX_WAKE, you can make
> > a lock with two bits.
>
> Recursive locks are teh most horrible crap ever. And having the tid in
> the word allows things like kernel based optimistic spins and possibly
> PI related things.
FWIW, robust futex have also the TID requirement.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Richard Guy Briggs @ 2018-05-06 16:51 UTC (permalink / raw)
To: Paul Moore
Cc: simo, jlayton, carlos, linux-api, containers, LKML, Eric Paris,
dhowells, Linux-Audit Mailing List, ebiederm, luto, netdev,
linux-fsdevel, cgroups, serge, viro
In-Reply-To: <CAHC9VhTyvxxj2e2Gn+iyW6iLLeYB7hp8a+JvfeMmJ2nUPqtEaw@mail.gmail.com>
On 2018-04-18 19:47, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Implement the proc fs write to set the audit container ID of a process,
> > emitting an AUDIT_CONTAINER record to document the event.
> >
> > This is a write from the container orchestrator task to a proc entry of
> > the form /proc/PID/containerid where PID is the process ID of the newly
> > created task that is to become the first task in a container, or an
> > additional task added to a container.
> >
> > The write expects up to a u64 value (unset: 18446744073709551615).
> >
> > This will produce a record such as this:
> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
> >
> > The "op" field indicates an initial set. The "pid" to "ses" fields are
> > the orchestrator while the "opid" field is the object's PID, the process
> > being "contained". Old and new container ID values are given in the
> > "contid" fields, while res indicates its success.
> >
> > It is not permitted to self-set, unset or re-set the container ID. A
> > child inherits its parent's container ID, but then can be set only once
> > after.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > fs/proc/base.c | 37 ++++++++++++++++++++
> > include/linux/audit.h | 16 +++++++++
> > include/linux/init_task.h | 4 ++-
> > include/linux/sched.h | 1 +
> > include/uapi/linux/audit.h | 2 ++
> > kernel/auditsc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++
> > 6 files changed, 143 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 60316b5..6ce4fbe 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
> > .read = proc_sessionid_read,
> > .llseek = generic_file_llseek,
> > };
> > +
> > +static ssize_t proc_containerid_write(struct file *file, const char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct inode *inode = file_inode(file);
> > + u64 containerid;
> > + int rv;
> > + struct task_struct *task = get_proc_task(inode);
> > +
> > + if (!task)
> > + return -ESRCH;
> > + if (*ppos != 0) {
> > + /* No partial writes. */
> > + put_task_struct(task);
> > + return -EINVAL;
> > + }
> > +
> > + rv = kstrtou64_from_user(buf, count, 10, &containerid);
> > + if (rv < 0) {
> > + put_task_struct(task);
> > + return rv;
> > + }
> > +
> > + rv = audit_set_containerid(task, containerid);
> > + put_task_struct(task);
> > + if (rv < 0)
> > + return rv;
> > + return count;
> > +}
> > +
> > +static const struct file_operations proc_containerid_operations = {
> > + .write = proc_containerid_write,
> > + .llseek = generic_file_llseek,
> > +};
> > +
> > #endif
> >
> > #ifdef CONFIG_FAULT_INJECTION
> > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file *m, struct pid_namespace *ns,
> > #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > + REG("containerid", S_IWUSR, proc_containerid_operations),
> > #endif
> > #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > @@ -3355,6 +3391,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
> > #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > + REG("containerid", S_IWUSR, proc_containerid_operations),
> > #endif
> > #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
...
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index d258826..1b82191 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -796,6 +796,7 @@ struct task_struct {
> > #ifdef CONFIG_AUDITSYSCALL
> > kuid_t loginuid;
> > unsigned int sessionid;
> > + u64 containerid;
>
> This one line addition to the task_struct scares me the most of
> anything in this patchset. Why? It's a field named "containerid" in
> a perhaps one of the most widely used core kernel structures; the
> possibilities for abuse are endless, and it's foolish to think we
> would ever be able to adequately police this.
>
> Unfortunately, we can't add the field to audit_context as things
> currently stand because we don't always allocate an audit_context,
> it's dependent on the system's configuration, and we need to track the
> audit container ID for a given process, regardless of the audit
> configuration. Pretty much the same reason why loginuid and sessionid
> are located directly in task_struct now. As I stressed during the
> design phase, I really want to keep this as an *audit* container ID
> and not a general purpose kernel wide container ID. If the kernel
> ever grows a general purpose container ID token, I'll be the first in
> line to convert the audit code, but I don't want audit to be that
> general purpose mechanism ... audit is hated enough as-is ;)
>
> I think the right solution to this is to create another new struct,
> audit_task_info (or similar, the name really isn't that important),
> which would be stored as a pointer in task_struct and would replace
> the audit_context pointer, loginuid, sessionid, and the newly proposed
> containerid. The new audit_task_info would always be allocated in the
> audit_alloc() function (please use kmem_cache), and the audit_context
> pointer included inside would continue to be allocated based on the
> existing conditions. By keeping audit_task_info as a pointer inside
> task_struct we could hide the structure definition inside
> kernel/audit*.c and make it much more difficult for other subsystems
> to abuse it.[1]
>
> struct audit_task_info {
> kuid_t loginuid;
> unsigned int sessionid;
> u64 containerid;
> struct audit_context *ctx;
> }
>
> Actually, we might even want to consider storing audit_context in
> audit_task_info (no pointer), or making it a zero length array
> (ctx[0]) and going with a variable sized allocation of audit_task_info
> ... but all that could be done as a follow up optimization once we get
> the basic idea sorted.
I tried statically allocating struct audit_task_info (with a pointer to
struct audit_context) in addition to dynamically allocating struct
audit_task_info due to a bug I'd introduced while dynamically allocating
audit_task_info, so I now have proof-of-concepts for working static and
almost working dynamic allocated struct audit_task_info.
Statically allocating it required a new header file, so I'm not that
crazy about it, but it proved it works.
Dynamically allocating it isn't quite as clean as was hoped since
init/init_task.c still needs initializaiton values for loginuid and
sessionid which could be supplied by a statically allocated struct
audit_task_info and still needs to know the internals of that struct to
do so. Dynamic allocation is also more disruptive initially, but in the
long run will be more stable to the rest of the kernel.
I'm not crazy about the idea of dynamically (or even statically)
allocating struct audit_task_info which includes allocated space for
struct audit_context since the latter is far larger than the former.
> [1] If for some reason allocating audit_task_info becomes too much
> overhead to bear (somewhat doubtful since we would only do it at task
> creation), we could do some ugly tricks to directly include an
> audit_task_struct chunk in task_struct but I'd like to avoid that if
> possible (and I think we can).
On allocation, I don't see too much of a problem. When calling
audit_free() if there is no audit context it is pretty lightweight, but
gets heavier if we eliminate the inline audit_free() and rename
__audit_free() back to audit_free(). Having struct audit_task_info
directly in struct task_struct would be faster and also allow defaults
to be set in init/init_task.c (which has recently been populated from
include/linux/init_task.h). I'm not sure this is enough of a reason to
avoid a pointer from task_struct.
(As an aside, converting allocation of audit_context could also benefit
from kmem_cache... and maybe even struct audit_names)
> > #endif
> > struct seccomp seccomp;
>
> ...
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* [PATCH 0/6 v1] statfs: handle mount propagation
From: Christian Brauner @ 2018-05-06 22:46 UTC (permalink / raw)
To: viro, linux-fsdevel, linux-kernel
Cc: torvalds, tglx, kstewart, gregkh, pombredanne, linux-api,
Christian Brauner
Hey,
This is v1 of this patchset. All changes from v0 to v1 are non-functional.
Specifically, the commit messages and justification have been extended as
requested by Linus and Al.
This little series does the following:
- unify the definition of constants in statfs.h and fs.h:
The definitions for MS_* flags are currently a mixture between hex values
and bit-shifts. All higher values are already initialized with bit-shifts
for MS_* constants starting with (1<<16). This patch switches the
definitions for MS_* constants over to uniformly use bit-shifts and
alignes the definitions of ST_* flags too.
Initializing them identically let's userspace easily detect when flags
indicate the same property but use a different value in doing so.
- extend statfs to handle mount propagation:
For all cases the only way to do this right now is by parsing
/proc/<pid>/mountinfo. Yes, it is doable but still it is somewhat costly
and annoying as e.g. those mount propagation fields are optional.
1. prevent propagation from happening:
From a userspace perspective we often run into the case where we
simply want to know whether a given mountpoint is MS_SHARED or is
MS_SLAVE. If it is we remount it as MS_PRIVATE to prevent any
propagation from happening. We don't care about the peer
relationship or how the propagation is exactly setup. We only want
to prevent any propagation from happening.
These mountpoints might be known in advance so parsing
/proc/<pid>/mountinfo should not be needed.
2. differentiate between MS_SLAVE and MS_SHARED mountpoints:
Mountpoints that are MS_SLAVE are kept intact and mountpoints that
are MS_SHARED are made MS_PRIVATE. These mountpoint might be known in
advance so parsing /proc/<pid>/mountinfo should not be needed.
3. retrieve propagation properties when procfs is not mounted:
When the set of interesting mountpoints is known and /proc is not
mounted calling statfs() is the only good way to reliably determine
the propagation property of a mountpoint.
4. inspecting file descriptors to mountpoints for propagation
properties:
When file descriptors to mountpoints are passed around between
processes it is useful to have fstatvfs() handle mount propagation
properties too.
To this end the flags:
- ST_UNBINDABLE
- ST_SHARED
- ST_PRIVATE
- ST_SLAVE
are added. They have the same value as their MS_* counterparts.
- Testing:
I verified that now userspace can do e.g.
int ret;
char *s = "/some/path";
struct statvfs sb;
ret = statvfs(s, &sb);
if (ret < 0)
return false;
if (sb.f_flag & ST_SHARED) {
ret = mount("", s, NULL, MS_SLAVE | MS_REC, NULL);
if (ret < 0)
return -1;
}
Thanks!
Christian
Christian Brauner (6):
fs: use << for MS_* flags
statfs: use << to align with fs header
statfs: add ST_UNBINDABLE
statfs: add ST_SHARED
statfs: add ST_SLAVE
statfs: add ST_PRIVATE
fs/statfs.c | 16 +++++++++++++++-
include/linux/statfs.h | 30 +++++++++++++++++-------------
include/uapi/linux/fs.h | 33 +++++++++++++++++----------------
3 files changed, 49 insertions(+), 30 deletions(-)
--
2.17.0
^ permalink raw reply
* [PATCH 1/6 v1] fs: use << for MS_* flags
From: Christian Brauner @ 2018-05-06 22:46 UTC (permalink / raw)
To: viro, linux-fsdevel, linux-kernel
Cc: torvalds, tglx, kstewart, gregkh, pombredanne, linux-api,
Christian Brauner
In-Reply-To: <20180506224704.14669-1-christian.brauner@ubuntu.com>
The definitions for MS_* flags are currently a mixture between hex values
and bit-shifts. All higher values are already initialized with bit-shifts
for MS_* constants starting with (1<<16).
This patch switches the definitions for MS_* constants over to uniformly
use bit-shifts.
Note that the BIT() macro cannot be used as it is not exported to uapi
files as was pointed out by gregkh.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
v0->v1:
* non-functional changes: extend commit message
---
include/uapi/linux/fs.h | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index d2a8313fabd7..9662790a657c 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -105,22 +105,23 @@ struct inodes_stat_t {
/*
* These are the fs-independent mount-flags: up to 32 flags are supported
*/
-#define MS_RDONLY 1 /* Mount read-only */
-#define MS_NOSUID 2 /* Ignore suid and sgid bits */
-#define MS_NODEV 4 /* Disallow access to device special files */
-#define MS_NOEXEC 8 /* Disallow program execution */
-#define MS_SYNCHRONOUS 16 /* Writes are synced at once */
-#define MS_REMOUNT 32 /* Alter flags of a mounted FS */
-#define MS_MANDLOCK 64 /* Allow mandatory locks on an FS */
-#define MS_DIRSYNC 128 /* Directory modifications are synchronous */
-#define MS_NOATIME 1024 /* Do not update access times. */
-#define MS_NODIRATIME 2048 /* Do not update directory access times */
-#define MS_BIND 4096
-#define MS_MOVE 8192
-#define MS_REC 16384
-#define MS_VERBOSE 32768 /* War is peace. Verbosity is silence.
- MS_VERBOSE is deprecated. */
-#define MS_SILENT 32768
+#define MS_RDONLY (1<<0) /* Mount read-only */
+#define MS_NOSUID (1<<1) /* Ignore suid and sgid bits */
+#define MS_NODEV (1<<2) /* Disallow access to device special files */
+#define MS_NOEXEC (1<<3) /* Disallow program execution */
+#define MS_SYNCHRONOUS (1<<4) /* Writes are synced at once */
+#define MS_REMOUNT (1<<5) /* Alter flags of a mounted FS */
+#define MS_MANDLOCK (1<<6) /* Allow mandatory locks on an FS */
+#define MS_DIRSYNC (1<<7) /* Directory modifications are synchronous */
+#define MS_NOATIME (1<<10) /* Do not update access times. */
+#define MS_NODIRATIME (1<<11) /* Do not update directory access times */
+#define MS_BIND (1<<12)
+#define MS_MOVE (1<<13)
+#define MS_REC (1<<14)
+#define MS_VERBOSE (1<<15) /* War is peace. Verbosity is silence.
+ * MS_VERBOSE is deprecated.
+ */
+#define MS_SILENT (1<<15)
#define MS_POSIXACL (1<<16) /* VFS does not apply the umask */
#define MS_UNBINDABLE (1<<17) /* change to unbindable */
#define MS_PRIVATE (1<<18) /* change to private */
--
2.17.0
^ permalink raw reply related
* [PATCH 2/6 v1] statfs: use << to align with fs header
From: Christian Brauner @ 2018-05-06 22:47 UTC (permalink / raw)
To: viro, linux-fsdevel, linux-kernel
Cc: torvalds, tglx, kstewart, gregkh, pombredanne, linux-api,
Christian Brauner
In-Reply-To: <20180506224704.14669-1-christian.brauner@ubuntu.com>
After switching to using bit-shifts to define MS_* flags switch over ST_*
flags too. ST_* and MS_* flags generally have the exact same value.
Initializing them identically let's userspace easily detect when flags
indicate the same property but use a different value in doing so.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
v0->v1:
* non-functional changes: extend commit message
---
include/linux/statfs.h | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/include/linux/statfs.h b/include/linux/statfs.h
index 3142e98546ac..b336c04e793c 100644
--- a/include/linux/statfs.h
+++ b/include/linux/statfs.h
@@ -27,18 +27,18 @@ struct kstatfs {
* ABI. The exception is ST_VALID which has the same value as MS_REMOUNT
* which doesn't make any sense for statfs.
*/
-#define ST_RDONLY 0x0001 /* mount read-only */
-#define ST_NOSUID 0x0002 /* ignore suid and sgid bits */
-#define ST_NODEV 0x0004 /* disallow access to device special files */
-#define ST_NOEXEC 0x0008 /* disallow program execution */
-#define ST_SYNCHRONOUS 0x0010 /* writes are synced at once */
-#define ST_VALID 0x0020 /* f_flags support is implemented */
-#define ST_MANDLOCK 0x0040 /* allow mandatory locks on an FS */
-/* 0x0080 used for ST_WRITE in glibc */
-/* 0x0100 used for ST_APPEND in glibc */
-/* 0x0200 used for ST_IMMUTABLE in glibc */
-#define ST_NOATIME 0x0400 /* do not update access times */
-#define ST_NODIRATIME 0x0800 /* do not update directory access times */
-#define ST_RELATIME 0x1000 /* update atime relative to mtime/ctime */
+#define ST_RDONLY (1<<0) /* mount read-only */
+#define ST_NOSUID (1<<1) /* ignore suid and sgid bits */
+#define ST_NODEV (1<<2) /* disallow access to device special files */
+#define ST_NOEXEC (1<<3) /* disallow program execution */
+#define ST_SYNCHRONOUS (1<<4) /* writes are synced at once */
+#define ST_VALID (1<<5) /* f_flags support is implemented */
+#define ST_MANDLOCK (1<<6) /* allow mandatory locks on an FS */
+/* (1<<7) used for ST_WRITE in glibc */
+/* (1<<8) used for ST_APPEND in glibc */
+/* (1<<9) used for ST_IMMUTABLE in glibc */
+#define ST_NOATIME (1<<10) /* do not update access times */
+#define ST_NODIRATIME (1<<11) /* do not update directory access times */
+#define ST_RELATIME (1<<12) /* update atime relative to mtime/ctime */
#endif
--
2.17.0
^ permalink raw reply related
* [PATCH 3/6 v1] statfs: add ST_UNBINDABLE
From: Christian Brauner @ 2018-05-06 22:47 UTC (permalink / raw)
To: viro, linux-fsdevel, linux-kernel
Cc: torvalds, tglx, kstewart, gregkh, pombredanne, linux-api,
Christian Brauner
In-Reply-To: <20180506224704.14669-1-christian.brauner@ubuntu.com>
Currently userspace can only determine whether a mountpoint is unbindable
by parsing /proc/<pid>/mountinfo. It would be convenient to simply retrieve
this property with a statvfs() call.
This let's userspace avoid costly parsing, supports cases where /proc is
not mounted, and supports usecases where file descriptors to mountpoints
are passed between processes that are inspected via fstatvfs().
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
v0->v1:
* non-functional changes: extend commit message
---
fs/statfs.c | 2 ++
include/linux/statfs.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/fs/statfs.c b/fs/statfs.c
index 5b2a24f0f263..61b3063d3921 100644
--- a/fs/statfs.c
+++ b/fs/statfs.c
@@ -29,6 +29,8 @@ static int flags_by_mnt(int mnt_flags)
flags |= ST_NODIRATIME;
if (mnt_flags & MNT_RELATIME)
flags |= ST_RELATIME;
+ if (mnt_flags & MNT_UNBINDABLE)
+ flags |= ST_UNBINDABLE;
return flags;
}
diff --git a/include/linux/statfs.h b/include/linux/statfs.h
index b336c04e793c..e1b84d0388c1 100644
--- a/include/linux/statfs.h
+++ b/include/linux/statfs.h
@@ -40,5 +40,6 @@ struct kstatfs {
#define ST_NOATIME (1<<10) /* do not update access times */
#define ST_NODIRATIME (1<<11) /* do not update directory access times */
#define ST_RELATIME (1<<12) /* update atime relative to mtime/ctime */
+#define ST_UNBINDABLE (1<<17) /* change to unbindable */
#endif
--
2.17.0
^ permalink raw reply related
* [PATCH 4/6 v1] statfs: add ST_SHARED
From: Christian Brauner @ 2018-05-06 22:47 UTC (permalink / raw)
To: viro, linux-fsdevel, linux-kernel
Cc: torvalds, tglx, kstewart, gregkh, pombredanne, linux-api,
Christian Brauner
In-Reply-To: <20180506224704.14669-1-christian.brauner@ubuntu.com>
Currently userspace can only determine whether a mountpoint is shared
by parsing /proc/<pid>/mountinfo. It would be convenient to simply retrieve
this property with a statvfs() call.
This let's userspace avoid costly parsing, supports cases where /proc is
not mounted, and supports usecases where file descriptors to mountpoints
are passed between processes that are inspected via fstatvfs().
This is especially helpful in all cases where userspace is not interested
in specific propagation properties but rather just wants to either turn on
or turn off propagation for specific mountpoints.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
v0->v1:
* non-functional changes: extend commit message
---
fs/statfs.c | 2 ++
include/linux/statfs.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/fs/statfs.c b/fs/statfs.c
index 61b3063d3921..2fc6f9c3793c 100644
--- a/fs/statfs.c
+++ b/fs/statfs.c
@@ -31,6 +31,8 @@ static int flags_by_mnt(int mnt_flags)
flags |= ST_RELATIME;
if (mnt_flags & MNT_UNBINDABLE)
flags |= ST_UNBINDABLE;
+ if (mnt_flags & MNT_SHARED)
+ flags |= ST_SHARED;
return flags;
}
diff --git a/include/linux/statfs.h b/include/linux/statfs.h
index e1b84d0388c1..5416b2936dd9 100644
--- a/include/linux/statfs.h
+++ b/include/linux/statfs.h
@@ -41,5 +41,6 @@ struct kstatfs {
#define ST_NODIRATIME (1<<11) /* do not update directory access times */
#define ST_RELATIME (1<<12) /* update atime relative to mtime/ctime */
#define ST_UNBINDABLE (1<<17) /* change to unbindable */
+#define ST_SHARED (1<<20) /* change to shared */
#endif
--
2.17.0
^ permalink raw reply related
* [PATCH 5/6 v1] statfs: add ST_SLAVE
From: Christian Brauner @ 2018-05-06 22:47 UTC (permalink / raw)
To: viro, linux-fsdevel, linux-kernel
Cc: torvalds, tglx, kstewart, gregkh, pombredanne, linux-api,
Christian Brauner
In-Reply-To: <20180506224704.14669-1-christian.brauner@ubuntu.com>
Currently userspace can only determine whether a mountpoint is slave
by parsing /proc/<pid>/mountinfo. It would be convenient to simply retrieve
this property with a statvfs() call.
This let's userspace avoid costly parsing, supports cases where /proc is
not mounted, and supports usecases where file descriptors to mountpoints
are passed between processes that are inspected via fstatvfs().
This is especially helpful in all cases where userspace is not interested
in specific propagation properties our peer group layouts but rather just
wants to either turn on or turn off propagation for specific mountpoints.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
v0->v1:
* non-functional changes: extend commit message
---
fs/statfs.c | 10 +++++++++-
include/linux/statfs.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/statfs.c b/fs/statfs.c
index 2fc6f9c3793c..35ad0402c9a3 100644
--- a/fs/statfs.c
+++ b/fs/statfs.c
@@ -10,6 +10,7 @@
#include <linux/uaccess.h>
#include <linux/compat.h>
#include "internal.h"
+#include "pnode.h"
static int flags_by_mnt(int mnt_flags)
{
@@ -50,8 +51,15 @@ static int flags_by_sb(int s_flags)
static int calculate_f_flags(struct vfsmount *mnt)
{
- return ST_VALID | flags_by_mnt(mnt->mnt_flags) |
+ int flags = 0;
+
+ flags = ST_VALID | flags_by_mnt(mnt->mnt_flags) |
flags_by_sb(mnt->mnt_sb->s_flags);
+
+ if (IS_MNT_SLAVE(real_mount(mnt)))
+ flags |= ST_SLAVE;
+
+ return flags;
}
static int statfs_by_dentry(struct dentry *dentry, struct kstatfs *buf)
diff --git a/include/linux/statfs.h b/include/linux/statfs.h
index 5416b2936dd9..048127effaad 100644
--- a/include/linux/statfs.h
+++ b/include/linux/statfs.h
@@ -41,6 +41,7 @@ struct kstatfs {
#define ST_NODIRATIME (1<<11) /* do not update directory access times */
#define ST_RELATIME (1<<12) /* update atime relative to mtime/ctime */
#define ST_UNBINDABLE (1<<17) /* change to unbindable */
+#define ST_SLAVE (1<<19) /* change to slave */
#define ST_SHARED (1<<20) /* change to shared */
#endif
--
2.17.0
^ permalink raw reply related
* [PATCH 6/6 v1] statfs: add ST_PRIVATE
From: Christian Brauner @ 2018-05-06 22:47 UTC (permalink / raw)
To: viro, linux-fsdevel, linux-kernel
Cc: torvalds, tglx, kstewart, gregkh, pombredanne, linux-api,
Christian Brauner
In-Reply-To: <20180506224704.14669-1-christian.brauner@ubuntu.com>
Currently userspace can only determine whether a mountpoint is private
by parsing /proc/<pid>/mountinfo. It would be convenient to simply retrieve
this property with a statvfs() call.
This let's userspace avoid costly parsing, supports cases where /proc is
not mounted, and supports usecases where file descriptors to mountpoints
are passed between processes that are inspected via fstatvfs().
A mountpoint is considered ST_PRIVATE iff and it is neither ST_SLAVE nor
ST_SHARED.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
v0->v1:
* non-functional changes: extend commit message
---
fs/statfs.c | 2 ++
include/linux/statfs.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/fs/statfs.c b/fs/statfs.c
index 35ad0402c9a3..899e899ee84c 100644
--- a/fs/statfs.c
+++ b/fs/statfs.c
@@ -58,6 +58,8 @@ static int calculate_f_flags(struct vfsmount *mnt)
if (IS_MNT_SLAVE(real_mount(mnt)))
flags |= ST_SLAVE;
+ else if (!(flags & ST_SHARED))
+ flags |= ST_PRIVATE;
return flags;
}
diff --git a/include/linux/statfs.h b/include/linux/statfs.h
index 048127effaad..663fa5498a7d 100644
--- a/include/linux/statfs.h
+++ b/include/linux/statfs.h
@@ -41,6 +41,7 @@ struct kstatfs {
#define ST_NODIRATIME (1<<11) /* do not update directory access times */
#define ST_RELATIME (1<<12) /* update atime relative to mtime/ctime */
#define ST_UNBINDABLE (1<<17) /* change to unbindable */
+#define ST_PRIVATE (1<<18) /* change to private */
#define ST_SLAVE (1<<19) /* change to slave */
#define ST_SHARED (1<<20) /* change to shared */
--
2.17.0
^ permalink raw reply related
* Re: [PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics
From: Florian Weimer @ 2018-05-07 9:47 UTC (permalink / raw)
To: Ram Pai, Andy Lutomirski
Cc: linux-arch, Linux-MM, Linux API, X86 ML, Dave Hansen,
linux-x86_64, linuxppc-dev
In-Reply-To: <20180502233848.GB5863@ram.oc3035372033.ibm.com>
On 05/03/2018 01:38 AM, Ram Pai wrote:
> This is a new requirement that I was not aware off. Its not documented
> anywhere AFAICT.
Correct. All inheritance behavior was deliberately left unspecified.
I'm surprised about the reluctance to fix the x86 behavior. Are there
any applications at all for the current semantics?
I guess I can implement this particular glibc hardening on POWER only
for now.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Christopher Lameter @ 2018-05-07 14:47 UTC (permalink / raw)
To: Prakash Sangappa
Cc: linux-kernel, linux-mm, linux-api, akpm, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes
In-Reply-To: <98e34010-d55a-5f2d-7d98-cba424de2e74@oracle.com>
[-- Attachment #1: Type: text/plain, Size: 586 bytes --]
On Fri, 4 May 2018, Prakash Sangappa wrote:
> Currently 'numa_maps' gives a list of numa nodes, memory is allocated per
> VMA.
> Ex. we get something like from numa_maps.
>
> 04000 N0=1,N2=2 kernelpagesize_KB=4
>
> First is the start address of a VMA. This VMA could be much larger then 3 4k
> pages.
> It does not say which address in the VMA has the pages mapped.
Not precise. First the address is there as you already said. That is the
virtual address of the beginning of the VMA. What is missing? You need
each address for each page? Length of the VMA segment?
Physical address?
^ permalink raw reply
* Re: [PATCH v5 04/28] fpga: mgr: add compat_id support
From: Alan Tull @ 2018-05-07 21:09 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z
In-Reply-To: <1525229431-3087-5-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
Looks good!
> This patch introduces compat_id support to fpga manager, it adds
> a fpga_compat_id pointer to fpga manager data structure to allow
> fpga manager drivers to save the compatibility id. This compat_id
> could be used for compatibility checking before doing partial
> reconfiguration to associated fpga regions.
>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
> ---
> include/linux/fpga/fpga-mgr.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 802eac8..f163f22 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -147,11 +147,23 @@ struct fpga_manager_ops {
> #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR BIT(4)
>
> /**
> + * struct fpga_compat_id - id for compatibility check
> + *
> + * @id_h: high 64bit of the compat_id
> + * @id_l: low 64bit of the compat_id
> + */
> +struct fpga_compat_id {
> + u64 id_h;
> + u64 id_l;
> +};
> +
> +/**
> * struct fpga_manager - fpga manager structure
> * @name: name of low level fpga manager
> * @dev: fpga manager device
> * @ref_mutex: only allows one reference to fpga manager
> * @state: state of fpga manager
> + * @compat_id: FPGA manager id for compatibility check.
> * @mops: pointer to struct of fpga manager ops
> * @priv: low level driver private date
> */
> @@ -160,6 +172,7 @@ struct fpga_manager {
> struct device dev;
> struct mutex ref_mutex;
> enum fpga_mgr_states state;
> + struct fpga_compat_id *compat_id;
> const struct fpga_manager_ops *mops;
> void *priv;
> };
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v5 05/28] fpga: region: add compat_id support
From: Alan Tull @ 2018-05-07 21:09 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z
In-Reply-To: <1525229431-3087-6-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
Thanks for making the requested changes. Looks good.
> This patch introduces a compat_id pointer member and sysfs interface
> for each fpga region, similar as compat_id for fpga manager, it allows
> applications to read the per region compat_id for compatibility
> checking before other actions on this fpga-region (e.g PR).
>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
> ---
> v5: use pointer for compat_id as it's optional to implement.
> ---
> Documentation/ABI/testing/sysfs-class-fpga-region | 9 +++++++++
> drivers/fpga/fpga-region.c | 22 ++++++++++++++++++++++
> include/linux/fpga/fpga-region.h | 2 ++
> 3 files changed, 33 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-region
>
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-region b/Documentation/ABI/testing/sysfs-class-fpga-region
> new file mode 100644
> index 0000000..9618f74
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-region
> @@ -0,0 +1,9 @@
> +What: /sys/class/fpga_region/<region>/compat_id
> +Date: May 2018
> +KernelVersion: 4.17
> +Contact: Wu Hao <hao.wu@intel.com>
> +Description: FPGA region id for compatibility check, e.g compatibility
> + of the FPGA reconfiguration hardware and image. This value
> + is defined or calculated by the layer that is creating the
> + FPGA region. This interface returns the compat_id value or
> + just error code -ENOENT in case compat_id is not used.
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index d6a9dd2..6929178 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -162,6 +162,27 @@ int fpga_region_program_fpga(struct fpga_region *region)
> }
> EXPORT_SYMBOL_GPL(fpga_region_program_fpga);
>
> +static ssize_t compat_id_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_region *region = to_fpga_region(dev);
> +
> + if (!region->compat_id)
> + return -ENOENT;
> +
> + return sprintf(buf, "%016llx%016llx\n",
> + (unsigned long long)region->compat_id->id_h,
> + (unsigned long long)region->compat_id->id_l);
> +}
> +
> +static DEVICE_ATTR_RO(compat_id);
> +
> +static struct attribute *fpga_region_attrs[] = {
> + &dev_attr_compat_id.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(fpga_region);
> +
> /**
> * fpga_region_create - alloc and init a struct fpga_region
> * @dev: device parent
> @@ -262,6 +283,7 @@ static int __init fpga_region_init(void)
> if (IS_ERR(fpga_region_class))
> return PTR_ERR(fpga_region_class);
>
> + fpga_region_class->dev_groups = fpga_region_groups;
> fpga_region_class->dev_release = fpga_region_dev_release;
>
> return 0;
> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> index f2e215b..e8ad979 100644
> --- a/include/linux/fpga/fpga-region.h
> +++ b/include/linux/fpga/fpga-region.h
> @@ -12,6 +12,7 @@
> * @bridge_list: list of FPGA bridges specified in region
> * @mgr: FPGA manager
> * @info: FPGA image info
> + * @compat_id: FPGA region id for compatibility check.
> * @priv: private data
> * @get_bridges: optional function to get bridges to a list
> */
> @@ -21,6 +22,7 @@ struct fpga_region {
> struct list_head bridge_list;
> struct fpga_manager *mgr;
> struct fpga_image_info *info;
> + struct fpga_compat_id *compat_id;
> void *priv;
> int (*get_bridges)(struct fpga_region *region);
> };
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v5 19/28] fpga: dfl: fme-mgr: add compat_id support
From: Alan Tull @ 2018-05-07 21:12 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z
In-Reply-To: <1525229431-3087-20-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> This patch adds compat_id support to fme manager driver, it
> reads the ID from the hardware register. And it could be used
> for compatibility check before partial reconfiguration.
>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
> ---
> drivers/fpga/dfl-fme-mgr.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> index 1c5bc5a..afcdb39 100644
> --- a/drivers/fpga/dfl-fme-mgr.c
> +++ b/drivers/fpga/dfl-fme-mgr.c
> @@ -272,9 +272,17 @@ static u64 fme_mgr_status(struct fpga_manager *mgr)
> .status = fme_mgr_status,
> };
>
> +static void fme_mgr_get_compat_id(void __iomem *fme_pr,
> + struct fpga_compat_id *id)
> +{
> + id->id_l = readq(fme_pr + FME_PR_INTFC_ID_L);
> + id->id_h = readq(fme_pr + FME_PR_INTFC_ID_H);
> +}
> +
> static int fme_mgr_probe(struct platform_device *pdev)
> {
> struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
> + struct fpga_compat_id *compat_id;
> struct device *dev = &pdev->dev;
> struct fme_mgr_priv *priv;
> struct fpga_manager *mgr;
> @@ -295,11 +303,18 @@ static int fme_mgr_probe(struct platform_device *pdev)
> return PTR_ERR(priv->ioaddr);
> }
>
> + compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
> + if (!compat_id)
> + return -ENOMEM;
> +
> + fme_mgr_get_compat_id(priv->ioaddr, compat_id);
> +
> mgr = fpga_mgr_create(dev, "DFL FME FPGA Manager",
> &fme_mgr_ops, priv);
> if (!mgr)
> return -ENOMEM;
>
> + mgr->compat_id = compat_id;
> platform_set_drvdata(pdev, mgr);
>
> ret = fpga_mgr_register(mgr);
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v5 22/28] fpga: dfl: fme-region: add support for compat_id
From: Alan Tull @ 2018-05-07 21:12 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z
In-Reply-To: <1525229431-3087-23-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> This patch adds compat_id support, it reuses fme manager's
> compat id, as the per region compat id is actually from the
> fme manager's register.
>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
> ---
> v5: reuse fme manager's compat_id as per region compat_id
> ---
> drivers/fpga/dfl-fme-region.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
> index 696b313..b82a381 100644
> --- a/drivers/fpga/dfl-fme-region.c
> +++ b/drivers/fpga/dfl-fme-region.c
> @@ -45,6 +45,7 @@ static int fme_region_probe(struct platform_device *pdev)
> }
>
> region->priv = pdata;
> + region->compat_id = mgr->compat_id;
> platform_set_drvdata(pdev, region);
>
> ret = fpga_region_register(region);
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: prakash.sangappa @ 2018-05-07 22:50 UTC (permalink / raw)
To: Christopher Lameter
Cc: linux-kernel, linux-mm, linux-api, akpm, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes
In-Reply-To: <alpine.DEB.2.21.1805070945200.21162@nuc-kabylake>
On 05/07/2018 07:47 AM, Christopher Lameter wrote:
> On Fri, 4 May 2018, Prakash Sangappa wrote:
>> Currently 'numa_maps' gives a list of numa nodes, memory is allocated per
>> VMA.
>> Ex. we get something like from numa_maps.
>>
>> 04000 N0=1,N2=2 kernelpagesize_KB=4
>>
>> First is the start address of a VMA. This VMA could be much larger then 3 4k
>> pages.
>> It does not say which address in the VMA has the pages mapped.
> Not precise. First the address is there as you already said. That is the
> virtual address of the beginning of the VMA. What is missing? You need
Yes,
> each address for each page? Length of the VMA segment?
> Physical address?
Need numa node information for each virtual address with pages mapped.
No need of physical address.
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: prakash.sangappa @ 2018-05-07 23:22 UTC (permalink / raw)
To: Dave Hansen, Anshuman Khandual, Andrew Morton
Cc: linux-kernel, linux-mm, linux-api, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi
In-Reply-To: <8569dabb-4930-aa20-6249-72457e2df51e@intel.com>
On 05/03/2018 03:26 PM, Dave Hansen wrote:
> On 05/03/2018 03:27 PM, prakash.sangappa wrote:
>> If each consecutive page comes from different node, yes in
>> the extreme case is this file will have a lot of lines. All the lines
>> are generated at the time file is read. The amount of data read will be
>> limited to the user read buffer size used in the read.
>>
>> /proc/<pid>/pagemap also has kind of similar issue. There is 1 64
>> bit value for each user page.
> But nobody reads it sequentially. Everybody lseek()s because it has a
> fixed block size. You can't do that in text.
The current text based files on /proc does allow seeking, but it will not
help to seek to a specific VA(vma) to start from, as the seek offset
will be the
offset in the text. This is the case with using 'seq_file' interface in the
kernel to generate the /proc file content.
However, with the proposed new file, we could allow seeking to specified
virtual address. The lseek offset in this case would represent the
virtual address
of the process. Subsequent read from the file would provide VA range to
numa node
information starting from that VA. In case the VA seek'ed to is invalid,
it will start
from the next valid mapped VA of the process. The implementation would
not be based on seq_file.
For example.
Getting numa node information for a process having the following VMAs
mapped,
starting from '006dc000'
00400000-004dd000
006dc000-006dd000
006dd000-006e6000
Can seek to VA 006dc000 and start reading, it would get following
006dc000-006dd000 N1=1 kernelpagesize_kB=4 anon=1 dirty=1
file=/usr/bin/bash
006dd000-006de000 N0=1 kernelpagesize_kB=4 anon=1 dirty=1
file=/usr/bin/bash
006de000-006e0000 N1=2 kernelpagesize_kB=4 anon=2 dirty=2
file=/usr/bin/bash
006e0000-006e6000 N0=6 kernelpagesize_kB=4 anon=6 dirty=6
file=/usr/bin/bash
..
One advantage with getting numa node information from this /proc file vs
say
using 'move_pages()' API, will be that the /proc file will be able to
provide address
range to numa node information, not one page at a time.
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Dave Hansen @ 2018-05-08 0:05 UTC (permalink / raw)
To: prakash.sangappa, Anshuman Khandual, Andrew Morton
Cc: linux-kernel, linux-mm, linux-api, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi
In-Reply-To: <51145ccb-fc0d-0281-9757-fb8a5112ec24@oracle.com>
On 05/07/2018 04:22 PM, prakash.sangappa wrote:
> However, with the proposed new file, we could allow seeking to
> specified virtual address. The lseek offset in this case would
> represent the virtual address of the process. Subsequent read from
> the file would provide VA range to numa node information starting
> from that VA. In case the VA seek'ed to is invalid, it will start
> from the next valid mapped VA of the process. The implementation
> would not be based on seq_file.
So you're proposing a new /proc/<pid> file that appears next to and is
named very similarly to the exiting /proc/<pid>, but which has entirely
different behavior?
^ permalink raw reply
* Re: [RFC] vfs: skip extra attributes check on removal for symlinks
From: Darrick J. Wong @ 2018-05-08 0:30 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: viro, linux-fsdevel, linux-api, sandeen, dhowells, tytso, fliu,
jack, jeffm, nborisov, jake.norris, mtk.manpages, linux-xfs,
linux-kernel
In-Reply-To: <20180501174512.GA27853@wotan.suse.de>
On Tue, May 01, 2018 at 05:45:12PM +0000, Luis R. Rodriguez wrote:
> On Tue, May 01, 2018 at 10:23:19AM -0700, Darrick J. Wong wrote:
> > On Thu, Apr 26, 2018 at 04:46:39PM -0700, Luis R. Rodriguez wrote:
> > > Linux filesystems cannot set extra file attributes (stx_attributes as per
> > > statx(2)) on a symbolic link. To set extra file attributes you issue
> > > ioctl(2) with FS_IOC_SETFLAGS, *all* ioctl(2) calls on a symbolic link
> > > yield EBADF.
> > >
> > > This is because ioctl(2) tries to obtain struct fd from the symbolic link
> > > file descriptor passed using fdget(), fdget() in turn always returns no
> > > file set when a file descriptor is open with O_PATH. As per symlink(2)
> > > O_PATH and O_NOFOLLOW must *always* be used when you want to get the file
> > > descriptor of a symbolic link, and this holds true for Linux, as such extra
> > > file attributes cannot possibly be set on symbolic links on Linux.
> > >
> > > Filesystems repair utilities should be updated to detect this as
> > > corruption and correct this, however, the VFS *does* respect these
> > > extra attributes on symlinks for removal.
> > >
> > > Since we cannot set these attributes we should special-case the
> > > immutable/append on delete for symlinks, this would be consistent with
> > > what we *do* allow on Linux for all filesystems.
> >
> > Ah, ok, so the problem here is that you can't rm an "immutable" symlink
> > nor can you clear the immutable flag on such a beast, so therefore
> > ignore the immutable (and append) flags if we're trying to delete a
> > symlink?
>
> Yup.
>
> > I think we ought to teach the xfs inode verifier to check for
> > immutable/append symlinks and return error so that we don't end up with
> > such things in core in the first place,
>
> Agreed. But note that one way to end up with these things is through
> corruption. Once a user finds this the first signs they'll run into
> (unless they have the awesome new online scrubber) is they cannot delete
> some odd file and not know why. And then they'll see they cannot change
> or remove either the immutable or append flag. The immutable attribute
> is known to not let you delete the file, but its less known that the
> append attribute implies the same. Folks would scratch their heads.
>
> Since one cannot *set* these attributes on symlinks on Linux, other than
> ignoring such attributes perhaps we should warn about it? As the only
> way you could end up with that is if your filesystem got corrupted.
>
> But without filesystems having a fix for that merged users can't do anything.
>
> > and fix xfs_repair to zap such things.
>
> This is what my patch for xfs_repair does, which is pending review.
>
> But note that special files are handled differently, I explain the logic on the
> RFC commit log, which is why I suggested splitting up adding a fix for special
> files as a separate patch.
>
> > That said, for the filesystems that aren't going to check their inodes,
> > I guess this is a (hackish) way to avoid presenting undeletable gunk in
> > the fs to the user...
>
> That's why its RFC. What is the right thing to do here?
Ideally, none of the filesystems should ever feed garbage to the vfs,
but it's not all that obvious exactly what things the vfs will trip over.
And knowing that a lot of the filesystems do minimal checking if any, I
guess I'll just say that...
...XFS (and probably ext4) should catch immutable symlinks in the inode
verifiers so that xfs_iget returns -EFSCORRUPTED. For the rest of the
filesystems, it's probably fine to let them delete the symlink since the
user wants to kill it anyway.
> My own logic here was since we cannot possibly allow extended attributes on
> symlinks is to not use them then as well, in this case for delete.
>
> > (Were it up to me I'd make a common vfs_check_inode() to reject
> > struct inode containing garbage that the vfs won't deal with,
>
> There certainly are cases that we could come up with for the VFS where
> if such things are found, regardless of the filesystem, we're very sure
> its a corruption. This is just one example, as you note.
>
> So it is correct that there are two things here:
>
> 1) Do we respect the attribute for symlink on delete
> 2) Do we warn to users of the fact that the inode is very likely corrupted
> regardless of the filesystem?
But I suppose we could WARN_ON_ONCE to state that we're allowing
deletion of an immutable symlink that we couldn't possibly have set.
Anyway, couple this patch with a second one to fix the xfs verifier and
I'll be happy.
--D
> This patch only addresses 1) and makes it match the VFS expectation, and
> I think this is safe if we're not doing 2). This is why I'm also looking
> for feedback from linux-api folks.
>
> This is one of those odd corner cases we run into, and very likely not
> well documented anywhere.
>
> > and teach the filesystems to use it;
>
> Or the VFS uses it.
>
> Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ 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