Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

[Hugh Dickins]

On Tue, 1 Jun 2021, Ming Lin wrote:

> 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).

> ---
>  include/linux/mm.h                     |  2 ++
>  include/linux/mman.h                   |  1 +
>  include/uapi/asm-generic/mman-common.h |  1 +
>  mm/mmap.c                              |  3 +++
>  mm/shmem.c                             | 17 ++++++++++++++++-
>  5 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e9d67bc..5d0e0dc 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -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 out-of-band shmem read */

"out-of-band shmem read" means nothing to me: "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)
>  
> diff --git a/include/linux/mman.h b/include/linux/mman.h
> index b2cbae9..c966b08 100644
> --- a/include/linux/mman.h
> +++ b/include/linux/mman.h
> @@ -154,6 +154,7 @@ static inline bool arch_validate_flags(unsigned long flags)
>  	       _calc_vm_trans(flags, MAP_DENYWRITE,  VM_DENYWRITE ) |
>  	       _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    ) |
>  	       _calc_vm_trans(flags, MAP_SYNC,	     VM_SYNC      ) |
> +	       _calc_vm_trans(flags, MAP_NOSIGBUS,   VM_NOSIGBUS  ) |
>  	       arch_calc_vm_flag_bits(flags);
>  }
>  
> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> index f94f65d..55f4be0 100644
> --- a/include/uapi/asm-generic/mman-common.h
> +++ b/include/uapi/asm-generic/mman-common.h
> @@ -29,6 +29,7 @@
>  #define MAP_HUGETLB		0x040000	/* create a huge page mapping */
>  #define MAP_SYNC		0x080000 /* perform synchronous page faults for the mapping */
>  #define MAP_FIXED_NOREPLACE	0x100000	/* MAP_FIXED which doesn't unmap underlying mapping */
> +#define MAP_NOSIGBUS		0x200000	/* do not SIGBUS on out-of-band shmem read */

Ditto.

>  
>  #define MAP_UNINITIALIZED 0x4000000	/* For anonymous mmap, memory could be
>  					 * uninitialized */
> diff --git a/mm/mmap.c b/mm/mmap.c
> 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).

>  	/*
>  	 * Does the application expect PROT_READ to imply PROT_EXEC?
>  	 *
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 5d46611..5d15b08 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1812,7 +1812,22 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,
>  repeat:
>  	if (sgp <= SGP_CACHE &&
>  	    ((loff_t)index << PAGE_SHIFT) >= i_size_read(inode)) {
> -		return -EINVAL;
> +		if (!vma || !(vma->vm_flags & VM_NOSIGBUS))
> +			return -EINVAL;
> +
> +		vma->vm_flags |= VM_MIXEDMAP;

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

>  	}
>  
>  	sbinfo = SHMEM_SB(inode->i_sb);
> -- 
> 1.8.3.1

Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

[Ming Lin]

On 6/1/2021 8:49 PM, Hugh Dickins wrote:

>> 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(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index e9d67bc..af9e277 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -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)
  
diff --git a/include/linux/mman.h b/include/linux/mman.h
index b2cbae9..c966b08 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -154,6 +154,7 @@ static inline bool arch_validate_flags(unsigned long flags)
  	       _calc_vm_trans(flags, MAP_DENYWRITE,  VM_DENYWRITE ) |
  	       _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    ) |
  	       _calc_vm_trans(flags, MAP_SYNC,	     VM_SYNC      ) |
+	       _calc_vm_trans(flags, MAP_NOSIGBUS,   VM_NOSIGBUS  ) |
  	       arch_calc_vm_flag_bits(flags);
  }
  
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index f94f65d..a2a5333 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -29,6 +29,7 @@
  #define MAP_HUGETLB		0x040000	/* create a huge page mapping */
  #define MAP_SYNC		0x080000 /* perform synchronous page faults for the mapping */
  #define MAP_FIXED_NOREPLACE	0x100000	/* MAP_FIXED which doesn't unmap underlying mapping */
+#define MAP_NOSIGBUS		0x200000	/* do not SIGBUS on fault */
  
  #define MAP_UNINITIALIZED 0x4000000	/* For anonymous mmap, memory could be
  					 * uninitialized */
