linux-kernel - Re: [f2fs-dev] [PATCH v3] f2fs: don't use GFP

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 linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180410064050.GA61388@jaegeuk-macbookpro.roam.corp.google.com>
Date:   Mon, 9 Apr 2018 23:40:50 -0700
From:   Jaegeuk Kim <jaegeuk@...nel.org>
To:     Chao Yu <yuchao0@...wei.com>
Cc:     linux-kernel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH v3] f2fs: don't use GFP_ZERO for page caches

On 04/10, Chao Yu wrote:
> On 2018/4/10 11:29, Jaegeuk Kim wrote:
> > Change log from v2:
> >  - consider IO error case when dealing with metapage
> >  - memset by fill_node_footer
> > 
> > Change log from v1:
> >  - don't memset for recovered page
> >  
> > Related to https://lkml.org/lkml/2018/4/8/661
> > 
> > Sometimes, we need to write meta data to new allocated block address,
> > then we will allocate a zeroed page in inner inode's address space, and
> > fill partial data in it, and leave other place with zero value which means
> > some fields are initial status.
> > 
> > There are two inner inodes (meta inode and node inode) setting __GFP_ZERO,
> > I have just checked them, for both of them, we can avoid using __GFP_ZERO,
> > and do initialization by ourselves to avoid unneeded/redundant zeroing
> > from mm.
> > 
> > Cc: <stable@...r.kernel.org>
> > Signed-off-by: Chao Yu <yuchao0@...wei.com>
> > Signed-off-by: Jaegeuk Kim <jaegeuk@...nel.org>
> > ---
> >  fs/f2fs/checkpoint.c | 4 +++-
> >  fs/f2fs/inode.c      | 4 ++--
> >  fs/f2fs/segment.c    | 3 +++
> >  fs/f2fs/segment.h    | 1 +
> >  4 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index bf779461df13..2e23b953d304 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -100,8 +100,10 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
> >  	 * readonly and make sure do not write checkpoint with non-uptodate
> >  	 * meta page.
> >  	 */
> > -	if (unlikely(!PageUptodate(page)))
> > +	if (unlikely(!PageUptodate(page))) {
> > +		memset(page_address(page), 0, PAGE_SIZE);
> >  		f2fs_stop_checkpoint(sbi, false);
> > +	}
> >  out:
> >  	return page;
> >  }
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 417c9dcd0269..87535bf63421 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -320,10 +320,10 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
> >  make_now:
> >  	if (ino == F2FS_NODE_INO(sbi)) {
> >  		inode->i_mapping->a_ops = &f2fs_node_aops;
> > -		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
> > +		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
> >  	} else if (ino == F2FS_META_INO(sbi)) {
> >  		inode->i_mapping->a_ops = &f2fs_meta_aops;
> > -		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
> > +		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
> >  	} else if (S_ISREG(inode->i_mode)) {
> >  		inode->i_op = &f2fs_file_inode_operations;
> >  		inode->i_fop = &f2fs_file_operations;
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index a4b8e3e24ccb..1f5db557ab96 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -2021,6 +2021,7 @@ static void write_current_sum_page(struct f2fs_sb_info *sbi,
> >  	struct f2fs_summary_block *dst;
> >  
> >  	dst = (struct f2fs_summary_block *)page_address(page);
> > +	memset(dst, 0, PAGE_SIZE);
> 
> We will fill all summary page later, so we don't need initialize it?

I'd like to reserve the uninitialzed space as zeros for future reference.

> 
> >  
> >  	mutex_lock(&curseg->curseg_mutex);
> >  
> > @@ -3117,6 +3118,7 @@ static void write_compacted_summaries(struct f2fs_sb_info *sbi, block_t blkaddr)
> >  
> >  	page = grab_meta_page(sbi, blkaddr++);
> >  	kaddr = (unsigned char *)page_address(page);
> > +	memset(kaddr, 0, PAGE_SIZE);
> 
> I checked here, we will not use uninitialized space due to:
> 
> read_compacted_summaries()
> {
> ...
> 	if (offset + SUMMARY_SIZE <= PAGE_SIZE - SUM_FOOTER_SIZE)
> 		continue;
> ...
> }
> 
> Are we safe here?
> 
> >  
> >  	/* Step 1: write nat cache */
> >  	seg_i = CURSEG_I(sbi, CURSEG_HOT_DATA);
> > @@ -3141,6 +3143,7 @@ static void write_compacted_summaries(struct f2fs_sb_info *sbi, block_t blkaddr)
> >  			if (!page) {
> >  				page = grab_meta_page(sbi, blkaddr++);
> >  				kaddr = (unsigned char *)page_address(page);
> > +				memset(kaddr, 0, PAGE_SIZE);
> 
> Ditto.
> 
> >  				written_size = 0;
> >  			}
> >  			summary = (struct f2fs_summary *)(kaddr + written_size);
> > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > index 3325d0769723..492ad0c86fa9 100644
> > --- a/fs/f2fs/segment.h
> > +++ b/fs/f2fs/segment.h
> > @@ -375,6 +375,7 @@ static inline void seg_info_to_sit_page(struct f2fs_sb_info *sbi,
> >  	int i;
> >  
> >  	raw_sit = (struct f2fs_sit_block *)page_address(page);
> > +	memset(raw_sit, 0, PAGE_SIZE);
> 
> I suspect that root cause of panic is that we don't memset here?
> 
> Thanks,
> 
> >  	for (i = 0; i < end - start; i++) {
> >  		rs = &raw_sit->entries[i];
> >  		se = get_seg_entry(sbi, start + i);
> >