It's useful to be able to determine the size of a VMA descriptor range used
on f_op->mmap_prepare, expressed both in bytes and pages, so add helpers
for both and update code that could make use of it to do so.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 fs/ntfs3/file.c    |  2 +-
 include/linux/mm.h | 10 ++++++++++
 mm/secretmem.c     |  2 +-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
index c1ece707b195..86eb88f62714 100644
--- a/fs/ntfs3/file.c
+++ b/fs/ntfs3/file.c
@@ -304,7 +304,7 @@ static int ntfs_file_mmap_prepare(struct vm_area_desc *desc)
 
 	if (rw) {
 		u64 to = min_t(loff_t, i_size_read(inode),
-			       from + desc->end - desc->start);
+			       from + vma_desc_size(desc));
 
 		if (is_sparsed(ni)) {
 			/* Allocate clusters for rw map. */
diff --git a/include/linux/mm.h b/include/linux/mm.h
index a6bfa46937a8..9d4508b20be3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3560,6 +3560,16 @@ static inline unsigned long vma_pages(const struct vm_area_struct *vma)
 	return (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
 }
 
+static inline unsigned long vma_desc_size(struct vm_area_desc *desc)
+{
+	return desc->end - desc->start;
+}
+
+static inline unsigned long vma_desc_pages(struct vm_area_desc *desc)
+{
+	return vma_desc_size(desc) >> PAGE_SHIFT;
+}
+
 /* Look up the first VMA which exactly match the interval vm_start ... vm_end */
 static inline struct vm_area_struct *find_exact_vma(struct mm_struct *mm,
 				unsigned long vm_start, unsigned long vm_end)
diff --git a/mm/secretmem.c b/mm/secretmem.c
index 60137305bc20..62066ddb1e9c 100644
--- a/mm/secretmem.c
+++ b/mm/secretmem.c
@@ -120,7 +120,7 @@ static int secretmem_release(struct inode *inode, struct file *file)
 
 static int secretmem_mmap_prepare(struct vm_area_desc *desc)
 {
-	const unsigned long len = desc->end - desc->start;
+	const unsigned long len = vma_desc_size(desc);
 
 	if ((desc->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
 		return -EINVAL;
-- 
2.51.0

On 08.09.25 13:10, Lorenzo Stoakes wrote:
> It's useful to be able to determine the size of a VMA descriptor range used
> on f_op->mmap_prepare, expressed both in bytes and pages, so add helpers
> for both and update code that could make use of it to do so.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>   fs/ntfs3/file.c    |  2 +-
>   include/linux/mm.h | 10 ++++++++++
>   mm/secretmem.c     |  2 +-
>   3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
> index c1ece707b195..86eb88f62714 100644
> --- a/fs/ntfs3/file.c
> +++ b/fs/ntfs3/file.c
> @@ -304,7 +304,7 @@ static int ntfs_file_mmap_prepare(struct vm_area_desc *desc)
>   
>   	if (rw) {
>   		u64 to = min_t(loff_t, i_size_read(inode),
> -			       from + desc->end - desc->start);
> +			       from + vma_desc_size(desc));
>   
>   		if (is_sparsed(ni)) {
>   			/* Allocate clusters for rw map. */
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a6bfa46937a8..9d4508b20be3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3560,6 +3560,16 @@ static inline unsigned long vma_pages(const struct vm_area_struct *vma)
>   	return (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
>   }
>   
> +static inline unsigned long vma_desc_size(struct vm_area_desc *desc)
> +{
> +	return desc->end - desc->start;
> +}
> +
> +static inline unsigned long vma_desc_pages(struct vm_area_desc *desc)
> +{
> +	return vma_desc_size(desc) >> PAGE_SHIFT;
> +}

"const struct vm_area_desc *" in both cases?

> +
>   /* Look up the first VMA which exactly match the interval vm_start ... vm_end */
>   static inline struct vm_area_struct *find_exact_vma(struct mm_struct *mm,
>   				unsigned long vm_start, unsigned long vm_end)
> diff --git a/mm/secretmem.c b/mm/secretmem.c
> index 60137305bc20..62066ddb1e9c 100644
> --- a/mm/secretmem.c
> +++ b/mm/secretmem.c
> @@ -120,7 +120,7 @@ static int secretmem_release(struct inode *inode, struct file *file)
>   
>   static int secretmem_mmap_prepare(struct vm_area_desc *desc)
>   {
> -	const unsigned long len = desc->end - desc->start;
> +	const unsigned long len = vma_desc_size(desc);
>   
>   	if ((desc->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
>   		return -EINVAL;

We really want to forbid any private mappings here, independent of cow.

Maybe a is_private_mapping() helper

or a

vma_desc_is_private_mapping()

helper if we really need it

-- 
Cheers

David / dhildenb

On Mon, Sep 08, 2025 at 12:10:34PM +0100, Lorenzo Stoakes wrote:
>  static int secretmem_mmap_prepare(struct vm_area_desc *desc)
>  {
> -	const unsigned long len = desc->end - desc->start;
> +	const unsigned long len = vma_desc_size(desc);
>  
>  	if ((desc->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
>  		return -EINVAL;

I wonder if we should have some helper for this shared check too, it
is a bit tricky with the two flags. Forced-shared checks are pretty
common.

vma_desc_must_be_shared(desc) ?

Also 'must not be exec' is common too.

Jason

On Mon, Sep 08, 2025 at 09:51:01AM -0300, Jason Gunthorpe wrote:
> On Mon, Sep 08, 2025 at 12:10:34PM +0100, Lorenzo Stoakes wrote:
> >  static int secretmem_mmap_prepare(struct vm_area_desc *desc)
> >  {
> > -	const unsigned long len = desc->end - desc->start;
> > +	const unsigned long len = vma_desc_size(desc);
> >
> >  	if ((desc->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
> >  		return -EINVAL;
>
> I wonder if we should have some helper for this shared check too, it
> is a bit tricky with the two flags. Forced-shared checks are pretty
> common.

Sure can add.

>
> vma_desc_must_be_shared(desc) ?

Maybe _could_be_shared()?

>
> Also 'must not be exec' is common too.

Right, will have a look! :)

>
> Jason

On Mon, Sep 08, 2025 at 02:12:00PM +0100, Lorenzo Stoakes wrote:
> On Mon, Sep 08, 2025 at 09:51:01AM -0300, Jason Gunthorpe wrote:
> > On Mon, Sep 08, 2025 at 12:10:34PM +0100, Lorenzo Stoakes wrote:
> > >  static int secretmem_mmap_prepare(struct vm_area_desc *desc)
> > >  {
> > > -	const unsigned long len = desc->end - desc->start;
> > > +	const unsigned long len = vma_desc_size(desc);
> > >
> > >  	if ((desc->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
> > >  		return -EINVAL;
> >
> > I wonder if we should have some helper for this shared check too, it
> > is a bit tricky with the two flags. Forced-shared checks are pretty
> > common.
> 
> Sure can add.
> 
> >
> > vma_desc_must_be_shared(desc) ?
> 
> Maybe _could_be_shared()?

It is not could, it is must. 

Perhaps

!vma_desc_cowable()

Is what many drivers are really trying to assert.

Jason

On Mon, Sep 08, 2025 at 10:32:24AM -0300, Jason Gunthorpe wrote:
> On Mon, Sep 08, 2025 at 02:12:00PM +0100, Lorenzo Stoakes wrote:
> > On Mon, Sep 08, 2025 at 09:51:01AM -0300, Jason Gunthorpe wrote:
> > > On Mon, Sep 08, 2025 at 12:10:34PM +0100, Lorenzo Stoakes wrote:
> > > >  static int secretmem_mmap_prepare(struct vm_area_desc *desc)
> > > >  {
> > > > -	const unsigned long len = desc->end - desc->start;
> > > > +	const unsigned long len = vma_desc_size(desc);
> > > >
> > > >  	if ((desc->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
> > > >  		return -EINVAL;
> > >
> > > I wonder if we should have some helper for this shared check too, it
> > > is a bit tricky with the two flags. Forced-shared checks are pretty
> > > common.
> >
> > Sure can add.
> >
> > >
> > > vma_desc_must_be_shared(desc) ?
> >
> > Maybe _could_be_shared()?
>
> It is not could, it is must.

I mean VM_MAYSHARE is a nonsense anyway, but _in theory_ VM_MAYSHARE &&
!VM_SHARE means we _could_ share it.

But in reality of course this isn't a real thing.

Perhaps vma_desc_is_shared() or something, I obviously don't want to get stuck
on semantics here :) [he says, while getting obviously stuck on semantics] :P

>
> Perhaps
>
> !vma_desc_cowable()
>
> Is what many drivers are really trying to assert.

Well no, because:

static inline bool is_cow_mapping(vm_flags_t flags)
{
	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
}

Read-only means !CoW.

Hey we've made a rod for own backs! Again!

>
> Jason

Cheers, Lorenzo

On Mon, Sep 08, 2025 at 03:09:43PM +0100, Lorenzo Stoakes wrote:
> > Perhaps
> >
> > !vma_desc_cowable()
> >
> > Is what many drivers are really trying to assert.
> 
> Well no, because:
> 
> static inline bool is_cow_mapping(vm_flags_t flags)
> {
> 	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
> }
> 
> Read-only means !CoW.

What drivers want when they check SHARED is to prevent COW. It is COW
that causes problems for whatever the driver is doing, so calling the
helper cowable and making the test actually right for is a good thing.

COW of this VMA, and no possibilty to remap/mprotect/fork/etc it into
something that is COW in future.

Drivers have commonly various things with VM_SHARED to establish !COW,
but if that isn't actually right then lets fix it to be clear and
correct.

Jason

On Mon, Sep 08, 2025 at 11:20:11AM -0300, Jason Gunthorpe wrote:
> On Mon, Sep 08, 2025 at 03:09:43PM +0100, Lorenzo Stoakes wrote:
> > > Perhaps
> > >
> > > !vma_desc_cowable()
> > >
> > > Is what many drivers are really trying to assert.
> >
> > Well no, because:
> >
> > static inline bool is_cow_mapping(vm_flags_t flags)
> > {
> > 	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
> > }
> >
> > Read-only means !CoW.
>
> What drivers want when they check SHARED is to prevent COW. It is COW
> that causes problems for whatever the driver is doing, so calling the
> helper cowable and making the test actually right for is a good thing.
>
> COW of this VMA, and no possibilty to remap/mprotect/fork/etc it into
> something that is COW in future.

But you can't do that if !VM_MAYWRITE.

I mean probably the driver's just wrong and should use is_cow_mapping() tbh.

>
> Drivers have commonly various things with VM_SHARED to establish !COW,
> but if that isn't actually right then lets fix it to be clear and
> correct.

I think we need to be cautious of scope here :) I don't want to accidentally
break things this way.

OK I think a sensible way forward - How about I add desc_is_cowable() or
vma_desc_cowable() and only set this if I'm confident it's correct?

That way I can achieve both aims at once.

>
> Jason

Cheers, Lorenzo

On Mon, Sep 08, 2025 at 03:47:34PM +0100, Lorenzo Stoakes wrote:
> On Mon, Sep 08, 2025 at 11:20:11AM -0300, Jason Gunthorpe wrote:
> > On Mon, Sep 08, 2025 at 03:09:43PM +0100, Lorenzo Stoakes wrote:
> > > > Perhaps
> > > >
> > > > !vma_desc_cowable()
> > > >
> > > > Is what many drivers are really trying to assert.
> > >
> > > Well no, because:
> > >
> > > static inline bool is_cow_mapping(vm_flags_t flags)
> > > {
> > > 	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
> > > }
> > >
> > > Read-only means !CoW.
> >
> > What drivers want when they check SHARED is to prevent COW. It is COW
> > that causes problems for whatever the driver is doing, so calling the
> > helper cowable and making the test actually right for is a good thing.
> >
> > COW of this VMA, and no possibilty to remap/mprotect/fork/etc it into
> > something that is COW in future.
> 
> But you can't do that if !VM_MAYWRITE.

See this is my fear, the drivers are wrong and you are talking about
edge cases nobody actually knows about.

The need is the created VMA, and its dups, never, ever becomes
COWable. This is what drivers actually want. We need to give them a
clear test to do that.

Anything using remap and checking for SHARED almost certainly falls
into this category as COWing remapped memory is rare and weird.
 
> I mean probably the driver's just wrong and should use
> is_cow_mapping() tbh.

Maybe.

> I think we need to be cautious of scope here :) I don't want to
> accidentally break things this way.

IMHO it is worth doing when you get into more driver places it is far
more obvious why the VM_SHARED is being checked.

> OK I think a sensible way forward - How about I add desc_is_cowable() or
> vma_desc_cowable() and only set this if I'm confident it's correct?

I'm thinking to call it vma_desc_never_cowable() as that is much much
clear what the purpose is.

I think anyone just checking VM_SHARED should be changed over..

Jason

> 
>> I think we need to be cautious of scope here :) I don't want to
>> accidentally break things this way.
> 
> IMHO it is worth doing when you get into more driver places it is far
> more obvious why the VM_SHARED is being checked.
> 
>> OK I think a sensible way forward - How about I add desc_is_cowable() or
>> vma_desc_cowable() and only set this if I'm confident it's correct?
> 
> I'm thinking to call it vma_desc_never_cowable() as that is much much
> clear what the purpose is.

Secretmem wants no private mappings. So we should check exactly that, 
not whether we might have a cow mapping.

-- 
Cheers

David / dhildenb

On Mon, Sep 08, 2025 at 05:24:23PM +0200, David Hildenbrand wrote:
> >
> > > I think we need to be cautious of scope here :) I don't want to
> > > accidentally break things this way.
> >
> > IMHO it is worth doing when you get into more driver places it is far
> > more obvious why the VM_SHARED is being checked.
> >
> > > OK I think a sensible way forward - How about I add desc_is_cowable() or
> > > vma_desc_cowable() and only set this if I'm confident it's correct?
> >
> > I'm thinking to call it vma_desc_never_cowable() as that is much much
> > clear what the purpose is.
>
> Secretmem wants no private mappings. So we should check exactly that, not
> whether we might have a cow mapping.

Well then :)

Probably in most cases what Jason is saying is valid for drivers.

So I can add a helper for both.

Maybe vma_desc_is_private() for this one?

>
> --
> Cheers
>
> David / dhildenb
>

On Mon, Sep 08, 2025 at 05:24:23PM +0200, David Hildenbrand wrote:
> > 
> > > I think we need to be cautious of scope here :) I don't want to
> > > accidentally break things this way.
> > 
> > IMHO it is worth doing when you get into more driver places it is far
> > more obvious why the VM_SHARED is being checked.
> > 
> > > OK I think a sensible way forward - How about I add desc_is_cowable() or
> > > vma_desc_cowable() and only set this if I'm confident it's correct?
> > 
> > I'm thinking to call it vma_desc_never_cowable() as that is much much
> > clear what the purpose is.
> 
> Secretmem wants no private mappings. So we should check exactly that, not
> whether we might have a cow mapping.

secretmem is checking shared for a different reason than many other places..

Jason

On 08.09.25 17:33, Jason Gunthorpe wrote:
> On Mon, Sep 08, 2025 at 05:24:23PM +0200, David Hildenbrand wrote:
>>>
>>>> I think we need to be cautious of scope here :) I don't want to
>>>> accidentally break things this way.
>>>
>>> IMHO it is worth doing when you get into more driver places it is far
>>> more obvious why the VM_SHARED is being checked.
>>>
>>>> OK I think a sensible way forward - How about I add desc_is_cowable() or
>>>> vma_desc_cowable() and only set this if I'm confident it's correct?
>>>
>>> I'm thinking to call it vma_desc_never_cowable() as that is much much
>>> clear what the purpose is.
>>
>> Secretmem wants no private mappings. So we should check exactly that, not
>> whether we might have a cow mapping.
> 
> secretmem is checking shared for a different reason than many other places..

I think many cases just don't want any private mappings.

After all, you need a R/O file (VM_MAYWRITE cleared) mapped MAP_PRIVATE 
to make is_cow_mapping() == false.

And at that point, you just mostly have a R/O MAP_SHARED mapping IIRC.

-- 
Cheers

David / dhildenb

On 08.09.25 17:46, David Hildenbrand wrote:
> On 08.09.25 17:33, Jason Gunthorpe wrote:
>> On Mon, Sep 08, 2025 at 05:24:23PM +0200, David Hildenbrand wrote:
>>>>
>>>>> I think we need to be cautious of scope here :) I don't want to
>>>>> accidentally break things this way.
>>>>
>>>> IMHO it is worth doing when you get into more driver places it is far
>>>> more obvious why the VM_SHARED is being checked.
>>>>
>>>>> OK I think a sensible way forward - How about I add desc_is_cowable() or
>>>>> vma_desc_cowable() and only set this if I'm confident it's correct?
>>>>
>>>> I'm thinking to call it vma_desc_never_cowable() as that is much much
>>>> clear what the purpose is.
>>>
>>> Secretmem wants no private mappings. So we should check exactly that, not
>>> whether we might have a cow mapping.
>>
>> secretmem is checking shared for a different reason than many other places..
> 
> I think many cases just don't want any private mappings.
> 
> After all, you need a R/O file (VM_MAYWRITE cleared) mapped MAP_PRIVATE
> to make is_cow_mapping() == false.

Sorry, was confused there. R/O file does not matter with MAP_PRIVATE. I 
think we default to VM_MAYWRITE with MAP_PRIVATE unless someone 
explicitly clears it.

So in practice there is indeed not a big difference between a private 
and cow mapping.

-- 
Cheers

David / dhildenb

On Mon, Sep 08, 2025 at 05:50:18PM +0200, David Hildenbrand wrote:

> So in practice there is indeed not a big difference between a private and
> cow mapping.

Right and most drivers just check SHARED.

But if we are being documentative why they check shared is because the
driver cannot tolerate COW.

I think if someone is cargo culting a diver and sees
'vma_never_cowable' they will have a better understanding of the
driver side issues.

Driver's don't actually care about private vs shared, except this
indirectly implies something about cow.

Jason

On 08.09.25 17:56, Jason Gunthorpe wrote:
> On Mon, Sep 08, 2025 at 05:50:18PM +0200, David Hildenbrand wrote:
> 
>> So in practice there is indeed not a big difference between a private and
>> cow mapping.
> 
> Right and most drivers just check SHARED.
> 
> But if we are being documentative why they check shared is because the
> driver cannot tolerate COW.
> 
> I think if someone is cargo culting a diver and sees
> 'vma_never_cowable' they will have a better understanding of the
> driver side issues.
> 
> Driver's don't actually care about private vs shared, except this
> indirectly implies something about cow.

I recall some corner cases, but yes, most drivers don't clear MAP_MAYWRITE so
is_cow_mapping() would just rule out what they wanted to rule out (no anon
pages / cow semantics).

FWIW, I recalled some VM_MAYWRITE magic in memfd, but it's really just for
!cow mappings, so the following should likely work:

diff --git a/mm/memfd.c b/mm/memfd.c
index 1de610e9f2ea2..2a3aa26444bbb 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -346,14 +346,11 @@ static int check_write_seal(vm_flags_t *vm_flags_ptr)
         vm_flags_t vm_flags = *vm_flags_ptr;
         vm_flags_t mask = vm_flags & (VM_SHARED | VM_WRITE);
  
-       /* If a private mapping then writability is irrelevant. */
-       if (!(mask & VM_SHARED))
+       /* If a CoW mapping then writability is irrelevant. */
+       if (is_cow_mapping(vm_flags))
                 return 0;
  
-       /*
-        * New PROT_WRITE and MAP_SHARED mmaps are not allowed when
-        * write seals are active.
-        */
+       /* New PROT_WRITE mappings are not allowed when write-sealed. */
         if (mask & VM_WRITE)
                 return -EPERM;
  


-- 
Cheers

David / dhildenb

On Mon, Sep 08, 2025 at 07:36:59PM +0200, David Hildenbrand wrote:
> On 08.09.25 17:56, Jason Gunthorpe wrote:
> > On Mon, Sep 08, 2025 at 05:50:18PM +0200, David Hildenbrand wrote:
> >
> > > So in practice there is indeed not a big difference between a private and
> > > cow mapping.
> >
> > Right and most drivers just check SHARED.
> >
> > But if we are being documentative why they check shared is because the
> > driver cannot tolerate COW.
> >
> > I think if someone is cargo culting a diver and sees
> > 'vma_never_cowable' they will have a better understanding of the
> > driver side issues.
> >
> > Driver's don't actually care about private vs shared, except this
> > indirectly implies something about cow.
>
> I recall some corner cases, but yes, most drivers don't clear MAP_MAYWRITE so
> is_cow_mapping() would just rule out what they wanted to rule out (no anon
> pages / cow semantics).
>
> FWIW, I recalled some VM_MAYWRITE magic in memfd, but it's really just for
> !cow mappings, so the following should likely work:

I was invovled in these dark arts :)

Since we gate the check_write_seal() function (which is the one that removes
VM_MAYWRITE) on the mapping being shared, then obviously we can't remove
VM_MAYWRITE in the first place.

The only other way VM_MAYWRITE could be got rid of is if it already a MAP_SHARED
or MAP_SHARED_VALIDATE mapping without write permission, and then it'd fail this
check anyway.

So I think the below patch is fine!

>
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 1de610e9f2ea2..2a3aa26444bbb 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -346,14 +346,11 @@ static int check_write_seal(vm_flags_t *vm_flags_ptr)
>         vm_flags_t vm_flags = *vm_flags_ptr;
>         vm_flags_t mask = vm_flags & (VM_SHARED | VM_WRITE);
> -       /* If a private mapping then writability is irrelevant. */
> -       if (!(mask & VM_SHARED))
> +       /* If a CoW mapping then writability is irrelevant. */
> +       if (is_cow_mapping(vm_flags))
>                 return 0;
> -       /*
> -        * New PROT_WRITE and MAP_SHARED mmaps are not allowed when
> -        * write seals are active.
> -        */
> +       /* New PROT_WRITE mappings are not allowed when write-sealed. */
>         if (mask & VM_WRITE)
>                 return -EPERM;

>
>
> --
> Cheers
>
> David / dhildenb
>

Cheers, Lorenzo

On 08.09.25 16:47, Lorenzo Stoakes wrote:
> On Mon, Sep 08, 2025 at 11:20:11AM -0300, Jason Gunthorpe wrote:
>> On Mon, Sep 08, 2025 at 03:09:43PM +0100, Lorenzo Stoakes wrote:
>>>> Perhaps
>>>>
>>>> !vma_desc_cowable()
>>>>
>>>> Is what many drivers are really trying to assert.
>>>
>>> Well no, because:
>>>
>>> static inline bool is_cow_mapping(vm_flags_t flags)
>>> {
>>> 	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
>>> }
>>>
>>> Read-only means !CoW.
>>
>> What drivers want when they check SHARED is to prevent COW. It is COW
>> that causes problems for whatever the driver is doing, so calling the
>> helper cowable and making the test actually right for is a good thing.
>>
>> COW of this VMA, and no possibilty to remap/mprotect/fork/etc it into
>> something that is COW in future.
> 
> But you can't do that if !VM_MAYWRITE.
> 
> I mean probably the driver's just wrong and should use is_cow_mapping() tbh.
> 
>>
>> Drivers have commonly various things with VM_SHARED to establish !COW,
>> but if that isn't actually right then lets fix it to be clear and
>> correct.
> 
> I think we need to be cautious of scope here :) I don't want to accidentally
> break things this way.
> 
> OK I think a sensible way forward - How about I add desc_is_cowable() or
> vma_desc_cowable() and only set this if I'm confident it's correct?

I'll note that the naming is bad.

Why?

Because the vma_desc is not cowable. The underlying mapping maybe is.

-- 
Cheers

David / dhildenb

On Mon, Sep 08, 2025 at 05:07:57PM +0200, David Hildenbrand wrote:
> On 08.09.25 16:47, Lorenzo Stoakes wrote:
> > On Mon, Sep 08, 2025 at 11:20:11AM -0300, Jason Gunthorpe wrote:
> > > On Mon, Sep 08, 2025 at 03:09:43PM +0100, Lorenzo Stoakes wrote:
> > > > > Perhaps
> > > > >
> > > > > !vma_desc_cowable()
> > > > >
> > > > > Is what many drivers are really trying to assert.
> > > >
> > > > Well no, because:
> > > >
> > > > static inline bool is_cow_mapping(vm_flags_t flags)
> > > > {
> > > > 	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
> > > > }
> > > >
> > > > Read-only means !CoW.
> > >
> > > What drivers want when they check SHARED is to prevent COW. It is COW
> > > that causes problems for whatever the driver is doing, so calling the
> > > helper cowable and making the test actually right for is a good thing.
> > >
> > > COW of this VMA, and no possibilty to remap/mprotect/fork/etc it into
> > > something that is COW in future.
> >
> > But you can't do that if !VM_MAYWRITE.
> >
> > I mean probably the driver's just wrong and should use is_cow_mapping() tbh.
> >
> > >
> > > Drivers have commonly various things with VM_SHARED to establish !COW,
> > > but if that isn't actually right then lets fix it to be clear and
> > > correct.
> >
> > I think we need to be cautious of scope here :) I don't want to accidentally
> > break things this way.
> >
> > OK I think a sensible way forward - How about I add desc_is_cowable() or
> > vma_desc_cowable() and only set this if I'm confident it's correct?
>
> I'll note that the naming is bad.
>
> Why?
>
> Because the vma_desc is not cowable. The underlying mapping maybe is.

Right, but the vma_desc desribes a VMA being set up.

I mean is_cow_mapping(desc->vm_flags) isn't too egregious anyway, so maybe
just use that for that case?

>
> --
> Cheers
>
> David / dhildenb
>

Cheers, Lorenzo

On 08.09.25 17:35, Lorenzo Stoakes wrote:
> On Mon, Sep 08, 2025 at 05:07:57PM +0200, David Hildenbrand wrote:
>> On 08.09.25 16:47, Lorenzo Stoakes wrote:
>>> On Mon, Sep 08, 2025 at 11:20:11AM -0300, Jason Gunthorpe wrote:
>>>> On Mon, Sep 08, 2025 at 03:09:43PM +0100, Lorenzo Stoakes wrote:
>>>>>> Perhaps
>>>>>>
>>>>>> !vma_desc_cowable()
>>>>>>
>>>>>> Is what many drivers are really trying to assert.
>>>>>
>>>>> Well no, because:
>>>>>
>>>>> static inline bool is_cow_mapping(vm_flags_t flags)
>>>>> {
>>>>> 	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
>>>>> }
>>>>>
>>>>> Read-only means !CoW.
>>>>
>>>> What drivers want when they check SHARED is to prevent COW. It is COW
>>>> that causes problems for whatever the driver is doing, so calling the
>>>> helper cowable and making the test actually right for is a good thing.
>>>>
>>>> COW of this VMA, and no possibilty to remap/mprotect/fork/etc it into
>>>> something that is COW in future.
>>>
>>> But you can't do that if !VM_MAYWRITE.
>>>
>>> I mean probably the driver's just wrong and should use is_cow_mapping() tbh.
>>>
>>>>
>>>> Drivers have commonly various things with VM_SHARED to establish !COW,
>>>> but if that isn't actually right then lets fix it to be clear and
>>>> correct.
>>>
>>> I think we need to be cautious of scope here :) I don't want to accidentally
>>> break things this way.
>>>
>>> OK I think a sensible way forward - How about I add desc_is_cowable() or
>>> vma_desc_cowable() and only set this if I'm confident it's correct?
>>
>> I'll note that the naming is bad.
>>
>> Why?
>>
>> Because the vma_desc is not cowable. The underlying mapping maybe is.
> 
> Right, but the vma_desc desribes a VMA being set up.
> 
> I mean is_cow_mapping(desc->vm_flags) isn't too egregious anyway, so maybe
> just use that for that case?

Yes, I don't think we would need another wrapper.

-- 
Cheers

David / dhildenb

On Mon, Sep 08, 2025 at 07:30:34PM +0200, David Hildenbrand wrote:
> On 08.09.25 17:35, Lorenzo Stoakes wrote:
> > On Mon, Sep 08, 2025 at 05:07:57PM +0200, David Hildenbrand wrote:
> > > On 08.09.25 16:47, Lorenzo Stoakes wrote:
> > > > On Mon, Sep 08, 2025 at 11:20:11AM -0300, Jason Gunthorpe wrote:
> > > > > On Mon, Sep 08, 2025 at 03:09:43PM +0100, Lorenzo Stoakes wrote:
> > > > > > > Perhaps
> > > > > > >
> > > > > > > !vma_desc_cowable()
> > > > > > >
> > > > > > > Is what many drivers are really trying to assert.
> > > > > >
> > > > > > Well no, because:
> > > > > >
> > > > > > static inline bool is_cow_mapping(vm_flags_t flags)
> > > > > > {
> > > > > > 	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
> > > > > > }
> > > > > >
> > > > > > Read-only means !CoW.
> > > > >
> > > > > What drivers want when they check SHARED is to prevent COW. It is COW
> > > > > that causes problems for whatever the driver is doing, so calling the
> > > > > helper cowable and making the test actually right for is a good thing.
> > > > >
> > > > > COW of this VMA, and no possibilty to remap/mprotect/fork/etc it into
> > > > > something that is COW in future.
> > > >
> > > > But you can't do that if !VM_MAYWRITE.
> > > >
> > > > I mean probably the driver's just wrong and should use is_cow_mapping() tbh.
> > > >
> > > > >
> > > > > Drivers have commonly various things with VM_SHARED to establish !COW,
> > > > > but if that isn't actually right then lets fix it to be clear and
> > > > > correct.
> > > >
> > > > I think we need to be cautious of scope here :) I don't want to accidentally
> > > > break things this way.
> > > >
> > > > OK I think a sensible way forward - How about I add desc_is_cowable() or
> > > > vma_desc_cowable() and only set this if I'm confident it's correct?
> > >
> > > I'll note that the naming is bad.
> > >
> > > Why?
> > >
> > > Because the vma_desc is not cowable. The underlying mapping maybe is.
> >
> > Right, but the vma_desc desribes a VMA being set up.
> >
> > I mean is_cow_mapping(desc->vm_flags) isn't too egregious anyway, so maybe
> > just use that for that case?
>
> Yes, I don't think we would need another wrapper.

Ack will use this in favour of a wrapper.

>
> --
> Cheers
>
> David / dhildenb
>

Cheers, Lorenzo