The next patch is going to remove .page_mkwrite from kernfs and will
WARN if an mmap implementation sets .page_mkwrite.

In preparation for that change, and to make it consistent, add a WARN to
the ->close check.

Co-developed-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Martin Oliveira <martin.oliveira@eideticom.com>
---
 fs/kernfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 8502ef68459b..72cc51dcf870 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -479,7 +479,7 @@ static int kernfs_fop_mmap(struct file *file, struct vm_area_struct *vma)
 	 * It is not possible to successfully wrap close.
 	 * So error if someone is trying to use close.
 	 */
-	if (vma->vm_ops && vma->vm_ops->close)
+	if (WARN_ON_ONCE(vma->vm_ops && vma->vm_ops->close))
 		goto out_put;
 
 	rc = 0;
-- 
2.43.0

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

On Thu, Aug 08, 2024 at 12:33:37PM -0600, Martin Oliveira wrote:
> The next patch is going to remove .page_mkwrite from kernfs and will
> WARN if an mmap implementation sets .page_mkwrite.
> 
> In preparation for that change, and to make it consistent, add a WARN to
> the ->close check.
> 
> Co-developed-by: Logan Gunthorpe <logang@deltatee.com>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Signed-off-by: Martin Oliveira <martin.oliveira@eideticom.com>
> ---
>  fs/kernfs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index 8502ef68459b..72cc51dcf870 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -479,7 +479,7 @@ static int kernfs_fop_mmap(struct file *file, struct vm_area_struct *vma)
>  	 * It is not possible to successfully wrap close.
>  	 * So error if someone is trying to use close.
>  	 */
> -	if (vma->vm_ops && vma->vm_ops->close)
> +	if (WARN_ON_ONCE(vma->vm_ops && vma->vm_ops->close))

So you just rebooted a machine that hits this, loosing data everywhere.
Not nice :(

On Fri, Aug 09, 2024 at 07:37:49AM +0200, Greg Kroah-Hartman wrote:
> >  	 * It is not possible to successfully wrap close.
> >  	 * So error if someone is trying to use close.
> >  	 */
> > -	if (vma->vm_ops && vma->vm_ops->close)
> > +	if (WARN_ON_ONCE(vma->vm_ops && vma->vm_ops->close))
> 
> So you just rebooted a machine that hits this, loosing data everywhere.
> Not nice :(

Huh.  if you are stupid enough to set panic_on_warn you get to keep
the pieces.  And our file systems are reliable to not use data on
an unclean shutdown anyway.

Pleaee stop these BS arguments.

On 2024-08-08 23:37, Greg Kroah-Hartman wrote:
>> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
>> index 8502ef68459b..72cc51dcf870 100644
>> --- a/fs/kernfs/file.c
>> +++ b/fs/kernfs/file.c
>> @@ -479,7 +479,7 @@ static int kernfs_fop_mmap(struct file *file, struct vm_area_struct *vma)
>>        * It is not possible to successfully wrap close.
>>        * So error if someone is trying to use close.
>>        */
>> -     if (vma->vm_ops && vma->vm_ops->close)
>> +     if (WARN_ON_ONCE(vma->vm_ops && vma->vm_ops->close))
> 
> So you just rebooted a machine that hits this, loosing data everywhere.
> Not nice :(

Well, apologies for that, but there's no way to know what every single machine
out there is doing...

However if this machine is using ->close when that's clearly marked as
unsupported then shouldn't we fix that?

On Fri, Aug 09, 2024 at 09:41:48AM -0600, Martin Oliveira wrote:
> On 2024-08-08 23:37, Greg Kroah-Hartman wrote:
> >> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> >> index 8502ef68459b..72cc51dcf870 100644
> >> --- a/fs/kernfs/file.c
> >> +++ b/fs/kernfs/file.c
> >> @@ -479,7 +479,7 @@ static int kernfs_fop_mmap(struct file *file, struct vm_area_struct *vma)
> >>        * It is not possible to successfully wrap close.
> >>        * So error if someone is trying to use close.
> >>        */
> >> -     if (vma->vm_ops && vma->vm_ops->close)
> >> +     if (WARN_ON_ONCE(vma->vm_ops && vma->vm_ops->close))
> > 
> > So you just rebooted a machine that hits this, loosing data everywhere.
> > Not nice :(
> 
> Well, apologies for that, but there's no way to know what every single machine
> out there is doing...
> 
> However if this machine is using ->close when that's clearly marked as
> unsupported then shouldn't we fix that?

return an error properly?