diff mbox series

[v2,3/3] mm: perform the mapping_map_writable() check after call_mmap()

Message ID 6f3aea05c9cc46094b029cbd1138d163c1ae7f9d.1682890156.git.lstoakes@gmail.com (mailing list archive)
State Mainlined, archived
Headers show
Series permit write-sealed memfd read-only shared mappings | expand

Commit Message

Lorenzo Stoakes April 30, 2023, 10:26 p.m. UTC 
In order for a F_SEAL_WRITE sealed memfd mapping to have an opportunity to
clear VM_MAYWRITE, we must be able to invoke the appropriate vm_ops->mmap()
handler to do so. We would otherwise fail the mapping_map_writable() check
before we had the opportunity to avoid it.

This patch moves this check after the call_mmap() invocation. Only memfd
actively denies write access causing a potential failure here (in
memfd_add_seals()), so there should be no impact on non-memfd cases.

This patch makes the userland-visible change that MAP_SHARED, PROT_READ
mappings of an F_SEAL_WRITE sealed memfd mapping will now succeed.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217238
Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
---
 mm/mmap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--
2.40.1

Comments

Andy Lutomirski May 1, 2023, 7:02 p.m. UTC | #1 
On Sun, Apr 30, 2023 at 3:26 PM Lorenzo Stoakes <lstoakes@gmail.com> wrote:
>
> In order for a F_SEAL_WRITE sealed memfd mapping to have an opportunity to
> clear VM_MAYWRITE, we must be able to invoke the appropriate vm_ops->mmap()
> handler to do so. We would otherwise fail the mapping_map_writable() check
> before we had the opportunity to avoid it.

Is there any reason this can't go before patch 3?

If I'm understanding correctly, a comment like the following might
make this a lot more comprehensible:

>
> This patch moves this check after the call_mmap() invocation. Only memfd
> actively denies write access causing a potential failure here (in
> memfd_add_seals()), so there should be no impact on non-memfd cases.
>
> This patch makes the userland-visible change that MAP_SHARED, PROT_READ
> mappings of an F_SEAL_WRITE sealed memfd mapping will now succeed.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217238
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>  mm/mmap.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 646e34e95a37..1608d7f5a293 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2642,17 +2642,17 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>         vma->vm_pgoff = pgoff;
>
>         if (file) {
> -               if (is_shared_maywrite(vm_flags)) {
> -                       error = mapping_map_writable(file->f_mapping);
> -                       if (error)
> -                               goto free_vma;
> -               }
> -
>                 vma->vm_file = get_file(file);
>                 error = call_mmap(file, vma);
>                 if (error)
>                         goto unmap_and_free_vma;
>

/* vm_ops->mmap() may have changed vma->flags.  Check for writability now. */

> +               if (vma_is_shared_maywrite(vma)) {
> +                       error = mapping_map_writable(file->f_mapping);
> +                       if (error)
> +                               goto close_and_free_vma;
> +               }
> +

Alternatively, if anyone is nervous about the change in ordering here,
there could be a whole new vm_op like adjust_vma_flags() that happens
before any of this.

--Andy
Lorenzo Stoakes May 2, 2023, 7:57 a.m. UTC | #2 
On Mon, May 01, 2023 at 12:02:00PM -0700, Andy Lutomirski wrote:
> On Sun, Apr 30, 2023 at 3:26 PM Lorenzo Stoakes <lstoakes@gmail.com> wrote:
> >
> > In order for a F_SEAL_WRITE sealed memfd mapping to have an opportunity to
> > clear VM_MAYWRITE, we must be able to invoke the appropriate vm_ops->mmap()
> > handler to do so. We would otherwise fail the mapping_map_writable() check
> > before we had the opportunity to avoid it.
>
> Is there any reason this can't go before patch 3?

I don't quite understand what you mean by this? I mean sure, we could, but
intent was to build to this point and leave the most controversial change
for last :)

