lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 5 Jan 2021 18:53:48 +0100
From:   Jan Kara <jack@...e.cz>
To:     Chengguang Xu <cgxu519@...ernel.net>
Cc:     jack@...e.com, linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext2: implement ->page_mkwrite

On Fri 18-12-20 21:27:57, Chengguang Xu wrote:
> Currently ext2 uses generic mmap operations for non DAX file and
> filemap_page_mkwrite() does not check the block allocation for
> shared writable mmapped area on pagefault. In some cases like
> disk space exhaustion or disk quota limitation, it will cause silent
> data loss. This patch tries to check and do block preallocation on
> pagefault if necessary and explicitly return error to user when
> allocation failure.
> 
> Signed-off-by: Chengguang Xu <cgxu519@...ernel.net>

Thanks for the patch and sorry for the delay in replying. I agree there's
the problem you describe but I'm not sure whether we should fix it.  ext2
has been like this since the beginning so well over 20 years.  Allocating
blocks on write fault has the unwelcome impact that the file layout is
likely going to be much worse (much more fragmented) - I remember getting
some reports about performance regressions from users back when I did a
similar change for ext3. And so I'm reluctant to change behavior of such
an old legacy filesystem as ext2...

								Honza

> ---
>  fs/ext2/file.c | 40 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext2/file.c b/fs/ext2/file.c
> index 96044f5dbc0e..a34119415ef1 100644
> --- a/fs/ext2/file.c
> +++ b/fs/ext2/file.c
> @@ -25,10 +25,34 @@
>  #include <linux/quotaops.h>
>  #include <linux/iomap.h>
>  #include <linux/uio.h>
> +#include <linux/buffer_head.h>
>  #include "ext2.h"
>  #include "xattr.h"
>  #include "acl.h"
>  
> +vm_fault_t ext2_page_mkwrite(struct vm_fault *vmf)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct inode *inode = file_inode(vma->vm_file);
> +	int err;
> +
> +	if (unlikely(IS_IMMUTABLE(inode)))
> +		return VM_FAULT_SIGBUS;
> +
> +	sb_start_pagefault(inode->i_sb);
> +	file_update_time(vma->vm_file);
> +	err = block_page_mkwrite(vma, vmf, ext2_get_block);
> +	sb_end_pagefault(inode->i_sb);
> +
> +	return block_page_mkwrite_return(err);
> +}
> +
> +const struct vm_operations_struct ext2_vm_ops = {
> +	.fault		= filemap_fault,
> +	.map_pages	= filemap_map_pages,
> +	.page_mkwrite	= ext2_page_mkwrite,
> +};
> +
>  #ifdef CONFIG_FS_DAX
>  static ssize_t ext2_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  {
> @@ -123,15 +147,23 @@ static const struct vm_operations_struct ext2_dax_vm_ops = {
>  
>  static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma)
>  {
> +	file_accessed(file);
>  	if (!IS_DAX(file_inode(file)))
> -		return generic_file_mmap(file, vma);
> +		vma->vm_ops = &ext2_vm_ops;
> +	else
> +		vma->vm_ops = &ext2_dax_vm_ops;
>  
> -	file_accessed(file);
> -	vma->vm_ops = &ext2_dax_vm_ops;
>  	return 0;
>  }
> +
>  #else
> -#define ext2_file_mmap	generic_file_mmap
> +static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	file_accessed(file);
> +	vma->vm_ops = &ext2_vm_ops;
> +	return 0;
> +}
> +
>  #endif
>  
>  /*
> -- 
> 2.18.4
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists