Linux userland API discussions
 help / color / mirror / Atom feed
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-16 16:12 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Dominique Martinet, Andy Lutomirski, Josh Snyder, Dave Chinner,
	Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
	Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wjVjecbGRcxZUSwoSgAq9ZbMxbA=MOiqDrPgx7_P3xGhg@mail.gmail.com>

On Wed, 16 Jan 2019, Linus Torvalds wrote:

> > "Being owner or has cap" (whichever cap) is probably OK. On the other 
> > hand, writeability check makes more sense in general - could we 
> > somehow check if the user has write access to the file instead of 
> > checking if it currently is opened read-write?
> 
> That's likely the best option. We could say "is it open for write, or
> _could_ we open it for writing?"
> 
> It's a slightly annoying special case, and I'd have preferred to avoid
> it, but it doesn't sound *compilcated*.
> 
> I'm on the road, but I did send out this:
> 
>     https://lore.kernel.org/lkml/CAHk-=wif_9nvNHJiyxHzJ80_WUb0P7CXNBvXkjZz-r1u0ozp7g@mail.gmail.com/
> 
> originally. The "let's try to only do the mmap residency" was the
> optimistic "maybe we can just get rid of this complexity entirely"
> version..
> 
> Anybody willing to test the above patch instead? And replace the
> 
>    || capable(CAP_SYS_ADMIN)
> 
> check with something like
> 
>    || inode_permission(inode, MAY_WRITE) == 0
> 
> instead?
> 
> (This is obviously after you've reverted the "only check mmap
> residency" patch..)

So that seems to deal with mincore() in a reasonable way indeed.

It doesn't unfortunately really solve the preadv2(RWF_NOWAIT), nor does it 
provide any good answer what to do about it, does it?

Thanks,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply

* [PATCH 2/5] ipc: introduce ksys_ipc()/compat_ksys_ipc() for s390
From: Arnd Bergmann @ 2019-01-16 13:15 UTC (permalink / raw)
  To: linux-s390, Martin Schwidefsky, Heiko Carstens
  Cc: linux-kernel, y2038, Dominik Brodowski, Mark Rutland,
	Arnd Bergmann, linux-api
In-Reply-To: <20190116131527.2071570-1-arnd@arndb.de>

The sys_ipc() and compat_ksys_ipc() functions are meant to only
be used from the system call table, not called by another function.

Introduce ksys_*() interfaces for this purpose, as we have done
for many other system calls.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/s390/kernel/compat_linux.c |  2 +-
 arch/s390/kernel/sys_s390.c     |  2 +-
 include/linux/syscalls.h        |  4 ++++
 ipc/syscall.c                   | 20 ++++++++++++++++----
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kernel/compat_linux.c b/arch/s390/kernel/compat_linux.c
index 8ac38d51ed7d..a47f6d3c6d5b 100644
--- a/arch/s390/kernel/compat_linux.c
+++ b/arch/s390/kernel/compat_linux.c
@@ -296,7 +296,7 @@ COMPAT_SYSCALL_DEFINE5(s390_ipc, uint, call, int, first, compat_ulong_t, second,
 {
 	if (call >> 16)		/* hack for backward compatibility */
 		return -EINVAL;
-	return compat_sys_ipc(call, first, second, third, ptr, third);
+	return compat_ksys_ipc(call, first, second, third, ptr, third);
 }
 #endif
 
diff --git a/arch/s390/kernel/sys_s390.c b/arch/s390/kernel/sys_s390.c
index 560bdaf8a74f..6aa8fe00b39e 100644
--- a/arch/s390/kernel/sys_s390.c
+++ b/arch/s390/kernel/sys_s390.c
@@ -74,7 +74,7 @@ SYSCALL_DEFINE5(s390_ipc, uint, call, int, first, unsigned long, second,
 	 * Therefore we can call the generic variant by simply passing the
 	 * third parameter also as fifth parameter.
 	 */
-	return sys_ipc(call, first, second, third, ptr, third);
+	return ksys_ipc(call, first, second, third, ptr, third);
 }
 
 SYSCALL_DEFINE1(s390_personality, unsigned int, personality)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 257cccba3062..fb63045a0fb6 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1185,6 +1185,10 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 			      unsigned long prot, unsigned long flags,
 			      unsigned long fd, unsigned long pgoff);
 ssize_t ksys_readahead(int fd, loff_t offset, size_t count);
+int ksys_ipc(unsigned int call, int first, unsigned long second,
+	unsigned long third, void __user * ptr, long fifth);
+int compat_ksys_ipc(u32 call, int first, int second,
+	u32 third, u32 ptr, u32 fifth);
 
 /*
  * The following kernel syscall equivalents are just wrappers to fs-internal
diff --git a/ipc/syscall.c b/ipc/syscall.c
index 1ac06e3983c0..3cf8ad703a4d 100644
--- a/ipc/syscall.c
+++ b/ipc/syscall.c
@@ -17,8 +17,8 @@
 #include <linux/shm.h>
 #include <linux/uaccess.h>
 
-SYSCALL_DEFINE6(ipc, unsigned int, call, int, first, unsigned long, second,
-		unsigned long, third, void __user *, ptr, long, fifth)
+int ksys_ipc(unsigned int call, int first, unsigned long second,
+	unsigned long third, void __user * ptr, long fifth)
 {
 	int version, ret;
 
@@ -106,6 +106,12 @@ SYSCALL_DEFINE6(ipc, unsigned int, call, int, first, unsigned long, second,
 		return -ENOSYS;
 	}
 }
+
+SYSCALL_DEFINE6(ipc, unsigned int, call, int, first, unsigned long, second,
+		unsigned long, third, void __user *, ptr, long, fifth)
+{
+	return ksys_ipc(call, first, second, third, ptr, fifth);
+}
 #endif
 
 #ifdef CONFIG_COMPAT
@@ -121,8 +127,8 @@ struct compat_ipc_kludge {
 };
 
 #ifdef CONFIG_ARCH_WANT_OLD_COMPAT_IPC
-COMPAT_SYSCALL_DEFINE6(ipc, u32, call, int, first, int, second,
-	u32, third, compat_uptr_t, ptr, u32, fifth)
+int compat_ksys_ipc(u32 call, int first, int second,
+	u32 third, compat_uptr_t ptr, u32 fifth)
 {
 	int version;
 	u32 pad;
@@ -195,5 +201,11 @@ COMPAT_SYSCALL_DEFINE6(ipc, u32, call, int, first, int, second,
 
 	return -ENOSYS;
 }
+
+COMPAT_SYSCALL_DEFINE6(ipc, u32, call, int, first, int, second,
+	u32, third, compat_uptr_t, ptr, u32, fifth)
+{
+	return compat_ksys_ipc(call, first, second, third, ptr, fifth);
+}
 #endif
 #endif
-- 
2.20.0

^ permalink raw reply related

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Matthew Wilcox @ 2019-01-16 12:36 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Josh Snyder, Dominique Martinet, Dave Chinner, Jiri Kosina,
	Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko,
	Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wgF9p9xNzZei_-ejGLy1bJf4VS1C5E9_V0kCTEpCkpCTQ@mail.gmail.com>

On Wed, Jan 16, 2019 at 05:00:25PM +1200, Linus Torvalds wrote:
> And if you're not the owner of the file, do you have another
> suggestion for that "Yes, I have the right to see what's in-core for
> this file". Because the problem is literally that if it's some random
> read-only system file, the kernel shouldn't leak access patterns to
> it..

This probably isn't a good heuristic, but thought I'd mention it
anyway ...  if the file is executable and you're not the owner, mincore
always/never says its pages are resident.  That'd fix all library leaks,
but then there's probably a smart way of figuring out something from
access patterns to a data file of some kind (/etc/passwd?)

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Kevin Easton @ 2019-01-16 12:18 UTC (permalink / raw)
  To: Josh Snyder
  Cc: Dominique Martinet, Linus Torvalds, Andy Lutomirski, Dave Chinner,
	Jiri Kosina, Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH,
	Peter Zijlstra, Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <CA+t-nXTfdo07EBvVo+mu8SRhrVyB=mEPLDQikHfpJue1jALJtQ@mail.gmail.com>

On Tue, Jan 15, 2019 at 11:52:25PM -0800, Josh Snyder wrote:
> On Tue, Jan 15, 2019 at 10:34 PM Dominique Martinet <asmadeus@codewreck.org>
> wrote:
> >
> > There is a difference with your previous patch though, that used to list no
> > page in core when it didn't know; this patch lists pages as in core when it
> > refuses to tell. I don't think that's very important, though.
> 
> Is there a reason not to return -EPERM in this case?

When I was looking through the Debian Code Search results, quite a few
of the hits were for code that uses mincore() as a way to check if
_anything_ is mapped at an address or not.  This code doesn't care about
the in core / not in core result of mincore(), just whether it returned
an error or not.

I think a new error return would break most of the instances of that
code I saw.

    - Kevin

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Josh Snyder @ 2019-01-16  7:52 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Linus Torvalds, Andy Lutomirski, Dave Chinner, Jiri Kosina,
	Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
	Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <20190116063430.GA22938@nautica>

On Tue, Jan 15, 2019 at 10:34 PM Dominique Martinet <asmadeus@codewreck.org>
wrote:
>
> There is a difference with your previous patch though, that used to list no
> page in core when it didn't know; this patch lists pages as in core when it
> refuses to tell. I don't think that's very important, though.

Is there a reason not to return -EPERM in this case?

>
> If anything, the 0400 user-owner file might be a problem in some edge
> case (e.g. if you're preloading git directories, many objects are 0444);
> should we *also* check ownership?...

Yes, this seems valuable. Some databases with immutable files (e.g. git, as
you've mentioned) conceivably operate this way.

Josh

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Dominique Martinet @ 2019-01-16  6:34 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andy Lutomirski, Josh Snyder, Dave Chinner, Jiri Kosina,
	Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
	Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wjVjecbGRcxZUSwoSgAq9ZbMxbA=MOiqDrPgx7_P3xGhg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]

Linus Torvalds wrote on Wed, Jan 16, 2019:
> Anybody willing to test the above patch instead? And replace the
> 
>    || capable(CAP_SYS_ADMIN)
> 
> check with something like
> 
>    || inode_permission(inode, MAY_WRITE) == 0
> 
> instead?
> 
> (This is obviously after you've reverted the "only check mmap
> residency" patch..)

That seems to work on an x86_64 vm.

I've tested with the attached patch:
 - root can lookup pages on any file I tried;
 - user can lookup page on file it owns, assuming it can write to it
(e.g. it won't work on a 0400 file you own)
 - user cannot lookup pages on e.g. /lib64/libc-2.28.so

There is a difference with your previous patch though, that used to list
no page in core when it didn't know; this patch lists pages as in core
when it refuses to tell. I don't think that's very important, though.

If anything, the 0400 user-owner file might be a problem in some edge
case (e.g. if you're preloading git directories, many objects are 0444);
should we *also* check ownership?...

-- 
Dominique

[-- Attachment #2: mincore.diff --]
[-- Type: text/x-diff, Size: 1197 bytes --]

 mm/mincore.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/mm/mincore.c b/mm/mincore.c
index 218099b5ed31..11ed7064f4eb 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -169,6 +169,13 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 	return 0;
 }
 
+static inline bool can_do_mincore(struct vm_area_struct *vma)
+{
+	return vma_is_anonymous(vma)
+		|| (vma->vm_file && (vma->vm_file->f_mode & FMODE_WRITE))
+		|| inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
+}
+
 /*
  * Do a chunk of "sys_mincore()". We've already checked
  * all the arguments, we hold the mmap semaphore: we should
@@ -189,8 +196,13 @@ static long do_mincore(unsigned long addr, unsigned long pages, unsigned char *v
 	vma = find_vma(current->mm, addr);
 	if (!vma || addr < vma->vm_start)
 		return -ENOMEM;
-	mincore_walk.mm = vma->vm_mm;
 	end = min(vma->vm_end, addr + (pages << PAGE_SHIFT));
+	if (!can_do_mincore(vma)) {
+		unsigned long pages = (end - addr) >> PAGE_SHIFT;
+		memset(vec, 1, pages);
+		return pages;
+	}
+	mincore_walk.mm = vma->vm_mm;
 	err = walk_page_range(addr, end, &mincore_walk);
 	if (err < 0)
 		return err;

^ permalink raw reply related

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-16  5:58 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Andy Lutomirski, Josh Snyder, Dave Chinner, Jiri Kosina,
	Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
	Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <20190116054613.GA11670@nautica>

On Wed, Jan 16, 2019 at 5:46 PM Dominique Martinet
<asmadeus@codewreck.org> wrote:
>
> "Being owner or has cap" (whichever cap) is probably OK.
> On the other hand, writeability check makes more sense in general -
> could we somehow check if the user has write access to the file instead
> of checking if it currently is opened read-write?

That's likely the best option. We could say "is it open for write, or
_could_ we open it for writing?"

It's a slightly annoying special case, and I'd have preferred to avoid
it, but it doesn't sound *compilcated*.

I'm on the road, but I did send out this:

    https://lore.kernel.org/lkml/CAHk-=wif_9nvNHJiyxHzJ80_WUb0P7CXNBvXkjZz-r1u0ozp7g@mail.gmail.com/

originally. The "let's try to only do the mmap residency" was the
optimistic "maybe we can just get rid of this complexity entirely"
version..

Anybody willing to test the above patch instead? And replace the

   || capable(CAP_SYS_ADMIN)

check with something like

   || inode_permission(inode, MAY_WRITE) == 0

instead?

(This is obviously after you've reverted the "only check mmap
residency" patch..)

            Linus

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-16  5:49 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Dominique Martinet, Jiri Kosina, Matthew Wilcox, Jann Horn,
	Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko, Linux-MM,
	kernel list, Linux API
In-Reply-To: <CAHk-=wjc2inOae8+9-DK4jFK78-7ZpNR=TEyZg0Dj57SYwP-ng@mail.gmail.com>

On Wed, Jan 16, 2019 at 4:54 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Jan 16, 2019 at 11:45 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > I'm assuming that you can invalidate the page cache reliably by a
> > means that does not repeated require probing to detect invalidation
> > has occurred. I've mentioned one method in this discussion
> > already...
>
> Yes. And it was made clear to you that it was a bug in xfs dio and
> what the right thing to do was.

Side note: I actually think we *do* the right thing. Even for xfs. I
couldn't find the alleged place that invalidates the page cache on dio
reads.

The *generic* dio code only does it for writes (which is correct and
fine). And maybe xfs has some extra invalidation, but I don't see it.

So I actually hope your "you can use direct-io read to do directed
invalidating of the page cache" isn't true. I admittedly did *not* try
to delve very deeply into it, but the invalidates I found looked
correct. The generic code does it for writes, and at least ext4 does
the "writeback and wait" for reads.

There *does* seem to be a 'invalidate_inode_pages2_range()' call in
iomap_dio_rw(). That has a *comment* that says it only is for writes,
but it looks to me like it would trigger for reads too.

Just a plain bug/oversight? Or me misreading things.

So yes, maybe xfs does that "invalidate on read", but it really seems
to be just a bug. If the xfs people insist on keeping the bug, fine
(looks like gfs2 and xfs are the only users), but it seems kind of
sad.

             Linus

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Dominique Martinet @ 2019-01-16  5:46 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andy Lutomirski, Josh Snyder, Dave Chinner, Jiri Kosina,
	Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
	Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wjqkbjL2_BwUYxJxJhdadiw6Zx-Yu_mK3E6P7kG3wSGcQ@mail.gmail.com>

Linus Torvalds wrote on Wed, Jan 16, 2019:
> *Very* few people want to run their databases as root.

In the case of happycache, this isn't the database doing the
dump/restore, but a separate process that could have the cap - it's
better if we can do without though, and from his readme he runs as user
cassandra in the /var/lib/cassandra directory for example so that'd
match the file owner.

For pgfincore, it's a postgres extension so the main process does it -
but it does have files open as write as well as being the owner.

> Jiri's original patch kind of acknowledged that by making the new test
> be conditional, and off by default. So then it's a "only do this for
> lockdown mode, because normal people won't find it acceptable".
> 
> And I'm not a huge fan of that approach. If you don't protect normal
> people, then what's the point, really?

I agree with that. 
"Being owner or has cap" (whichever cap) is probably OK.
On the other hand, writeability check makes more sense in general -
could we somehow check if the user has write access to the file instead
of checking if it currently is opened read-write?

-- 
Dominique

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-16  5:34 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Josh Snyder, Dominique Martinet, Dave Chinner, Jiri Kosina,
	Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
	Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <9E337EA6-7CDA-457B-96C6-E91F83742587@amacapital.net>

On Wed, Jan 16, 2019 at 5:25 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
> Something like CAP_DAC_READ_SEARCH might not be crazy.

I agree that it would work. In fact' it's what Jiri's patch basically
did. Except Jiri used CAP_SYS_ADMIN instead.

But that then basically limits it to root (or root-like with
capability masks), which is quite likely to not work in practice all
that well. That's why I wanted to find alternatives.

*Very* few people want to run their databases as root.

Jiri's original patch kind of acknowledged that by making the new test
be conditional, and off by default. So then it's a "only do this for
lockdown mode, because normal people won't find it acceptable".

And I'm not a huge fan of that approach. If you don't protect normal
people, then what's the point, really?

              Linus

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Andy Lutomirski @ 2019-01-16  5:25 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Josh Snyder, Dominique Martinet, Dave Chinner, Jiri Kosina,
	Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
	Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wgF9p9xNzZei_-ejGLy1bJf4VS1C5E9_V0kCTEpCkpCTQ@mail.gmail.com>



> On Jan 15, 2019, at 9:00 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Wed, Jan 16, 2019 at 12:42 PM Josh Snyder <joshs@netflix.com> wrote:
>> 
>> For Netflix, losing accurate information from the mincore syscall would
>> lengthen database cluster maintenance operations from days to months.  We
>> rely on cross-process mincore to migrate the contents of a page cache from
>> machine to machine, and across reboots.
> 
> Ok, this is the kind of feedback we need, and means I guess we can't
> just use the mapping existence for mincore.
> 
> The two other ways that we considered were:
> 
> (a) owner of the file gets to know cache information for that file.
> 
> (b) having the fd opened *writably* gets you cache residency information.
> 
> Sadly, taking a look at happycache, you open the file read-only, so
> (b) doesn't work.
> 
> Judging just from the source code, I can't tell how the user ownership
> works. Any input on that?
> 
> And if you're not the owner of the file, do you have another
> suggestion for that "Yes, I have the right to see what's in-core for
> this file". Because the problem is literally that if it's some random
> read-only system file, the kernel shouldn't leak access patterns to
> it..
> 
> 


Something like CAP_DAC_READ_SEARCH might not be crazy.

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-16  5:00 UTC (permalink / raw)
  To: Josh Snyder
  Cc: Dominique Martinet, Dave Chinner, Jiri Kosina, Matthew Wilcox,
	Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko,
	Linux-MM, kernel list, Linux API
In-Reply-To: <5c3e7de6.1c69fb81.4aebb.3fec@mx.google.com>

On Wed, Jan 16, 2019 at 12:42 PM Josh Snyder <joshs@netflix.com> wrote:
>
> For Netflix, losing accurate information from the mincore syscall would
> lengthen database cluster maintenance operations from days to months.  We
> rely on cross-process mincore to migrate the contents of a page cache from
> machine to machine, and across reboots.

Ok, this is the kind of feedback we need, and means I guess we can't
just use the mapping existence for mincore.

The two other ways that we considered were:

 (a) owner of the file gets to know cache information for that file.

 (b) having the fd opened *writably* gets you cache residency information.

Sadly, taking a look at happycache, you open the file read-only, so
(b) doesn't work.

Judging just from the source code, I can't tell how the user ownership
works. Any input on that?

And if you're not the owner of the file, do you have another
suggestion for that "Yes, I have the right to see what's in-core for
this file". Because the problem is literally that if it's some random
read-only system file, the kernel shouldn't leak access patterns to
it..

                     Linus

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-16  4:54 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Dominique Martinet, Jiri Kosina, Matthew Wilcox, Jann Horn,
	Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko, Linux-MM,
	kernel list, Linux API
In-Reply-To: <20190115234510.GA6173@dastard>

On Wed, Jan 16, 2019 at 11:45 AM Dave Chinner <david@fromorbit.com> wrote:
>
> I'm assuming that you can invalidate the page cache reliably by a
> means that does not repeated require probing to detect invalidation
> has occurred. I've mentioned one method in this discussion
> already...

Yes. And it was made clear to you that it was a bug in xfs dio and
what the right thing to do was.

And you ignored that, and claimed it was a feature.

Why do you then bother arguing this thing? We absolutely agree that
xfs has an information leak. If you don't care, just _say_ so. Don't
try to argue against other people who are trying to fix things.

We can easily just say "ok, xfs people don't care", and ignore the xfs
invalidation issue. That's fine.

But don't try to make it a big deal for other filesystems that _don't_
have the bug. I even pointed out how ext4 does the page cache flushing
correcrly. You pooh-poohed it.

You can't have it both ways.

Either you care or you don't. If you don't care (and so far everything
you said seems to imply you don't), then why are you even discussing
this? Just admit you don't care, and we're done.

                  Linus

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Josh Snyder @ 2019-01-16  0:42 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Dominique Martinet, Dave Chinner, Jiri Kosina, Matthew Wilcox,
	Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko,
	Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wip2CPrdOwgF0z4n2tsdW7uu+Egtcx9Mxxe3gPfPW_JmQ@mail.gmail.com>

Linus Torvalds wrote on Thu, Jan 10, 2019:
> So right now, I consider the mincore change to be a "try to probe the
> state of mincore users", and we haven't really gotten a lot of
> information back yet.

<raises hand>

For Netflix, losing accurate information from the mincore syscall would
lengthen database cluster maintenance operations from days to months.  We
rely on cross-process mincore to migrate the contents of a page cache from
machine to machine, and across reboots.

To do this, I wrote and maintain happycache [1], a page cache dumper/loader
tool. It is quite similar in architecture to pgfincore, except that it is
agnostic to workload. The gist of happycache's operation is "produce a dump
of residence status for each page, do some operation, then reload exactly
the same pages which were present before." happycache is entirely dependent
on accurate reporting of the in-core status of file-backed pages, as
accessed by another process.

We primarily use happycache with Cassandra, which (like Postgres +
pgfincore) relies heavily on OS page cache to reduce disk accesses. Because
our workloads never experience a cold page cache, we are able to provision
hardware for a peak utilization level that is far lower than the
hypothetical "every query is a cache miss" peak.

A database warmed by happycache can be ready for service in seconds
(bounded only by the performance of the drives and the I/O subsystem), with
no period of in-service degradation. By contrast, putting a database in
service without a page cache entails a potentially unbounded period of
degradation (at Netflix, the time to populate a single node's cache via
natural cache misses varies by workload from hours to weeks). If a single
node upgrade were to take weeks, then upgrading an entire cluster would
take months. Since we want to apply security upgrades (and other things) on
a somewhat tighter schedule, we would have to develop more complex
solutions to provide the same functionality already provided by mincore.

At the bottom line, happycache is designed to benignly exploit the same
information leak documented in the paper [2]. I think it makes perfect
sense to remove cross-process mincore functionality from unprivileged
users, but not to remove it entirely.

Josh Snyder
Netflix Cloud Database Engineering

[1] https://github.com/hashbrowncipher/happycache
[2] https://arxiv.org/abs/1901.01161

^ permalink raw reply

* Re: [PATCH 14/15] arch: add split IPC system calls where needed
From: Michael Ellerman @ 2019-01-16  0:10 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Rich Felker, linux-ia64, Linux-sh list, Catalin Marinas,
	Heiko Carstens, Dominik Brodowski, linux-mips, Max Filippov,
	Deepa Dinamani, H. Peter Anvin, sparclinux, linux-s390,
	Davidlohr Bueso, y2038 Mailman List, Helge Deller,
	the arch/x86 maintainers, Russell King - ARM Linux, Ingo Molnar,
	Geert Uytterhoeven, Firoz Khan, Matt Turner, Fenghua Yu,
	Will Deacon
In-Reply-To: <CAK8P3a0uas76i2W1yjFvxHT-2=A8_egUZeAUM-Vya6386+87Xg@mail.gmail.com>

Arnd Bergmann <arnd@arndb.de> writes:
> On Mon, Jan 14, 2019 at 4:59 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Arnd Bergmann <arnd@arndb.de> writes:
>> >  arch/m68k/kernel/syscalls/syscall.tbl     | 11 +++++++++++
>> >  arch/mips/kernel/syscalls/syscall_o32.tbl | 11 +++++++++++
>> >  arch/powerpc/kernel/syscalls/syscall.tbl  | 12 ++++++++++++
>>
>> I have some changes I'd like to make to our syscall table that will
>> clash with this.
>>
>> I'll try and send them today.
>
> Ok. Are those for 5.0 or 5.1? If they are intended for 5.0, it would be
> nice for me to have a branch based on 5.0-rc1 that I can put
> the other patches on top of.

For 5.1.

I can put them in a topic branch for you.

>> > diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
>> > index db3bbb8744af..1bffab54ff35 100644
>> > --- a/arch/powerpc/kernel/syscalls/syscall.tbl
>> > +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
>> > @@ -425,3 +425,15 @@
>> >  386  nospu   pkey_mprotect                   sys_pkey_mprotect
>> >  387  nospu   rseq                            sys_rseq
>> >  388  nospu   io_pgetevents                   sys_io_pgetevents               compat_sys_io_pgetevents
>> > +# room for arch specific syscalls
>> > +392  64      semtimedop                      sys_semtimedop
>> > +393  common  semget                          sys_semget
>> > +394  common  semctl                          sys_semctl                      compat_sys_semctl
>> > +395  common  shmget                          sys_shmget
>> > +396  common  shmctl                          sys_shmctl                      compat_sys_shmctl
>> > +397  common  shmat                           sys_shmat                       compat_sys_shmat
>> > +398  common  shmdt                           sys_shmdt
>> > +399  common  msgget                          sys_msgget
>> > +400  common  msgsnd                          sys_msgsnd                      compat_sys_msgsnd
>> > +401  common  msgrcv                          sys_msgrcv                      compat_sys_msgrcv
>> > +402  common  msgctl                          sys_msgctl                      compat_sys_msgctl
>>
>> We already have a gap at 366-377 from when we tried to add the split IPC
>> calls a few years back.
>>
>> I guess I don't mind leaving that gap and using the common numbers as
>> you've done here.
>>
>> But it would be good to add a comment pointing out that we have room
>> at 366 for more arch specific syscalls as well.
>
> Ah, I missed that. I've added this to my patch now:
>
> index 5c0936d862fc..2ddfba536d5f 100644
> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> @@ -460,6 +460,7 @@
>  363    spu     switch_endian                   sys_ni_syscall
>  364    common  userfaultfd                     sys_userfaultfd
>  365    common  membarrier                      sys_membarrier
> +# 366-377 originally left for IPC, now unused
>  378    nospu   mlock2                          sys_mlock2
>  379    nospu   copy_file_range                 sys_copy_file_range
>  380    common  preadv2                         sys_preadv2
>          compat_sys_preadv2

Thanks.

cheers

^ permalink raw reply

* Re: [PATCH 14/15] arch: add split IPC system calls where needed
From: Michael Ellerman @ 2019-01-16  0:09 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Rich Felker, linux-ia64, Linux-sh list, Catalin Marinas,
	Heiko Carstens, Dominik Brodowski, linux-mips, Max Filippov,
	Deepa Dinamani, H. Peter Anvin, sparclinux, linux-s390,
	Davidlohr Bueso, y2038 Mailman List, Helge Deller,
	the arch/x86 maintainers, Russell King - ARM Linux, Ingo Molnar,
	Geert Uytterhoeven, Firoz Khan, Matt Turner, Fenghua Yu,
	Will Deacon
In-Reply-To: <CAK8P3a3eUzGDrJtV2ySpgHwjKHwZYr+1xHp6tChCt1gWi=mJ+g@mail.gmail.com>

Arnd Bergmann <arnd@arndb.de> writes:

> On Tue, Jan 15, 2019 at 4:01 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Mon, Jan 14, 2019 at 4:59 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>> > Arnd Bergmann <arnd@arndb.de> writes:
>> > >  arch/m68k/kernel/syscalls/syscall.tbl     | 11 +++++++++++
>> > >  arch/mips/kernel/syscalls/syscall_o32.tbl | 11 +++++++++++
>> > >  arch/powerpc/kernel/syscalls/syscall.tbl  | 12 ++++++++++++
>> >
>> > I have some changes I'd like to make to our syscall table that will
>> > clash with this.
>> >
>> > I'll try and send them today.
>>
>> Ok. Are those for 5.0 or 5.1? If they are intended for 5.0, it would be
>> nice for me to have a branch based on 5.0-rc1 that I can put
>> the other patches on top of.
>
> There is also another change that I considered:
>
> At the end of my series, we have a lot of entries like
>
> 245     32      clock_settime                   sys_clock_settime32
> 245     64      clock_settime                   sys_clock_settime
> 245     spu     clock_settime                   sys_clock_settime
>
> which could be folded into
>
> 245     32      clock_settime                   sys_clock_settime32
> 245     spu64 clock_settime                   sys_clock_settime
>
> if we just add another option to the ABI field. Any thoughts on
> that?

My series splits spu out into a separate field. So the above would be:

245     32	-	clock_settime                   sys_clock_settime32
245     64	spu	clock_settime                   sys_clock_settime

cheers
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

^ permalink raw reply

* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Dave Chinner @ 2019-01-15 23:45 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Dominique Martinet, Jiri Kosina, Matthew Wilcox, Jann Horn,
	Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko, Linux-MM,
	kernel list, Linux API
In-Reply-To: <CAHk-=wj+xyz_GKjgKpU6SF3qeqouGmRoR8uFxzg_c1VpeGEJMw@mail.gmail.com>

On Fri, Jan 11, 2019 at 08:26:14AM -0800, Linus Torvalds wrote:
> On Thu, Jan 10, 2019 at 11:36 PM Dave Chinner <david@fromorbit.com> wrote:
> >
> > > It's only that single page that *matters*. That's the page that the
> > > probe reveals the status of - but it's also the page that the probe
> > > then *changes* the status of.
> >
> > It changes the state of it /after/ we've already got the information
> > we need from it. It's not up to date, it has to come from disk, we
> > return EAGAIN, which means it was not in the cache.
> 
> Oh, I see the confusion.
> 
> Yes, you get the information about whether something was in the cache
> or not, so the side channel does exist to some degree.
> 
> But it's actually hugely reduced for a rather important reason: the
> _primary_ reason for needing to know whether some page is in the cache
> or not is not actually to see if it was ever accessed - it's to see
> that the cache has been scrubbed (and to _guide_ the scrubbing), and
> *when* it was accessed.

Oh, you're assuming that you need to probe the page cache to
determine if brute force invalidation has progressed far enough
to invalidate the page in question.

I'm assuming that you can invalidate the page cache reliably by a
means that does not repeated require probing to detect invalidation
has occurred. I've mentioned one method in this discussion
already...

IOWs, just because the specific brute force attack documented in the
paper required repeated probing it doesn't mean all future
invalidation attacks will require repeated probing. Hence a robust
defense mechanism should not rely on the attacker requiring multiple
observations of the page cache to extract the information they are
seeking...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply

* Re: [PATCH 14/15] arch: add split IPC system calls where needed
From: Arnd Bergmann @ 2019-01-15 21:24 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Michael Ellerman, y2038 Mailman List, Linux API,
	Linux Kernel Mailing List, Ivan Kokshaysky, Matt Turner,
	Russell King - ARM Linux, Catalin Marinas, Will Deacon, Tony Luck,
	Fenghua Yu, Michal Simek, Paul Burton, Helge Deller,
	Martin Schwidefsky, Heiko Carstens, Rich Felker, David Miller,
	Andy Lutomirski
In-Reply-To: <CAMuHMdVw1mBLGdn4hkiNVyoJ_oxMwi3d=_e7WL5ru+ALA4MKbw@mail.gmail.com>

On Tue, Jan 15, 2019 at 5:36 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Jan 15, 2019 at 4:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tue, Jan 15, 2019 at 4:01 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Mon, Jan 14, 2019 at 4:59 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> > > > Arnd Bergmann <arnd@arndb.de> writes:
> > > > >  arch/m68k/kernel/syscalls/syscall.tbl     | 11 +++++++++++
> > > > >  arch/mips/kernel/syscalls/syscall_o32.tbl | 11 +++++++++++
> > > > >  arch/powerpc/kernel/syscalls/syscall.tbl  | 12 ++++++++++++
> > > >
> > > > I have some changes I'd like to make to our syscall table that will
> > > > clash with this.
> > > >
> > > > I'll try and send them today.
> > >
> > > Ok. Are those for 5.0 or 5.1? If they are intended for 5.0, it would be
> > > nice for me to have a branch based on 5.0-rc1 that I can put
> > > the other patches on top of.
> >
> > There is also another change that I considered:
> >
> > At the end of my series, we have a lot of entries like
> >
> > 245     32      clock_settime                   sys_clock_settime32
> > 245     64      clock_settime                   sys_clock_settime
> > 245     spu     clock_settime                   sys_clock_settime
> >
> > which could be folded into
> >
> > 245     32      clock_settime                   sys_clock_settime32
> > 245     spu64 clock_settime                   sys_clock_settime
> >
> > if we just add another option to the ABI field. Any thoughts on
> > that?
>
> So "spu64" would mean "spu + 64"?
> That makes it more difficult to read, and to grep.
> What about allowing multiple ABIs, separated by commas?
> So that line would become:
>
>     245     spu,64 clock_settime                   sys_clock_settime

I agree that would be a nice representation, but doing this would
again require changing the script, which then in turn clashes with
Firoz' patches to unify it under the scripts/ directory.

       Arnd

^ permalink raw reply

* Re: [PATCH v4 1/2] mm/memfd: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: John Stultz @ 2019-01-15 17:36 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: lkml, Andy Lutomirski, Al Viro, Andrew Morton, Daniel Colascione,
	Hugh Dickins, Jann Horn, J. Bruce Fields, Jeff Layton, Linux API,
	linux-fsdevel, linux-kselftest, linux-mm, Marc-André Lureau,
	Matthew Wilcox, Mike Kravetz, Minchan Kim, Shuah Khan
In-Reply-To: <20190112203816.85534-2-joel@joelfernandes.org>

On Sat, Jan 12, 2019 at 12:38 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
>
> Android uses ashmem for sharing memory regions.  We are looking forward to
> migrating all usecases of ashmem to memfd so that we can possibly remove
> the ashmem driver in the future from staging while also benefiting from
> using memfd and contributing to it.  Note staging drivers are also not ABI
> and generally can be removed at anytime.
>
> One of the main usecases Android has is the ability to create a region and
> mmap it as writeable, then add protection against making any "future"
> writes while keeping the existing already mmap'ed writeable-region active.
> This allows us to implement a usecase where receivers of the shared
> memory buffer can get a read-only view, while the sender continues to
> write to the buffer.  See CursorWindow documentation in Android for more
> details:
> https://developer.android.com/reference/android/database/CursorWindow
>
> This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
> To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
> which prevents any future mmap and write syscalls from succeeding while
> keeping the existing mmap active.
>
> A better way to do F_SEAL_FUTURE_WRITE seal was discussed [1] last week
> where we don't need to modify core VFS structures to get the same
> behavior of the seal. This solves several side-effects pointed by Andy.
> self-tests are provided in later patch to verify the expected semantics.
>
> [1] https://lore.kernel.org/lkml/20181111173650.GA256781@google.com/
>
> [Thanks a lot to Andy for suggestions to improve code]
> Cc: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  fs/hugetlbfs/inode.c       |  2 +-
>  include/uapi/linux/fcntl.h |  1 +
>  mm/memfd.c                 |  3 ++-
>  mm/shmem.c                 | 25 ++++++++++++++++++++++---
>  4 files changed, 26 insertions(+), 5 deletions(-)

Acked-by: John Stultz <john.stultz@linaro.org>

^ permalink raw reply

* Re: [PATCH 14/15] arch: add split IPC system calls where needed
From: Geert Uytterhoeven @ 2019-01-15 16:35 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Rich Felker, linux-ia64@vger.kernel.org, Linux-sh list,
	Catalin Marinas, Heiko Carstens, Dominik Brodowski, linux-mips,
	Max Filippov, Deepa Dinamani, H. Peter Anvin, sparclinux,
	linux-s390, Davidlohr Bueso, y2038 Mailman List, Michael Ellerman,
	Helge Deller, the arch/x86 maintainers, Russell King - ARM Linux,
	Ingo Molnar, Firoz Khan, Matt Turner, Fenghua Yu
In-Reply-To: <CAK8P3a3eUzGDrJtV2ySpgHwjKHwZYr+1xHp6tChCt1gWi=mJ+g@mail.gmail.com>

Hi Arnd,

On Tue, Jan 15, 2019 at 4:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jan 15, 2019 at 4:01 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Jan 14, 2019 at 4:59 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> > > Arnd Bergmann <arnd@arndb.de> writes:
> > > >  arch/m68k/kernel/syscalls/syscall.tbl     | 11 +++++++++++
> > > >  arch/mips/kernel/syscalls/syscall_o32.tbl | 11 +++++++++++
> > > >  arch/powerpc/kernel/syscalls/syscall.tbl  | 12 ++++++++++++
> > >
> > > I have some changes I'd like to make to our syscall table that will
> > > clash with this.
> > >
> > > I'll try and send them today.
> >
> > Ok. Are those for 5.0 or 5.1? If they are intended for 5.0, it would be
> > nice for me to have a branch based on 5.0-rc1 that I can put
> > the other patches on top of.
>
> There is also another change that I considered:
>
> At the end of my series, we have a lot of entries like
>
> 245     32      clock_settime                   sys_clock_settime32
> 245     64      clock_settime                   sys_clock_settime
> 245     spu     clock_settime                   sys_clock_settime
>
> which could be folded into
>
> 245     32      clock_settime                   sys_clock_settime32
> 245     spu64 clock_settime                   sys_clock_settime
>
> if we just add another option to the ABI field. Any thoughts on
> that?

So "spu64" would mean "spu + 64"?
That makes it more difficult to read, and to grep.
What about allowing multiple ABIs, separated by commas?
So that line would become:

    245     spu,64 clock_settime                   sys_clock_settime

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH 14/15] arch: add split IPC system calls where needed
From: Arnd Bergmann @ 2019-01-15 15:18 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: y2038 Mailman List, Linux API, Linux Kernel Mailing List,
	Ivan Kokshaysky, Matt Turner, Russell King - ARM Linux,
	Catalin Marinas, Will Deacon, Tony Luck, Fenghua Yu,
	Geert Uytterhoeven, Michal Simek, Paul Burton, Helge Deller,
	Martin Schwidefsky, Heiko Carstens, Rich Felker, David Miller,
	Andy Lutomirski
In-Reply-To: <CAK8P3a0uas76i2W1yjFvxHT-2=A8_egUZeAUM-Vya6386+87Xg@mail.gmail.com>

On Tue, Jan 15, 2019 at 4:01 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Jan 14, 2019 at 4:59 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> > Arnd Bergmann <arnd@arndb.de> writes:
> > >  arch/m68k/kernel/syscalls/syscall.tbl     | 11 +++++++++++
> > >  arch/mips/kernel/syscalls/syscall_o32.tbl | 11 +++++++++++
> > >  arch/powerpc/kernel/syscalls/syscall.tbl  | 12 ++++++++++++
> >
> > I have some changes I'd like to make to our syscall table that will
> > clash with this.
> >
> > I'll try and send them today.
>
> Ok. Are those for 5.0 or 5.1? If they are intended for 5.0, it would be
> nice for me to have a branch based on 5.0-rc1 that I can put
> the other patches on top of.

There is also another change that I considered:

At the end of my series, we have a lot of entries like

245     32      clock_settime                   sys_clock_settime32
245     64      clock_settime                   sys_clock_settime
245     spu     clock_settime                   sys_clock_settime

which could be folded into

245     32      clock_settime                   sys_clock_settime32
245     spu64 clock_settime                   sys_clock_settime

if we just add another option to the ABI field. Any thoughts on
that?

      Arnd

^ permalink raw reply

* Re: [PATCH 14/15] arch: add split IPC system calls where needed
From: Arnd Bergmann @ 2019-01-15 15:01 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Rich Felker, linux-ia64, Linux-sh list, Catalin Marinas,
	Heiko Carstens, Dominik Brodowski, linux-mips, Max Filippov,
	Deepa Dinamani, H. Peter Anvin, sparclinux, linux-s390,
	Davidlohr Bueso, y2038 Mailman List, Helge Deller,
	the arch/x86 maintainers, Russell King - ARM Linux, Ingo Molnar,
	Geert Uytterhoeven, Firoz Khan, Matt Turner, Fenghua Yu,
	Will Deacon
In-Reply-To: <87pnsz28k2.fsf@concordia.ellerman.id.au>

On Mon, Jan 14, 2019 at 4:59 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> >  arch/m68k/kernel/syscalls/syscall.tbl     | 11 +++++++++++
> >  arch/mips/kernel/syscalls/syscall_o32.tbl | 11 +++++++++++
> >  arch/powerpc/kernel/syscalls/syscall.tbl  | 12 ++++++++++++
>
> I have some changes I'd like to make to our syscall table that will
> clash with this.
>
> I'll try and send them today.

Ok. Are those for 5.0 or 5.1? If they are intended for 5.0, it would be
nice for me to have a branch based on 5.0-rc1 that I can put
the other patches on top of.

> > diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> > index db3bbb8744af..1bffab54ff35 100644
> > --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> > +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> > @@ -425,3 +425,15 @@
> >  386  nospu   pkey_mprotect                   sys_pkey_mprotect
> >  387  nospu   rseq                            sys_rseq
> >  388  nospu   io_pgetevents                   sys_io_pgetevents               compat_sys_io_pgetevents
> > +# room for arch specific syscalls
> > +392  64      semtimedop                      sys_semtimedop
> > +393  common  semget                          sys_semget
> > +394  common  semctl                          sys_semctl                      compat_sys_semctl
> > +395  common  shmget                          sys_shmget
> > +396  common  shmctl                          sys_shmctl                      compat_sys_shmctl
> > +397  common  shmat                           sys_shmat                       compat_sys_shmat
> > +398  common  shmdt                           sys_shmdt
> > +399  common  msgget                          sys_msgget
> > +400  common  msgsnd                          sys_msgsnd                      compat_sys_msgsnd
> > +401  common  msgrcv                          sys_msgrcv                      compat_sys_msgrcv
> > +402  common  msgctl                          sys_msgctl                      compat_sys_msgctl
>
> We already have a gap at 366-377 from when we tried to add the split IPC
> calls a few years back.
>
> I guess I don't mind leaving that gap and using the common numbers as
> you've done here.
>
> But it would be good to add a comment pointing out that we have room
> at 366 for more arch specific syscalls as well.

Ah, I missed that. I've added this to my patch now:

index 5c0936d862fc..2ddfba536d5f 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -460,6 +460,7 @@
 363    spu     switch_endian                   sys_ni_syscall
 364    common  userfaultfd                     sys_userfaultfd
 365    common  membarrier                      sys_membarrier
+# 366-377 originally left for IPC, now unused
 378    nospu   mlock2                          sys_mlock2
 379    nospu   copy_file_range                 sys_copy_file_range
 380    common  preadv2                         sys_preadv2
         compat_sys_preadv2

       Arnd

^ permalink raw reply related

* Re: [PATCH v4 2/2] selftests/memfd: Add tests for F_SEAL_FUTURE_WRITE seal
From: Joel Fernandes @ 2019-01-15 14:50 UTC (permalink / raw)
  To: shuah
  Cc: linux-kernel, dancol, minchan, Jann Horn, John Stultz, Al Viro,
	Andrew Morton, Andy Lutomirski, Hugh Dickins, J. Bruce Fields,
	Jeff Layton, linux-api, linux-fsdevel, linux-kselftest, linux-mm,
	Marc-André Lureau, Matthew Wilcox, Mike Kravetz
In-Reply-To: <f9ffb7f8-1ff8-3bec-ce79-f9322d8715dc@kernel.org>

On Mon, Jan 14, 2019 at 06:39:59PM -0700, shuah wrote:
> On 1/12/19 1:38 PM, Joel Fernandes wrote:
> > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > 
> > Add tests to verify sealing memfds with the F_SEAL_FUTURE_WRITE works as
> > expected.
> > 
> > Cc: dancol@google.com
> > Cc: minchan@kernel.org
> > Cc: Jann Horn <jannh@google.com>
> > Cc: John Stultz <john.stultz@linaro.org>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> 
> Looks good to me. For selftest part of the series:
> 
> Reviewed-by: Shuah Khan <shuah@kernel.org>

Thanks!

John, could you provide your Reviewed-by again for patch 1/2 ? I had dropped
it since the patch had some more changes.

thanks,

 - Joel

^ permalink raw reply

* Re: [PATCH 15/15] arch: add pkey and rseq syscall numbers everywhere
From: Arnd Bergmann @ 2019-01-15 14:47 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Rich Felker, linux-ia64, Linux-sh list, Catalin Marinas,
	Heiko Carstens, Dominik Brodowski, linux-mips, Max Filippov,
	Deepa Dinamani, H. Peter Anvin, sparclinux, linux-s390,
	Davidlohr Bueso, y2038 Mailman List, Michael Ellerman,
	Helge Deller, the arch/x86 maintainers, Ingo Molnar,
	Geert Uytterhoeven, Firoz Khan, Matt Turner, Fenghua Yu,
	Will Deacon
In-Reply-To: <20190115115229.mcrmmpwdmjxf2cra@e5254000004ec.dyn.armlinux.org.uk>

On Tue, Jan 15, 2019 at 12:52 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Jan 10, 2019 at 05:24:35PM +0100, Arnd Bergmann wrote:
> > Most architectures define system call numbers for the rseq and pkey system
> > calls, even when they don't support the features, and perhaps never will.
> >
> > Only a few architectures are missing these, so just define them anyway
> > for consistency. If we decide to add them later to one of these, the
> > system call numbers won't get out of sync then.
>
> I was lambasted for adding the pkey syscalls for 32-bit ARM in 2016,
> which will probably never support it.  Why has the attitude towards
> this kind of thing now apparently become acceptable?

I was (and still am) a bit unsure about this one. A number of architectures
added the numbers that won't ever support them, but I wasn't sure if
any of those that didn't add them might need it later.

I tried to just go by the rule that anything that we list in
asm-generic/unistd.h
is probably important enough that we want to list it everywhere, even if
that includes a couple that we end up being rather architecture specific.

I'm happy to drop this patch if you or others feel that we're better off
without it though.

      Arnd

^ permalink raw reply

* Re: [PATCH 15/15] arch: add pkey and rseq syscall numbers everywhere
From: Russell King - ARM Linux admin @ 2019-01-15 11:52 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: y2038, linux-api, linux-kernel, ink, mattst88, catalin.marinas,
	will.deacon, tony.luck, fenghua.yu, geert, monstr, paul.burton,
	deller, mpe, schwidefsky, heiko.carstens, dalias, davem, luto,
	tglx, mingo, hpa, x86, jcmvbkbc, firoz.khan, ebiederm,
	deepa.kernel, linux, akpm, dave, linux-alpha, linux-arm-kernel,
	linux-ia64, linux-m68k
In-Reply-To: <20190110162435.309262-16-arnd@arndb.de>

On Thu, Jan 10, 2019 at 05:24:35PM +0100, Arnd Bergmann wrote:
> Most architectures define system call numbers for the rseq and pkey system
> calls, even when they don't support the features, and perhaps never will.
> 
> Only a few architectures are missing these, so just define them anyway
> for consistency. If we decide to add them later to one of these, the
> system call numbers won't get out of sync then.

I was lambasted for adding the pkey syscalls for 32-bit ARM in 2016,
which will probably never support it.  Why has the attitude towards
this kind of thing now apparently become acceptable?

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/alpha/include/asm/unistd.h         | 4 ----
>  arch/alpha/kernel/syscalls/syscall.tbl  | 4 ++++
>  arch/ia64/kernel/syscalls/syscall.tbl   | 4 ++++
>  arch/m68k/kernel/syscalls/syscall.tbl   | 4 ++++
>  arch/parisc/include/asm/unistd.h        | 3 ---
>  arch/parisc/kernel/syscalls/syscall.tbl | 4 ++++
>  arch/s390/include/asm/unistd.h          | 3 ---
>  arch/s390/kernel/syscalls/syscall.tbl   | 3 +++
>  arch/sh/kernel/syscalls/syscall.tbl     | 4 ++++
>  arch/sparc/include/asm/unistd.h         | 5 -----
>  arch/sparc/kernel/syscalls/syscall.tbl  | 4 ++++
>  arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
>  12 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/alpha/include/asm/unistd.h b/arch/alpha/include/asm/unistd.h
> index 564ba87bdc38..31ad350b58a0 100644
> --- a/arch/alpha/include/asm/unistd.h
> +++ b/arch/alpha/include/asm/unistd.h
> @@ -29,9 +29,5 @@
>  #define __IGNORE_getppid
>  #define __IGNORE_getuid
>  
> -/* Alpha doesn't have protection keys. */
> -#define __IGNORE_pkey_mprotect
> -#define __IGNORE_pkey_alloc
> -#define __IGNORE_pkey_free
>  
>  #endif /* _ALPHA_UNISTD_H */
> diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> index b0e247287908..25b4a7e76943 100644
> --- a/arch/alpha/kernel/syscalls/syscall.tbl
> +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> @@ -452,3 +452,7 @@
>  521	common	pwritev2			sys_pwritev2
>  522	common	statx				sys_statx
>  523	common	io_pgetevents			sys_io_pgetevents
> +524	common	pkey_alloc			sys_pkey_alloc
> +525	common	pkey_free			sys_pkey_free
> +526	common	pkey_mprotect			sys_pkey_mprotect
> +527	common	rseq				sys_rseq
> diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
> index 2e93dbdcdb80..84e03de00177 100644
> --- a/arch/ia64/kernel/syscalls/syscall.tbl
> +++ b/arch/ia64/kernel/syscalls/syscall.tbl
> @@ -339,3 +339,7 @@
>  327	common	io_pgetevents			sys_io_pgetevents
>  328	common	perf_event_open			sys_perf_event_open
>  329	common	seccomp				sys_seccomp
> +330	common	pkey_alloc			sys_pkey_alloc
> +331	common	pkey_free			sys_pkey_free
> +332	common	pkey_mprotect			sys_pkey_mprotect
> +333	common	rseq				sys_rseq
> diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> index 5354ba02eed2..ae88b85d068e 100644
> --- a/arch/m68k/kernel/syscalls/syscall.tbl
> +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> @@ -388,6 +388,10 @@
>  378	common	pwritev2			sys_pwritev2
>  379	common	statx				sys_statx
>  380	common	seccomp				sys_seccomp
> +381	common	pkey_alloc			sys_pkey_alloc
> +382	common	pkey_free			sys_pkey_free
> +383	common	pkey_mprotect			sys_pkey_mprotect
> +384	common	rseq				sys_rseq
>  # room for arch specific calls
>  393	common	semget				sys_semget
>  394	common	semctl				sys_semctl
> diff --git a/arch/parisc/include/asm/unistd.h b/arch/parisc/include/asm/unistd.h
> index c2c2afb28941..9ec1026af877 100644
> --- a/arch/parisc/include/asm/unistd.h
> +++ b/arch/parisc/include/asm/unistd.h
> @@ -12,9 +12,6 @@
>  
>  #define __IGNORE_select			/* newselect */
>  #define __IGNORE_fadvise64		/* fadvise64_64 */
> -#define __IGNORE_pkey_mprotect
> -#define __IGNORE_pkey_alloc
> -#define __IGNORE_pkey_free
>  
>  #ifndef ASM_LINE_SEP
>  # define ASM_LINE_SEP ;
> diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> index 9bbd2f9f56c8..e07231de3597 100644
> --- a/arch/parisc/kernel/syscalls/syscall.tbl
> +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> @@ -367,3 +367,7 @@
>  348	common	pwritev2		sys_pwritev2			compat_sys_pwritev2
>  349	common	statx			sys_statx
>  350	common	io_pgetevents		sys_io_pgetevents		compat_sys_io_pgetevents
> +351	common	pkey_alloc		sys_pkey_alloc
> +352	common	pkey_free		sys_pkey_free
> +353	common	pkey_mprotect		sys_pkey_mprotect
> +354	common	rseq			sys_rseq
> diff --git a/arch/s390/include/asm/unistd.h b/arch/s390/include/asm/unistd.h
> index a1fbf15d53aa..ed08f114ee91 100644
> --- a/arch/s390/include/asm/unistd.h
> +++ b/arch/s390/include/asm/unistd.h
> @@ -11,9 +11,6 @@
>  #include <asm/unistd_nr.h>
>  
>  #define __IGNORE_time
> -#define __IGNORE_pkey_mprotect
> -#define __IGNORE_pkey_alloc
> -#define __IGNORE_pkey_free
>  
>  #define __ARCH_WANT_NEW_STAT
>  #define __ARCH_WANT_OLD_READDIR
> diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
> index 428cf512a757..f84ea364a302 100644
> --- a/arch/s390/kernel/syscalls/syscall.tbl
> +++ b/arch/s390/kernel/syscalls/syscall.tbl
> @@ -391,6 +391,9 @@
>  381  common	kexec_file_load		sys_kexec_file_load		compat_sys_kexec_file_load
>  382  common	io_pgetevents		sys_io_pgetevents		compat_sys_io_pgetevents
>  383  common	rseq			sys_rseq			compat_sys_rseq
> +384  common	pkey_alloc		sys_pkey_alloc			sys_pkey_alloc
> +385  common	pkey_free		sys_pkey_free			sys_pkey_free
> +386  common	pkey_mprotect		sys_pkey_mprotect		sys_pkey_mprotect
>  # room for arch specific syscalls
>  392	64	semtimedop		sys_semtimedop			-
>  393  common	semget			sys_semget			sys_semget
> diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> index 6d0b84e3ef2d..3f96ad0424e1 100644
> --- a/arch/sh/kernel/syscalls/syscall.tbl
> +++ b/arch/sh/kernel/syscalls/syscall.tbl
> @@ -391,6 +391,10 @@
>  381	common	preadv2				sys_preadv2
>  382	common	pwritev2			sys_pwritev2
>  383	common	statx				sys_statx
> +384	common	pkey_alloc			sys_pkey_alloc
> +385	common	pkey_free			sys_pkey_free
> +386	common	pkey_mprotect			sys_pkey_mprotect
> +387	common	rseq				sys_rseq
>  # room for arch specific syscalls
>  393	common	semget				sys_semget
>  394	common	semctl				sys_semctl
> diff --git a/arch/sparc/include/asm/unistd.h b/arch/sparc/include/asm/unistd.h
> index 5194d86ef72d..08696ea5dca8 100644
> --- a/arch/sparc/include/asm/unistd.h
> +++ b/arch/sparc/include/asm/unistd.h
> @@ -59,9 +59,4 @@
>  #define __IGNORE_getresgid
>  #endif
>  
> -/* Sparc doesn't have protection keys. */
> -#define __IGNORE_pkey_mprotect
> -#define __IGNORE_pkey_alloc
> -#define __IGNORE_pkey_free
> -
>  #endif /* _SPARC_UNISTD_H */
> diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
> index 8c9580302422..24ebef675184 100644
> --- a/arch/sparc/kernel/syscalls/syscall.tbl
> +++ b/arch/sparc/kernel/syscalls/syscall.tbl
> @@ -407,6 +407,10 @@
>  359	common	pwritev2		sys_pwritev2			compat_sys_pwritev2
>  360	common	statx			sys_statx
>  361	common	io_pgetevents		sys_io_pgetevents		compat_sys_io_pgetevents
> +362	common	pkey_alloc		sys_pkey_alloc
> +363	common	pkey_free		sys_pkey_free
> +364	common	pkey_mprotect		sys_pkey_mprotect
> +365	common	rseq			sys_rseq
>  # room for arch specific syscalls
>  392	64	semtimedop			sys_semtimedop
>  393	common	semget			sys_semget
> diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
> index f8befa11b0c4..c699e014e0dd 100644
> --- a/arch/xtensa/kernel/syscalls/syscall.tbl
> +++ b/arch/xtensa/kernel/syscalls/syscall.tbl
> @@ -372,3 +372,4 @@
>  349	common	pkey_alloc			sys_pkey_alloc
>  350	common	pkey_free			sys_pkey_free
>  351	common	statx				sys_statx
> +352	common	rseq				sys_rseq
> -- 
> 2.20.0
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox