Adds new flag MAP_NOSIGBUS of mmap() to specify the behavior of
"don't SIGBUS on read beyond i_size". This flag is only allowed
for read only shmem mapping.
If you use MAP_NOSIGBUS, and you access pages that don't have a backing
store, you will get zero pages, and they will NOT BE SYNCHRONIZED with
the backing store possibly later being updated.
Any user that uses MAP_NOSIGBUS had better just accept that it's not
compatible with expanding the shmem backing store later.
Signed-off-by: Ming Lin <mlin@kernel.org>
I disagree with Linus on this: I think it's a mistake,
and is being targeted at tmpfs to avoid wider scrutiny.
Though I have a more constructive suggestion under your mmap.c mod.
I've added linux-fsdevel and linux-api to the Cc list:
linux-api definitely needed to approve any MAP_NOSIGBUS semantics;
linux-fsdevel shouldn't be affected, but they need to know about it.
The prior discussion on "Sealed memfd & no-fault mmap" is at
https://lore.kernel.org/linux-mm/vs1Us2sm4qmfvLOqNat0-r16GyfmWzqUzQ4KHbXJwEcjhzeoQ4sBTxx7QXDG9B6zk5AeT7FsNb3CSr94LaKy6Novh1fbbw8D_BBxYsbPLms=@emersion.fr/
I've not yet seen a response from Simon Ser, as to whether this
kind of "opaque blob of zeroes" implementation would be of any
use to Wayland: you expected it to be a problem, and we shouldn't
waste any time on it if it's not going to be useful to someone.
Maybe there will be other takers (certainly SIGBUS is unpopular).
@@ -1419,6 +1419,9 @@ unsigned long do_mmap(struct file *file, unsigned long addr,if(!len)return-EINVAL;+if((flags&MAP_NOSIGBUS)&&((prot&PROT_WRITE)||!shmem_file(file)))+return-EINVAL;+
No, for several reasons.
This has nothing to do with shmem really, that's just where this patch
hacks it in - and where you have a first user in mind. If this goes
forward, please modify mm/memory.c not mm/shmem.c, to make
VM_FAULT_SIGBUS on fault to VM_NOSIGBUS vma do the mapping of zero page.
(prot & PROT_WRITE) tells you about the mmap() flags, but says nothing
about what mprotect() could do later on. Look out for VM_SHARED and
VM_MAYSHARE and VM_MAYWRITE further down; and beware the else (!file)
block below them, shared anonymous would need more protection too.
Constructive comment: I guess much of my objection to this feature
comes from allowing it in the MAP_SHARED case. If you restrict it
to MAP_PRIVATE mapping of file, then it's less objectionable, and
you won't have to worry (so much?) about write protection. Copy
on write is normal there, and it's well established that subsequent
changes in the file will not be shared; you'd just be extending that
behaviour from writes to sigbusy reads.
And by restricting to MAP_PRIVATE, you would allow for adding a
proper MAP_SHARED implementation later, if it's thought useful
(that being the implementation which can subsequently unmap a
zero page to let new page cache be mapped).
quoted hunk
/*
* Does the application expect PROT_READ to imply PROT_EXEC?
*
No. Presumably you hit the BUG_ON(mmap_read_trylock(vma->vm_mm))
in vm_insert_page(), so decided to modify the vm_flags here: no,
that BUG is saying you need mmap_write_lock() to write vm_flags.
And I have no idea of the ramifications of shmem in a VM_MIXEDMAP
vma; perhaps it works out fine, but I'd have to research that.
I'd rather not.
+ /*
+ * Get zero page for MAP_NOSIGBUS mapping, which isn't
+ * coherent wrt shmem contents that are expanded and
+ * filled in later.
+ */
+ error = vm_insert_page(vma, (unsigned long)vmf->address,
+ ZERO_PAGE(0));
+ if (error)
+ return error;
+
+ *fault_type = VM_FAULT_NOPAGE;
+ return 0;
But there are other ways in which shmem_getpage_gfp() can fail and
shmem_fault() end up returning VM_FAULT_SIGBUS. Notably -ENOSPC.
It's trivial for someone to pass the MAP_NOSIGBUS user the fd of a
sparse file in a full filesystem, causing SIGBUS on access despite
MAP_NOSIGBUS. On shmem or some other filesystem.
I say the VM_FAULT_SIGBUS->map-in-zero-page handling should be back
in mm/memory.c, where it calls ->fault(): where others can review it.
One other thing while it crosses my mind. You'll need to decide
what truncating or hole-punching the file does to the zero pages
in its userspace mappings. I may turn out wrong, but I think you'll
find that truncation removes them, but hole-punch leaves them, and
ought to be modified to remove them too (it's a matter of how the
"even_cows" arg to unmap_mapping_range() is treated).
Hugh
From: Ming Lin <mlin@kernel.org> Date: 2021-06-03 00:05:47
On 6/1/2021 8:49 PM, Hugh Dickins wrote:
quoted
index 096bba4..69cd856 100644
--- a/mm/mmap.c+++ b/mm/mmap.c
@@ -1419,6 +1419,9 @@ unsigned long do_mmap(struct file *file, unsigned long addr,if(!len)return-EINVAL;+if((flags&MAP_NOSIGBUS)&&((prot&PROT_WRITE)||!shmem_file(file)))+return-EINVAL;+
No, for several reasons.
This has nothing to do with shmem really, that's just where this patch
hacks it in - and where you have a first user in mind. If this goes
forward, please modify mm/memory.c not mm/shmem.c, to make
VM_FAULT_SIGBUS on fault to VM_NOSIGBUS vma do the mapping of zero page.
(prot & PROT_WRITE) tells you about the mmap() flags, but says nothing
about what mprotect() could do later on. Look out for VM_SHARED and
VM_MAYSHARE and VM_MAYWRITE further down; and beware the else (!file)
block below them, shared anonymous would need more protection too.
Constructive comment: I guess much of my objection to this feature
comes from allowing it in the MAP_SHARED case. If you restrict it
to MAP_PRIVATE mapping of file, then it's less objectionable, and
you won't have to worry (so much?) about write protection. Copy
on write is normal there, and it's well established that subsequent
changes in the file will not be shared; you'd just be extending that
behaviour from writes to sigbusy reads.
And by restricting to MAP_PRIVATE, you would allow for adding a
proper MAP_SHARED implementation later, if it's thought useful
(that being the implementation which can subsequently unmap a
zero page to let new page cache be mapped).
This is what I wrote so far.
---
include/linux/mm.h | 2 ++
include/linux/mman.h | 1 +
include/uapi/asm-generic/mman-common.h | 1 +
mm/memory.c | 12 ++++++++++++
mm/mmap.c | 4 ++++
5 files changed, 20 insertions(+)
@@ -373,6 +373,8 @@ int __add_to_page_cache_locked(struct page *page, struct address_space *mapping,# define VM_UFFD_MINOR VM_NONE#endif /* CONFIG_HAVE_ARCH_USERFAULTFD_MINOR */+#define VM_NOSIGBUS VM_FLAGS_BIT(38) /* Do not SIGBUS on fault */+/* Bits set in the VMA until the stack is in its final location */#define VM_STACK_INCOMPLETE_SETUP (VM_RAND_READ | VM_SEQ_READ)
No. Presumably you hit the BUG_ON(mmap_read_trylock(vma->vm_mm))
in vm_insert_page(), so decided to modify the vm_flags here: no,
that BUG is saying you need mmap_write_lock() to write vm_flags.
But the comments above vm_insert_page() told me to set VM_MIXEDMAP on vma
* Usually this function is called from f_op->mmap() handler
* under mm->mmap_lock write-lock, so it can change vma->vm_flags.
* Caller must set VM_MIXEDMAP on vma if it wants to call this
* function from other places, for example from page-fault handler.
One other thing while it crosses my mind. You'll need to decide
what truncating or hole-punching the file does to the zero pages
in its userspace mappings. I may turn out wrong, but I think you'll
find that truncation removes them, but hole-punch leaves them, and
ought to be modified to remove them too (it's a matter of how the
"even_cows" arg to unmap_mapping_range() is treated).
I did a quick test, after inserting zero pages, seems that truncation
also leaves the mappings.
I'm still reading code to learn this part ...
{
+ /*
+ * Get zero page for MAP_NOSIGBUS mapping, which isn't
+ * coherent wrt shmem contents that are expanded and
+ * filled in later.
+ */
+ vma->vm_flags |= VM_MIXEDMAP;
+ if (!vm_insert_page(vma, (unsigned long)vmf->address,
+ ZERO_PAGE(vmf->address)))
+ return VM_FAULT_NOPAGE;
+ }
+
if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY
|
VM_FAULT_DONE_COW)))
return ret;
Sorry, I directed you to mm/memory.c without indicating what's
appropriate here. Please don't attempt to use VM_MIXEDMAP and
vm_insert_page(): they're for special driver mmaps, they're no
better here than they were in mm/shmem.c.
It's do_anonymous_page()'s business to map in the zero page on
read fault (see "my_zero_pfn(vmf->address)" in there), or fill
a freshly allocated page with zeroes on write fault - and now
you're sticking to MAP_PRIVATE, write faults in VM_WRITE areas
are okay for VM_NOSIGBUS.
Ideally you can simply call do_anonymous_page() from __do_fault()
in the VM_FAULT_SIGBUS on VM_NOSIGBUS case. That's what to start
from anyway: but look to see if there's state to be adjusted to
achieve that; and it won't be surprising if somewhere down in
do_anonymous_page() or something it calls, there's a BUG on it
being called when vma->vm_file is set, or something like that.
May need some tweaking.
Hugh
On Wed, Jun 2, 2021 at 5:46 PM Hugh Dickins [off-list ref] wrote:
Ideally you can simply call do_anonymous_page() from __do_fault()
in the VM_FAULT_SIGBUS on VM_NOSIGBUS case.
Heh.
We're actually then back to my original patch.
That one doesn't handle shared mappings (even read-only ones), for the
simple reason that do_anonymous_page() refuses to insert anonymous
pages into a shared mapping, and has
/* File mapping without ->vm_ops ? */
if (vma->vm_flags & VM_SHARED)
return VM_FAULT_SIGBUS;
at the very top.
But yes, if we just remove that check, I think my original patch
should actually "JustWork(tm)".
I'm attaching it again, with old name and old commentary (ie that
/* FIXME! We don't have a VM_NOFAULT bit */
should just be replaced with that VM_NOSIGBUS bit instead, and the
#if'ed out region should be enabled.
Oh, and we need to think hard about one more case: mprotect().
In particular, I think the attached patch fails horribly for the case
of a shared mapping that starts out read-only, then inserts a zero
page, then somebody does mprotect(MAP_WRITE), and then writes to the
page. I haven't checked what the write protect fault handler does, but
I think that for a shared mapping it will just make the page dirty and
writable.
Which would be horribly wrong for VM_NOSIGBUS.
So that support infrastructure that adds MAP_NOSIGBUS, and checks that
it is only done on a read-only mapping, also has to make sure that it
clears the VM_MAYWRITE bit when it sets VM_NOSIGBUS.
That way mprotect can't then later make it writable.
Hugh, comments on this approach?
Again: this patch is my *OLD* one, I didn't try to update it to the
new world order. It requires
- Ming's MAP_NOSIGBUS ccode
- removal of that "File mapping without ->vm_ops" case
- that FIXME fixed and name updated
- and that VM_MAYWRITE clearing if VM_NOSIGBUS is set, to avoid the
mprotect issue.
Hmm?
Linus
On Wed, Jun 2, 2021 at 5:46 PM Hugh Dickins [off-list ref] wrote:
quoted
Ideally you can simply call do_anonymous_page() from __do_fault()
in the VM_FAULT_SIGBUS on VM_NOSIGBUS case.
Heh.
We're actually then back to my original patch.
That one doesn't handle shared mappings (even read-only ones), for the
simple reason that do_anonymous_page() refuses to insert anonymous
pages into a shared mapping, and has
/* File mapping without ->vm_ops ? */
if (vma->vm_flags & VM_SHARED)
return VM_FAULT_SIGBUS;
at the very top.
But yes, if we just remove that check, I think my original patch
should actually "JustWork(tm)".
But no!
Sorry, I don't have time for this at present, so haven't looked at
your original patch.
But the point that we've arrived at, that I'm actually now fairly
happy with, is do *not* permit MAP_NOSIGBUS on MAP_SHARED mappings.
I didn't check the placement yet, easy to get wrong, but I believe
Ming Lin is now enforcing that over at the mmap() end.
On a MAP_PRIVATE mapping, the nasty opaque blob of zeroes can
claim some precedent in what already happens with COW'ed pages.
Which leaves MAP_NOSIGBUS on MAP_SHARED as currently unsupported,
perhaps never supported on anything, perhaps one day supported on
shmem; but if it's ever supported then that one will naturally be
transparent to future changes in page cache - we call that "shared".
Of course, internally, there's the in-between case of MAP_SHARED
without PROT_WRITE and without writable fd: VM_MAYSHARE without
VM_SHARED or VM_MAYWRITE. We *could* let that one accept
MAP_NOSIGBUS, but who wants to write the manpage for it?
Please stick to MAP_PRIVATE: that's good enough.
I'm attaching it again, with old name and old commentary (ie that
/* FIXME! We don't have a VM_NOFAULT bit */
should just be replaced with that VM_NOSIGBUS bit instead, and the
#if'ed out region should be enabled.
Oh, and we need to think hard about one more case: mprotect().
In particular, I think the attached patch fails horribly for the case
of a shared mapping that starts out read-only, then inserts a zero
page, then somebody does mprotect(MAP_WRITE), and then writes to the
page. I haven't checked what the write protect fault handler does, but
I think that for a shared mapping it will just make the page dirty and
writable.
Obviously the finished patch will need to be scrutinized carefully, but
I think the mprotect() questions vanish when restricted to MAP_PRIVATE.
Which would be horribly wrong for VM_NOSIGBUS.
So that support infrastructure that adds MAP_NOSIGBUS, and checks that
it is only done on a read-only mapping, also has to make sure that it
clears the VM_MAYWRITE bit when it sets VM_NOSIGBUS.
That way mprotect can't then later make it writable.
Hugh, comments on this approach?
Comments above, just stick to MAP_PRIVATE.
Hugh
Again: this patch is my *OLD* one, I didn't try to update it to the
new world order. It requires
- Ming's MAP_NOSIGBUS ccode
- removal of that "File mapping without ->vm_ops" case
- that FIXME fixed and name updated
- and that VM_MAYWRITE clearing if VM_NOSIGBUS is set, to avoid the
mprotect issue.
Hmm?
Linus
On Thu, Jun 3, 2021 at 12:07 PM Hugh Dickins [off-list ref] wrote:
But the point that we've arrived at, that I'm actually now fairly
happy with, is do *not* permit MAP_NOSIGBUS on MAP_SHARED mappings.
Yeah, if that's sufficient, then that original patch should just work as-is.
But there was some reason why people didn't like that patch
originally, and I think it was literally about how it only worked on
private mappings (the "we don't have a flag for it in the vm_flags"
part was just a small detail.
I guess that objection ended up changing over time.
Linus
On Thu, Jun 3, 2021 at 12:12 PM Linus Torvalds
[off-list ref] wrote:
Yeah, if that's sufficient, then that original patch should just work as-is.
To clarify: it obviously needs the VM_xyz flags things, but the
VM_SHARED check in do_anonymous_page() is fine, and the whole issue
with VM_MAYWRITE is entirely moot.
MAP_PRIVATE works fine with zero pages even when writable - they get
COW'ed properly, of course.
Linus
From: Andy Lutomirski <luto@amacapital.net> Date: 2021-06-03 19:25:16
On Jun 3, 2021, at 12:14 PM, Linus Torvalds [off-list ref] wrote:
On Thu, Jun 3, 2021 at 12:07 PM Hugh Dickins [off-list ref] wrote:
quoted
But the point that we've arrived at, that I'm actually now fairly
happy with, is do *not* permit MAP_NOSIGBUS on MAP_SHARED mappings.
Yeah, if that's sufficient, then that original patch should just work as-is.
But there was some reason why people didn't like that patch
originally, and I think it was literally about how it only worked on
private mappings (the "we don't have a flag for it in the vm_flags"
part was just a small detail.
I guess that objection ended up changing over time.
I don’t understand the use case well enough to comment on whether MAP_PRIVATE is sufficient, but I’m with Hugh: if this feature is implemented for MAP_SHARED, it should be fully coherent.
From: Simon Ser <hidden> Date: 2021-06-03 19:35:41
On Thursday, June 3rd, 2021 at 9:24 PM, Andy Lutomirski [off-list ref] wrote:
I don’t understand the use case well enough to comment on whether MAP_PRIVATE
is sufficient, but I’m with Hugh: if this feature is implemented for
MAP_SHARED, it should be fully coherent.
From: Ming Lin <mlin@kernel.org> Date: 2021-06-03 19:57:06
On 6/2/2021 5:46 PM, Hugh Dickins wrote
It's do_anonymous_page()'s business to map in the zero page on
read fault (see "my_zero_pfn(vmf->address)" in there), or fill
a freshly allocated page with zeroes on write fault - and now
you're sticking to MAP_PRIVATE, write faults in VM_WRITE areas
are okay for VM_NOSIGBUS.
Ideally you can simply call do_anonymous_page() from __do_fault()
in the VM_FAULT_SIGBUS on VM_NOSIGBUS case. That's what to start
from anyway: but look to see if there's state to be adjusted to
achieve that; and it won't be surprising if somewhere down in
do_anonymous_page() or something it calls, there's a BUG on it
being called when vma->vm_file is set, or something like that.
May need some tweaking.
do_anonymous_page() works nicely for read fault and write fault.
I didn't see any BUG() thing in my test.
But I'm still struggling with how to do "punch hole should remove the mapping of zero page".
Here is the hack I have now.
@@ -373,6 +373,8 @@ int __add_to_page_cache_locked(struct page *page, struct address_space *mapping,# define VM_UFFD_MINOR VM_NONE#endif /* CONFIG_HAVE_ARCH_USERFAULTFD_MINOR */+#define VM_NOSIGBUS VM_FLAGS_BIT(38) /* Do not SIGBUS on fault */+/* Bits set in the VMA until the stack is in its final location */#define VM_STACK_INCOMPLETE_SETUP (VM_RAND_READ | VM_SEQ_READ)