diff --git a/mm/memory.c b/mm/memory.c
index eff2a47..7195dac 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3676,6 +3676,18 @@ static vm_fault_t __do_fault(struct vm_fault *vmf)
  	}
  
  	ret = vma->vm_ops->fault(vmf);
+	if (unlikely(ret & VM_FAULT_SIGBUS) && (vma->vm_flags & VM_NOSIGBUS)) {
+		/*
+		 * 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;
diff --git a/mm/mmap.c b/mm/mmap.c
index 096bba4..74fb49a 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1419,6 +1419,10 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
  	if (!len)
  		return -EINVAL;
  
+	/* Restrict MAP_NOSIGBUS to MAP_PRIVATE mapping */
+	if ((flags & MAP_NOSIGBUS) && !(flags & MAP_PRIVATE))
+		return -EINVAL;
+
  	/*
  	 * Does the application expect PROT_READ to imply PROT_EXEC?
  	 *

> 
>>   	/*
>>   	 * Does the application expect PROT_READ to imply PROT_EXEC?
>>   	 *
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 5d46611..5d15b08 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -1812,7 +1812,22 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,
>>   repeat:
>>   	if (sgp <= SGP_CACHE &&
>>   	    ((loff_t)index << PAGE_SHIFT) >= i_size_read(inode)) {
>> -		return -EINVAL;
>> +		if (!vma || !(vma->vm_flags & VM_NOSIGBUS))
>> +			return -EINVAL;
>> +
>> +		vma->vm_flags |= VM_MIXEDMAP;
> 
> 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 ...

Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

[Hugh Dickins]

On Wed, 2 Jun 2021, Ming Lin wrote:
> 
> 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(+)

I have not looked at the rest, just looking at mm/memory.c:

> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3676,6 +3676,18 @@ static vm_fault_t __do_fault(struct vm_fault *vmf)
>  	}
>   	ret = vma->vm_ops->fault(vmf);
> +	if (unlikely(ret & VM_FAULT_SIGBUS) && (vma->vm_flags & VM_NOSIGBUS))
> {
> +		/*
> +		 * 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

Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

[Linus Torvalds]

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

On Wed, Jun 2, 2021 at 5:46 PM Hugh Dickins <hughd@google.com> 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

[-- Attachment #2: VM_NOSIGBUS.patch --]
[-- Type: text/x-patch, Size: 904 bytes --]

 mm/memory.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 550405fc3b5e..bbede6b52f7a 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4312,10 +4312,21 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
 	}
 
 	if (!vmf->pte) {
-		if (vma_is_anonymous(vmf->vma))
-			return do_anonymous_page(vmf);
-		else
-			return do_fault(vmf);
+		if (!vma_is_anonymous(vmf->vma)) {
+			vm_fault_t ret = do_fault(vmf);
+			if (ret & VM_FAULT_RETRY)
+				return ret;
+			if (!(ret & VM_FAULT_SIGBUS))
+				return ret;
+/* FIXME! We don't have a VM_NOFAULT bit */
+#if 0
+			/* See if we should turn a SIGBUS into an anonymous page */
+			if (!(vma->vm_flags & VM_NOFAULT))
+				return ret;
+#endif
+/* Fall back on do_anonymous_page() instead of SIGBUS */
+		}
+		return do_anonymous_page(vmf);
 	}
 
 	if (!pte_present(vmf->orig_pte))

Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

[Hugh Dickins]

On Thu, 3 Jun 2021, Linus Torvalds wrote:
> On Wed, Jun 2, 2021 at 5:46 PM Hugh Dickins <hughd@google.com> 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)".

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

Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

[Linus Torvalds]

On Thu, Jun 3, 2021 at 12:07 PM Hugh Dickins <hughd@google.com> 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

Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

[Linus Torvalds]

On Thu, Jun 3, 2021 at 12:12 PM Linus Torvalds
<torvalds@linux-foundation.org> 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

Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

[Andy Lutomirski]

