* Re: [RFD] A mount api that notices previous mounts
From: Eric W. Biederman @ 2019-01-30 12:47 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-api, linux-fsdevel, linux-kernel, Al Viro, David Howells,
Miklos Szeredi, Linus Torvalds, Karel Zak, util-linux,
Andy Lutomirski, LSM
In-Reply-To: <87ef8vx7jz.fsf@xmission.com>
ebiederm@xmission.com (Eric W. Biederman) writes:
> ebiederm@xmission.com (Eric W. Biederman) writes:
>
>> Casey Schaufler <casey@schaufler-ca.com> writes:
>>> Are you taking the LSM specific mount options into account?
>>
>> In the design yes, and I allow setting them. It appears in the code
>> to retrieve the mount options I forgot to call security_sb_show_options.
>>
>> For finding the super block that you are going to mount the LSM mount
>> options are not relevant. Even nfs will not want to set those early as
>> they do not help determine the nfs super block. So the only place where
>> there is anything interesting in my api is in reading back the security
>> options so they can be compared to the options the mounter is setting.
>>
>> I will add the missing call to security_sb_show_options which is enough
>> to fix selinux. Unfortunately smack does not currently implement
>> .sb_show_options. Not implementing smack_sb_show_options means
>> /proc/mounts fails to match /etc/mtab which is a bug and it is likely
>> a real workd bug for the people who use smack and don't want to depend
>> on /etc/mtab, or are transitioning away from it.
>>
>> Casey do you want to implement smack_sb_show_options or should I put it
>> on my todo list?
>
> Oh. I should add that I am always parsing the LSM mount options out so
> that there is not a chance of the individual filesystems implementing
> comflicting options even when there are no LSMs active. Without that I
> am afraid we run the risk of having LSM mount otions in conflict with
> ordinary filesystems options at some point and by the time we discover
> it it would start introducing filesystem regressions.
>
> That does help with stack though as there is no fundamental reason only
> one LSM could process mount options.
Sigh. I just realized that there is a smack variant of the bug I am
working to fix.
smack on remount does not fail if you change the smack mount options.
It just silently ignores the smack mount options. Which is exactly the
same poor interaction with userspace that has surprised user space
and caused CVEs.
How much do you think the smack users will care if you start verifying
that if smack options are present in remount that they are unchanged
from mount?
I suspect the smack userbase is small enough, and the corner case is
crazy enough we can fix this poor communication by smack. Otherwise it
looks like there needs to be a new security hook so old and new remounts
can be distinguished by the LSMs, and smack can be fixed in the new
version.
Eric
^ permalink raw reply
* [PATCH 3/3] mm/mincore: provide mapped status when cached status is not allowed
From: Vlastimil Babka @ 2019-01-30 12:44 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds
Cc: linux-kernel, linux-mm, linux-api, Peter Zijlstra, Greg KH,
Jann Horn, Vlastimil Babka, Jiri Kosina, Dominique Martinet,
Andy Lutomirski, Dave Chinner, Kevin Easton, Matthew Wilcox,
Cyril Hrubis, Tejun Heo, Kirill A . Shutemov, Daniel Gruss,
Jiri Kosina
In-Reply-To: <20190130124420.1834-1-vbabka@suse.cz>
After "mm/mincore: make mincore() more conservative" we sometimes restrict the
information about page cache residency, which we have to do without breaking
existing userspace, if possible. We thus fake the resulting values as 1, which
should be safer than faking them as 0, as there might theoretically exist code
that would try to fault in the page(s) until mincore() returns 1.
Faking 1 however means that such code would not fault in a page even if it was
not in page cache, with unwanted performance implications. We can improve the
situation by revisting the approach of 574823bfab82 ("Change mincore() to count
"mapped" pages rather than "cached" pages") but only applying it to cases where
page cache residency check is restricted. Thus mincore() will return 0 for an
unmapped page (which may or may not be resident in a pagecache), and 1 after
the process faults it in.
One potential downside is that mincore() will be again able to recognize when a
previously mapped page was reclaimed. While that might be useful for some
attack scenarios, it's not as crucial as recognizing that somebody else faulted
the page in, and there are also other ways to recognize reclaimed pages anyway.
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Dominique Martinet <asmadeus@codewreck.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Chinner <david@fromorbit.com>
Cc: Kevin Easton <kevin@guarana.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Cyril Hrubis <chrubis@suse.cz>
Cc: Tejun Heo <tj@kernel.org>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Cc: Daniel Gruss <daniel@gruss.cc>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/mincore.c | 49 +++++++++++++++++++++++++++++++++----------------
1 file changed, 33 insertions(+), 16 deletions(-)
diff --git a/mm/mincore.c b/mm/mincore.c
index 747a4907a3ac..d6784a803ae7 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -21,12 +21,18 @@
#include <linux/uaccess.h>
#include <asm/pgtable.h>
+struct mincore_walk_private {
+ unsigned char *vec;
+ bool can_check_pagecache;
+};
+
static int mincore_hugetlb(pte_t *pte, unsigned long hmask, unsigned long addr,
unsigned long end, struct mm_walk *walk)
{
#ifdef CONFIG_HUGETLB_PAGE
unsigned char present;
- unsigned char *vec = walk->private;
+ struct mincore_walk_private *walk_private = walk->private;
+ unsigned char *vec = walk_private->vec;
/*
* Hugepages under user process are always in RAM and never
@@ -35,7 +41,7 @@ static int mincore_hugetlb(pte_t *pte, unsigned long hmask, unsigned long addr,
present = pte && !huge_pte_none(huge_ptep_get(pte));
for (; addr != end; vec++, addr += PAGE_SIZE)
*vec = present;
- walk->private = vec;
+ walk_private->vec = vec;
#else
BUG();
#endif
@@ -85,7 +91,8 @@ static unsigned char mincore_page(struct address_space *mapping, pgoff_t pgoff)
}
static int __mincore_unmapped_range(unsigned long addr, unsigned long end,
- struct vm_area_struct *vma, unsigned char *vec)
+ struct vm_area_struct *vma, unsigned char *vec,
+ bool can_check_pagecache)
{
unsigned long nr = (end - addr) >> PAGE_SHIFT;
int i;
@@ -95,7 +102,9 @@ static int __mincore_unmapped_range(unsigned long addr, unsigned long end,
pgoff = linear_page_index(vma, addr);
for (i = 0; i < nr; i++, pgoff++)
- vec[i] = mincore_page(vma->vm_file->f_mapping, pgoff);
+ vec[i] = can_check_pagecache ?
+ mincore_page(vma->vm_file->f_mapping, pgoff)
+ : 0;
} else {
for (i = 0; i < nr; i++)
vec[i] = 0;
@@ -106,8 +115,11 @@ static int __mincore_unmapped_range(unsigned long addr, unsigned long end,
static int mincore_unmapped_range(unsigned long addr, unsigned long end,
struct mm_walk *walk)
{
- walk->private += __mincore_unmapped_range(addr, end,
- walk->vma, walk->private);
+ struct mincore_walk_private *walk_private = walk->private;
+ unsigned char *vec = walk_private->vec;
+
+ walk_private->vec += __mincore_unmapped_range(addr, end, walk->vma,
+ vec, walk_private->can_check_pagecache);
return 0;
}
@@ -117,7 +129,8 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
spinlock_t *ptl;
struct vm_area_struct *vma = walk->vma;
pte_t *ptep;
- unsigned char *vec = walk->private;
+ struct mincore_walk_private *walk_private = walk->private;
+ unsigned char *vec = walk_private->vec;
int nr = (end - addr) >> PAGE_SHIFT;
ptl = pmd_trans_huge_lock(pmd, vma);
@@ -128,7 +141,8 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
}
if (pmd_trans_unstable(pmd)) {
- __mincore_unmapped_range(addr, end, vma, vec);
+ __mincore_unmapped_range(addr, end, vma, vec,
+ walk_private->can_check_pagecache);
goto out;
}
@@ -138,7 +152,7 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
if (pte_none(pte))
__mincore_unmapped_range(addr, addr + PAGE_SIZE,
- vma, vec);
+ vma, vec, walk_private->can_check_pagecache);
else if (pte_present(pte))
*vec = 1;
else { /* pte is a swap entry */
@@ -152,8 +166,12 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
*vec = 1;
} else {
#ifdef CONFIG_SWAP
- *vec = mincore_page(swap_address_space(entry),
+ if (walk_private->can_check_pagecache)
+ *vec = mincore_page(
+ swap_address_space(entry),
swp_offset(entry));
+ else
+ *vec = 0;
#else
WARN_ON(1);
*vec = 1;
@@ -187,22 +205,21 @@ static long do_mincore(unsigned long addr, unsigned long pages, unsigned char *v
struct vm_area_struct *vma;
unsigned long end;
int err;
+ struct mincore_walk_private walk_private = {
+ .vec = vec
+ };
struct mm_walk mincore_walk = {
.pmd_entry = mincore_pte_range,
.pte_hole = mincore_unmapped_range,
.hugetlb_entry = mincore_hugetlb,
- .private = vec,
+ .private = &walk_private
};
vma = find_vma(current->mm, addr);
if (!vma || addr < vma->vm_start)
return -ENOMEM;
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;
- }
+ walk_private.can_check_pagecache = can_do_mincore(vma);
mincore_walk.mm = vma->vm_mm;
err = walk_page_range(addr, end, &mincore_walk);
if (err < 0)
--
2.20.1
^ permalink raw reply related
* [PATCH 2/3] mm/filemap: initiate readahead even if IOCB_NOWAIT is set for the I/O
From: Vlastimil Babka @ 2019-01-30 12:44 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds
Cc: linux-kernel, linux-mm, linux-api, Peter Zijlstra, Greg KH,
Jann Horn, Jiri Kosina, Dominique Martinet, Andy Lutomirski,
Dave Chinner, Kevin Easton, Matthew Wilcox, Cyril Hrubis,
Tejun Heo, Kirill A . Shutemov, Daniel Gruss, Vlastimil Babka,
Jiri Kosina
In-Reply-To: <20190130124420.1834-1-vbabka@suse.cz>
From: Jiri Kosina <jkosina@suse.cz>
preadv2(RWF_NOWAIT) can be used to open a side-channel to pagecache contents, as
it reveals metadata about residency of pages in pagecache.
If preadv2(RWF_NOWAIT) returns immediately, it provides a clear "page not
resident" information, and vice versa.
Close that sidechannel by always initiating readahead on the cache if we
encounter a cache miss for preadv2(RWF_NOWAIT); with that in place, probing
the pagecache residency itself will actually populate the cache, making the
sidechannel useless.
Originally-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Dominique Martinet <asmadeus@codewreck.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Chinner <david@fromorbit.com>
Cc: Kevin Easton <kevin@guarana.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Cyril Hrubis <chrubis@suse.cz>
Cc: Tejun Heo <tj@kernel.org>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Cc: Daniel Gruss <daniel@gruss.cc>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/filemap.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index 9f5e323e883e..7bcdd36e629d 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2075,8 +2075,6 @@ static ssize_t generic_file_buffered_read(struct kiocb *iocb,
page = find_get_page(mapping, index);
if (!page) {
- if (iocb->ki_flags & IOCB_NOWAIT)
- goto would_block;
page_cache_sync_readahead(mapping,
ra, filp,
index, last_index - index);
--
2.20.1
^ permalink raw reply related
* [PATCH 1/3] mm/mincore: make mincore() more conservative
From: Vlastimil Babka @ 2019-01-30 12:44 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds
Cc: linux-kernel, linux-mm, linux-api, Peter Zijlstra, Greg KH,
Jann Horn, Jiri Kosina, Dominique Martinet, Andy Lutomirski,
Dave Chinner, Kevin Easton, Matthew Wilcox, Cyril Hrubis,
Tejun Heo, Kirill A . Shutemov, Daniel Gruss, Vlastimil Babka,
Jiri Kosina
In-Reply-To: <20190130124420.1834-1-vbabka@suse.cz>
From: Jiri Kosina <jkosina@suse.cz>
The semantics of what mincore() considers to be resident is not completely
clear, but Linux has always (since 2.3.52, which is when mincore() was
initially done) treated it as "page is available in page cache".
That's potentially a problem, as that [in]directly exposes meta-information
about pagecache / memory mapping state even about memory not strictly belonging
to the process executing the syscall, opening possibilities for sidechannel
attacks.
Change the semantics of mincore() so that it only reveals pagecache information
for non-anonymous mappings that belog to files that the calling process could
(if it tried to) successfully open for writing.
Originally-by: Linus Torvalds <torvalds@linux-foundation.org>
Originally-by: Dominique Martinet <asmadeus@codewreck.org>
Cc: Dominique Martinet <asmadeus@codewreck.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Chinner <david@fromorbit.com>
Cc: Kevin Easton <kevin@guarana.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Cyril Hrubis <chrubis@suse.cz>
Cc: Tejun Heo <tj@kernel.org>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Cc: Daniel Gruss <daniel@gruss.cc>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/mincore.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/mm/mincore.c b/mm/mincore.c
index 218099b5ed31..747a4907a3ac 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -169,6 +169,14 @@ 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 &&
+ (inode_owner_or_capable(file_inode(vma->vm_file))
+ || 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 +197,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;
--
2.20.1
^ permalink raw reply related
* [PATCH 0/3] mincore() and IOCB_NOWAIT adjustments
From: Vlastimil Babka @ 2019-01-30 12:44 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds
Cc: linux-kernel, linux-mm, linux-api, Peter Zijlstra, Greg KH,
Jann Horn, Vlastimil Babka, Andy Lutomirski, Cyril Hrubis,
Daniel Gruss, Dave Chinner, Dominique Martinet, Jiri Kosina,
Jiri Kosina, Kevin Easton, Kirill A. Shutemov, Matthew Wilcox,
Tejun Heo
In-Reply-To: <nycvar.YFH.7.76.1901051817390.16954@cbobk.fhfr.pm>
I've collected the patches from the discussion for formal posting. The first
two should be settled already, third one is the possible improvement I've
mentioned earlier, where only in restricted case we resort to existence of page
table mapping (the original and later reverted approach from Linus) instead of
faking the result completely. Review and testing welcome.
The consensus seems to be going through -mm tree for 5.1, unless Linus wants
them alredy for 5.0.
Jiri Kosina (2):
mm/mincore: make mincore() more conservative
mm/filemap: initiate readahead even if IOCB_NOWAIT is set for the I/O
Vlastimil Babka (1):
mm/mincore: provide mapped status when cached status is not allowed
mm/filemap.c | 2 --
mm/mincore.c | 54 ++++++++++++++++++++++++++++++++++++++++------------
2 files changed, 42 insertions(+), 14 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-30 12:29 UTC (permalink / raw)
To: Michal Hocko
Cc: Dominique Martinet, Linus Torvalds, Andy Lutomirski, Josh Snyder,
Dave Chinner, Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH,
Peter Zijlstra, Linux-MM, kernel list, Linux API
In-Reply-To: <20190130090945.GS18811@dhcp22.suse.cz>
On Wed, 30 Jan 2019, Michal Hocko wrote:
> > > I'm not sure why I'm the main recipient of that mail but answering
> > > because I am -- let's get these patches in through the regular -mm
> > > tree though
> >
> > *prod to mm maintainers* (at least for an opinion)
>
> Could you repost those patches please? The thread is long and it is not
> really clear what is the most up-to-date state of patches (at least to
> me).
Vlastimil seems to have one extra patch to go on top, so we agreed that
he'll be sending that as a complete self-contained series (either as a
followup to the very first e-mail in this monsterthread, or completely
separately) shortly.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [RFD] A mount api that notices previous mounts
From: Karel Zak @ 2019-01-30 12:06 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-api, linux-fsdevel, linux-kernel, Al Viro, David Howells,
Miklos Szeredi, Linus Torvalds, util-linux, Andy Lutomirski
In-Reply-To: <87va2716mh.fsf@xmission.com>
On Tue, Jan 29, 2019 at 03:44:22PM -0600, Eric W. Biederman wrote:
> so I am proposing we change this in the new mount api.
Well, this forces me to ask what the new API is? :-)
It seems that David uses fsconfig() and fsinfo() to set and get
mount options, and your patch introduces fsset() and fsoptions().
IMHO differentiate between FS driver and FS instance is a good idea it
makes things more extendable. The sequence number in the instance is a
good example.
But for me David's fsinfo() seems better that fsoptions() and
fsspecifier(). I'm not sure about "all mount options as one string"
>From your example is pretty obvious how much energy is necessary to
split and join the strings.
It seems more elegant is to ask for Nth option as expected by fsinfo().
It also seems that fsinfo() is able to replace fsname() and fstype().
It would be better to extend David's fsinfo() to work with FS instance
and to return specifiers. And use fsconfig() rather than fsset().
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Michal Hocko @ 2019-01-30 9:09 UTC (permalink / raw)
To: Jiri Kosina
Cc: Dominique Martinet, Linus Torvalds, Andy Lutomirski, Josh Snyder,
Dave Chinner, Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH,
Peter Zijlstra, Linux-MM, kernel list, Linux API
In-Reply-To: <nycvar.YFH.7.76.1901300050550.6626@cbobk.fhfr.pm>
On Wed 30-01-19 00:52:02, Jiri Kosina wrote:
> On Mon, 28 Jan 2019, Dominique Martinet wrote:
>
> > > So, any objections to aproaching it this way?
> >
> > I'm not sure why I'm the main recipient of that mail but answering
> > because I am -- let's get these patches in through the regular -mm tree
> > though
>
> *prod to mm maintainers* (at least for an opinion)
Could you repost those patches please? The thread is long and it is not
really clear what is the most up-to-date state of patches (at least to
me).
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at C startup and thread creation (v6)
From: Joseph Myers @ 2019-01-30 2:40 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: carlos, Florian Weimer, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Will Deacon, Dave Watson, Paul Turner, Rich Felker,
linux-kernel, linux-api
In-Reply-To: <596949707.3888.1548812359874.JavaMail.zimbra@efficios.com>
On Tue, 29 Jan 2019, Mathieu Desnoyers wrote:
> My thinking was to put the #error in the generic header, so architectures that
> are not supported yet cannot build against rseq.h at all, so we don't end up
> in a broken upgrade scenario. I'm open to alternative ways to do it though, as
> long as we don't let not-yet-supported architectures build broken code.
Any case with #error in installed glibc headers needs special-casing in
check-installed-headers.sh (and, thus, such errors are to be discouraged).
Cases where architectures commonly need their own bits/ headers,
especially where those are likely to need updating for new kernel
versions, are also discouraged. Furthermore, a normal check for glibc
headers updates needed for a new kernel version would only involve
examining uapi headers (and the non-uapi linux/socket.h for new address
families, an unfortunate existing wart in this area). As far as I can
see, this value isn't defined in any uapi header, which makes it
especially likely to be missed in such a check. Furthermore, I'm hoping
to add more glibc tests for consistency of such constants between glibc
and the kernel, to ensure any such updates missing are caught
automatically through test failures - but that doesn't work if the
constants in question aren't in a uapi header.
If this constant were in a uapi header, the glibc header could just
include that - is the issue that it's not actually an interface between
glibc and the kernel at all, but some kind of purely-userspace interface?
We very definitely wish to keep to a minimum the cases where updates need
to be done separately in glibc by each architecture maintainer (that's
just a recipe for some updates getting missed accidentally) - meaning that
there needs to be a clear way in which someone can tell, globally for all
architectures, whether the set of such architecture-specific headers for
this constant in glibc is complete and current, and when it needs updating
(and this should be as similar to possible to such checks for any other
header constant).
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply
* Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at C startup and thread creation (v6)
From: Mathieu Desnoyers @ 2019-01-30 1:39 UTC (permalink / raw)
To: Joseph Myers
Cc: carlos, Florian Weimer, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Will Deacon, Dave Watson, Paul Turner, Rich Felker,
linux-kernel, linux-api
In-Reply-To: <alpine.DEB.2.21.1901292153190.24454@digraph.polyomino.org.uk>
----- On Jan 29, 2019, at 4:56 PM, Joseph Myers joseph@codesourcery.com wrote:
> On Tue, 29 Jan 2019, Mathieu Desnoyers wrote:
>
>> I recalled that aarch64 defines RSEQ_SIG to a different value which maps to
>> a valid trap instruction. So I plan to move the RSEQ_SIG define to per-arch
>> headers like this:
>>
>> sysdeps/unix/sysv/linux/aarch64/bits/rseq.h | 24 ++
>> sysdeps/unix/sysv/linux/arm/bits/rseq.h | 24 ++
>> sysdeps/unix/sysv/linux/bits/rseq.h | 23 ++
>> sysdeps/unix/sysv/linux/mips/bits/rseq.h | 24 ++
>> sysdeps/unix/sysv/linux/powerpc/bits/rseq.h | 24 ++
>> sysdeps/unix/sysv/linux/s390/bits/rseq.h | 24 ++
>> sysdeps/unix/sysv/linux/x86/bits/rseq.h | 24 ++
>>
>> where "bits/rseq.h" contains a #error:
>>
>> # error "Architecture does not define RSEQ_SIG.
>>
>> sys/rseq.h will now include <bits/rseq.h>.
>
> We're trying to reduce the number of cases where most or all new glibc
> architecture ports need to provide a bits/ header, by making the generic
> headers handle the common case. So a generic header with a #error, and
> lots of architecture-specific headers mostly with the same value for
> RSEQ_SIG, seems unfortunate. I'd hope the generic header could use a
> generic value, with architecture-specific variants only for architectures
> with some reason for a different value.
The issue here is that it would require us to decide right away what RSEQ_SIG
is appropriate for all other Linux architectures supported by glibc. There are
a few reasons for which an architecture can be required to specify its own
RSEQ_SIG. For instance, it may need to map to an instruction defined in the
instruction set, thus ensuring objdump does not get confused, and in other
cases that the processor speculative execution happening just before the
RSEQ_SIG really stops at the signature (hence the trap instruction on aarch64).
I'm worried that if we introduce a "default" RSEQ_SIG value for architectures
currently not supported by RSEQ and we then introduce an architecture-specific
signature value in the future, some applications will try to build with
wrong signatures, and when the rseq system call gets eventually implemented for
those architecture and a end-user upgrades his kernel, those signatures won't
match between glibc rseq registration and the application rseq abort handlers,
thus leading to hard-to-reproduce segmentation faults delivered by the kernel
checking those signatures upon rseq abort.
This upgrade story is far from ideal.
My thinking was to put the #error in the generic header, so architectures that
are not supported yet cannot build against rseq.h at all, so we don't end up
in a broken upgrade scenario. I'm open to alternative ways to do it though, as
long as we don't let not-yet-supported architectures build broken code.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 13/18] io_uring: add file set registration
From: Jann Horn @ 2019-01-30 1:29 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-aio, linux-block, Linux API, hch, jmoyer, avi, Al Viro,
linux-fsdevel
In-Reply-To: <20190129192702.3605-14-axboe@kernel.dk>
On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
> We normally have to fget/fput for each IO we do on a file. Even with
> the batching we do, the cost of the atomic inc/dec of the file usage
> count adds up.
>
> This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
> for the io_uring_register(2) system call. The arguments passed in must
> be an array of __s32 holding file descriptors, and nr_args should hold
> the number of file descriptors the application wishes to pin for the
> duration of the io_uring context (or until IORING_UNREGISTER_FILES is
> called).
>
> When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
> member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
> to the index in the array passed in to IORING_REGISTER_FILES.
>
> Files are automatically unregistered when the io_uring context is
> torn down. An application need only unregister if it wishes to
> register a new set of fds.
Crazy idea:
Taking a step back, at a high level, basically this patch creates sort
of the same difference that you get when you compare the following
scenarios for normal multithreaded I/O in userspace:
===========================================================
~/tests/fdget_perf$ cat fdget_perf.c
#define _GNU_SOURCE
#include <sys/wait.h>
#include <sched.h>
#include <unistd.h>
#include <stdbool.h>
#include <string.h>
#include <err.h>
#include <signal.h>
#include <sys/eventfd.h>
#include <stdio.h>
// two different physical processors on my machine
#define CORE_A 0
#define CORE_B 14
static void pin_to_core(int coreid) {
cpu_set_t set;
CPU_ZERO(&set);
CPU_SET(coreid, &set);
if (sched_setaffinity(0, sizeof(cpu_set_t), &set))
err(1, "sched_setaffinity");
}
static int fd = -1;
static volatile int time_over = 0;
static void alarm_handler(int sig) { time_over = 1; }
static void run_stuff(void) {
unsigned long long iterations = 0;
if (signal(SIGALRM, alarm_handler) == SIG_ERR) err(1, "signal");
alarm(10);
while (1) {
uint64_t val;
read(fd, &val, sizeof(val));
if (time_over) {
printf("iterations = 0x%llx\n", iterations);
return;
}
iterations++;
}
}
static int child_fn(void *dummy) {
pin_to_core(CORE_B);
run_stuff();
return 0;
}
static char child_stack[1024*1024];
int main(int argc, char **argv) {
fd = eventfd(0, EFD_NONBLOCK);
if (fd == -1) err(1, "eventfd");
if (argc != 2) errx(1, "bad usage");
int flags = SIGCHLD;
if (strcmp(argv[1], "shared") == 0) {
flags |= CLONE_FILES;
} else if (strcmp(argv[1], "cloned") == 0) {
/* nothing */
} else {
errx(1, "bad usage");
}
pid_t child = clone(child_fn, child_stack+sizeof(child_stack), flags, NULL);
if (child == -1) err(1, "clone");
pin_to_core(CORE_A);
run_stuff();
int status;
if (wait(&status) != child) err(1, "wait");
return 0;
}
~/tests/fdget_perf$ gcc -Wall -o fdget_perf fdget_perf.c
~/tests/fdget_perf$ ./fdget_perf shared
iterations = 0x8d3010
iterations = 0x92d894
~/tests/fdget_perf$ ./fdget_perf cloned
iterations = 0xad3bbd
iterations = 0xb08838
~/tests/fdget_perf$ ./fdget_perf shared
iterations = 0x8cc340
iterations = 0x8e4e64
~/tests/fdget_perf$ ./fdget_perf cloned
iterations = 0xada5f3
iterations = 0xb04b6f
===========================================================
This kinda makes me wonder whether this is really something that
should be implemented specifically for the io_uring API, or whether it
would make sense to somehow handle part of this in the generic VFS
code and give the user the ability to prepare a new files_struct that
can then be transferred to the worker thread, or something like
that... I'm not sure whether there's a particularly clean way to do
that though.
Or perhaps you could add a userspace API for marking file descriptor
table entries as "has percpu refcounting" somehow, with one percpu
refcount per files_struct and one bit per fd, allocated when percpu
refcounting is activated for the files_struct the first time, or
something like that...
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
> fs/io_uring.c | 138 +++++++++++++++++++++++++++++-----
> include/uapi/linux/io_uring.h | 9 ++-
> 2 files changed, 127 insertions(+), 20 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 17c869f3ea2f..13c3f8212815 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -100,6 +100,14 @@ struct io_ring_ctx {
> struct fasync_struct *cq_fasync;
> } ____cacheline_aligned_in_smp;
>
> + /*
> + * If used, fixed file set. Writers must ensure that ->refs is dead,
> + * readers must ensure that ->refs is alive as long as the file* is
> + * used. Only updated through io_uring_register(2).
> + */
> + struct file **user_files;
> + unsigned nr_user_files;
> +
> /* if used, fixed mapped user buffers */
> unsigned nr_user_bufs;
> struct io_mapped_ubuf *user_bufs;
> @@ -136,6 +144,7 @@ struct io_kiocb {
> unsigned int flags;
> #define REQ_F_FORCE_NONBLOCK 1 /* inline submission attempt */
> #define REQ_F_IOPOLL_COMPLETED 2 /* polled IO has completed */
> +#define REQ_F_FIXED_FILE 4 /* ctx owns file */
> u64 user_data;
> u64 error;
>
> @@ -350,15 +359,17 @@ static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
> * Batched puts of the same file, to avoid dirtying the
> * file usage count multiple times, if avoidable.
> */
> - if (!file) {
> - file = req->rw.ki_filp;
> - file_count = 1;
> - } else if (file == req->rw.ki_filp) {
> - file_count++;
> - } else {
> - fput_many(file, file_count);
> - file = req->rw.ki_filp;
> - file_count = 1;
> + if (!(req->flags & REQ_F_FIXED_FILE)) {
> + if (!file) {
> + file = req->rw.ki_filp;
> + file_count = 1;
> + } else if (file == req->rw.ki_filp) {
> + file_count++;
> + } else {
> + fput_many(file, file_count);
> + file = req->rw.ki_filp;
> + file_count = 1;
> + }
> }
>
> if (to_free == ARRAY_SIZE(reqs))
> @@ -491,13 +502,19 @@ static void kiocb_end_write(struct kiocb *kiocb)
> }
> }
>
> +static void io_fput(struct io_kiocb *req)
> +{
> + if (!(req->flags & REQ_F_FIXED_FILE))
> + fput(req->rw.ki_filp);
> +}
> +
> static void io_complete_rw(struct kiocb *kiocb, long res, long res2)
> {
> struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw);
>
> kiocb_end_write(kiocb);
>
> - fput(kiocb->ki_filp);
> + io_fput(req);
> io_cqring_add_event(req->ctx, req->user_data, res, 0);
> io_free_req(req);
> }
> @@ -596,11 +613,22 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> {
> struct io_ring_ctx *ctx = req->ctx;
> struct kiocb *kiocb = &req->rw;
> - unsigned ioprio;
> + unsigned ioprio, flags;
> int fd, ret;
>
> + flags = READ_ONCE(sqe->flags);
> fd = READ_ONCE(sqe->fd);
> - kiocb->ki_filp = io_file_get(state, fd);
> +
> + if (flags & IOSQE_FIXED_FILE) {
> + if (unlikely(!ctx->user_files ||
> + (unsigned) fd >= ctx->nr_user_files))
> + return -EBADF;
> + kiocb->ki_filp = ctx->user_files[fd];
> + req->flags |= REQ_F_FIXED_FILE;
> + } else {
> + kiocb->ki_filp = io_file_get(state, fd);
> + }
> +
> if (unlikely(!kiocb->ki_filp))
> return -EBADF;
> kiocb->ki_pos = READ_ONCE(sqe->off);
> @@ -641,7 +669,8 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> }
> return 0;
> out_fput:
> - io_file_put(state, kiocb->ki_filp);
> + if (!(flags & IOSQE_FIXED_FILE))
> + io_file_put(state, kiocb->ki_filp);
> return ret;
> }
>
> @@ -765,7 +794,7 @@ static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> kfree(iovec);
> out_fput:
> if (unlikely(ret))
> - fput(file);
> + io_fput(req);
> return ret;
> }
>
> @@ -820,7 +849,7 @@ static ssize_t io_write(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> kfree(iovec);
> out_fput:
> if (unlikely(ret))
> - fput(file);
> + io_fput(req);
> return ret;
> }
>
> @@ -846,7 +875,7 @@ static int io_fsync(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> loff_t sqe_off = READ_ONCE(sqe->off);
> loff_t sqe_len = READ_ONCE(sqe->len);
> loff_t end = sqe_off + sqe_len;
> - unsigned fsync_flags;
> + unsigned fsync_flags, flags;
> struct file *file;
> int ret, fd;
>
> @@ -864,14 +893,23 @@ static int io_fsync(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> return -EINVAL;
>
> fd = READ_ONCE(sqe->fd);
> - file = fget(fd);
> + flags = READ_ONCE(sqe->flags);
> +
> + if (flags & IOSQE_FIXED_FILE) {
> + if (unlikely(!ctx->user_files || fd >= ctx->nr_user_files))
> + return -EBADF;
> + file = ctx->user_files[fd];
> + } else {
> + file = fget(fd);
> + }
> if (unlikely(!file))
> return -EBADF;
>
> ret = vfs_fsync_range(file, sqe_off, end > 0 ? end : LLONG_MAX,
> fsync_flags & IORING_FSYNC_DATASYNC);
>
> - fput(file);
> + if (!(flags & IOSQE_FIXED_FILE))
> + fput(file);
> io_cqring_add_event(ctx, sqe->user_data, ret, 0);
> io_free_req(req);
> return 0;
> @@ -1002,7 +1040,7 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, const struct sqe_submit *s,
> ssize_t ret;
>
> /* enforce forwards compatibility on users */
> - if (unlikely(s->sqe->flags))
> + if (unlikely(s->sqe->flags & ~IOSQE_FIXED_FILE))
> return -EINVAL;
>
> req = io_get_req(ctx, state);
> @@ -1220,6 +1258,58 @@ static int __io_uring_enter(struct io_ring_ctx *ctx, unsigned to_submit,
> return submitted ? submitted : ret;
> }
>
> +static int io_sqe_files_unregister(struct io_ring_ctx *ctx)
> +{
> + int i;
> +
> + if (!ctx->user_files)
> + return -ENXIO;
> +
> + for (i = 0; i < ctx->nr_user_files; i++)
> + fput(ctx->user_files[i]);
> +
> + kfree(ctx->user_files);
> + ctx->user_files = NULL;
> + ctx->nr_user_files = 0;
> + return 0;
> +}
> +
> +static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
> + unsigned nr_args)
> +{
> + __s32 __user *fds = (__s32 __user *) arg;
> + int fd, ret = 0;
> + unsigned i;
> +
> + if (ctx->user_files)
> + return -EBUSY;
> + if (!nr_args)
> + return -EINVAL;
> +
> + ctx->user_files = kcalloc(nr_args, sizeof(struct file *), GFP_KERNEL);
> + if (!ctx->user_files)
> + return -ENOMEM;
> +
> + for (i = 0; i < nr_args; i++) {
> + ret = -EFAULT;
> + if (copy_from_user(&fd, &fds[i], sizeof(fd)))
> + break;
> +
> + ctx->user_files[i] = fget(fd);
> +
> + ret = -EBADF;
> + if (!ctx->user_files[i])
> + break;
> + ctx->nr_user_files++;
> + ret = 0;
> + }
> +
> + if (ret)
> + io_sqe_files_unregister(ctx);
> +
> + return ret;
> +}
> +
> static int io_sq_offload_start(struct io_ring_ctx *ctx)
> {
> int ret;
> @@ -1509,6 +1599,7 @@ static void io_ring_ctx_free(struct io_ring_ctx *ctx)
>
> io_iopoll_reap_events(ctx);
> io_sqe_buffer_unregister(ctx);
> + io_sqe_files_unregister(ctx);
>
> io_mem_free(ctx->sq_ring);
> io_mem_free(ctx->sq_sqes);
> @@ -1806,6 +1897,15 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
> break;
> ret = io_sqe_buffer_unregister(ctx);
> break;
> + case IORING_REGISTER_FILES:
> + ret = io_sqe_files_register(ctx, arg, nr_args);
> + break;
> + case IORING_UNREGISTER_FILES:
> + ret = -EINVAL;
> + if (arg || nr_args)
> + break;
> + ret = io_sqe_files_unregister(ctx);
> + break;
> default:
> ret = -EINVAL;
> break;
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 16c423d74f2e..3e79feb34a9c 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -16,7 +16,7 @@
> */
> struct io_uring_sqe {
> __u8 opcode; /* type of operation for this sqe */
> - __u8 flags; /* as of now unused */
> + __u8 flags; /* IOSQE_ flags */
> __u16 ioprio; /* ioprio for the request */
> __s32 fd; /* file descriptor to do IO on */
> __u64 off; /* offset into file */
> @@ -33,6 +33,11 @@ struct io_uring_sqe {
> };
> };
>
> +/*
> + * sqe->flags
> + */
> +#define IOSQE_FIXED_FILE (1U << 0) /* use fixed fileset */
> +
> /*
> * io_uring_setup() flags
> */
> @@ -112,5 +117,7 @@ struct io_uring_params {
> */
> #define IORING_REGISTER_BUFFERS 0
> #define IORING_UNREGISTER_BUFFERS 1
> +#define IORING_REGISTER_FILES 2
> +#define IORING_UNREGISTER_FILES 3
>
> #endif
> --
> 2.17.1
>
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [RFD] A mount api that notices previous mounts
From: Eric W. Biederman @ 2019-01-30 1:23 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Al Viro, David Howells,
Miklos Szeredi, Linus Torvalds, Karel Zak,
util-linux-u79uwXL29TY76Z2rM5mHXA, Andy Lutomirski, LSM
In-Reply-To: <87r2cvx7wz.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) writes:
> Casey Schaufler <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> writes:
>> Are you taking the LSM specific mount options into account?
>
> In the design yes, and I allow setting them. It appears in the code
> to retrieve the mount options I forgot to call security_sb_show_options.
>
> For finding the super block that you are going to mount the LSM mount
> options are not relevant. Even nfs will not want to set those early as
> they do not help determine the nfs super block. So the only place where
> there is anything interesting in my api is in reading back the security
> options so they can be compared to the options the mounter is setting.
>
> I will add the missing call to security_sb_show_options which is enough
> to fix selinux. Unfortunately smack does not currently implement
> .sb_show_options. Not implementing smack_sb_show_options means
> /proc/mounts fails to match /etc/mtab which is a bug and it is likely
> a real workd bug for the people who use smack and don't want to depend
> on /etc/mtab, or are transitioning away from it.
>
> Casey do you want to implement smack_sb_show_options or should I put it
> on my todo list?
Oh. I should add that I am always parsing the LSM mount options out so
that there is not a chance of the individual filesystems implementing
comflicting options even when there are no LSMs active. Without that I
am afraid we run the risk of having LSM mount otions in conflict with
ordinary filesystems options at some point and by the time we discover
it it would start introducing filesystem regressions.
That does help with stack though as there is no fundamental reason only
one LSM could process mount options.
Eric
^ permalink raw reply
* Re: [RFD] A mount api that notices previous mounts
From: Eric W. Biederman @ 2019-01-30 1:15 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-api, linux-fsdevel, linux-kernel, Al Viro, David Howells,
Miklos Szeredi, Linus Torvalds, Karel Zak, util-linux,
Andy Lutomirski, LSM
In-Reply-To: <03e0993b-21db-1cc4-7a33-0236de7be20d@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> writes:
> On 1/29/2019 1:44 PM, Eric W. Biederman wrote:
>> All,
>>
>> With the existing mount API it is possible to mount a filesystem
>> like:
>>
>> mount -t ext4 /dev/sda1 -o user_xattr /some/path
>> mount -t ext4 /dev/sda1 -o nouser_xattr /some/other/path
>>
>> And have both mount commands succeed and have two mounts of the same
>> filesystem. If the mounter is not attentive or the first mount is added
>> earlier it may not be immediately noticed that a mount option that is
>> needed for the correct operation or the security of the system is lost.
>>
>> We have seen this failure mode with both devpts and proc. So it is not
>> theoretical, and it has resulted in CVEs.
>>
>> In some cases the existing mount API (such as a conflict between ro and
>> rw) handles this by returning -EBUSY. So we may be able to correct this
>> in the existing mount API. But it is always very tricky to to get
>> adequate testing for a change like that to avoid regressions, so I am
>> proposing we change this in the new mount api.
>>
>> This has been brought up before and I have been told it is technically
>> infeasible to make this work. To counter that I have sat down and
>> implemented it.
>>
>> The basic idea is:
>> - get a handle to a filesystem
>> (passing enough options to uniquely identify the super block).
>> Also capture enough state in the file handle to let you know if
>> the file system has it's mount options changed between system calls.
>> (essentially this is just the fs code that calls sget)
>>
>> - If the super block has not been configured allow setting the file
>> systems options.
>>
>> - If the super block has already been configured require reading the
>> file systems mount options before setting/updating the file systems
>> mount options.
>>
>> To complement that I have functionality that:
>> - Allows reading a file systems current mount options.
>> - Allows reading the mount options that are needed to get a handle to
>> the filesystem. For most filesystems it is just the block device
>> name. For nfs is is essentially all mount options. For btrfs
>> it is the block device name, and the "devices=" mount option for
>> secondary block device names.
>
> Are you taking the LSM specific mount options into account?
In the design yes, and I allow setting them. It appears in the code
to retrieve the mount options I forgot to call security_sb_show_options.
For finding the super block that you are going to mount the LSM mount
options are not relevant. Even nfs will not want to set those early as
they do not help determine the nfs super block. So the only place where
there is anything interesting in my api is in reading back the security
options so they can be compared to the options the mounter is setting.
I will add the missing call to security_sb_show_options which is enough
to fix selinux. Unfortunately smack does not currently implement
.sb_show_options. Not implementing smack_sb_show_options means
/proc/mounts fails to match /etc/mtab which is a bug and it is likely
a real workd bug for the people who use smack and don't want to depend
on /etc/mtab, or are transitioning away from it.
Casey do you want to implement smack_sb_show_options or should I put it
on my todo list?
Eric
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-29 23:52 UTC (permalink / raw)
To: Dominique Martinet
Cc: Linus Torvalds, 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: <20190128000547.GA25155@nautica>
On Mon, 28 Jan 2019, Dominique Martinet wrote:
> > So, any objections to aproaching it this way?
>
> I'm not sure why I'm the main recipient of that mail but answering
> because I am -- let's get these patches in through the regular -mm tree
> though
*prod to mm maintainers* (at least for an opinion)
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jens Axboe @ 2019-01-29 23:51 UTC (permalink / raw)
To: Jann Horn; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <CAG48ez3Jtq3ka+cSVxQWjMLQU=YRqV2dJuq5Kp2Nx_YLz3FsNQ@mail.gmail.com>
On 1/29/19 4:42 PM, Jann Horn wrote:
> On Wed, Jan 30, 2019 at 12:14 AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/29/19 4:08 PM, Jann Horn wrote:
>>> On Wed, Jan 30, 2019 at 12:06 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 1/29/19 4:03 PM, Jann Horn wrote:
>>>>> On Tue, Jan 29, 2019 at 11:56 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> On 1/29/19 3:44 PM, Jann Horn wrote:
>>>>>>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>> If we have fixed user buffers, we can map them into the kernel when we
>>>>>>>> setup the io_context. That avoids the need to do get_user_pages() for
>>>>>>>> each and every IO.
>>>>>>>>
>>>>>>>> To utilize this feature, the application must call io_uring_register()
>>>>>>>> after having setup an io_uring context, passing in
>>>>>>>> IORING_REGISTER_BUFFERS as the opcode. The argument must be a pointer
>>>>>>>> to an iovec array, and the nr_args should contain how many iovecs the
>>>>>>>> application wishes to map.
>>>>>>>>
>>>>>>>> If successful, these buffers are now mapped into the kernel, eligible
>>>>>>>> for IO. To use these fixed buffers, the application must use the
>>>>>>>> IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED opcodes, and then
>>>>>>>> set sqe->index to the desired buffer index. sqe->addr..sqe->addr+seq->len
>>>>>>>> must point to somewhere inside the indexed buffer.
>>>>>>>>
>>>>>>>> The application may register buffers throughout the lifetime of the
>>>>>>>> io_uring context. It can call io_uring_register() with
>>>>>>>> IORING_UNREGISTER_BUFFERS as the opcode to unregister the current set of
>>>>>>>> buffers, and then register a new set. The application need not
>>>>>>>> unregister buffers explicitly before shutting down the io_uring context.
> [...]
>> Just folded in this incremental, which should fix all the issues outlined
>> in your email.
>>
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 7364feebafed..d42541357969 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
> [...]
>> @@ -1602,6 +1605,7 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, struct sqe_submit *sqes,
>>
>> static int io_sq_thread(void *data)
>> {
>> + struct io_uring_sqe lsqe[IO_IOPOLL_BATCH];
>> struct sqe_submit sqes[IO_IOPOLL_BATCH];
>> struct io_ring_ctx *ctx = data;
>> struct mm_struct *cur_mm = NULL;
>> @@ -1701,6 +1705,14 @@ static int io_sq_thread(void *data)
>> i = 0;
>> all_fixed = true;
>> do {
>> + /*
>> + * Ensure sqe is stable between checking if we need
>> + * user access, and actually importing the iovec
>> + * further down the stack.
>> + */
>> + memcpy(&lsqe[i], sqes[i].sqe, sizeof(lsqe[i]));
>> + sqes[i].sqe = &lsqe[i];
>> +
>
> What if io_submit_sqe() gets an -EAGAIN from __io_submit_sqe() and
> queues io_sq_wq_submit_work()? Could that cause io_sq_wq_submit_work()
> to read from io_sq_thread()'s lsge[i] while io_sq_thread() is already
> in the next loop iteration of the outer loop and has copied new data
> into lsge[i]?
Hmm yes. I think we'll need to both embed a pointer to an sqe into
sqe_submit, and an actual sqe. The former can be used in the fast
path, while the latter will be our copy for the offload path for
io_sq_thread() and io_sq_wq_submit_work().
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 09/18] io_uring: use fget/fput_many() for file references
From: Jens Axboe @ 2019-01-29 23:44 UTC (permalink / raw)
To: Jann Horn; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <CAG48ez3_1mY1Fb91Q3H-S2+4OiMsV5F9v78JX+YMjWqLteHpzw@mail.gmail.com>
On 1/29/19 4:31 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>> Add a separate io_submit_state structure, to cache some of the things
>> we need for IO submission.
>>
>> One such example is file reference batching. io_submit_state. We get as
>> many references as the number of sqes we are submitting, and drop
>> unused ones if we end up switching files. The assumption here is that
>> we're usually only dealing with one fd, and if there are multiple,
>> hopefuly they are at least somewhat ordered. Could trivially be extended
>> to cover multiple fds, if needed.
>>
>> On the completion side we do the same thing, except this is trivially
>> done just locally in io_iopoll_reap().
> [...]
>> +static void io_file_put(struct io_submit_state *state, struct file *file)
>> +{
>> + if (!state) {
>> + fput(file);
>> + } else if (state->file) {
>> + int diff = state->has_refs - state->used_refs;
>> +
>> + if (diff)
>> + fput_many(state->file, diff);
>> + state->file = NULL;
>> + }
>> +}
>
> Hmm, this function confuses me.
> The state==NULL path works as I'd expect, it calls fput() on the file.
> But if `state!=NULL && state->file==NULL`, it does nothing, it never
> uses `file`.
> And if `state->file!=NULL`, it drops the excess bias on the file's
> refcount, but it doesn't drop the current reference - and again
> without even looking at `file`.
>
> So when io_prep_rw() uses io_file_get() to grab a reference on a file
> it hasn't seen before, it will acquire `ios_left` references and
> actually use one of them; then if it goes through the out_fput error
> path, it goes through the path for `state->file!=NULL`, drops
> `ios_left-1` references (leaving the refcount elevated by 1), and
> forgets about the file?
I'll take a look, it's not impossible there's an off-by-one there.
>> +/*
>> + * Get as many references to a file as we have IOs left in this submission,
>> + * assuming most submissions are for one file, or at least that each file
>> + * has more than one submission.
>> + */
>> +static struct file *io_file_get(struct io_submit_state *state, int fd)
>> +{
>> + if (!state)
>> + return fget(fd);
>> +
>> + if (state->file) {
>> + if (state->fd == fd) {
>> + state->used_refs++;
>> + state->ios_left--;
>> + return state->file;
>> + }
>> + io_file_put(state, NULL);
>> + }
>> + state->file = fget_many(fd, state->ios_left);
>> + if (!state->file)
>> + return NULL;
>> +
>> + state->fd = fd;
>> + state->has_refs = state->ios_left;
>> + state->used_refs = 1;
>> + state->ios_left--;
>> + return state->file;
>> +}
>> +
>> static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>> - bool force_nonblock)
>> + bool force_nonblock, struct io_submit_state *state)
>> {
>> struct io_ring_ctx *ctx = req->ctx;
>> struct kiocb *kiocb = &req->rw;
>> @@ -487,7 +560,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>> int fd, ret;
>>
>> fd = READ_ONCE(sqe->fd);
>> - kiocb->ki_filp = fget(fd);
>> + kiocb->ki_filp = io_file_get(state, fd);
>> if (unlikely(!kiocb->ki_filp))
>> return -EBADF;
>> kiocb->ki_pos = READ_ONCE(sqe->off);
>> @@ -528,7 +601,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>> }
>> return 0;
>> out_fput:
>> - fput(kiocb->ki_filp);
>> + io_file_put(state, kiocb->ki_filp);
>> return ret;
>> }
> [...]
>> +static void io_submit_state_start(struct io_submit_state *state,
>> + struct io_ring_ctx *ctx, unsigned max_ios)
>
> There are various places in your series where you use raw "unsigned"
> instead of "unsigned int"; when I run your tree through checkpatch.pl,
> it complains about that and a few other things. Please fix the
> checkpatch warnings (except for warnings where you know that they
> shouldn't apply here for some reason).
Using unsigned is just fine, it's the same thing. checkpatch.pl
complains about a lot of stuff that doesn't matter, that's one of them.
I don't mind fixing valid warnings, but this particular one is just
noise.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jann Horn @ 2019-01-29 23:42 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <379a631f-e55b-cc31-f84a-ace73fd66ea1@kernel.dk>
On Wed, Jan 30, 2019 at 12:14 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/29/19 4:08 PM, Jann Horn wrote:
> > On Wed, Jan 30, 2019 at 12:06 AM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 1/29/19 4:03 PM, Jann Horn wrote:
> >>> On Tue, Jan 29, 2019 at 11:56 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> On 1/29/19 3:44 PM, Jann Horn wrote:
> >>>>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>> If we have fixed user buffers, we can map them into the kernel when we
> >>>>>> setup the io_context. That avoids the need to do get_user_pages() for
> >>>>>> each and every IO.
> >>>>>>
> >>>>>> To utilize this feature, the application must call io_uring_register()
> >>>>>> after having setup an io_uring context, passing in
> >>>>>> IORING_REGISTER_BUFFERS as the opcode. The argument must be a pointer
> >>>>>> to an iovec array, and the nr_args should contain how many iovecs the
> >>>>>> application wishes to map.
> >>>>>>
> >>>>>> If successful, these buffers are now mapped into the kernel, eligible
> >>>>>> for IO. To use these fixed buffers, the application must use the
> >>>>>> IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED opcodes, and then
> >>>>>> set sqe->index to the desired buffer index. sqe->addr..sqe->addr+seq->len
> >>>>>> must point to somewhere inside the indexed buffer.
> >>>>>>
> >>>>>> The application may register buffers throughout the lifetime of the
> >>>>>> io_uring context. It can call io_uring_register() with
> >>>>>> IORING_UNREGISTER_BUFFERS as the opcode to unregister the current set of
> >>>>>> buffers, and then register a new set. The application need not
> >>>>>> unregister buffers explicitly before shutting down the io_uring context.
[...]
> Just folded in this incremental, which should fix all the issues outlined
> in your email.
>
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 7364feebafed..d42541357969 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
[...]
> @@ -1602,6 +1605,7 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, struct sqe_submit *sqes,
>
> static int io_sq_thread(void *data)
> {
> + struct io_uring_sqe lsqe[IO_IOPOLL_BATCH];
> struct sqe_submit sqes[IO_IOPOLL_BATCH];
> struct io_ring_ctx *ctx = data;
> struct mm_struct *cur_mm = NULL;
> @@ -1701,6 +1705,14 @@ static int io_sq_thread(void *data)
> i = 0;
> all_fixed = true;
> do {
> + /*
> + * Ensure sqe is stable between checking if we need
> + * user access, and actually importing the iovec
> + * further down the stack.
> + */
> + memcpy(&lsqe[i], sqes[i].sqe, sizeof(lsqe[i]));
> + sqes[i].sqe = &lsqe[i];
> +
What if io_submit_sqe() gets an -EAGAIN from __io_submit_sqe() and
queues io_sq_wq_submit_work()? Could that cause io_sq_wq_submit_work()
to read from io_sq_thread()'s lsge[i] while io_sq_thread() is already
in the next loop iteration of the outer loop and has copied new data
into lsge[i]?
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 09/18] io_uring: use fget/fput_many() for file references
From: Jann Horn @ 2019-01-29 23:31 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <20190129192702.3605-10-axboe@kernel.dk>
On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
> Add a separate io_submit_state structure, to cache some of the things
> we need for IO submission.
>
> One such example is file reference batching. io_submit_state. We get as
> many references as the number of sqes we are submitting, and drop
> unused ones if we end up switching files. The assumption here is that
> we're usually only dealing with one fd, and if there are multiple,
> hopefuly they are at least somewhat ordered. Could trivially be extended
> to cover multiple fds, if needed.
>
> On the completion side we do the same thing, except this is trivially
> done just locally in io_iopoll_reap().
[...]
> +static void io_file_put(struct io_submit_state *state, struct file *file)
> +{
> + if (!state) {
> + fput(file);
> + } else if (state->file) {
> + int diff = state->has_refs - state->used_refs;
> +
> + if (diff)
> + fput_many(state->file, diff);
> + state->file = NULL;
> + }
> +}
Hmm, this function confuses me.
The state==NULL path works as I'd expect, it calls fput() on the file.
But if `state!=NULL && state->file==NULL`, it does nothing, it never
uses `file`.
And if `state->file!=NULL`, it drops the excess bias on the file's
refcount, but it doesn't drop the current reference - and again
without even looking at `file`.
So when io_prep_rw() uses io_file_get() to grab a reference on a file
it hasn't seen before, it will acquire `ios_left` references and
actually use one of them; then if it goes through the out_fput error
path, it goes through the path for `state->file!=NULL`, drops
`ios_left-1` references (leaving the refcount elevated by 1), and
forgets about the file?
> +/*
> + * Get as many references to a file as we have IOs left in this submission,
> + * assuming most submissions are for one file, or at least that each file
> + * has more than one submission.
> + */
> +static struct file *io_file_get(struct io_submit_state *state, int fd)
> +{
> + if (!state)
> + return fget(fd);
> +
> + if (state->file) {
> + if (state->fd == fd) {
> + state->used_refs++;
> + state->ios_left--;
> + return state->file;
> + }
> + io_file_put(state, NULL);
> + }
> + state->file = fget_many(fd, state->ios_left);
> + if (!state->file)
> + return NULL;
> +
> + state->fd = fd;
> + state->has_refs = state->ios_left;
> + state->used_refs = 1;
> + state->ios_left--;
> + return state->file;
> +}
> +
> static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> - bool force_nonblock)
> + bool force_nonblock, struct io_submit_state *state)
> {
> struct io_ring_ctx *ctx = req->ctx;
> struct kiocb *kiocb = &req->rw;
> @@ -487,7 +560,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> int fd, ret;
>
> fd = READ_ONCE(sqe->fd);
> - kiocb->ki_filp = fget(fd);
> + kiocb->ki_filp = io_file_get(state, fd);
> if (unlikely(!kiocb->ki_filp))
> return -EBADF;
> kiocb->ki_pos = READ_ONCE(sqe->off);
> @@ -528,7 +601,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> }
> return 0;
> out_fput:
> - fput(kiocb->ki_filp);
> + io_file_put(state, kiocb->ki_filp);
> return ret;
> }
[...]
> +static void io_submit_state_start(struct io_submit_state *state,
> + struct io_ring_ctx *ctx, unsigned max_ios)
There are various places in your series where you use raw "unsigned"
instead of "unsigned int"; when I run your tree through checkpatch.pl,
it complains about that and a few other things. Please fix the
checkpatch warnings (except for warnings where you know that they
shouldn't apply here for some reason).
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jens Axboe @ 2019-01-29 23:14 UTC (permalink / raw)
To: Jann Horn; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <CAG48ez1m22z0Pqi0cT=UZdsA14SCM5579T1d40cZdyD6KqBw_g@mail.gmail.com>
On 1/29/19 4:08 PM, Jann Horn wrote:
> On Wed, Jan 30, 2019 at 12:06 AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/29/19 4:03 PM, Jann Horn wrote:
>>> On Tue, Jan 29, 2019 at 11:56 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 1/29/19 3:44 PM, Jann Horn wrote:
>>>>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> If we have fixed user buffers, we can map them into the kernel when we
>>>>>> setup the io_context. That avoids the need to do get_user_pages() for
>>>>>> each and every IO.
>>>>>>
>>>>>> To utilize this feature, the application must call io_uring_register()
>>>>>> after having setup an io_uring context, passing in
>>>>>> IORING_REGISTER_BUFFERS as the opcode. The argument must be a pointer
>>>>>> to an iovec array, and the nr_args should contain how many iovecs the
>>>>>> application wishes to map.
>>>>>>
>>>>>> If successful, these buffers are now mapped into the kernel, eligible
>>>>>> for IO. To use these fixed buffers, the application must use the
>>>>>> IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED opcodes, and then
>>>>>> set sqe->index to the desired buffer index. sqe->addr..sqe->addr+seq->len
>>>>>> must point to somewhere inside the indexed buffer.
>>>>>>
>>>>>> The application may register buffers throughout the lifetime of the
>>>>>> io_uring context. It can call io_uring_register() with
>>>>>> IORING_UNREGISTER_BUFFERS as the opcode to unregister the current set of
>>>>>> buffers, and then register a new set. The application need not
>>>>>> unregister buffers explicitly before shutting down the io_uring context.
>>> [...]
>>>>>> + imu = &ctx->user_bufs[index];
>>>>>> + buf_addr = READ_ONCE(sqe->addr);
>>>>>> + if (buf_addr < imu->ubuf || buf_addr + len > imu->ubuf + imu->len)
>>>>>
>>>>> This can wrap around if `buf_addr` or `len` is very big, right? Then
>>>>> you e.g. get past the first check because `buf_addr` is sufficiently
>>>>> big, and get past the second check because `buf_addr + len` wraps
>>>>> around and becomes small.
>>>>
>>>> Good point. I wonder if we have a verification helper for something like
>>>> this?
>>>
>>> check_add_overflow() exists, I guess that might help a bit. I don't
>>> think I've seen a more specific helper for this situation.
>>
>> Hmm, not super appropriate. How about something ala:
>>
>> if (buf_addr + len < buf_addr)
>> ... overflow ...
>>
>> ?
>
> Sure, sounds good.
Just folded in this incremental, which should fix all the issues outlined
in your email.
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 7364feebafed..d42541357969 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -751,7 +751,7 @@ static int io_import_fixed(struct io_ring_ctx *ctx, int rw,
{
size_t len = READ_ONCE(sqe->len);
struct io_mapped_ubuf *imu;
- int buf_index, index;
+ unsigned index, buf_index;
size_t offset;
u64 buf_addr;
@@ -763,9 +763,12 @@ static int io_import_fixed(struct io_ring_ctx *ctx, int rw,
if (unlikely(buf_index >= ctx->nr_user_bufs))
return -EFAULT;
- index = array_index_nospec(buf_index, ctx->sq_entries);
+ index = array_index_nospec(buf_index, ctx->nr_user_bufs);
imu = &ctx->user_bufs[index];
buf_addr = READ_ONCE(sqe->addr);
+
+ if (buf_addr + len < buf_addr)
+ return -EFAULT;
if (buf_addr < imu->ubuf || buf_addr + len > imu->ubuf + imu->len)
return -EFAULT;
@@ -1602,6 +1605,7 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, struct sqe_submit *sqes,
static int io_sq_thread(void *data)
{
+ struct io_uring_sqe lsqe[IO_IOPOLL_BATCH];
struct sqe_submit sqes[IO_IOPOLL_BATCH];
struct io_ring_ctx *ctx = data;
struct mm_struct *cur_mm = NULL;
@@ -1701,6 +1705,14 @@ static int io_sq_thread(void *data)
i = 0;
all_fixed = true;
do {
+ /*
+ * Ensure sqe is stable between checking if we need
+ * user access, and actually importing the iovec
+ * further down the stack.
+ */
+ memcpy(&lsqe[i], sqes[i].sqe, sizeof(lsqe[i]));
+ sqes[i].sqe = &lsqe[i];
+
if (all_fixed && io_sqe_needs_user(sqes[i].sqe))
all_fixed = false;
@@ -2081,7 +2093,7 @@ static int io_copy_iov(struct io_ring_ctx *ctx, struct iovec *dst,
struct iovec __user *src;
#ifdef CONFIG_COMPAT
- if (in_compat_syscall()) {
+ if (ctx->compat) {
struct compat_iovec __user *ciovs;
struct compat_iovec ciov;
@@ -2103,7 +2115,6 @@ static int io_copy_iov(struct io_ring_ctx *ctx, struct iovec *dst,
static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg,
unsigned nr_args)
{
- struct vm_area_struct **vmas = NULL;
struct page **pages = NULL;
int i, j, got_pages = 0;
int ret = -EINVAL;
@@ -2138,7 +2149,7 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg,
* submitted if they are wrong.
*/
ret = -EFAULT;
- if (!iov.iov_base)
+ if (!iov.iov_base || !iov.iov_len)
goto err;
/* arbitrary limit, but we need something */
@@ -2155,14 +2166,10 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg,
goto err;
if (!pages || nr_pages > got_pages) {
- kfree(vmas);
kfree(pages);
pages = kmalloc_array(nr_pages, sizeof(struct page *),
GFP_KERNEL);
- vmas = kmalloc_array(nr_pages,
- sizeof(struct vma_area_struct *),
- GFP_KERNEL);
- if (!pages || !vmas) {
+ if (!pages) {
io_unaccount_mem(ctx, nr_pages);
goto err;
}
@@ -2176,32 +2183,18 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg,
goto err;
}
- down_write(¤t->mm->mmap_sem);
- pret = get_user_pages_longterm(ubuf, nr_pages, FOLL_WRITE,
- pages, vmas);
- if (pret == nr_pages) {
- /* don't support file backed memory */
- for (j = 0; j < nr_pages; j++) {
- struct vm_area_struct *vma = vmas[j];
+ down_read(¤t->mm->mmap_sem);
+ pret = get_user_pages_longterm(ubuf, nr_pages,
+ FOLL_WRITE | FOLL_ANON, pages,
+ NULL);
+ up_read(¤t->mm->mmap_sem);
- if (vma->vm_file) {
- ret = -EOPNOTSUPP;
- break;
- }
- }
- } else {
- ret = pret < 0 ? pret : -EFAULT;
- }
- up_write(¤t->mm->mmap_sem);
- if (ret) {
- /*
- * if we did partial map, or found file backed vmas,
- * release any pages we did get
- */
+ if (pret != nr_pages) {
if (pret > 0) {
for (j = 0; j < pret; j++)
put_page(pages[j]);
}
+ ret = pret < 0 ? pret : -EFAULT;
io_unaccount_mem(ctx, nr_pages);
goto err;
}
@@ -2224,12 +2217,10 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg,
imu->nr_bvecs = nr_pages;
}
kfree(pages);
- kfree(vmas);
ctx->nr_user_bufs = nr_args;
return 0;
err:
kfree(pages);
- kfree(vmas);
io_sqe_buffer_unregister(ctx);
return ret;
}
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jann Horn @ 2019-01-29 23:08 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <87b25ce2-a5bc-3e81-ddba-5c932c71b40f@kernel.dk>
On Wed, Jan 30, 2019 at 12:06 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/29/19 4:03 PM, Jann Horn wrote:
> > On Tue, Jan 29, 2019 at 11:56 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 1/29/19 3:44 PM, Jann Horn wrote:
> >>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> If we have fixed user buffers, we can map them into the kernel when we
> >>>> setup the io_context. That avoids the need to do get_user_pages() for
> >>>> each and every IO.
> >>>>
> >>>> To utilize this feature, the application must call io_uring_register()
> >>>> after having setup an io_uring context, passing in
> >>>> IORING_REGISTER_BUFFERS as the opcode. The argument must be a pointer
> >>>> to an iovec array, and the nr_args should contain how many iovecs the
> >>>> application wishes to map.
> >>>>
> >>>> If successful, these buffers are now mapped into the kernel, eligible
> >>>> for IO. To use these fixed buffers, the application must use the
> >>>> IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED opcodes, and then
> >>>> set sqe->index to the desired buffer index. sqe->addr..sqe->addr+seq->len
> >>>> must point to somewhere inside the indexed buffer.
> >>>>
> >>>> The application may register buffers throughout the lifetime of the
> >>>> io_uring context. It can call io_uring_register() with
> >>>> IORING_UNREGISTER_BUFFERS as the opcode to unregister the current set of
> >>>> buffers, and then register a new set. The application need not
> >>>> unregister buffers explicitly before shutting down the io_uring context.
> > [...]
> >>>> + imu = &ctx->user_bufs[index];
> >>>> + buf_addr = READ_ONCE(sqe->addr);
> >>>> + if (buf_addr < imu->ubuf || buf_addr + len > imu->ubuf + imu->len)
> >>>
> >>> This can wrap around if `buf_addr` or `len` is very big, right? Then
> >>> you e.g. get past the first check because `buf_addr` is sufficiently
> >>> big, and get past the second check because `buf_addr + len` wraps
> >>> around and becomes small.
> >>
> >> Good point. I wonder if we have a verification helper for something like
> >> this?
> >
> > check_add_overflow() exists, I guess that might help a bit. I don't
> > think I've seen a more specific helper for this situation.
>
> Hmm, not super appropriate. How about something ala:
>
> if (buf_addr + len < buf_addr)
> ... overflow ...
>
> ?
Sure, sounds good.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jens Axboe @ 2019-01-29 23:06 UTC (permalink / raw)
To: Jann Horn; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <CAG48ez3x5x+P=8L1p-bHvcUnypeT1TizkXq2+Q0bCP0GdE-bog@mail.gmail.com>
On 1/29/19 4:03 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 11:56 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/29/19 3:44 PM, Jann Horn wrote:
>>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> If we have fixed user buffers, we can map them into the kernel when we
>>>> setup the io_context. That avoids the need to do get_user_pages() for
>>>> each and every IO.
>>>>
>>>> To utilize this feature, the application must call io_uring_register()
>>>> after having setup an io_uring context, passing in
>>>> IORING_REGISTER_BUFFERS as the opcode. The argument must be a pointer
>>>> to an iovec array, and the nr_args should contain how many iovecs the
>>>> application wishes to map.
>>>>
>>>> If successful, these buffers are now mapped into the kernel, eligible
>>>> for IO. To use these fixed buffers, the application must use the
>>>> IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED opcodes, and then
>>>> set sqe->index to the desired buffer index. sqe->addr..sqe->addr+seq->len
>>>> must point to somewhere inside the indexed buffer.
>>>>
>>>> The application may register buffers throughout the lifetime of the
>>>> io_uring context. It can call io_uring_register() with
>>>> IORING_UNREGISTER_BUFFERS as the opcode to unregister the current set of
>>>> buffers, and then register a new set. The application need not
>>>> unregister buffers explicitly before shutting down the io_uring context.
> [...]
>>>> + imu = &ctx->user_bufs[index];
>>>> + buf_addr = READ_ONCE(sqe->addr);
>>>> + if (buf_addr < imu->ubuf || buf_addr + len > imu->ubuf + imu->len)
>>>
>>> This can wrap around if `buf_addr` or `len` is very big, right? Then
>>> you e.g. get past the first check because `buf_addr` is sufficiently
>>> big, and get past the second check because `buf_addr + len` wraps
>>> around and becomes small.
>>
>> Good point. I wonder if we have a verification helper for something like
>> this?
>
> check_add_overflow() exists, I guess that might help a bit. I don't
> think I've seen a more specific helper for this situation.
Hmm, not super appropriate. How about something ala:
if (buf_addr + len < buf_addr)
... overflow ...
?
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jann Horn @ 2019-01-29 23:03 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <dd366179-c003-752f-743d-2bea4f6b796c@kernel.dk>
On Tue, Jan 29, 2019 at 11:56 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/29/19 3:44 PM, Jann Horn wrote:
> > On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> If we have fixed user buffers, we can map them into the kernel when we
> >> setup the io_context. That avoids the need to do get_user_pages() for
> >> each and every IO.
> >>
> >> To utilize this feature, the application must call io_uring_register()
> >> after having setup an io_uring context, passing in
> >> IORING_REGISTER_BUFFERS as the opcode. The argument must be a pointer
> >> to an iovec array, and the nr_args should contain how many iovecs the
> >> application wishes to map.
> >>
> >> If successful, these buffers are now mapped into the kernel, eligible
> >> for IO. To use these fixed buffers, the application must use the
> >> IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED opcodes, and then
> >> set sqe->index to the desired buffer index. sqe->addr..sqe->addr+seq->len
> >> must point to somewhere inside the indexed buffer.
> >>
> >> The application may register buffers throughout the lifetime of the
> >> io_uring context. It can call io_uring_register() with
> >> IORING_UNREGISTER_BUFFERS as the opcode to unregister the current set of
> >> buffers, and then register a new set. The application need not
> >> unregister buffers explicitly before shutting down the io_uring context.
[...]
> >> + imu = &ctx->user_bufs[index];
> >> + buf_addr = READ_ONCE(sqe->addr);
> >> + if (buf_addr < imu->ubuf || buf_addr + len > imu->ubuf + imu->len)
> >
> > This can wrap around if `buf_addr` or `len` is very big, right? Then
> > you e.g. get past the first check because `buf_addr` is sufficiently
> > big, and get past the second check because `buf_addr + len` wraps
> > around and becomes small.
>
> Good point. I wonder if we have a verification helper for something like
> this?
check_add_overflow() exists, I guess that might help a bit. I don't
think I've seen a more specific helper for this situation.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [RFD] A mount api that notices previous mounts
From: Casey Schaufler @ 2019-01-29 23:01 UTC (permalink / raw)
To: Eric W. Biederman, linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Al Viro, David Howells,
Miklos Szeredi, Linus Torvalds, Karel Zak,
util-linux-u79uwXL29TY76Z2rM5mHXA, Andy Lutomirski, LSM
In-Reply-To: <87va2716mh.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
On 1/29/2019 1:44 PM, Eric W. Biederman wrote:
> All,
>
> With the existing mount API it is possible to mount a filesystem
> like:
>
> mount -t ext4 /dev/sda1 -o user_xattr /some/path
> mount -t ext4 /dev/sda1 -o nouser_xattr /some/other/path
>
> And have both mount commands succeed and have two mounts of the same
> filesystem. If the mounter is not attentive or the first mount is added
> earlier it may not be immediately noticed that a mount option that is
> needed for the correct operation or the security of the system is lost.
>
> We have seen this failure mode with both devpts and proc. So it is not
> theoretical, and it has resulted in CVEs.
>
> In some cases the existing mount API (such as a conflict between ro and
> rw) handles this by returning -EBUSY. So we may be able to correct this
> in the existing mount API. But it is always very tricky to to get
> adequate testing for a change like that to avoid regressions, so I am
> proposing we change this in the new mount api.
>
> This has been brought up before and I have been told it is technically
> infeasible to make this work. To counter that I have sat down and
> implemented it.
>
> The basic idea is:
> - get a handle to a filesystem
> (passing enough options to uniquely identify the super block).
> Also capture enough state in the file handle to let you know if
> the file system has it's mount options changed between system calls.
> (essentially this is just the fs code that calls sget)
>
> - If the super block has not been configured allow setting the file
> systems options.
>
> - If the super block has already been configured require reading the
> file systems mount options before setting/updating the file systems
> mount options.
>
> To complement that I have functionality that:
> - Allows reading a file systems current mount options.
> - Allows reading the mount options that are needed to get a handle to
> the filesystem. For most filesystems it is just the block device
> name. For nfs is is essentially all mount options. For btrfs
> it is the block device name, and the "devices=" mount option for
> secondary block device names.
Are you taking the LSM specific mount options into account?
>
> Please find below a tree where all of this is implemented and working.
> Not all file systems have been converted but the most of the unconverted
> ones are just a couple minutes work as I have converted all of the file
> system mount helper routines.
>
> Also please find below an example mount program that takes the same set
> of mount options as mount(8) today and mounts filesystems with the
> proposed new mount api.
> - Without having any filesystem mount knowledge it sucessfully figures
>
> out which system calls different mount options needs to be applied
> to.
>
> - Without having any filesystem specific knowledge in most cases it
> can detect if a mount option you specify is already specified to an
> existing mount or not. For duplicates it can detect it ignores them.
> For the other cases it fails the mount as it thinks the mount options
> are different.
>
> - Which demonstrates it safe to put the detection and remediation of
> multiple mount commands resolving to the same filesystem in user
> space.
>
> I really don't care whose code gets used as long as it works. I do very
> much care that we don't add a new mount api that has the confusion flaws
> of the existing mount api.
>
> Along the way I have also detected a lot of room for improvement on the
> mount path for filesystems. Those cleanup patches are in my tree below
> and will be extracting them and sending them along shortly.
>
> Comments?
>
> git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git no-losing-mount-options-proof-of-concept
>
>
>
> Eric
>
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jens Axboe @ 2019-01-29 22:56 UTC (permalink / raw)
To: Jann Horn; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <CAG48ez2VkeSEZA+9H4UAcg86uC77gU=UkTG8L1V58TuR9pDEqg@mail.gmail.com>
On 1/29/19 3:44 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>> If we have fixed user buffers, we can map them into the kernel when we
>> setup the io_context. That avoids the need to do get_user_pages() for
>> each and every IO.
>>
>> To utilize this feature, the application must call io_uring_register()
>> after having setup an io_uring context, passing in
>> IORING_REGISTER_BUFFERS as the opcode. The argument must be a pointer
>> to an iovec array, and the nr_args should contain how many iovecs the
>> application wishes to map.
>>
>> If successful, these buffers are now mapped into the kernel, eligible
>> for IO. To use these fixed buffers, the application must use the
>> IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED opcodes, and then
>> set sqe->index to the desired buffer index. sqe->addr..sqe->addr+seq->len
>> must point to somewhere inside the indexed buffer.
>>
>> The application may register buffers throughout the lifetime of the
>> io_uring context. It can call io_uring_register() with
>> IORING_UNREGISTER_BUFFERS as the opcode to unregister the current set of
>> buffers, and then register a new set. The application need not
>> unregister buffers explicitly before shutting down the io_uring context.
> [...]
>> +static int io_import_fixed(struct io_ring_ctx *ctx, int rw,
>> + const struct io_uring_sqe *sqe,
>> + struct iov_iter *iter)
>> +{
>> + size_t len = READ_ONCE(sqe->len);
>> + struct io_mapped_ubuf *imu;
>> + int buf_index, index;
>> + size_t offset;
>> + u64 buf_addr;
>> +
>> + /* attempt to use fixed buffers without having provided iovecs */
>> + if (unlikely(!ctx->user_bufs))
>> + return -EFAULT;
>> +
>> + buf_index = READ_ONCE(sqe->buf_index);
>> + if (unlikely(buf_index >= ctx->nr_user_bufs))
>> + return -EFAULT;
>
> Nit: If you make the local copy of buf_index unsigned, it is slightly
> easier to see that this code is correct. (I know, it has to be
> positive anyway because the value in shared memory is a u16.)
I'll definitely fit, but I can make it unsigned.
>> + index = array_index_nospec(buf_index, ctx->sq_entries);
>
> This looks weird. Did you mean s/ctx->sq_entries/ctx->nr_user_bufs/?
I did, too much copy/paste for that one. Fixed.
>> + imu = &ctx->user_bufs[index];
>> + buf_addr = READ_ONCE(sqe->addr);
>> + if (buf_addr < imu->ubuf || buf_addr + len > imu->ubuf + imu->len)
>
> This can wrap around if `buf_addr` or `len` is very big, right? Then
> you e.g. get past the first check because `buf_addr` is sufficiently
> big, and get past the second check because `buf_addr + len` wraps
> around and becomes small.
Good point. I wonder if we have a verification helper for something like
this?
>> + return -EFAULT;
>> +
>> + /*
>> + * May not be a start of buffer, set size appropriately
>> + * and advance us to the beginning.
>> + */
>> + offset = buf_addr - imu->ubuf;
>> + iov_iter_bvec(iter, rw, imu->bvec, imu->nr_bvecs, offset + len);
>> + if (offset)
>> + iov_iter_advance(iter, offset);
>> + return 0;
>> +}
>> +
>> static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
>> const struct io_uring_sqe *sqe, struct iovec **iovec,
>> struct iov_iter *iter)
>> {
>> void __user *buf = u64_to_user_ptr(READ_ONCE(sqe->addr));
>> size_t sqe_len = READ_ONCE(sqe->len);
>> + int opcode;
>> +
>> + opcode = READ_ONCE(sqe->opcode);
>> + if (opcode == IORING_OP_READ_FIXED ||
>> + opcode == IORING_OP_WRITE_FIXED) {
>> + ssize_t ret = io_import_fixed(ctx, rw, sqe, iter);
>> + *iovec = NULL;
>> + return ret;
>> + }
> [...]
>>
>> +static inline bool io_sqe_needs_user(const struct io_uring_sqe *sqe)
>> +{
>> + return !(sqe->opcode == IORING_OP_READ_FIXED ||
>> + sqe->opcode == IORING_OP_WRITE_FIXED);
>> +}
>
> This still looks racy to me?
I didn't change it because the below one you quoted below
(io_sq_wq_submit_work()) is using a local copy, but we do need it for
for the SQPOLL io_sq_thread() case. I'll get that one fixed up.
I suspect the easiest fix is to ensure that io_sq_thread() copies the
sqe.
>> +static int io_copy_iov(struct io_ring_ctx *ctx, struct iovec *dst,
>> + void __user *arg, unsigned index)
>> +{
>
> This function doesn't actually use the "ctx" parameter, right? You
> might want to remove it.
It should just use ctx->compat for this check, we're now only calling
in_compat_syscall() when we setup the ctx. This keeps it all in one
spot.
>> + struct iovec __user *src;
>> +
>> +#ifdef CONFIG_COMPAT
>> + if (in_compat_syscall()) {
>> + struct compat_iovec __user *ciovs;
>> + struct compat_iovec ciov;
>> +
>> + ciovs = (struct compat_iovec __user *) arg;
>> + if (copy_from_user(&ciov, &ciovs[index], sizeof(ciov)))
>> + return -EFAULT;
>> +
>> + dst->iov_base = (void __user *) (unsigned long) ciov.iov_base;
>> + dst->iov_len = ciov.iov_len;
>> + return 0;
>> + }
>> +#endif
>> + src = (struct iovec __user *) arg;
>> + if (copy_from_user(dst, &src[index], sizeof(*dst)))
>> + return -EFAULT;
>> + return 0;
>> +}
>> +
>> +static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg,
>> + unsigned nr_args)
>> +{
>> + struct vm_area_struct **vmas = NULL;
>> + struct page **pages = NULL;
>> + int i, j, got_pages = 0;
>> + int ret = -EINVAL;
>> +
>> + if (ctx->user_bufs)
>> + return -EBUSY;
>> + if (!nr_args || nr_args > UIO_MAXIOV)
>> + return -EINVAL;
>> +
>> + ctx->user_bufs = kcalloc(nr_args, sizeof(struct io_mapped_ubuf),
>> + GFP_KERNEL);
>> + if (!ctx->user_bufs)
>> + return -ENOMEM;
>> +
>> + if (!capable(CAP_IPC_LOCK))
>> + ctx->user = get_uid(current_user());
>> +
>> + for (i = 0; i < nr_args; i++) {
>> + struct io_mapped_ubuf *imu = &ctx->user_bufs[i];
>> + unsigned long off, start, end, ubuf;
>> + int pret, nr_pages;
>> + struct iovec iov;
>> + size_t size;
>> +
>> + ret = io_copy_iov(ctx, &iov, arg, i);
>> + if (ret)
>> + break;
>> +
>> + /*
>> + * Don't impose further limits on the size and buffer
>> + * constraints here, we'll -EINVAL later when IO is
>> + * submitted if they are wrong.
>> + */
>> + ret = -EFAULT;
>> + if (!iov.iov_base)
>> + goto err;
>> +
>> + /* arbitrary limit, but we need something */
>> + if (iov.iov_len > SZ_1G)
>> + goto err;
>
> You might also want to check for iov_len==0? Otherwise, if iov_base
> isn't page-aligned, the following code might grab a reference to one
> page even though the iov covers zero pages, that'd be kinda weird.
Good catch, will do.
>
>> + ubuf = (unsigned long) iov.iov_base;
>> + end = (ubuf + iov.iov_len + PAGE_SIZE - 1) >> PAGE_SHIFT;
>> + start = ubuf >> PAGE_SHIFT;
>> + nr_pages = end - start;
>> +
>> + ret = io_account_mem(ctx, nr_pages);
>> + if (ret)
>> + goto err;
>> +
>> + if (!pages || nr_pages > got_pages) {
>> + kfree(vmas);
>> + kfree(pages);
>> + pages = kmalloc_array(nr_pages, sizeof(struct page *),
>> + GFP_KERNEL);
>> + vmas = kmalloc_array(nr_pages,
>> + sizeof(struct vma_area_struct *),
>> + GFP_KERNEL);
>> + if (!pages || !vmas) {
>> + io_unaccount_mem(ctx, nr_pages);
>> + goto err;
>> + }
>> + got_pages = nr_pages;
>> + }
>> +
>> + imu->bvec = kmalloc_array(nr_pages, sizeof(struct bio_vec),
>> + GFP_KERNEL);
>> + if (!imu->bvec) {
>> + io_unaccount_mem(ctx, nr_pages);
>> + goto err;
>> + }
>> +
>> + down_write(¤t->mm->mmap_sem);
>
> Is there a reason why you're using down_write() and not down_read()?
> As far as I can tell, down_read() is all you need...
Looks like you are right, I'll change that.
>> + pret = get_user_pages_longterm(ubuf, nr_pages, FOLL_WRITE,
>> + pages, vmas);
>> + if (pret == nr_pages) {
>> + /* don't support file backed memory */
>> + for (j = 0; j < nr_pages; j++) {
>> + struct vm_area_struct *vma = vmas[j];
>> +
>> + if (vma->vm_file) {
>> + ret = -EOPNOTSUPP;
>> + break;
>> + }
>> + }
>
> Are you intentionally doing the check for vma->vm_file instead of
> calling GUP with FOLL_ANON, which would automatically verify
> vma->vm_ops==NULL for you using vma_is_anonymous()? FOLL_ANON is what
> procfs uses to avoid blocking on page faults when reading remote
> process memory via /proc/*/{cmdline,environ}. I don't entirely
> understand the motivation for this check, so I can't really tell
> whether FOLL_ANON would do the job.
I wasn't aware of FOLL_ANON, it looks exactly like what I need. All I
care about is the mapping being anon, and not file backed. If FOLL_ANON
ensures me that (or fails), then that'll kill this vma checking code.
Thanks!
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jann Horn @ 2019-01-29 22:44 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <20190129192702.3605-13-axboe@kernel.dk>
On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
> If we have fixed user buffers, we can map them into the kernel when we
> setup the io_context. That avoids the need to do get_user_pages() for
> each and every IO.
>
> To utilize this feature, the application must call io_uring_register()
> after having setup an io_uring context, passing in
> IORING_REGISTER_BUFFERS as the opcode. The argument must be a pointer
> to an iovec array, and the nr_args should contain how many iovecs the
> application wishes to map.
>
> If successful, these buffers are now mapped into the kernel, eligible
> for IO. To use these fixed buffers, the application must use the
> IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED opcodes, and then
> set sqe->index to the desired buffer index. sqe->addr..sqe->addr+seq->len
> must point to somewhere inside the indexed buffer.
>
> The application may register buffers throughout the lifetime of the
> io_uring context. It can call io_uring_register() with
> IORING_UNREGISTER_BUFFERS as the opcode to unregister the current set of
> buffers, and then register a new set. The application need not
> unregister buffers explicitly before shutting down the io_uring context.
[...]
> +static int io_import_fixed(struct io_ring_ctx *ctx, int rw,
> + const struct io_uring_sqe *sqe,
> + struct iov_iter *iter)
> +{
> + size_t len = READ_ONCE(sqe->len);
> + struct io_mapped_ubuf *imu;
> + int buf_index, index;
> + size_t offset;
> + u64 buf_addr;
> +
> + /* attempt to use fixed buffers without having provided iovecs */
> + if (unlikely(!ctx->user_bufs))
> + return -EFAULT;
> +
> + buf_index = READ_ONCE(sqe->buf_index);
> + if (unlikely(buf_index >= ctx->nr_user_bufs))
> + return -EFAULT;
Nit: If you make the local copy of buf_index unsigned, it is slightly
easier to see that this code is correct. (I know, it has to be
positive anyway because the value in shared memory is a u16.)
> + index = array_index_nospec(buf_index, ctx->sq_entries);
This looks weird. Did you mean s/ctx->sq_entries/ctx->nr_user_bufs/?
> + imu = &ctx->user_bufs[index];
> + buf_addr = READ_ONCE(sqe->addr);
> + if (buf_addr < imu->ubuf || buf_addr + len > imu->ubuf + imu->len)
This can wrap around if `buf_addr` or `len` is very big, right? Then
you e.g. get past the first check because `buf_addr` is sufficiently
big, and get past the second check because `buf_addr + len` wraps
around and becomes small.
> + return -EFAULT;
> +
> + /*
> + * May not be a start of buffer, set size appropriately
> + * and advance us to the beginning.
> + */
> + offset = buf_addr - imu->ubuf;
> + iov_iter_bvec(iter, rw, imu->bvec, imu->nr_bvecs, offset + len);
> + if (offset)
> + iov_iter_advance(iter, offset);
> + return 0;
> +}
> +
> static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
> const struct io_uring_sqe *sqe, struct iovec **iovec,
> struct iov_iter *iter)
> {
> void __user *buf = u64_to_user_ptr(READ_ONCE(sqe->addr));
> size_t sqe_len = READ_ONCE(sqe->len);
> + int opcode;
> +
> + opcode = READ_ONCE(sqe->opcode);
> + if (opcode == IORING_OP_READ_FIXED ||
> + opcode == IORING_OP_WRITE_FIXED) {
> + ssize_t ret = io_import_fixed(ctx, rw, sqe, iter);
> + *iovec = NULL;
> + return ret;
> + }
[...]
>
> +static inline bool io_sqe_needs_user(const struct io_uring_sqe *sqe)
> +{
> + return !(sqe->opcode == IORING_OP_READ_FIXED ||
> + sqe->opcode == IORING_OP_WRITE_FIXED);
> +}
This still looks racy to me?
> static void io_sq_wq_submit_work(struct work_struct *work)
> {
[...]
> - if (!mmget_not_zero(ctx->sqo_mm)) {
> - ret = -EFAULT;
> - goto err;
> + /*
> + * If we're doing IO to fixed buffers, we don't need to get/set
> + * user context
> + */
> + needs_user = io_sqe_needs_user(&sqe);
> + if (needs_user) {
> + if (!mmget_not_zero(ctx->sqo_mm)) {
> + ret = -EFAULT;
> + goto err;
> + }
> + use_mm(ctx->sqo_mm);
> + old_fs = get_fs();
> + set_fs(USER_DS);
> }
>
> - use_mm(ctx->sqo_mm);
> - set_fs(USER_DS);
> -
> ret = __io_submit_sqe(ctx, req, s, false, NULL);
>
> - set_fs(old_fs);
> - unuse_mm(ctx->sqo_mm);
> - mmput(ctx->sqo_mm);
> + if (needs_user) {
> + set_fs(old_fs);
> + unuse_mm(ctx->sqo_mm);
> + mmput(ctx->sqo_mm);
> + }
> err:
> if (ret) {
> - io_cqring_add_event(ctx, user_data, ret, 0);
> + io_cqring_add_event(ctx, sqe.user_data, ret, 0);
> io_free_req(req);
> }
[...]
> +static int io_copy_iov(struct io_ring_ctx *ctx, struct iovec *dst,
> + void __user *arg, unsigned index)
> +{
This function doesn't actually use the "ctx" parameter, right? You
might want to remove it.
> + struct iovec __user *src;
> +
> +#ifdef CONFIG_COMPAT
> + if (in_compat_syscall()) {
> + struct compat_iovec __user *ciovs;
> + struct compat_iovec ciov;
> +
> + ciovs = (struct compat_iovec __user *) arg;
> + if (copy_from_user(&ciov, &ciovs[index], sizeof(ciov)))
> + return -EFAULT;
> +
> + dst->iov_base = (void __user *) (unsigned long) ciov.iov_base;
> + dst->iov_len = ciov.iov_len;
> + return 0;
> + }
> +#endif
> + src = (struct iovec __user *) arg;
> + if (copy_from_user(dst, &src[index], sizeof(*dst)))
> + return -EFAULT;
> + return 0;
> +}
> +
> +static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg,
> + unsigned nr_args)
> +{
> + struct vm_area_struct **vmas = NULL;
> + struct page **pages = NULL;
> + int i, j, got_pages = 0;
> + int ret = -EINVAL;
> +
> + if (ctx->user_bufs)
> + return -EBUSY;
> + if (!nr_args || nr_args > UIO_MAXIOV)
> + return -EINVAL;
> +
> + ctx->user_bufs = kcalloc(nr_args, sizeof(struct io_mapped_ubuf),
> + GFP_KERNEL);
> + if (!ctx->user_bufs)
> + return -ENOMEM;
> +
> + if (!capable(CAP_IPC_LOCK))
> + ctx->user = get_uid(current_user());
> +
> + for (i = 0; i < nr_args; i++) {
> + struct io_mapped_ubuf *imu = &ctx->user_bufs[i];
> + unsigned long off, start, end, ubuf;
> + int pret, nr_pages;
> + struct iovec iov;
> + size_t size;
> +
> + ret = io_copy_iov(ctx, &iov, arg, i);
> + if (ret)
> + break;
> +
> + /*
> + * Don't impose further limits on the size and buffer
> + * constraints here, we'll -EINVAL later when IO is
> + * submitted if they are wrong.
> + */
> + ret = -EFAULT;
> + if (!iov.iov_base)
> + goto err;
> +
> + /* arbitrary limit, but we need something */
> + if (iov.iov_len > SZ_1G)
> + goto err;
You might also want to check for iov_len==0? Otherwise, if iov_base
isn't page-aligned, the following code might grab a reference to one
page even though the iov covers zero pages, that'd be kinda weird.
> + ubuf = (unsigned long) iov.iov_base;
> + end = (ubuf + iov.iov_len + PAGE_SIZE - 1) >> PAGE_SHIFT;
> + start = ubuf >> PAGE_SHIFT;
> + nr_pages = end - start;
> +
> + ret = io_account_mem(ctx, nr_pages);
> + if (ret)
> + goto err;
> +
> + if (!pages || nr_pages > got_pages) {
> + kfree(vmas);
> + kfree(pages);
> + pages = kmalloc_array(nr_pages, sizeof(struct page *),
> + GFP_KERNEL);
> + vmas = kmalloc_array(nr_pages,
> + sizeof(struct vma_area_struct *),
> + GFP_KERNEL);
> + if (!pages || !vmas) {
> + io_unaccount_mem(ctx, nr_pages);
> + goto err;
> + }
> + got_pages = nr_pages;
> + }
> +
> + imu->bvec = kmalloc_array(nr_pages, sizeof(struct bio_vec),
> + GFP_KERNEL);
> + if (!imu->bvec) {
> + io_unaccount_mem(ctx, nr_pages);
> + goto err;
> + }
> +
> + down_write(¤t->mm->mmap_sem);
Is there a reason why you're using down_write() and not down_read()?
As far as I can tell, down_read() is all you need...
> + pret = get_user_pages_longterm(ubuf, nr_pages, FOLL_WRITE,
> + pages, vmas);
> + if (pret == nr_pages) {
> + /* don't support file backed memory */
> + for (j = 0; j < nr_pages; j++) {
> + struct vm_area_struct *vma = vmas[j];
> +
> + if (vma->vm_file) {
> + ret = -EOPNOTSUPP;
> + break;
> + }
> + }
Are you intentionally doing the check for vma->vm_file instead of
calling GUP with FOLL_ANON, which would automatically verify
vma->vm_ops==NULL for you using vma_is_anonymous()? FOLL_ANON is what
procfs uses to avoid blocking on page faults when reading remote
process memory via /proc/*/{cmdline,environ}. I don't entirely
understand the motivation for this check, so I can't really tell
whether FOLL_ANON would do the job.
> + } else {
> + ret = pret < 0 ? pret : -EFAULT;
> + }
> + up_write(¤t->mm->mmap_sem);
> + if (ret) {
> + /*
> + * if we did partial map, or found file backed vmas,
> + * release any pages we did get
> + */
> + if (pret > 0) {
> + for (j = 0; j < pret; j++)
> + put_page(pages[j]);
> + }
> + io_unaccount_mem(ctx, nr_pages);
> + goto err;
> + }
> +
> + off = ubuf & ~PAGE_MASK;
> + size = iov.iov_len;
> + for (j = 0; j < nr_pages; j++) {
> + size_t vec_len;
> +
> + vec_len = min_t(size_t, size, PAGE_SIZE - off);
> + imu->bvec[j].bv_page = pages[j];
> + imu->bvec[j].bv_len = vec_len;
> + imu->bvec[j].bv_offset = off;
> + off = 0;
> + size -= vec_len;
> + }
> + /* store original address for later verification */
> + imu->ubuf = ubuf;
> + imu->len = iov.iov_len;
> + imu->nr_bvecs = nr_pages;
> + }
> + kfree(pages);
> + kfree(vmas);
> + ctx->nr_user_bufs = nr_args;
> + return 0;
> +err:
> + kfree(pages);
> + kfree(vmas);
> + io_sqe_buffer_unregister(ctx);
> + return ret;
> +}
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ 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