>
> If I'm understanding correctly, a comment like the following might
> make this a lot more comprehensible:
>
> >
> > This patch moves this check after the call_mmap() invocation. Only memfd
> > actively denies write access causing a potential failure here (in
> > memfd_add_seals()), so there should be no impact on non-memfd cases.
> >
> > This patch makes the userland-visible change that MAP_SHARED, PROT_READ
> > mappings of an F_SEAL_WRITE sealed memfd mapping will now succeed.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217238
> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> > ---
> >  mm/mmap.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 646e34e95a37..1608d7f5a293 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2642,17 +2642,17 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >         vma->vm_pgoff = pgoff;
> >
> >         if (file) {
> > -               if (is_shared_maywrite(vm_flags)) {
> > -                       error = mapping_map_writable(file->f_mapping);
> > -                       if (error)
> > -                               goto free_vma;
> > -               }
> > -
> >                 vma->vm_file = get_file(file);
> >                 error = call_mmap(file, vma);
> >                 if (error)
> >                         goto unmap_and_free_vma;
> >
>
> /* vm_ops->mmap() may have changed vma->flags.  Check for writability now. */
>

Ack, will add on next spin.

> > +               if (vma_is_shared_maywrite(vma)) {
> > +                       error = mapping_map_writable(file->f_mapping);
> > +                       if (error)
> > +                               goto close_and_free_vma;
> > +               }
> > +
>
> Alternatively, if anyone is nervous about the change in ordering here,
> there could be a whole new vm_op like adjust_vma_flags() that happens
> before any of this.

Agreed, clearly this change is the most controversial thing here. I did
look around and couldn't find any instance where this could cause an issue,
since it is purely the mapping_map_writable() that gets run at a different
point, but this is certainly an alterative.

I have a feeling people might find adding a new op there possibly _more_
nerve-inducing :) but it's an option.

>
> --Andy
Yujie Liu May 16, 2023, 5:52 a.m. UTC | #3 
Hello,

kernel test robot noticed "assertion_failure" on:

commit: a0e22a91f487957346732c6613eb6bd1b7c72ab1 ("[PATCH v2 3/3] mm: perform the mapping_map_writable() check after call_mmap()")
url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/mm-drop-the-assumption-that-VM_SHARED-always-implies-writable/20230501-062815
base: https://git.kernel.org/cgit/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/all/6f3aea05c9cc46094b029cbd1138d163c1ae7f9d.1682890156.git.lstoakes@gmail.com/
patch subject: [PATCH v2 3/3] mm: perform the mapping_map_writable() check after call_mmap()

in testcase: igt
version: igt-x86_64-9e9cd7e6-1_20230506
with following parameters:

	group: group-11

compiler: gcc-11
test machine: 20 threads 1 sockets (Commet Lake) with 16G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)


If you fix the issue, kindly add following tag
| Reported-by: kernel test robot <yujie.liu@intel.com>
| Link: https://lore.kernel.org/oe-lkp/202305161044.bba89e76-yujie.liu@intel.com


2023-05-11 12:29:38 build/tests/gem_mmap_gtt --run-subtest basic-copy
IGT-Version: 1.27.1-g9e9cd7e6 (x86_64) (Linux: 6.3.0-10673-ga0e22a91f487 x86_64)
Starting subtest: basic-copy
(gem_mmap_gtt:1138) i915/gem_mman-CRITICAL: Test assertion failure function gem_mmap__gtt, file ../lib/i915/gem_mman.c:146:
(gem_mmap_gtt:1138) i915/gem_mman-CRITICAL: Failed assertion: ptr
(gem_mmap_gtt:1138) i915/gem_mman-CRITICAL: Last errno: 1, Operation not permitted
Subtest basic-copy failed.
**** DEBUG ****
(gem_mmap_gtt:1138) i915/gem_mman-CRITICAL: Test assertion failure function gem_mmap__gtt, file ../lib/i915/gem_mman.c:146:
(gem_mmap_gtt:1138) i915/gem_mman-CRITICAL: Failed assertion: ptr
(gem_mmap_gtt:1138) i915/gem_mman-CRITICAL: Last errno: 1, Operation not permitted
(gem_mmap_gtt:1138) igt_core-INFO: Stack trace:
(gem_mmap_gtt:1138) igt_core-INFO:   #0 [__igt_fail_assert+0x106]
(gem_mmap_gtt:1138) igt_core-INFO:   #1 [gem_mmap__gtt+0x3c]
(gem_mmap_gtt:1138) igt_core-INFO:   #2 ../tests/i915/gem_mmap_gtt.c:89 create_pointer_size()
(gem_mmap_gtt:1138) igt_core-INFO:   #3 ../tests/i915/gem_mmap_gtt.c:97 __igt_unique____real_main1261()
(gem_mmap_gtt:1138) igt_core-INFO:   #4 ../tests/i915/gem_mmap_gtt.c:1261 main()
(gem_mmap_gtt:1138) igt_core-INFO:   #5 ../csu/libc-start.c:308 __libc_start_main()
(gem_mmap_gtt:1138) igt_core-INFO:   #6 [_start+0x2a]
****  END  ****
Stack trace:
  #0 [__igt_fail_assert+0x106]
  #1 [gem_mmap__gtt+0x3c]
  #2 ../tests/i915/gem_mmap_gtt.c:89 create_pointer_size()
  #3 ../tests/i915/gem_mmap_gtt.c:97 __igt_unique____real_main1261()
  #4 ../tests/i915/gem_mmap_gtt.c:1261 main()
  #5 ../csu/libc-start.c:308 __libc_start_main()
  #6 [_start+0x2a]
