The man page for fcntl() describing memfd file seals states the following
about F_SEAL_WRITE:-
Furthermore, trying to create new shared, writable memory-mappings via
mmap(2) will also fail with EPERM.
With emphasis on _writable_. In turns out in fact that currently the kernel
simply disallows _all_ new shared memory mappings for a memfd with
F_SEAL_WRITE applied, rendering this documentation inaccurate.
This matters because users are therefore unable to obtain a shared mapping
to a memfd after write sealing altogether, which limits their
usefulness. This was reported in the discussion thread [1] originating from
a bug report [2].
This is a product of both using the struct address_space->i_mmap_writable
atomic counter to determine whether writing may be permitted, and the
kernel adjusting this counter when _any_ VM_SHARED mapping is performed.
It seems sensible that we should only update this mapping if VM_MAYWRITE is
specified, i.e. whether it is possible that this mapping could at any point
be written to.
If we do so then all we need to do to permit write seals to function as
documented is to clear VM_MAYWRITE when mapping read-only. It turns out
this functionality already exists for F_SEAL_FUTURE_WRITE - we can
therefore simply adapt this logic to do the same for F_SEAL_WRITE.
The final change required is to invoke call_mmap() before
mapping_map_writable() - doing so ensures that the memfd-relevant
shmem_mmap() or hugetlbfs_file_mmap() custom mmap handlers will be called
before the writable test, enabling us to clear VM_MAYWRITE first.
Thanks to Andy Lutomirski for the suggestion!
[1]:https://lore.kernel.org/all/[email protected]/
[2]:https://bugzilla.kernel.org/show_bug.cgi?id=217238
v2:
- Removed RFC tag.
- Correct incorrect goto pointed out by Jan.
- Reworded cover letter as suggested by Jan.
v1:
https://lore.kernel.org/all/[email protected]/
Lorenzo Stoakes (3):
mm: drop the assumption that VM_SHARED always implies writable
mm: update seal_check_[future_]write() to include F_SEAL_WRITE as well
mm: perform the mapping_map_writable() check after call_mmap()
fs/hugetlbfs/inode.c | 2 +-
include/linux/fs.h | 4 ++--
include/linux/mm.h | 24 ++++++++++++++++++------
kernel/fork.c | 2 +-
mm/filemap.c | 2 +-
mm/madvise.c | 2 +-
mm/mmap.c | 22 +++++++++++-----------
mm/shmem.c | 2 +-
8 files changed, 36 insertions(+), 24 deletions(-)
--
2.40.1
There are places in the kernel where there is an implicit assumption that
VM_SHARED VMAs must either be writable or might become writable via
e.g. mprotect().
We can explicitly check for the writable, shared case while remaining
conservative - If VM_MAYWRITE is not set then, by definition, the memory
can never be written to.
Update these checks to also check for VM_MAYWRITE.
Suggested-by: Andy Lutomirski <[email protected]>
Signed-off-by: Lorenzo Stoakes <[email protected]>
---
include/linux/fs.h | 4 ++--
include/linux/mm.h | 11 +++++++++++
kernel/fork.c | 2 +-
mm/filemap.c | 2 +-
mm/madvise.c | 2 +-
mm/mmap.c | 12 ++++++------
6 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 67495ef79bb2..874fe0e38e65 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -413,7 +413,7 @@ extern const struct address_space_operations empty_aops;
* It is also used to block modification of page cache contents through
* memory mappings.
* @gfp_mask: Memory allocation flags to use for allocating pages.
- * @i_mmap_writable: Number of VM_SHARED mappings.
+ * @i_mmap_writable: Number of VM_SHARED, VM_MAYWRITE mappings.
* @nr_thps: Number of THPs in the pagecache (non-shmem only).
* @i_mmap: Tree of private and shared mappings.
* @i_mmap_rwsem: Protects @i_mmap and @i_mmap_writable.
@@ -516,7 +516,7 @@ static inline int mapping_mapped(struct address_space *mapping)
/*
* Might pages of this file have been modified in userspace?
- * Note that i_mmap_writable counts all VM_SHARED vmas: do_mmap
+ * Note that i_mmap_writable counts all VM_SHARED, VM_MAYWRITE vmas: do_mmap
* marks vma as VM_SHARED if it is shared, and the file was opened for
* writing i.e. vma may be mprotected writable even if now readonly.
*
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 27ce77080c79..3e8fb4601520 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -851,6 +851,17 @@ static inline bool vma_is_accessible(struct vm_area_struct *vma)
return vma->vm_flags & VM_ACCESS_FLAGS;
}
+static inline bool is_shared_maywrite(vm_flags_t vm_flags)
+{
+ return (vm_flags & (VM_SHARED | VM_MAYWRITE)) ==
+ (VM_SHARED | VM_MAYWRITE);
+}
+
+static inline bool vma_is_shared_maywrite(struct vm_area_struct *vma)
+{
+ return is_shared_maywrite(vma->vm_flags);
+}
+
static inline
struct vm_area_struct *vma_find(struct vma_iterator *vmi, unsigned long max)
{
diff --git a/kernel/fork.c b/kernel/fork.c
index 4342200d5e2b..7ebd6229219a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -733,7 +733,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
get_file(file);
i_mmap_lock_write(mapping);
- if (tmp->vm_flags & VM_SHARED)
+ if (vma_is_shared_maywrite(tmp))
mapping_allow_writable(mapping);
flush_dcache_mmap_lock(mapping);
/* insert tmp into the share list, just after mpnt */
diff --git a/mm/filemap.c b/mm/filemap.c
index a34abfe8c654..4d896515032c 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3607,7 +3607,7 @@ int generic_file_mmap(struct file *file, struct vm_area_struct *vma)
*/
int generic_file_readonly_mmap(struct file *file, struct vm_area_struct *vma)
{
- if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_MAYWRITE))
+ if (vma_is_shared_maywrite(vma))
return -EINVAL;
return generic_file_mmap(file, vma);
}
diff --git a/mm/madvise.c b/mm/madvise.c
index b5ffbaf616f5..5eb59854e285 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -969,7 +969,7 @@ static long madvise_remove(struct vm_area_struct *vma,
return -EINVAL;
}
- if ((vma->vm_flags & (VM_SHARED|VM_WRITE)) != (VM_SHARED|VM_WRITE))
+ if (!vma_is_shared_maywrite(vma))
return -EACCES;
offset = (loff_t)(start - vma->vm_start)
diff --git a/mm/mmap.c b/mm/mmap.c
index 5522130ae606..646e34e95a37 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -107,7 +107,7 @@ void vma_set_page_prot(struct vm_area_struct *vma)
static void __remove_shared_vm_struct(struct vm_area_struct *vma,
struct file *file, struct address_space *mapping)
{
- if (vma->vm_flags & VM_SHARED)
+ if (vma_is_shared_maywrite(vma))
mapping_unmap_writable(mapping);
flush_dcache_mmap_lock(mapping);
@@ -428,7 +428,7 @@ static unsigned long count_vma_pages_range(struct mm_struct *mm,
static void __vma_link_file(struct vm_area_struct *vma,
struct address_space *mapping)
{
- if (vma->vm_flags & VM_SHARED)
+ if (vma_is_shared_maywrite(vma))
mapping_allow_writable(mapping);
flush_dcache_mmap_lock(mapping);
@@ -2642,7 +2642,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
vma->vm_pgoff = pgoff;
if (file) {
- if (vm_flags & VM_SHARED) {
+ if (is_shared_maywrite(vm_flags)) {
error = mapping_map_writable(file->f_mapping);
if (error)
goto free_vma;
@@ -2717,7 +2717,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
vma_iter_store(&vmi, vma);
mm->map_count++;
if (vma->vm_file) {
- if (vma->vm_flags & VM_SHARED)
+ if (vma_is_shared_maywrite(vma))
mapping_allow_writable(vma->vm_file->f_mapping);
flush_dcache_mmap_lock(vma->vm_file->f_mapping);
@@ -2734,7 +2734,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
/* Once vma denies write, undo our temporary denial count */
unmap_writable:
- if (file && vm_flags & VM_SHARED)
+ if (file && is_shared_maywrite(vm_flags))
mapping_unmap_writable(file->f_mapping);
file = vma->vm_file;
ksm_add_vma(vma);
@@ -2781,7 +2781,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
unmap_region(mm, &mm->mm_mt, vma, prev, next, vma->vm_start,
vma->vm_end, true);
}
- if (file && (vm_flags & VM_SHARED))
+ if (file && is_shared_maywrite(vm_flags))
mapping_unmap_writable(file->f_mapping);
free_vma:
vm_area_free(vma);
--
2.40.1
Check for F_SEAL_WRITE as well for which the precise same logic can
reasonably be applied, however so far this code will simply not be run as
the mapping_map_writable() call occurs before shmem_mmap() or
hugetlbfs_file_mmap() and thus would error out in the case of a read-only
shared mapping before the logic could be applied.
This therefore has no impact until the following patch which changes the
order in which the *_mmap() functions are called.
Signed-off-by: Lorenzo Stoakes <[email protected]>
---
fs/hugetlbfs/inode.c | 2 +-
include/linux/mm.h | 13 +++++++------
mm/shmem.c | 2 +-
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index ecfdfb2529a3..4abe3d4a6d1c 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -135,7 +135,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
vma->vm_ops = &hugetlb_vm_ops;
- ret = seal_check_future_write(info->seals, vma);
+ ret = seal_check_write(info->seals, vma);
if (ret)
return ret;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 3e8fb4601520..6bf63ee1b769 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3784,16 +3784,17 @@ static inline void mem_dump_obj(void *object) {}
#endif
/**
- * seal_check_future_write - Check for F_SEAL_FUTURE_WRITE flag and handle it
+ * seal_check_write - Check for F_SEAL_WRITE or F_SEAL_FUTURE_WRITE flag and
+ * handle it.
* @seals: the seals to check
* @vma: the vma to operate on
*
- * Check whether F_SEAL_FUTURE_WRITE is set; if so, do proper check/handling on
- * the vma flags. Return 0 if check pass, or <0 for errors.
+ * Check whether F_SEAL_WRITE or F_SEAL_FUTURE_WRITE are set; if so, do proper
+ * check/handling on the vma flags. Return 0 if check pass, or <0 for errors.
*/
-static inline int seal_check_future_write(int seals, struct vm_area_struct *vma)
+static inline int seal_check_write(int seals, struct vm_area_struct *vma)
{
- if (seals & F_SEAL_FUTURE_WRITE) {
+ if (seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
/*
* New PROT_WRITE and MAP_SHARED mmaps are not allowed when
* "future write" seal active.
@@ -3802,7 +3803,7 @@ static inline int seal_check_future_write(int seals, struct vm_area_struct *vma)
return -EPERM;
/*
- * Since an F_SEAL_FUTURE_WRITE sealed memfd can be mapped as
+ * Since an F_SEAL_[FUTURE_]WRITE sealed memfd can be mapped as
* MAP_SHARED and read-only, take care to not allow mprotect to
* revert protections on such mappings. Do this only for shared
* mappings. For private mappings, don't need to mask
diff --git a/mm/shmem.c b/mm/shmem.c
index e517ab50afd9..c54df8e36bc1 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2325,7 +2325,7 @@ static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
struct shmem_inode_info *info = SHMEM_I(inode);
int ret;
- ret = seal_check_future_write(info->seals, vma);
+ ret = seal_check_write(info->seals, vma);
if (ret)
return ret;
--
2.40.1
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 <[email protected]>
---
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;
+ 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.
--
2.40.1
On Sun, Apr 30, 2023 at 3:26 PM Lorenzo Stoakes <[email protected]> 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 <[email protected]>
> ---
> 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
On Mon, May 01, 2023 at 12:02:00PM -0700, Andy Lutomirski wrote:
> On Sun, Apr 30, 2023 at 3:26 PM Lorenzo Stoakes <[email protected]> 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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
| Link: https://lore.kernel.org/oe-lkp/[email protected]
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.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
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 <[email protected]>
> | Link: https://lore.kernel.org/oe-lkp/[email protected]
>
>
> 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.