> On Jun 3, 2021, at 12:14 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Thu, Jun 3, 2021 at 12:07 PM Hugh Dickins <hughd@google.com> 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.
> 
> 

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.

Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

[Simon Ser]

On Thursday, June 3rd, 2021 at 9:24 PM, Andy Lutomirski <luto@amacapital.net> 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.

I've tried to explain what we'd need from user-space PoV in [1].
tl;dr the MAP_PRIVATE restriction would get us pretty far, even if it
won't allow us to have all of the bells and whistles.

[1]: https://lore.kernel.org/linux-mm/vs1Us2sm4qmfvLOqNat0-r16GyfmWzqUzQ4KHbXJwEcjhzeoQ4sBTxx7QXDG9B6zk5AeT7FsNb3CSr94LaKy6Novh1fbbw8D_BBxYsbPLms=@emersion.fr/T/#mb321a8d39e824740877ba95f1df780ffd52c3862

Re: [PATCH 2/2] mm: adds NOSIGBUS extension for out-of-band shmem read

[Ming Lin]

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.

diff --git a/mm/memory.c b/mm/memory.c
index 46ecda5..6b5a897 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1241,7 +1241,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
                         struct page *page;
  
                         page = vm_normal_page(vma, addr, ptent);
-                       if (unlikely(details) && page) {
+                       if (unlikely(details) && page && !(vma->vm_flags & VM_NOSIGBUS)) {
                                 /*
                                  * unmap_shared_mapping_pages() wants to
                                  * invalidate cache without truncating:


And other parts of the patch is following,

----

diff --git a/include/linux/mm.h b/include/linux/mm.h
index e9d67bc..af9e277 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -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)
  
diff --git a/include/linux/mman.h b/include/linux/mman.h
index b2cbae9..c966b08 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -154,6 +154,7 @@ static inline bool arch_validate_flags(unsigned long flags)
  	       _calc_vm_trans(flags, MAP_DENYWRITE,  VM_DENYWRITE ) |
  	       _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    ) |
  	       _calc_vm_trans(flags, MAP_SYNC,	     VM_SYNC      ) |
+	       _calc_vm_trans(flags, MAP_NOSIGBUS,   VM_NOSIGBUS  ) |
  	       arch_calc_vm_flag_bits(flags);
  }
  
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index f94f65d..a2a5333 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -29,6 +29,7 @@
  #define MAP_HUGETLB		0x040000	/* create a huge page mapping */
  #define MAP_SYNC		0x080000 /* perform synchronous page faults for the mapping */
  #define MAP_FIXED_NOREPLACE	0x100000	/* MAP_FIXED which doesn't unmap underlying mapping */
+#define MAP_NOSIGBUS		0x200000	/* do not SIGBUS on fault */
  
  #define MAP_UNINITIALIZED 0x4000000	/* For anonymous mmap, memory could be
  					 * uninitialized */
diff --git a/mm/memory.c b/mm/memory.c
index eff2a47..46ecda5 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3676,6 +3676,17 @@ static vm_fault_t __do_fault(struct vm_fault *vmf)
  	}
  
  	ret = vma->vm_ops->fault(vmf);
+	if (unlikely(ret & VM_FAULT_SIGBUS) && (vma->vm_flags & VM_NOSIGBUS)) {
+		/*
+		 * For MAP_NOSIGBUS mapping, map in the zero page on read fault
+		 * or fill a freshly allocated page with zeroes on write fault
+		 */
+		ret = do_anonymous_page(vmf);
+		if (!ret)
+			ret = VM_FAULT_NOPAGE;
+		return ret;
+	}
+
  	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY |
  			    VM_FAULT_DONE_COW)))
  		return ret;
diff --git a/mm/mmap.c b/mm/mmap.c
index 096bba4..74fb49a 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1419,6 +1419,10 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
  	if (!len)
  		return -EINVAL;
  
+	/* Restrict MAP_NOSIGBUS to MAP_PRIVATE mapping */
+	if ((flags & MAP_NOSIGBUS) && !(flags & MAP_PRIVATE))
+		return -EINVAL;
+
  	/*
  	 * Does the application expect PROT_READ to imply PROT_EXEC?
  	 *