Subtest basic-copy: FAIL (0.039s)
...


To reproduce:

        git clone https://github.com/intel/lkp-tests.git
        cd lkp-tests
        sudo bin/lkp install job.yaml           # job file is attached in this email
        bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
        sudo bin/lkp run generated-yaml-file

        # if come across any failure that blocks the test,
        # please remove ~/.lkp and /lkp dir to run from a clean state.
Lorenzo Stoakes Oct. 7, 2023, 8:07 p.m. UTC | #4 
On Tue, May 16, 2023 at 01:52:06PM +0800, kernel test robot wrote:
> Hello,
>
> kernel test robot noticed "assertion_failure" on:
>
> commit: a0e22a91f487957346732c6613eb6bd1b7c72ab1 ("[PATCH v2 3/3] mm: perform the mapping_map_writable() check after call_mmap()")
> url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/mm-drop-the-assumption-that-VM_SHARED-always-implies-writable/20230501-062815
> base: https://git.kernel.org/cgit/linux/kernel/git/akpm/mm.git mm-everything
> patch link: https://lore.kernel.org/all/6f3aea05c9cc46094b029cbd1138d163c1ae7f9d.1682890156.git.lstoakes@gmail.com/
> patch subject: [PATCH v2 3/3] mm: perform the mapping_map_writable() check after call_mmap()
>
> in testcase: igt
> version: igt-x86_64-9e9cd7e6-1_20230506
> with following parameters:
>
> 	group: group-11
>
> compiler: gcc-11
> test machine: 20 threads 1 sockets (Commet Lake) with 16G memory
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
> If you fix the issue, kindly add following tag
> | Reported-by: kernel test robot <yujie.liu@intel.com>
> | Link: https://lore.kernel.org/oe-lkp/202305161044.bba89e76-yujie.liu@intel.com
>
>
> 2023-05-11 12:29:38 build/tests/gem_mmap_gtt --run-subtest basic-copy
> IGT-Version: 1.27.1-g9e9cd7e6 (x86_64) (Linux: 6.3.0-10673-ga0e22a91f487 x86_64)
> Starting subtest: basic-copy
> (gem_mmap_gtt:1138) i915/gem_mman-CRITICAL: Test assertion failure function gem_mmap__gtt, file ../lib/i915/gem_mman.c:146:
> (gem_mmap_gtt:1138) i915/gem_mman-CRITICAL: Failed assertion: ptr
> (gem_mmap_gtt:1138) i915/gem_mman-CRITICAL: Last errno: 1, Operation not permitted
> Subtest basic-copy failed.
[snip]

I don't have the hardware to test this (the repro steps don't work and
manually running the test indicates the actual hardware is required) but I
suspect it's a result of i915_gem_mmap() somehow causing
mapping_unmap_writable() to be invoked, which sets mapping->i_mmap_writable
negative, and thus the check after call_mmap() is performed reports the error.

In v3 I will change this to continue to mark the file writable before
invoking call_mmap() which should fix this issue.
diff mbox series

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index 646e34e95a37..1608d7f5a293 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2642,17 +2642,17 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 	vma->vm_pgoff = pgoff;

 	if (file) {
-		if (is_shared_maywrite(vm_flags)) {
-			error = mapping_map_writable(file->f_mapping);
-			if (error)
-				goto free_vma;
-		}
-
 		vma->vm_file = get_file(file);
 		error = call_mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;

+		if (vma_is_shared_maywrite(vma)) {
+			error = mapping_map_writable(file->f_mapping);
+			if (error)
+				goto close_and_free_vma;
+		}
+
 		/*
 		 * Expansion is handled above, merging is handled below.
 		 * Drivers should not alter the address of the VMA.