[v3,1/3] kernfs: remove page_mkwrite() from vm_operations_struct

Commit Message

Martin Oliveira July 4, 2024, 4:37 p.m. UTC

The .page_mkwrite operator of kernfs just calls file_update_time().
This is the same behaviour that the fault code does if .page_mkwrite is
not set.

Furthermore, having the page_mkwrite() operator causes
writable_file_mapping_allowed() to fail due to
vma_needs_dirty_tracking() on the gup flow, which is a pre-requisite for
enabling P2PDMA over RDMA.

There are no users of .page_mkwrite and no known valid use cases, so
just remove the .page_mkwrite from kernfs_ops and WARN_ON() if an mmap()
implementation sets .page_mkwrite.

Co-developed-by: Logan Gunthorpe <[e-mail]>
Signed-off-by: Logan Gunthorpe <[e-mail]>
Signed-off-by: Martin Oliveira <[e-mail]>
---
 fs/kernfs/file.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

Comments

[e-mail] July 4, 2024, 4:48 p.m. UTC | #1

On Thu, Jul 04, 2024 at 10:37:22AM -0600, Martin Oliveira wrote:
> The .page_mkwrite operator of kernfs just calls file_update_time().
> This is the same behaviour that the fault code does if .page_mkwrite is
> not set.
> 
> Furthermore, having the page_mkwrite() operator causes
> writable_file_mapping_allowed() to fail due to
> vma_needs_dirty_tracking() on the gup flow, which is a pre-requisite for
> enabling P2PDMA over RDMA.
> 
> There are no users of .page_mkwrite and no known valid use cases, so
> just remove the .page_mkwrite from kernfs_ops and WARN_ON() if an mmap()
> implementation sets .page_mkwrite.
> 
> Co-developed-by: Logan Gunthorpe <[e-mail]>
> Signed-off-by: Logan Gunthorpe <[e-mail]>
> Signed-off-by: Martin Oliveira <[e-mail]>

Acked-by: Greg Kroah-Hartman <[e-mail]>

Matthew Wilcox July 4, 2024, 5:02 p.m. UTC | #2

On Thu, Jul 04, 2024 at 10:37:22AM -0600, Martin Oliveira wrote:
> @@ -482,6 +459,8 @@ static int kernfs_fop_mmap(struct file *file, struct vm_area_struct *vma)
>  	if (vma->vm_ops && vma->vm_ops->close)
>  		goto out_put;
>  
> +	WARN_ON(vma->vm_ops && vma->vm_ops->page_mkwrite);
> +
>  	rc = 0;
>  	if (!of->mmapped) {
>  		of->mmapped = true;

Seems to me we should actually _handle_ that, not do something wrong.
eg:

	if (vma->vm_ops) {
		if (vma->vm_ops->close)
			goto out_put;
		if (WARN_ON(vma->vm_ops->page_mkwrite))
			goto out_put;
	}

or maybe this doesn't need to be a WARN at all?  After all, there
isn't one for having a ->close method, so why is page_mkwrite special?

Martin Oliveira July 4, 2024, 8:43 p.m. UTC | #3

On 2024-07-04 11:02, Matthew Wilcox wrote:> Seems to me we should actually _handle_ that, not do something wrong.
> eg:
> 
>         if (vma->vm_ops) {
>                 if (vma->vm_ops->close)
>                         goto out_put;
>                 if (WARN_ON(vma->vm_ops->page_mkwrite))
>                         goto out_put;
>         }

Good point.

> or maybe this doesn't need to be a WARN at all?  After all, there
> isn't one for having a ->close method, so why is page_mkwrite special?

Hmm yeah, they should probably be treated the same.

Maybe ->close should be converted to WARN as well? It would be easier to
catch an error this way than chasing the EINVAL, but I'm OK either way.

Thanks,
Martin

Christoph Hellwig July 5, 2024, 7:20 a.m. UTC | #4

On Thu, Jul 04, 2024 at 02:43:04PM -0600, Martin Oliveira wrote:
> On 2024-07-04 11:02, Matthew Wilcox wrote:> Seems to me we should actually _handle_ that, not do something wrong.
> > eg:
> > 
> >         if (vma->vm_ops) {
> >                 if (vma->vm_ops->close)
> >                         goto out_put;
> >                 if (WARN_ON(vma->vm_ops->page_mkwrite))
> >                         goto out_put;
> >         }
> 
> Good point.

Btw, sorry if I mislead you with my WARN_ON_ONCE suggestion.  That
was always intended in addition to the error handling, not instead.
(In fact there are very few reasons to use WARN_ON* without actually
handling the error as well).

> 
> > or maybe this doesn't need to be a WARN at all?  After all, there
> > isn't one for having a ->close method, so why is page_mkwrite special?
> 
> Hmm yeah, they should probably be treated the same.
> 
> Maybe ->close should be converted to WARN as well? It would be easier to
> catch an error this way than chasing the EINVAL, but I'm OK either way.

Yes, doing the same for ->close or anything unimplemented would be
nice.  But it's not really in scope for this series.

kernfs really should be using it's own ops instead of abusing
file_operations, but that's even more out of scope..

Patch

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 8502ef68459b..90603664de7f 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -386,28 +386,6 @@  static vm_fault_t kernfs_vma_fault(struct vm_fault *vmf)
 	return ret;
 }
 
-static vm_fault_t kernfs_vma_page_mkwrite(struct vm_fault *vmf)
-{
-	struct file *file = vmf->vma->vm_file;
-	struct kernfs_open_file *of = kernfs_of(file);
-	vm_fault_t ret;
-
-	if (!of->vm_ops)
-		return VM_FAULT_SIGBUS;
-
-	if (!kernfs_get_active(of->kn))
-		return VM_FAULT_SIGBUS;
-
-	ret = 0;
-	if (of->vm_ops->page_mkwrite)
-		ret = of->vm_ops->page_mkwrite(vmf);
-	else
-		file_update_time(file);
-
-	kernfs_put_active(of->kn);
-	return ret;
-}
-
 static int kernfs_vma_access(struct vm_area_struct *vma, unsigned long addr,
 			     void *buf, int len, int write)
 {
@@ -432,7 +410,6 @@  static int kernfs_vma_access(struct vm_area_struct *vma, unsigned long addr,
 static const struct vm_operations_struct kernfs_vm_ops = {
 	.open		= kernfs_vma_open,
 	.fault		= kernfs_vma_fault,
-	.page_mkwrite	= kernfs_vma_page_mkwrite,
 	.access		= kernfs_vma_access,
 };
 
@@ -482,6 +459,8 @@  static int kernfs_fop_mmap(struct file *file, struct vm_area_struct *vma)
 	if (vma->vm_ops && vma->vm_ops->close)
 		goto out_put;
 
+	WARN_ON(vma->vm_ops && vma->vm_ops->page_mkwrite);
+
 	rc = 0;
 	if (!of->mmapped) {
 		of->mmapped = true;