* Re: [PATCH v3 0/3] namei: implement various lookup restriction AT_* flags
From: Aleksa Sarai @ 2018-10-17 15:23 UTC (permalink / raw)
To: Al Viro, Eric Biederman
Cc: Andy Lutomirski, David Howells, Jann Horn, Christian Brauner,
David Drysdale, containers, linux-fsdevel, linux-api, Jeff Layton,
J. Bruce Fields, Arnd Bergmann, Tycho Andersen, dev, linux-kernel,
linux-arch
In-Reply-To: <20181009070230.12884-1-cyphar@cyphar.com>
[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]
On 2018-10-09, Aleksa Sarai <cyphar@cyphar.com> wrote:
> The need for some sort of control over VFS's path resolution (to avoid
> malicious paths resulting in inadvertent breakouts) has been a very
> long-standing desire of many userspace applications. This patchset is a
> revival of Al Viro's old AT_NO_JUMPS[1,2] patchset (which was a variant
> of David Drysdale's O_BENEATH patchset[3] which was a spin-off of the
> Capsicum project[4]) with a few additions and changes made based on the
> previous discussion within [5] as well as others I felt were useful.
I've been thinking about this problem a little more (from the UX side of
things) and I have a feeling that adding 5 different O_* flags related
to resolution -- rather than properties related to opening the file --
might be less than ideal (even though, as discussed in previous threads,
there is a need for these flags and for them to be separated).
There is *some* precedence for this with O_PATH[**] changing fairly
large semantics of openat(2) but there are some things about O_PATH
which I think could be improved.
What if we had a resolveat(2) which acted like openat(..., O_PATH) *but*
it allowed us to have new flags and to separate the scoping flags from
the (fairly limited) space of O_* flags. Then O_PATH could effectively
just be a legacy way of doing resolveat(2) -- with only O_CLOEXEC,
O_DIRECTORY, and O_NOFOLLOW support.
And the main things we could add would be:
* These resolution flags, with only support available from
resolveat(2) for the moment. The idea would be that AT_EMPTY_PATH
would be the recommended way to make use of this.
* Support for RESOLVE_{NOPERM,RDONLY,WRONLY,RDWR} (which after some
discussions with Eric last year might be necessary in order to make
/proc/$pid/fd/$fd re-opening of O_PATH descriptors safer -- which is
something that we use in both runc and LXC).
Is this idea palatable, or was this something considered during the
development of O_PATH and someone had an argument why augmenting O_PATH
is better than a new syscall?
[**] And while writing this paragraph I noticed that I didn't update the
O_PATH "flag whitelist" to allow the scoping flags to affect it. I
will include a fix for this in v4 (I must've lost it in an early
rebase before I sent v1).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: David Howells @ 2018-10-17 15:24 UTC (permalink / raw)
To: Eric W. Biederman
Cc: dhowells, Alan Jenkins, Al Viro, linux-api, torvalds,
linux-fsdevel, linux-kernel, mszeredi
In-Reply-To: <87woqgiqta.fsf@xmission.com>
Eric W. Biederman <ebiederm@xmission.com> wrote:
> Davids check will work for bind mounts as well. It just won't work it
> just won't work for files or subdirectories of some mountpoint.
Bind mounts have to be done with open_tree() and move_mount(). You can't now
do fsmount() on something fspick()'d.
David
^ permalink raw reply
* Re: [RFC PATCH for 4.21 04/16] mm: Introduce vm_map_user_ram, vm_unmap_user_ram
From: Sergey Senozhatsky @ 2018-10-17 15:34 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Sergey Senozhatsky, rostedt, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, linux-kernel, linux-api, Thomas Gleixner,
Andy Lutomirski, Dave Watson, Paul Turner, Andrew Morton,
Russell King, Ingo Molnar, H. Peter Anvin, Andi Kleen,
Chris Lameter, Ben Maurer, Josh Triplett, Linus Torvalds
In-Reply-To: <2060193272.771.1539788641739.JavaMail.zimbra@efficios.com>
On (10/17/18 11:04), Mathieu Desnoyers wrote:
> >>
> >>> if (WARN_ON(x))
> >>> return;
> >>
> >> Given that this somewhat MM-related, I'd may be say VM_WARN_ON().
>
> I notice that VM_WARN_ON() casts the result of WARN_ON() to (void), so it
> cannot be used in a if () statement.
>
> VM_WARN_ON() will only warn if CONFIG_DEBUG_VM is set.
>
> Is it really what we want ?
Oh, indeed... Sorry, I forgot that they cast WANR_ON() return to void.
Let's do what Steven suggested.
-ss
^ permalink raw reply
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: Eric W. Biederman @ 2018-10-17 15:38 UTC (permalink / raw)
To: David Howells
Cc: Alan Jenkins, Al Viro, linux-api, torvalds, linux-fsdevel,
linux-kernel, mszeredi
In-Reply-To: <13800.1539789863@warthog.procyon.org.uk>
David Howells <dhowells@redhat.com> writes:
> Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>> Davids check will work for bind mounts as well. It just won't work it
>> just won't work for files or subdirectories of some mountpoint.
>
> Bind mounts have to be done with open_tree() and move_mount(). You can't now
> do fsmount() on something fspick()'d.
But a bind mount will have mnt_root set to the the dentry that was
bound.
Therefore fspick as you are proposing modifying will work for the root
of bind mounts, as well as the root of regular mounts. My apologies for
not being clear.
Eric
^ permalink raw reply
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: David Howells @ 2018-10-17 15:45 UTC (permalink / raw)
To: Alan Jenkins
Cc: dhowells, Al Viro, linux-api, torvalds, ebiederm, linux-fsdevel,
linux-kernel, mszeredi
In-Reply-To: <f6f2553e-f64d-9c26-4290-baf43e5f533b@gmail.com>
Alan Jenkins <alan.christopher.jenkins@gmail.com> wrote:
> I agree. I'm happy to see this is using the same check as do_remount().
>
>
> * change filesystem flags. dir should be a physical root of filesystem.
> * If you've mounted a non-root directory somewhere and want to do remount
> * on it - tough luck.
> */
Are you suggesting that it should only work at the ultimate root of a
superblock and not a bind mount somewhere within?
That's tricky to make work for NFS because s_root is a dummy dentry.
David
^ permalink raw reply
* Re: [RFC PATCH for 4.21 06/16] cpu_opv: Provide cpu_opv system call (v8)
From: Mathieu Desnoyers @ 2018-10-17 16:09 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will
In-Reply-To: <1006597066.825.1539789100381.JavaMail.zimbra@efficios.com>
----- On Oct 17, 2018, at 11:11 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> ----- On Oct 17, 2018, at 3:19 AM, Srikar Dronamraju srikar@linux.vnet.ibm.com
> wrote:
>
>> Hi Mathieu,
>>
>>> +static int do_cpu_opv(struct cpu_op *cpuop, int cpuopcnt,
>>> + struct cpu_opv_vaddr *vaddr_ptrs, int cpu)
>>> +{
>>> + struct mm_struct *mm = current->mm;
>>> + int ret;
>>> +
>>> +retry:
>>> + if (cpu != raw_smp_processor_id()) {
>>> + ret = push_task_to_cpu(current, cpu);
>>> + if (ret)
>>> + goto check_online;
>>> + }
>>> + down_read(&mm->mmap_sem);
>>> + ret = vaddr_ptrs_check(vaddr_ptrs);
>>> + if (ret)
>>> + goto end;
>>> + preempt_disable();
>>> + if (cpu != smp_processor_id()) {
>>> + preempt_enable();
>>> + up_read(&mm->mmap_sem);
>>> + goto retry;
>>> + }
>>
>> If we have a higher priority task/s either pinned to the cpu, dont we end up
>> in busy-looping till the task exits/sleeps?
>
> You're right!
>
> How about we ditch the thread migration altogether, and simply perform
> the cpu_opv operations in a IPI handler ?
>
> This is possible now that cpu_opv uses a temporary vmap() rather than
> try to touch the user-space page through the current thread's page table.
>
> Thoughts ?
Here is the associated implementation on top of this patchset:
commit 759c5a8860d867091e168900329f0955e5101989
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date: Wed Oct 17 11:32:02 2018 -0400
cpu opv: use ipi
diff --git a/kernel/cpu_opv.c b/kernel/cpu_opv.c
index db144b71d51a..30405e0cc049 100644
--- a/kernel/cpu_opv.c
+++ b/kernel/cpu_opv.c
@@ -31,6 +31,7 @@
#include <linux/mm.h>
#include <linux/vmalloc.h>
#include <linux/atomic.h>
+#include <linux/smp.h>
#include <asm/ptrace.h>
#include <asm/byteorder.h>
#include <asm/cacheflush.h>
@@ -1039,41 +1040,48 @@ static int vaddr_ptrs_check(struct cpu_opv_vaddr *vaddr_ptrs)
return 0;
}
+struct opv_ipi_args {
+ struct cpu_op *cpuop;
+ int cpuopcnt;
+ int ret;
+};
+
+static void cpu_opv_ipi(void *info)
+{
+ struct opv_ipi_args *args = info;
+
+ rseq_preempt(current);
+ args->ret = __do_cpu_opv(args->cpuop, args->cpuopcnt);
+}
+
static int do_cpu_opv(struct cpu_op *cpuop, int cpuopcnt,
struct cpu_opv_vaddr *vaddr_ptrs, int cpu)
{
struct mm_struct *mm = current->mm;
+ struct opv_ipi_args args = {
+ .cpuop = cpuop,
+ .cpuopcnt = cpuopcnt,
+ };
int ret;
retry:
- if (cpu != raw_smp_processor_id()) {
- ret = push_task_to_cpu(current, cpu);
- if (ret)
- goto check_online;
- }
+ if (!cpumask_test_cpu(cpu, ¤t->cpus_allowed))
+ return -EINVAL;
down_read(&mm->mmap_sem);
ret = vaddr_ptrs_check(vaddr_ptrs);
if (ret)
goto end;
- preempt_disable();
- if (cpu != smp_processor_id()) {
- preempt_enable();
+ ret = smp_call_function_single(cpu, cpu_opv_ipi, &args, 1);
+ if (ret) {
up_read(&mm->mmap_sem);
- goto retry;
+ goto check_online;
}
- ret = __do_cpu_opv(cpuop, cpuopcnt);
- preempt_enable();
+ ret = args.ret;
end:
up_read(&mm->mmap_sem);
return ret;
check_online:
- /*
- * push_task_to_cpu() returns -EINVAL if the requested cpu is not part
- * of the current thread's cpus_allowed mask.
- */
- if (ret == -EINVAL)
- return ret;
get_online_cpus();
if (cpu_online(cpu)) {
put_online_cpus();
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply related
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: Alan Jenkins @ 2018-10-17 17:41 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, linux-api, torvalds, ebiederm, linux-fsdevel,
linux-kernel, mszeredi
In-Reply-To: <15867.1539791153@warthog.procyon.org.uk>
On 17/10/2018 16:45, David Howells wrote:
> Alan Jenkins <alan.christopher.jenkins@gmail.com> wrote:
>
>> I agree. I'm happy to see this is using the same check as do_remount().
>>
>>
>> * change filesystem flags. dir should be a physical root of filesystem.
>> * If you've mounted a non-root directory somewhere and want to do remount
>> * on it - tough luck.
>> */
> Are you suggesting that it should only work at the ultimate root of a
> superblock and not a bind mount somewhere within?
>
> That's tricky to make work for NFS because s_root is a dummy dentry.
>
> David
Retro-actively: I do not suggest that.
I tried to answer this question in my reply to Eric correcting me. Eric
was right to correct me. I now understand the comment above
do_remount() is incorrect. I re-reviewed your diff in light of that. I
re-endorse your diff as a way to solve the problem I raised.
(I think it would be useful to remove the misleading comment above
do_remount(), to avoid future confusion.)
> @@ -186,6 +186,10 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char
__user *, path, unsigned int, flags
> if (ret < 0)
> goto err;
>
> + ret = -EINVAL;
> + if (target.mnt->mnt_root != target.dentry)
> + goto err_path;
> +
( the "if" statement it adds to fspick() is equivalent to the second
"if" statement in do_remount(): )
static int do_remount <https://elixir.bootlin.com/linux/v4.18/ident/do_remount>(struct path <https://elixir.bootlin.com/linux/v4.18/ident/path> *path <https://elixir.bootlin.com/linux/v4.18/ident/path>, int ms_flags, int sb_flags,
int mnt_flags, void *data)
{
int err;
struct super_block <https://elixir.bootlin.com/linux/v4.18/ident/super_block> *sb = path <https://elixir.bootlin.com/linux/v4.18/ident/path>->mnt
<https://elixir.bootlin.com/linux/v4.18/ident/mnt>->mnt_sb;
struct mount <https://elixir.bootlin.com/linux/v4.18/ident/mount> *mnt <https://elixir.bootlin.com/linux/v4.18/ident/mnt> = real_mount
<https://elixir.bootlin.com/linux/v4.18/ident/real_mount>(path
<https://elixir.bootlin.com/linux/v4.18/ident/path>->mnt
<https://elixir.bootlin.com/linux/v4.18/ident/mnt>);
if (!check_mnt <https://elixir.bootlin.com/linux/v4.18/ident/check_mnt>(mnt
<https://elixir.bootlin.com/linux/v4.18/ident/mnt>))
return -EINVAL <https://elixir.bootlin.com/linux/v4.18/ident/EINVAL>;
if (path <https://elixir.bootlin.com/linux/v4.18/ident/path>->dentry != path <https://elixir.bootlin.com/linux/v4.18/ident/path>->mnt
<https://elixir.bootlin.com/linux/v4.18/ident/mnt>->mnt_root)
return -EINVAL <https://elixir.bootlin.com/linux/v4.18/ident/EINVAL>;
Thanks
Alan
^ permalink raw reply
* statx(2) API and documentation
From: Miklos Szeredi @ 2018-10-17 18:24 UTC (permalink / raw)
To: Michael Kerrisk, David Howells; +Cc: linux-fsdevel, linux-kernel, Linux API
I'm trying to implement statx for fuse and ran into the following issues:
- Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
for stx_attribute; otherwise if querying has non-zero cost, then
filesystem cannot do it without regressing performance.
- STATX_ALL definition is unclear, can this change, or is it fixed?
If it's the former, than that's a backward compatibility nightmare.
If it's the latter, then what's the point?
- STATX_ATIME is cleared from stx_mask on SB_RDONLY, and on NFS it is
also cleared on MNT_NOATIME, but not on MNT_RDONLY. We need some sort
of guideline in the documentation about what constitutes
"unsupported": does atime become unsupported because filesystem is
remounted r/o? If so, why isn't this case handled consistently in the
VFS and filesystems?
- What about fields that are not cached when statx() is called with
AT_STATX_DONT_SYNC? E.g. stx_btime is supported by the filesystem,
but getting it requires a roundtrip to the server. Requesting
STATX_BTIME in the mask and adding AT_STATX_DONT_SYNC to the flags
means the filesystem has to decide which it will honor. My feeling
is that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in
stx_mask. Documentation has no word about this case.
Thanks,
Miklos
^ permalink raw reply
* Re: statx(2) API and documentation
From: Andreas Dilger @ 2018-10-17 18:45 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Michael Kerrisk, David Howells, Linux FS-devel Mailing List,
Linux Kernel Mailing List, Linux API
In-Reply-To: <CAJfpegsxFUGeczcC-pzK0WWROjv48_PvXtNun-f5QJMoUDn1Dw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2372 bytes --]
On Oct 17, 2018, at 12:24 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> I'm trying to implement statx for fuse and ran into the following issues:
>
> - Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
> for stx_attribute; otherwise if querying has non-zero cost, then
> filesystem cannot do it without regressing performance.
Seems reasonable.
> - STATX_ALL definition is unclear, can this change, or is it fixed?
> If it's the former, than that's a backward compatibility nightmare.
> If it's the latter, then what's the point?
The value can change over time. It is intended to reflect the current
state of affairs at the time the userspace program and kernel are compiled.
The value sent from userspace lets the kernel know what fields are in
the userspace struct, so it doesn't try to set fields that aren't there.
The value in the kernel allows masking off new fields from userspace that
it doesn't understand.
> - STATX_ATIME is cleared from stx_mask on SB_RDONLY, and on NFS it is
> also cleared on MNT_NOATIME, but not on MNT_RDONLY. We need some sort
> of guideline in the documentation about what constitutes
> "unsupported": does atime become unsupported because filesystem is
> remounted r/o? If so, why isn't this case handled consistently in the
> VFS and filesystems?
Strange. I'd think that if userspace is requesting atime, it should
get an atime value. The fact that the kernel is not updating atime
due to mount options just means that atime might be old. That doesn't
mean (IMHO) that atime doesn't exist.
> - What about fields that are not cached when statx() is called with
> AT_STATX_DONT_SYNC? E.g. stx_btime is supported by the filesystem,
> but getting it requires a roundtrip to the server. Requesting
> STATX_BTIME in the mask and adding AT_STATX_DONT_SYNC to the flags
> means the filesystem has to decide which it will honor. My feeling
> is that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in
> stx_mask. Documentation has no word about this case.
The btime value shouldn't change over the lifetime of a file, so
DONT_SYNC shouldn't have any effect on its validity?
I think DONT_SYNC applies to values like size, blocks, mtime, ctime
that may change rapidly over the lifetime of the file, so a totally
up-to-date value is not needed from the server in that case.
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply
* Re: statx(2) API and documentation
From: Miklos Szeredi @ 2018-10-17 19:04 UTC (permalink / raw)
To: Andreas Dilger
Cc: Michael Kerrisk, David Howells, Linux FS-devel Mailing List,
Linux Kernel Mailing List, Linux API
In-Reply-To: <006890C4-64D4-4DE2-A1F0-335FFFD585BB@dilger.ca>
On Wed, Oct 17, 2018 at 8:45 PM, Andreas Dilger <adilger@dilger.ca> wrote:
> On Oct 17, 2018, at 12:24 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> I'm trying to implement statx for fuse and ran into the following issues:
>>
>> - Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
>> for stx_attribute; otherwise if querying has non-zero cost, then
>> filesystem cannot do it without regressing performance.
>
> Seems reasonable.
>
>> - STATX_ALL definition is unclear, can this change, or is it fixed?
>> If it's the former, than that's a backward compatibility nightmare.
>> If it's the latter, then what's the point?
>
> The value can change over time. It is intended to reflect the current
> state of affairs at the time the userspace program and kernel are compiled.
> The value sent from userspace lets the kernel know what fields are in
> the userspace struct, so it doesn't try to set fields that aren't there.
What's the point of a userspace program specifying STATX_ALL? Without
a way to programmatically query the interface definition it's useless:
there's no way to guess which mask bit corresponds to which field, and
what that field represents.
And there will be programs out there which specify STATX_ALL without
considering that in the future it may become slower as it is now due
to a recompile.
So what's the point exactly?
> The value in the kernel allows masking off new fields from userspace that
> it doesn't understand.
Okay, but that has nothing to do with the UAPI. Even as an internal
flag we should be careful, as it might grow uses which can have
similar issues as the userspace one above.
>
>> - STATX_ATIME is cleared from stx_mask on SB_RDONLY, and on NFS it is
>> also cleared on MNT_NOATIME, but not on MNT_RDONLY. We need some sort
>> of guideline in the documentation about what constitutes
>> "unsupported": does atime become unsupported because filesystem is
>> remounted r/o? If so, why isn't this case handled consistently in the
>> VFS and filesystems?
>
> Strange. I'd think that if userspace is requesting atime, it should
> get an atime value. The fact that the kernel is not updating atime
> due to mount options just means that atime might be old. That doesn't
> mean (IMHO) that atime doesn't exist.
Right, OTOH I sort of see the value in NFS: no roundtrip to server if
atime value is useless anyway.
>> - What about fields that are not cached when statx() is called with
>> AT_STATX_DONT_SYNC? E.g. stx_btime is supported by the filesystem,
>> but getting it requires a roundtrip to the server. Requesting
>> STATX_BTIME in the mask and adding AT_STATX_DONT_SYNC to the flags
>> means the filesystem has to decide which it will honor. My feeling
>> is that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in
>> stx_mask. Documentation has no word about this case.
>
> The btime value shouldn't change over the lifetime of a file, so
> DONT_SYNC shouldn't have any effect on its validity?
Not validity, but presence in the cache. Network filesystem or fuse
may or may not have it available in the cache at the time the inode is
first initialized on loookup. So when statx(..., AT_STATX_DONT_SYNC,
STATX_BTIME) comes along, it may need a roundtrip to the server. What
should it do?
Thanks,
Miklos
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: David Rientjes @ 2018-10-17 19:59 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20181017070531.GC18839@dhcp22.suse.cz>
On Wed, 17 Oct 2018, Michal Hocko wrote:
> Do you know of any other userspace except your usecase? Is there
> anything fundamental that would prevent a proper API adoption for you?
>
Yes, it would require us to go back in time and build patched binaries.
^ permalink raw reply
* Re: statx(2) API and documentation
From: Andreas Dilger @ 2018-10-17 20:22 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Michael Kerrisk, David Howells, Linux FS-devel Mailing List,
Linux Kernel Mailing List, Linux API
In-Reply-To: <CAJfpeguvJZbP83mSULc74Yhpp_CPMBW1XvcgbvCbLnF7QCncUA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5096 bytes --]
On Oct 17, 2018, at 1:04 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, Oct 17, 2018 at 8:45 PM, Andreas Dilger <adilger@dilger.ca> wrote:
>> On Oct 17, 2018, at 12:24 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>
>>> I'm trying to implement statx for fuse and ran into the following issues:
>>>
>>> - Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
>>> for stx_attribute; otherwise if querying has non-zero cost, then
>>> filesystem cannot do it without regressing performance.
>>
>> Seems reasonable.
>>
>>> - STATX_ALL definition is unclear, can this change, or is it fixed?
>>> If it's the former, than that's a backward compatibility nightmare.
>>> If it's the latter, then what's the point?
>>
>> The value can change over time. It is intended to reflect the current
>> state of affairs at the time the userspace program and kernel are compiled.
>> The value sent from userspace lets the kernel know what fields are in
>> the userspace struct, so it doesn't try to set fields that aren't there.
>
> What's the point of a userspace program specifying STATX_ALL? Without
> a way to programmatically query the interface definition it's useless:
> there's no way to guess which mask bit corresponds to which field, and
> what that field represents.
>
> And there will be programs out there which specify STATX_ALL without
> considering that in the future it may become slower as it is now due
> to a recompile.
The whole point of statx is that applications should only be requesting
fields that they care about, so "ls --color=never" shouldn't be asking for
the file mode/size/etc., and size/blocks should only be requested with
"ls -ls", etc.
> So what's the point exactly?
Ah, I see your point... STATX_ALL seems to be mostly useful for the kernel
to mask off flags that it doesn't currently understand. It doesn't make
much sense for applications to specify STATX_ALL, since they don't have any
way to know what each flag means unless they are hard-coded to check each of
the STATX_* flags individually. They should build up a mask of STATX_* flags
based on what they care about (e.g. "find" should only request attributes
based on the command-line options given).
>> The value in the kernel allows masking off new fields from userspace that
>> it doesn't understand.
>
> Okay, but that has nothing to do with the UAPI. Even as an internal
> flag we should be careful, as it might grow uses which can have
> similar issues as the userspace one above.
>
>>> - STATX_ATIME is cleared from stx_mask on SB_RDONLY, and on NFS it is
>>> also cleared on MNT_NOATIME, but not on MNT_RDONLY. We need some sort
>>> of guideline in the documentation about what constitutes
>>> "unsupported": does atime become unsupported because filesystem is
>>> remounted r/o? If so, why isn't this case handled consistently in the
>>> VFS and filesystems?
>>
>> Strange. I'd think that if userspace is requesting atime, it should
>> get an atime value. The fact that the kernel is not updating atime
>> due to mount options just means that atime might be old. That doesn't
>> mean (IMHO) that atime doesn't exist.
>
> Right, OTOH I sort of see the value in NFS: no roundtrip to server if
> atime value is useless anyway.
Well, "noatime" might be a client-only option, which doesn't mean that the
server or some other client isn't updating the atime. Returning an old
atime isn't the same as not returning anything at all.
>>> - What about fields that are not cached when statx() is called with
>>> AT_STATX_DONT_SYNC? E.g. stx_btime is supported by the filesystem,
>>> but getting it requires a roundtrip to the server. Requesting
>>> STATX_BTIME in the mask and adding AT_STATX_DONT_SYNC to the flags
>>> means the filesystem has to decide which it will honor. My feeling
>>> is that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in
>>> stx_mask. Documentation has no word about this case.
>>
>> The btime value shouldn't change over the lifetime of a file, so
>> DONT_SYNC shouldn't have any effect on its validity?
>
> Not validity, but presence in the cache. Network filesystem or fuse
> may or may not have it available in the cache at the time the inode is
> first initialized on loookup. So when statx(..., AT_STATX_DONT_SYNC,
> STATX_BTIME) comes along, it may need a roundtrip to the server. What
> should it do?
To my thinking, if the user/app requests btime/ctime/size/blocks the kernel
should return these fields. If DONT_SYNC is set, it means the kernel can
return potentially stale values, but if the kernel doesn't have *any*
values for these fields at all it still should query the server instead of
just returning random garbage.
That said, it seems that it isn't even possible to get into nfs_getattr()
without the VFS having instantiated a dentry and inode (i.e. queried the
server via nfs_lookup() at some point), so the inode fields have already
been filled with *something*.
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Jeff Moyer @ 2018-10-17 20:23 UTC (permalink / raw)
To: Jan Kara
Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20181002142959.GD9127-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org>
Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> writes:
> [Added ext4, xfs, and linux-api folks to CC for the interface discussion]
>
> On Tue 02-10-18 14:10:39, Johannes Thumshirn wrote:
>> On Tue, Oct 02, 2018 at 12:05:31PM +0200, Jan Kara wrote:
>> > Hello,
>> >
>> > commit e1fb4a086495 "dax: remove VM_MIXEDMAP for fsdax and device dax" has
>> > removed VM_MIXEDMAP flag from DAX VMAs. Now our testing shows that in the
>> > mean time certain customer of ours started poking into /proc/<pid>/smaps
>> > and looks at VMA flags there and if VM_MIXEDMAP is missing among the VMA
>> > flags, the application just fails to start complaining that DAX support is
>> > missing in the kernel. The question now is how do we go about this?
>>
>> OK naive question from me, how do we want an application to be able to
>> check if it is running on a DAX mapping?
>
> The question from me is: Should application really care? After all DAX is
> just a caching decision. Sure it affects performance characteristics and
> memory usage of the kernel but it is not a correctness issue (in particular
> we took care for MAP_SYNC to return EOPNOTSUPP if the feature cannot be
> supported for current mapping). And in the future the details of what we do
> with DAX mapping can change - e.g. I could imagine we might decide to cache
> writes in DRAM but do direct PMEM access on reads. And all this could be
> auto-tuned based on media properties. And we don't want to tie our hands by
> specifying too narrowly how the kernel is going to behave.
For read and write, I would expect the O_DIRECT open flag to still work,
even for dax-capable persistent memory. Is that a contentious opinion?
So, what we're really discussing is the behavior for mmap. MAP_SYNC
will certainly ensure that the page cache is not used for writes. It
would also be odd for us to decide to cache reads. The only issue I can
see is that perhaps the application doesn't want to take a performance
hit on write faults. I haven't heard that concern expressed in this
thread, though.
Just to be clear, this is my understanding of the world:
MAP_SYNC
- file system guarantees that metadata required to reach faulted-in file
data is consistent on media before a write fault is completed. A
side-effect is that the page cache will not be used for
writably-mapped pages.
and what I think Dan had proposed:
mmap flag, MAP_DIRECT
- file system guarantees that page cache will not be used to front storage.
storage MUST be directly addressable. This *almost* implies MAP_SYNC.
The subtle difference is that a write fault /may/ not result in metadata
being written back to media.
and this is what I think you were proposing, Jan:
madvise flag, MADV_DIRECT_ACCESS
- same semantics as MAP_DIRECT, but specified via the madvise system call
Cheers,
Jeff
^ permalink raw reply
* Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-17 20:29 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Linux Containers, Linux API, Andy Lutomirski, Oleg Nesterov,
Eric W . Biederman, Serge E . Hallyn, Christian Brauner,
Tyler Hicks, Akihiro Suda, Jann Horn,
linux-fsdevel@vger.kernel.org
In-Reply-To: <CAGXu5jJr3JWLYGrJ+knu3sj4wwJ_irVvs+JRrY5MuYT3voLegg@mail.gmail.com>
On Thu, Sep 27, 2018 at 02:31:24PM -0700, Kees Cook wrote:
> On Thu, Sep 27, 2018 at 8:11 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> > @@ -60,4 +62,29 @@ struct seccomp_data {
> > __u64 args[6];
> > };
> >
> > +struct seccomp_notif {
> > + __u16 len;
> > + __u64 id;
> > + __u32 pid;
> > + __u8 signaled;
> > + struct seccomp_data data;
> > +};
> > +
> > +struct seccomp_notif_resp {
> > + __u16 len;
> > + __u64 id;
> > + __s32 error;
> > + __s64 val;
> > +};
>
> So, len has to come first, for versioning. However, since it's ahead
> of a u64, this leaves a struct padding hole. pahole output:
>
> struct seccomp_notif {
> __u16 len; /* 0 2 */
>
> /* XXX 6 bytes hole, try to pack */
>
> __u64 id; /* 8 8 */
> __u32 pid; /* 16 4 */
> __u8 signaled; /* 20 1 */
>
> /* XXX 3 bytes hole, try to pack */
>
> struct seccomp_data data; /* 24 64 */
> /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
>
> /* size: 88, cachelines: 2, members: 5 */
> /* sum members: 79, holes: 2, sum holes: 9 */
> /* last cacheline: 24 bytes */
> };
> struct seccomp_notif_resp {
> __u16 len; /* 0 2 */
>
> /* XXX 6 bytes hole, try to pack */
>
> __u64 id; /* 8 8 */
> __s32 error; /* 16 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> __s64 val; /* 24 8 */
>
> /* size: 32, cachelines: 1, members: 4 */
> /* sum members: 22, holes: 2, sum holes: 10 */
> /* last cacheline: 32 bytes */
> };
>
> How about making len u32, and moving pid and error above "id"? This
> leaves a hole after signaled, so changing "len" won't be sufficient
> for versioning here. Perhaps move it after data?
Just to confirm my understanding; I've got these as:
struct seccomp_notif {
__u32 len; /* 0 4 */
__u32 pid; /* 4 4 */
__u64 id; /* 8 8 */
__u8 signaled; /* 16 1 */
/* XXX 7 bytes hole, try to pack */
struct seccomp_data data; /* 24 64 */
/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
/* size: 88, cachelines: 2, members: 5 */
/* sum members: 81, holes: 1, sum holes: 7 */
/* last cacheline: 24 bytes */
};
struct seccomp_notif_resp {
__u32 len; /* 0 4 */
__s32 error; /* 4 4 */
__u64 id; /* 8 8 */
__s64 val; /* 16 8 */
/* size: 24, cachelines: 1, members: 4 */
/* last cacheline: 24 bytes */
};
in the next version. Since the structure has no padding at the end of
it, I think the Right Thing will happen. Note that this is slightly
different than what Kees suggested, if I add signaled after data, then
I end up with:
struct seccomp_notif {
__u32 len; /* 0 4 */
__u32 pid; /* 4 4 */
__u64 id; /* 8 8 */
struct seccomp_data data; /* 16 64 */
/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
__u8 signaled; /* 80 1 */
/* size: 88, cachelines: 2, members: 5 */
/* padding: 7 */
/* last cacheline: 24 bytes */
};
which I think will have the versioning problem if the next member
introduces is < 7 bytes.
Tycho
^ permalink raw reply
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: David Howells @ 2018-10-17 21:20 UTC (permalink / raw)
To: Alan Jenkins
Cc: dhowells, Al Viro, linux-api, torvalds, ebiederm, linux-fsdevel,
linux-kernel, mszeredi
In-Reply-To: <7b409325-9e23-aa5e-916f-cb362a6940aa@gmail.com>
Alan Jenkins <alan.christopher.jenkins@gmail.com> wrote:
> static int do_remount <https://elixir.bootlin.com/linux/v4.18/ident/do_remount>(struct path <https://elixir.bootlin.com/linux/v4.18/ident/path> *path <https://elixir.bootlin.com/linux/v4.18/ident/path>, int ms_flags, int sb_flags,
> int mnt_flags, void *data)
What happened there? You seem to have had a load of URLs substituted in.
David
^ permalink raw reply
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: Alan Jenkins @ 2018-10-17 22:13 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, linux-api, torvalds, ebiederm, linux-fsdevel,
linux-kernel, mszeredi
In-Reply-To: <15867.1539791153@warthog.procyon.org.uk>
[resent, hopefully with slightly less formatting damage]
On 17/10/2018 16:45, David Howells wrote:
> Alan Jenkins <alan.christopher.jenkins@gmail.com> wrote:
>
>> I agree. I'm happy to see this is using the same check as do_remount().
>>
>>
>> * change filesystem flags. dir should be a physical root of filesystem.
>> * If you've mounted a non-root directory somewhere and want to do remount
>> * on it - tough luck.
>> */
> Are you suggesting that it should only work at the ultimate root of a
> superblock and not a bind mount somewhere within?
>
> That's tricky to make work for NFS because s_root is a dummy dentry.
>
> David
Retro-actively: I do not suggest that.
I tried to answer this question in my reply to Eric correcting me. Eric
was right to correct me. I now understand the comment above
do_remount() is incorrect. I re-reviewed your diff in light of that. I
re-endorse your diff as a way to solve the problem I raised.
(I think it would be useful to remove the misleading comment above
do_remount(), to avoid future confusion.)
> @@ -186,6 +186,10 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
> if (ret < 0)
> goto err;
>
> + ret = -EINVAL;
> + if (target.mnt->mnt_root != target.dentry)
> + goto err_path;
> +
> fc = vfs_new_fs_context(target.dentry->d_sb->s_type, target.dentry,
> 0, 0, FS_CONTEXT_FOR_RECONFIGURE);
> if (IS_ERR(fc)) {
( the "if" statement it adds to fspick() is equivalent to the second
"if" statement in do_remount(): )
static int do_remount(struct path *path, int ms_flags, int sb_flags,
int mnt_flags, void *data)
{
int err;
struct super_block *sb = path->mnt->mnt_sb;
struct mount *mnt = real_mount(path->mnt);
if (!check_mnt(mnt))
return -EINVAL;
if (path->dentry != path->mnt->mnt_root)
return -EINVAL;
Thanks
Alan
^ permalink raw reply
* Re: statx(2) API and documentation
From: Amir Goldstein @ 2018-10-17 22:15 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Andreas Dilger, Michael Kerrisk (man-pages), David Howells,
linux-fsdevel, linux-kernel, linux-api, Jan Kara
In-Reply-To: <CAJfpeguvJZbP83mSULc74Yhpp_CPMBW1XvcgbvCbLnF7QCncUA@mail.gmail.com>
On Wed, Oct 17, 2018 at 10:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >> - STATX_ALL definition is unclear, can this change, or is it fixed?
> >> If it's the former, than that's a backward compatibility nightmare.
> >> If it's the latter, then what's the point?
> >
> > The value can change over time. It is intended to reflect the current
> > state of affairs at the time the userspace program and kernel are compiled.
> > The value sent from userspace lets the kernel know what fields are in
> > the userspace struct, so it doesn't try to set fields that aren't there.
>
> What's the point of a userspace program specifying STATX_ALL? Without
> a way to programmatically query the interface definition it's useless:
> there's no way to guess which mask bit corresponds to which field, and
> what that field represents.
>
> And there will be programs out there which specify STATX_ALL without
> considering that in the future it may become slower as it is now due
> to a recompile.
>
> So what's the point exactly?
>
> > The value in the kernel allows masking off new fields from userspace that
> > it doesn't understand.
>
> Okay, but that has nothing to do with the UAPI. Even as an internal
> flag we should be careful, as it might grow uses which can have
> similar issues as the userspace one above.
>
FYI, I identified a similar anti-pattern in fanotify UAPI when I wanted to
add new flags and did not want to change the UAPI _ALL_ constants.
This is how we plan to solve it:
https://github.com/amir73il/linux/commit/8c2b1acadb88ee4505ccc8bfdc665863111fb4cc
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace
From: Kees Cook @ 2018-10-17 22:21 UTC (permalink / raw)
To: Tycho Andersen
Cc: LKML, Linux Containers, Linux API, Andy Lutomirski, Oleg Nesterov,
Eric W . Biederman, Serge E . Hallyn, Christian Brauner,
Tyler Hicks, Akihiro Suda, Jann Horn,
linux-fsdevel@vger.kernel.org
In-Reply-To: <20181017202933.GB14047@cisco>
On Wed, Oct 17, 2018 at 1:29 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> On Thu, Sep 27, 2018 at 02:31:24PM -0700, Kees Cook wrote:
>> On Thu, Sep 27, 2018 at 8:11 AM, Tycho Andersen <tycho@tycho.ws> wrote:
>> > @@ -60,4 +62,29 @@ struct seccomp_data {
>> > __u64 args[6];
>> > };
>> >
>> > +struct seccomp_notif {
>> > + __u16 len;
>> > + __u64 id;
>> > + __u32 pid;
>> > + __u8 signaled;
>> > + struct seccomp_data data;
>> > +};
>> > +
>> > +struct seccomp_notif_resp {
>> > + __u16 len;
>> > + __u64 id;
>> > + __s32 error;
>> > + __s64 val;
>> > +};
>>
>> So, len has to come first, for versioning. However, since it's ahead
>> of a u64, this leaves a struct padding hole. pahole output:
>>
>> struct seccomp_notif {
>> __u16 len; /* 0 2 */
>>
>> /* XXX 6 bytes hole, try to pack */
>>
>> __u64 id; /* 8 8 */
>> __u32 pid; /* 16 4 */
>> __u8 signaled; /* 20 1 */
>>
>> /* XXX 3 bytes hole, try to pack */
>>
>> struct seccomp_data data; /* 24 64 */
>> /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
>>
>> /* size: 88, cachelines: 2, members: 5 */
>> /* sum members: 79, holes: 2, sum holes: 9 */
>> /* last cacheline: 24 bytes */
>> };
>> struct seccomp_notif_resp {
>> __u16 len; /* 0 2 */
>>
>> /* XXX 6 bytes hole, try to pack */
>>
>> __u64 id; /* 8 8 */
>> __s32 error; /* 16 4 */
>>
>> /* XXX 4 bytes hole, try to pack */
>>
>> __s64 val; /* 24 8 */
>>
>> /* size: 32, cachelines: 1, members: 4 */
>> /* sum members: 22, holes: 2, sum holes: 10 */
>> /* last cacheline: 32 bytes */
>> };
>>
>> How about making len u32, and moving pid and error above "id"? This
>> leaves a hole after signaled, so changing "len" won't be sufficient
>> for versioning here. Perhaps move it after data?
>
> Just to confirm my understanding; I've got these as:
>
> struct seccomp_notif {
> __u32 len; /* 0 4 */
> __u32 pid; /* 4 4 */
> __u64 id; /* 8 8 */
> __u8 signaled; /* 16 1 */
>
> /* XXX 7 bytes hole, try to pack */
>
> struct seccomp_data data; /* 24 64 */
> /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
>
> /* size: 88, cachelines: 2, members: 5 */
> /* sum members: 81, holes: 1, sum holes: 7 */
> /* last cacheline: 24 bytes */
> };
> struct seccomp_notif_resp {
> __u32 len; /* 0 4 */
> __s32 error; /* 4 4 */
> __u64 id; /* 8 8 */
> __s64 val; /* 16 8 */
>
> /* size: 24, cachelines: 1, members: 4 */
> /* last cacheline: 24 bytes */
> };
>
> in the next version. Since the structure has no padding at the end of
> it, I think the Right Thing will happen. Note that this is slightly
> different than what Kees suggested, if I add signaled after data, then
> I end up with:
>
> struct seccomp_notif {
> __u32 len; /* 0 4 */
> __u32 pid; /* 4 4 */
> __u64 id; /* 8 8 */
> struct seccomp_data data; /* 16 64 */
> /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> __u8 signaled; /* 80 1 */
>
> /* size: 88, cachelines: 2, members: 5 */
> /* padding: 7 */
> /* last cacheline: 24 bytes */
> };
>
> which I think will have the versioning problem if the next member
> introduces is < 7 bytes.
It'll be a problem in either place. What I was thinking was that
specific versioning is required instead of just length.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: statx(2) API and documentation
From: Florian Weimer @ 2018-10-17 22:22 UTC (permalink / raw)
To: Andreas Dilger
Cc: Miklos Szeredi, Michael Kerrisk, David Howells,
Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API
In-Reply-To: <E9903911-9289-4038-9835-9353762A3BAF@dilger.ca>
* Andreas Dilger:
>> So what's the point exactly?
>
> Ah, I see your point... STATX_ALL seems to be mostly useful for the kernel
> to mask off flags that it doesn't currently understand. It doesn't make
> much sense for applications to specify STATX_ALL, since they don't have any
> way to know what each flag means unless they are hard-coded to check each of
> the STATX_* flags individually. They should build up a mask of STATX_* flags
> based on what they care about (e.g. "find" should only request attributes
> based on the command-line options given).
Could you remove it from the UAPI header? I didn't want to put it
into the glibc header, but was overruled.
^ permalink raw reply
* Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-17 22:33 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Linux Containers, Linux API, Andy Lutomirski, Oleg Nesterov,
Eric W . Biederman, Serge E . Hallyn, Christian Brauner,
Tyler Hicks, Akihiro Suda, Jann Horn,
linux-fsdevel@vger.kernel.org
In-Reply-To: <CAGXu5jK5yN7zcPUMzFYnUbRrVkLNE0YDCioYdNC3P6mF437+5g@mail.gmail.com>
On Wed, Oct 17, 2018 at 03:21:02PM -0700, Kees Cook wrote:
> On Wed, Oct 17, 2018 at 1:29 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > On Thu, Sep 27, 2018 at 02:31:24PM -0700, Kees Cook wrote:
> >> On Thu, Sep 27, 2018 at 8:11 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> >> > @@ -60,4 +62,29 @@ struct seccomp_data {
> >> > __u64 args[6];
> >> > };
> >> >
> >> > +struct seccomp_notif {
> >> > + __u16 len;
> >> > + __u64 id;
> >> > + __u32 pid;
> >> > + __u8 signaled;
> >> > + struct seccomp_data data;
> >> > +};
> >> > +
> >> > +struct seccomp_notif_resp {
> >> > + __u16 len;
> >> > + __u64 id;
> >> > + __s32 error;
> >> > + __s64 val;
> >> > +};
> >>
> >> So, len has to come first, for versioning. However, since it's ahead
> >> of a u64, this leaves a struct padding hole. pahole output:
> >>
> >> struct seccomp_notif {
> >> __u16 len; /* 0 2 */
> >>
> >> /* XXX 6 bytes hole, try to pack */
> >>
> >> __u64 id; /* 8 8 */
> >> __u32 pid; /* 16 4 */
> >> __u8 signaled; /* 20 1 */
> >>
> >> /* XXX 3 bytes hole, try to pack */
> >>
> >> struct seccomp_data data; /* 24 64 */
> >> /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> >>
> >> /* size: 88, cachelines: 2, members: 5 */
> >> /* sum members: 79, holes: 2, sum holes: 9 */
> >> /* last cacheline: 24 bytes */
> >> };
> >> struct seccomp_notif_resp {
> >> __u16 len; /* 0 2 */
> >>
> >> /* XXX 6 bytes hole, try to pack */
> >>
> >> __u64 id; /* 8 8 */
> >> __s32 error; /* 16 4 */
> >>
> >> /* XXX 4 bytes hole, try to pack */
> >>
> >> __s64 val; /* 24 8 */
> >>
> >> /* size: 32, cachelines: 1, members: 4 */
> >> /* sum members: 22, holes: 2, sum holes: 10 */
> >> /* last cacheline: 32 bytes */
> >> };
> >>
> >> How about making len u32, and moving pid and error above "id"? This
> >> leaves a hole after signaled, so changing "len" won't be sufficient
> >> for versioning here. Perhaps move it after data?
> >
> > Just to confirm my understanding; I've got these as:
> >
> > struct seccomp_notif {
> > __u32 len; /* 0 4 */
> > __u32 pid; /* 4 4 */
> > __u64 id; /* 8 8 */
> > __u8 signaled; /* 16 1 */
> >
> > /* XXX 7 bytes hole, try to pack */
> >
> > struct seccomp_data data; /* 24 64 */
> > /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> >
> > /* size: 88, cachelines: 2, members: 5 */
> > /* sum members: 81, holes: 1, sum holes: 7 */
> > /* last cacheline: 24 bytes */
> > };
> > struct seccomp_notif_resp {
> > __u32 len; /* 0 4 */
> > __s32 error; /* 4 4 */
> > __u64 id; /* 8 8 */
> > __s64 val; /* 16 8 */
> >
> > /* size: 24, cachelines: 1, members: 4 */
> > /* last cacheline: 24 bytes */
> > };
> >
> > in the next version. Since the structure has no padding at the end of
> > it, I think the Right Thing will happen. Note that this is slightly
> > different than what Kees suggested, if I add signaled after data, then
> > I end up with:
> >
> > struct seccomp_notif {
> > __u32 len; /* 0 4 */
> > __u32 pid; /* 4 4 */
> > __u64 id; /* 8 8 */
> > struct seccomp_data data; /* 16 64 */
> > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> > __u8 signaled; /* 80 1 */
> >
> > /* size: 88, cachelines: 2, members: 5 */
> > /* padding: 7 */
> > /* last cacheline: 24 bytes */
> > };
> >
> > which I think will have the versioning problem if the next member
> > introduces is < 7 bytes.
>
> It'll be a problem in either place. What I was thinking was that
> specific versioning is required instead of just length.
Oh, if we decide to use the padded space? Yes, that makes sense. Ok,
I'll switch it to a version.
Tycho
^ permalink raw reply
* Re: [PATCH v5 03/27] x86/fpu/xstate: Introduce XSAVES system states
From: Randy Dunlap @ 2018-10-17 22:39 UTC (permalink / raw)
To: Borislav Petkov, Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pa
In-Reply-To: <20181017104137.GE22535@zn.tnic>
On 10/17/18 3:41 AM, Borislav Petkov wrote:
>> @@ -702,6 +703,7 @@ static int init_xstate_size(void)
>> */
>> static void fpu__init_disable_system_xstate(void)
>> {
>> + xfeatures_mask_all = 0;
>> xfeatures_mask_user = 0;
>> cr4_clear_bits(X86_CR4_OSXSAVE);
>> fpu__xstate_clear_all_cpu_caps();
>> @@ -717,6 +719,8 @@ void __init fpu__init_system_xstate(void)
>> static int on_boot_cpu __initdata = 1;
>> int err;
>> int i;
>> + u64 cpu_user_xfeatures_mask;
>> + u64 cpu_system_xfeatures_mask;
>
> Please sort function local variables declaration in a reverse christmas
> tree order:
>
> <type> longest_variable_name;
> <type> shorter_var_name;
> <type> even_shorter;
> <type> i;
Hi,
Would you mind explaining this request? (requirement?)
Other than to say that it is the preference of some maintainers,
please say Why it is preferred.
and since the <type>s above won't typically be the same length,
it's not for variable name alignment, right?
thanks,
--
~Randy
^ permalink raw reply
* Re: [PATCH v5 03/27] x86/fpu/xstate: Introduce XSAVES system states
From: Borislav Petkov @ 2018-10-17 22:58 UTC (permalink / raw)
To: Randy Dunlap
Cc: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit
In-Reply-To: <32da559b-7958-60db-e328-f0eb316e668e@infradead.org>
On Wed, Oct 17, 2018 at 03:39:47PM -0700, Randy Dunlap wrote:
> Would you mind explaining this request? (requirement?)
> Other than to say that it is the preference of some maintainers,
> please say Why it is preferred.
>
> and since the <type>s above won't typically be the same length,
> it's not for variable name alignment, right?
Searching the net a little, it shows you have asked that question
before. So what is it you really wanna know?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH v5 03/27] x86/fpu/xstate: Introduce XSAVES system states
From: Randy Dunlap @ 2018-10-17 23:17 UTC (permalink / raw)
To: Borislav Petkov
Cc: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit
In-Reply-To: <20181017225829.GA32023@zn.tnic>
On 10/17/18 3:58 PM, Borislav Petkov wrote:
> On Wed, Oct 17, 2018 at 03:39:47PM -0700, Randy Dunlap wrote:
>> Would you mind explaining this request? (requirement?)
>> Other than to say that it is the preference of some maintainers,
>> please say Why it is preferred.
>>
>> and since the <type>s above won't typically be the same length,
>> it's not for variable name alignment, right?
>
> Searching the net a little, it shows you have asked that question
> before. So what is it you really wanna know?
OK, you have shown that your web search skills are better than mine.
I asked what I really wanted to know.
ta.
--
~Randy
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Dave Chinner @ 2018-10-18 0:25 UTC (permalink / raw)
To: Jeff Moyer
Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Jan Kara,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <x49h8hkfhk9.fsf-RRHT56Q3PSP4kTEheFKJxxDDeQx5vsVwAInAS/Ez/D0@public.gmane.org>
On Wed, Oct 17, 2018 at 04:23:50PM -0400, Jeff Moyer wrote:
> Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> writes:
>
> > [Added ext4, xfs, and linux-api folks to CC for the interface discussion]
> >
> > On Tue 02-10-18 14:10:39, Johannes Thumshirn wrote:
> >> On Tue, Oct 02, 2018 at 12:05:31PM +0200, Jan Kara wrote:
> >> > Hello,
> >> >
> >> > commit e1fb4a086495 "dax: remove VM_MIXEDMAP for fsdax and device dax" has
> >> > removed VM_MIXEDMAP flag from DAX VMAs. Now our testing shows that in the
> >> > mean time certain customer of ours started poking into /proc/<pid>/smaps
> >> > and looks at VMA flags there and if VM_MIXEDMAP is missing among the VMA
> >> > flags, the application just fails to start complaining that DAX support is
> >> > missing in the kernel. The question now is how do we go about this?
> >>
> >> OK naive question from me, how do we want an application to be able to
> >> check if it is running on a DAX mapping?
> >
> > The question from me is: Should application really care? After all DAX is
> > just a caching decision. Sure it affects performance characteristics and
> > memory usage of the kernel but it is not a correctness issue (in particular
> > we took care for MAP_SYNC to return EOPNOTSUPP if the feature cannot be
> > supported for current mapping). And in the future the details of what we do
> > with DAX mapping can change - e.g. I could imagine we might decide to cache
> > writes in DRAM but do direct PMEM access on reads. And all this could be
> > auto-tuned based on media properties. And we don't want to tie our hands by
> > specifying too narrowly how the kernel is going to behave.
>
> For read and write, I would expect the O_DIRECT open flag to still work,
> even for dax-capable persistent memory. Is that a contentious opinion?
Not contentious at all, because that's the way it currently works.
FYI, XFS decides what to do with read (and similarly writes) like
this:
if (IS_DAX(inode))
ret = xfs_file_dax_read(iocb, to);
else if (iocb->ki_flags & IOCB_DIRECT)
ret = xfs_file_dio_aio_read(iocb, to);
else
ret = xfs_file_buffered_aio_read(iocb, to);
Neither DAX or O_DIRECT on pmem use the page cache - the only difference
between the DAX read/write path and the O_DIRECT read/write path
is where the memcpy() into the user buffer is done. For DAX
it's done in the fsdax layer, for O_DIRECT it's done in the pmem
block driver.
> So, what we're really discussing is the behavior for mmap.
Yes.
> MAP_SYNC
> will certainly ensure that the page cache is not used for writes. It
> would also be odd for us to decide to cache reads. The only issue I can
> see is that perhaps the application doesn't want to take a performance
> hit on write faults. I haven't heard that concern expressed in this
> thread, though.
>
> Just to be clear, this is my understanding of the world:
>
> MAP_SYNC
> - file system guarantees that metadata required to reach faulted-in file
> data is consistent on media before a write fault is completed. A
> side-effect is that the page cache will not be used for
> writably-mapped pages.
I think you are conflating current implementation with API
requirements - MAP_SYNC doesn't guarantee anything about page cache
use. The man page definition simply says "supported only for files
supporting DAX" and that it provides certain data integrity
guarantees. It does not define the implementation.
We've /implemented MAP_SYNC/ as O_DSYNC page fault behaviour,
because that's the only way we can currently provide the required
behaviour to userspace. However, if a filesystem can use the page
cache to provide the required functionality, then it's free to do
so.
i.e. if someone implements a pmem-based page cache, MAP_SYNC data
integrity could be provided /without DAX/ by any filesystem using
that persistent page cache. i.e. MAP_SYNC really only requires
mmap() of CPU addressable persistent memory - it does not require
DAX. Right now, however, the only way to get this functionality is
through a DAX capable filesystem on dax capable storage.
And, FWIW, this is pretty much how NOVA maintains DAX w/ COW - it
COWs new pages in pmem and attaches them a special per-inode cache
on clean->dirty transition. Then on data sync, background writeback
or crash recovery, it migrates them from the cache into the file map
proper via atomic metadata pointer swaps.
IOWs, NOVA provides the correct MAP_SYNC semantics by using a
separate persistent per-inode write cache to provide the correct
crash recovery semantics for MAP_SYNC.
> and what I think Dan had proposed:
>
> mmap flag, MAP_DIRECT
> - file system guarantees that page cache will not be used to front storage.
> storage MUST be directly addressable. This *almost* implies MAP_SYNC.
> The subtle difference is that a write fault /may/ not result in metadata
> being written back to media.
SIimilar to O_DIRECT, these semantics do not allow userspace apps to
replace msync/fsync with CPU cache flush operations. So any
application that uses this mode still needs to use either MAP_SYNC
or issue msync/fsync for data integrity.
If the app is using MAP_DIRECT, the what do we do if the filesystem
can't provide the required semantics for that specific operation? In
the case of O_DIRECT, we fall back to buffered IO because it has the
same data integrity semantics as O_DIRECT and will always work. It's
just slower and consumes more memory, but the app continues to work
just fine.
Sending SIGBUS to apps when we can't perform MAP_DIRECT operations
without using the pagecache seems extremely problematic to me. e.g.
an app already has an open MAP_DIRECT file, and a third party
reflinks it or dedupes it and the fs has to fall back to buffered IO
to do COW operations. This isn't the app's fault - the kernel should
just fall back transparently to using the page cache for the
MAP_DIRECT app and just keep working, just like it would if it was
using O_DIRECT read/write.
The point I'm trying to make here is that O_DIRECT is a /hint/, not
a guarantee, and it's done that way to prevent applications from
being presented with transient, potentially fatal error cases
because a filesystem implementation can't do a specific operation
through the direct IO path.
IMO, MAP_DIRECT needs to be a hint like O_DIRECT and not a
guarantee. Over time we'll end up with filesystems that can
guarantee that MAP_DIRECT is always going to use DAX, in the same
way we have filesystems that guarantee O_DIRECT will always be
O_DIRECT (e.g. XFS). But if we decide that MAP_DIRECT must guarantee
no page cache will ever be used, then we are basically saying
"filesystems won't provide MAP_DIRECT even in common, useful cases
because they can't provide MAP_DIRECT in all cases." And that
doesn't seem like a very good solution to me.
> and this is what I think you were proposing, Jan:
>
> madvise flag, MADV_DIRECT_ACCESS
> - same semantics as MAP_DIRECT, but specified via the madvise system call
Seems to be the equivalent of fcntl(F_SETFL, O_DIRECT). Makes sense
to have both MAP_DIRECT and MADV_DIRECT_ACCESS to me - one is an
init time flag, the other is a run time flag.
Cheers,
Dave.
--
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-10-18 7:00 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1810171256330.60837@chino.kir.corp.google.com>
On Wed 17-10-18 12:59:18, David Rientjes wrote:
> On Wed, 17 Oct 2018, Michal Hocko wrote:
>
> > Do you know of any other userspace except your usecase? Is there
> > anything fundamental that would prevent a proper API adoption for you?
> >
>
> Yes, it would require us to go back in time and build patched binaries.
I read that as there is a fundamental problem to update existing
binaries. If that is the case then there surely is no way around it
and another sad page in the screwed up APIs book we provide.
But I was under impression that the SW stack which actually does the
monitoring is under your controll. Moreover I was under impression that
you do not use the current vanilla kernel so there is no need for an
immediate change on your end. It is trivial to come up with a backward
compatible way to check for the new flag (if it is not present then
fallback to vma flags).
I am sorry for pushing here but if this is just a matter of a _single_
user which _can_ be fixed with a reasonable effort then I would love to
see the future api unscrewed.
--
Michal Hocko
SUSE Labs
^ 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