| 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
| ||
|
Message-ID: <20160106040204.GA7697@jaegeuk.gateway>
Date: Tue, 5 Jan 2016 20:02:04 -0800
From: Jaegeuk Kim <jaegeuk@...nel.org>
To: Chao Yu <chao2.yu@...sung.com>
Cc: linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from
disk
On Wed, Jan 06, 2016 at 11:14:22AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@...nel.org]
> > Sent: Wednesday, January 06, 2016 10:30 AM
> > To: Chao Yu
> > Cc: linux-kernel@...r.kernel.org; linux-fsdevel@...r.kernel.org;
> > linux-f2fs-devel@...ts.sourceforge.net
> > Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk
> >
> > Hi,
> >
> > On Wed, Jan 06, 2016 at 09:21:29AM +0800, Chao Yu wrote:
> > > Hi Jaegeuk,
> > >
> > > > -----Original Message-----
> > > > From: Jaegeuk Kim [mailto:jaegeuk@...nel.org]
> > > > Sent: Wednesday, January 06, 2016 1:49 AM
> > > > To: Chao Yu
> > > > Cc: linux-kernel@...r.kernel.org; linux-fsdevel@...r.kernel.org;
> > > > linux-f2fs-devel@...ts.sourceforge.net
> > > > Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk
> > > >
> > > > Hi Chao,
> > > >
> > > > On Tue, Jan 05, 2016 at 05:31:51PM +0800, Chao Yu wrote:
> > > > > Hi Jaegeuk,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jaegeuk Kim [mailto:jaegeuk@...nel.org]
> > > > > > Sent: Sunday, January 03, 2016 9:26 AM
> > > > > > To: linux-kernel@...r.kernel.org; linux-fsdevel@...r.kernel.org;
> > > > > > linux-f2fs-devel@...ts.sourceforge.net
> > > > > > Cc: Jaegeuk Kim
> > > > > > Subject: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk
> > > > > >
> > > > > > After reading a page, we need to check whether there is any error.
> > > > > >
> > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@...nel.org>
> > > > > > ---
> > > > > > fs/f2fs/data.c | 8 ++++++++
> > > > > > 1 file changed, 8 insertions(+)
> > > > > >
> > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > > > index 89a978c..11b2111 100644
> > > > > > --- a/fs/f2fs/data.c
> > > > > > +++ b/fs/f2fs/data.c
> > > > > > @@ -448,6 +448,14 @@ repeat:
> > > > > >
> > > > > > /* wait for read completion */
> > > > > > lock_page(page);
> > > > > > + if (unlikely(!PageUptodate(page))) {
> > > > > > + f2fs_put_page(page, 1);
> > > > > > + return ERR_PTR(-EIO);
> > > > >
> > > > > There is a convention in get_new_data_page, anyway we should release ipage
> > > > > if there is any error occurs, but I think it will be ok to return directly
> > > > > since it seems impossible the new dentry page has its real block address.
> > > >
> > > > Makes sense, but definitely ipage should be put. :)
> > >
> > > Alright. :)
> > >
> > > >
> > > > >
> > > > > To avoid any bug here or wrong usage, how about add bug_on as following patch?
> > > > >
> > > > > >From d92f0f34493b27ef28da67c446d552ce721b5d6f Mon Sep 17 00:00:00 2001
> > > > > From: Chao Yu <chao2.yu@...sung.com>
> > > > > Date: Tue, 5 Jan 2016 15:28:56 +0800
> > > > > Subject: [PATCH] f2fs: add f2fs_bug_on in get_new_data_page
> > > > >
> > > > > In get_new_data_page, locked inode page should not be hold before
> > > > > get_read_data_page, this patch adds f2fs_bug_on to detect this
> > > > > condition.
> > > > >
> > > > > Signed-off-by: Chao Yu <chao2.yu@...sung.com>
> > > > > ---
> > > > > fs/f2fs/data.c | 2 ++
> > > > > 1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > > index 48f0bd3..2c5e3f6 100644
> > > > > --- a/fs/f2fs/data.c
> > > > > +++ b/fs/f2fs/data.c
> > > > > @@ -440,6 +440,8 @@ repeat:
> > > > > zero_user_segment(page, 0, PAGE_CACHE_SIZE);
> > > > > SetPageUptodate(page);
> > > > > } else {
> > > > > + f2fs_bug_on(F2FS_I_SB(inode), ipage);
> > > > > +
> > > > > f2fs_put_page(page, 1);
> > > > >
> > > > > page = get_read_data_page(inode, index, READ_SYNC, true);
> > > > > --
> > > > > 2.6.3
> > > > >
> > > > >
> > > > > > + }
> > > > > > + if (unlikely(page->mapping != mapping)) {
> > > > > > + f2fs_put_page(page, 1);
> > > > > > + goto repeat;
> > > > > > + }
> > > > >
> > > > > How about use get_lock_data_page to avoid duplicated code?
> > > >
> > > > Agreed.
> > > >
> > > > How about this?
> > > >
> > > > From fef77fb244a706491e8e4c46cb245e99e22003c3 Mon Sep 17 00:00:00 2001
> > > > From: Jaegeuk Kim <jaegeuk@...nel.org>
> > > > Date: Fri, 1 Jan 2016 22:03:47 -0800
> > > > Subject: [PATCH] f2fs: check the page status filled from disk
> > > >
> > > > After reading a page, we need to check whether there is any error.
> > > >
> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@...nel.org>
> > > > ---
> > > > fs/f2fs/data.c | 14 +++++++++-----
> > > > 1 file changed, 9 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > index 89a978c..89d633a 100644
> > > > --- a/fs/f2fs/data.c
> > > > +++ b/fs/f2fs/data.c
> > > > @@ -442,12 +442,16 @@ repeat:
> > > > } else {
> > > > f2fs_put_page(page, 1);
> > > >
> > > > - page = get_read_data_page(inode, index, READ_SYNC, true);
> > > > - if (IS_ERR(page))
> > > > - goto repeat;
> > > > + f2fs_bug_on(F2FS_I_SB(inode), ipage);
> > > >
> > > > - /* wait for read completion */
> > > > - lock_page(page);
> > > > + page = get_lock_data_page(inode, index, true);
> > > > + if (IS_ERR(page)) {
> > > > + if (PTR_ERR(page) == -EIO) {
> > > > + f2fs_put_page(ipage, 1);
> > > > + return page;
> > > > + }
> > > > + goto repeat;
> > >
> > > Seems if get_lock_data_page always return -EFAULT, we may run into an
> > > infinite loop. IMO, it's not a bad thing to tolerate other error more
> > > than EIO returned from get_lock_data_page. How about return directly
> > > when error is returned? And add a bug_on for ENOENT which seems not
> > > impossible here?
> >
> > Hmm. I can only expect EIO, ENOMEM, and ENOENT.
> > What condition can we get EFAULT?
>
> It's possible in following call path:
> - get_new_data_page
> - get_read_data_page
> - f2fs_submit_page_bio
> - bio_add_page failed and return -EFAULT
>
> Right?
Indeed. But seems that it's impossible to get that error in this path.
Anyway, yes, it is not a big deal to return any error directly.
I'll modify this again. :)
Thanks,
>
> Thanks,
>
> >
> > Thanks,
> >
> > >
> > > Thanks,
> > >
> > > > + }
> > > > }
> > > > got_it:
> > > > if (new_i_size && i_size_read(inode) <
> > > > --
> > > > 2.6.3
> > >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists