From: Jaegeuk Kim <jaegeuk@kernel.org>
To: Chao Yu <chao@kernel.org>
Cc: Ming Yan <yanming@tju.edu.cn>,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode
Date: Fri, 13 May 2022 14:35:27 -0700 [thread overview]
Message-ID: <Yn7PH6BDBSDtiaZV@google.com> (raw)
In-Reply-To: <YnlH929igOSi+Iv3@google.com>
On 05/09, Jaegeuk Kim wrote:
> On 05/08, Chao Yu wrote:
> > Ping,
>
> This is in my TODO list, but will take some time. Sorry.
Got this.
sanity_check_inode: inode (ino=1125d, mode=41471) reason(0, 98, 3452, 4, 0, 0) should not have inline_data, run fsck to fix
Ok, this is a symlink, which was encrypted having inline_data.
>
> >
> > On 2022/5/5 22:33, Chao Yu wrote:
> > > On 2022/5/5 11:31, Jaegeuk Kim wrote:
> > > > On 05/05, Chao Yu wrote:
> > > > > On 2022/5/5 5:28, Jaegeuk Kim wrote:
> > > > > > On 04/28, Chao Yu wrote:
> > > > > > > As Yanming reported in bugzilla:
> > > > > > >
> > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=215895
> > > > > > >
> > > > > > > I have encountered a bug in F2FS file system in kernel v5.17.
> > > > > > >
> > > > > > > The kernel message is shown below:
> > > > > > >
> > > > > > > kernel BUG at fs/inode.c:611!
> > > > > > > Call Trace:
> > > > > > > evict+0x282/0x4e0
> > > > > > > __dentry_kill+0x2b2/0x4d0
> > > > > > > dput+0x2dd/0x720
> > > > > > > do_renameat2+0x596/0x970
> > > > > > > __x64_sys_rename+0x78/0x90
> > > > > > > do_syscall_64+0x3b/0x90
> > > > > > >
> > > > > > > The root cause is: fuzzed inode has both inline_data flag and encrypted
> > > > > > > flag, so after it was deleted by rename(), during f2fs_evict_inode(),
> > > > > > > it will cause inline data conversion due to flags confilction, then
> > > > > > > page cache will be polluted and trigger panic in clear_inode().
> > > > > > >
> > > > > > > This patch tries to fix the issue by do more sanity checks for inline
> > > > > > > data inode in sanity_check_inode().
> > > > > > >
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Reported-by: Ming Yan <yanming@tju.edu.cn>
> > > > > > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > > > > > ---
> > > > > > > fs/f2fs/f2fs.h | 7 +++++++
> > > > > > > fs/f2fs/inode.c | 3 +--
> > > > > > > 2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > > > index 27aa93caec06..64c511b498cc 100644
> > > > > > > --- a/fs/f2fs/f2fs.h
> > > > > > > +++ b/fs/f2fs/f2fs.h
> > > > > > > @@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
> > > > > > > */
> > > > > > > static inline bool f2fs_post_read_required(struct inode *inode)
> > > > > > > {
> > > > > > > + /*
> > > > > > > + * used by sanity_check_inode(), when disk layout fields has not
> > > > > > > + * been synchronized to inmem fields.
> > > > > > > + */
> > > > > > > + if (file_is_encrypt(inode) || file_is_verity(inode) ||
> > > > > > > + F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
> > > > > > > + return true;
> > > > > > > return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
> > > > > > > f2fs_compressed_file(inode);
> > > > > > > }
> > > > > > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > > > > index 83639238a1fe..234b8ed02644 100644
> > > > > > > --- a/fs/f2fs/inode.c
> > > > > > > +++ b/fs/f2fs/inode.c
> > > > > > > @@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
> > > > > > > }
> > > > > > > }
> > > > > > > - if (f2fs_has_inline_data(inode) &&
> > > > > > > - (!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
> > > > > > > + if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
> > > > > >
> > > > > > It seems f2fs_may_inline_data() is breaking the atomic write case. Please fix.
> > > > >
> > > > > sanity_check_inode() change only affect f2fs_iget(), during inode initialization,
> > > > > file should not be set as atomic one, right?
> > > > >
> > > > > I didn't see any failure during 'f2fs_io write atomic_write' testcase... could you
> > > > > please provide me detail of the testcase?
> > > >
> > > > I just applied this into my device and was getting lots of the below error
> > > > messages resulting in open failures of database files.
> > >
> > > Could you please help to apply below patch and dump the log?
> > >
> > > From: Chao Yu <chao@kernel.org>
> > > Subject: [PATCH] f2fs: fix to do sanity check for inline inode
> > >
> > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > ---
> > > fs/f2fs/f2fs.h | 7 +++++++
> > > fs/f2fs/inode.c | 11 +++++++----
> > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 0f8c426aed50..13a9212d6cb6 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -4159,6 +4159,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
> > > */
> > > static inline bool f2fs_post_read_required(struct inode *inode)
> > > {
> > > + /*
> > > + * used by sanity_check_inode(), when disk layout fields has not
> > > + * been synchronized to inmem fields.
> > > + */
> > > + if (file_is_encrypt(inode) || file_is_verity(inode) ||
> > > + F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
> > > + return true;
> > > return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
> > > f2fs_compressed_file(inode);
> > > }
> > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > index 02630c17da93..a98614a24ad0 100644
> > > --- a/fs/f2fs/inode.c
> > > +++ b/fs/f2fs/inode.c
> > > @@ -276,11 +276,14 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
> > > }
> > > }
> > >
> > > - if (f2fs_has_inline_data(inode) &&
> > > - (!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
> > > + if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
> > > set_sbi_flag(sbi, SBI_NEED_FSCK);
> > > - f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
> > > - __func__, inode->i_ino, inode->i_mode);
> > > + f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) reason(%d, %llu, %ld, %d, %d, %lu) should not have inline_data, run fsck to fix",
> > > + __func__, inode->i_ino, inode->i_mode,
> > > + f2fs_is_atomic_file(inode),
> > > + i_size_read(inode), MAX_INLINE_DATA(inode),
> > > + file_is_encrypt(inode), file_is_verity(inode),
> > > + F2FS_I(inode)->i_flags & F2FS_COMPR_FL);
> > > return false;
> > > }
> > >
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
WARNING: multiple messages have this Message-ID (diff)
From: Jaegeuk Kim <jaegeuk@kernel.org>
To: Chao Yu <chao@kernel.org>
Cc: Ming Yan <yanming@tju.edu.cn>,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode
Date: Fri, 13 May 2022 14:35:27 -0700 [thread overview]
Message-ID: <Yn7PH6BDBSDtiaZV@google.com> (raw)
In-Reply-To: <YnlH929igOSi+Iv3@google.com>
On 05/09, Jaegeuk Kim wrote:
> On 05/08, Chao Yu wrote:
> > Ping,
>
> This is in my TODO list, but will take some time. Sorry.
Got this.
sanity_check_inode: inode (ino=1125d, mode=41471) reason(0, 98, 3452, 4, 0, 0) should not have inline_data, run fsck to fix
Ok, this is a symlink, which was encrypted having inline_data.
>
> >
> > On 2022/5/5 22:33, Chao Yu wrote:
> > > On 2022/5/5 11:31, Jaegeuk Kim wrote:
> > > > On 05/05, Chao Yu wrote:
> > > > > On 2022/5/5 5:28, Jaegeuk Kim wrote:
> > > > > > On 04/28, Chao Yu wrote:
> > > > > > > As Yanming reported in bugzilla:
> > > > > > >
> > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=215895
> > > > > > >
> > > > > > > I have encountered a bug in F2FS file system in kernel v5.17.
> > > > > > >
> > > > > > > The kernel message is shown below:
> > > > > > >
> > > > > > > kernel BUG at fs/inode.c:611!
> > > > > > > Call Trace:
> > > > > > > evict+0x282/0x4e0
> > > > > > > __dentry_kill+0x2b2/0x4d0
> > > > > > > dput+0x2dd/0x720
> > > > > > > do_renameat2+0x596/0x970
> > > > > > > __x64_sys_rename+0x78/0x90
> > > > > > > do_syscall_64+0x3b/0x90
> > > > > > >
> > > > > > > The root cause is: fuzzed inode has both inline_data flag and encrypted
> > > > > > > flag, so after it was deleted by rename(), during f2fs_evict_inode(),
> > > > > > > it will cause inline data conversion due to flags confilction, then
> > > > > > > page cache will be polluted and trigger panic in clear_inode().
> > > > > > >
> > > > > > > This patch tries to fix the issue by do more sanity checks for inline
> > > > > > > data inode in sanity_check_inode().
> > > > > > >
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Reported-by: Ming Yan <yanming@tju.edu.cn>
> > > > > > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > > > > > ---
> > > > > > > fs/f2fs/f2fs.h | 7 +++++++
> > > > > > > fs/f2fs/inode.c | 3 +--
> > > > > > > 2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > > > index 27aa93caec06..64c511b498cc 100644
> > > > > > > --- a/fs/f2fs/f2fs.h
> > > > > > > +++ b/fs/f2fs/f2fs.h
> > > > > > > @@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
> > > > > > > */
> > > > > > > static inline bool f2fs_post_read_required(struct inode *inode)
> > > > > > > {
> > > > > > > + /*
> > > > > > > + * used by sanity_check_inode(), when disk layout fields has not
> > > > > > > + * been synchronized to inmem fields.
> > > > > > > + */
> > > > > > > + if (file_is_encrypt(inode) || file_is_verity(inode) ||
> > > > > > > + F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
> > > > > > > + return true;
> > > > > > > return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
> > > > > > > f2fs_compressed_file(inode);
> > > > > > > }
> > > > > > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > > > > index 83639238a1fe..234b8ed02644 100644
> > > > > > > --- a/fs/f2fs/inode.c
> > > > > > > +++ b/fs/f2fs/inode.c
> > > > > > > @@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
> > > > > > > }
> > > > > > > }
> > > > > > > - if (f2fs_has_inline_data(inode) &&
> > > > > > > - (!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
> > > > > > > + if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
> > > > > >
> > > > > > It seems f2fs_may_inline_data() is breaking the atomic write case. Please fix.
> > > > >
> > > > > sanity_check_inode() change only affect f2fs_iget(), during inode initialization,
> > > > > file should not be set as atomic one, right?
> > > > >
> > > > > I didn't see any failure during 'f2fs_io write atomic_write' testcase... could you
> > > > > please provide me detail of the testcase?
> > > >
> > > > I just applied this into my device and was getting lots of the below error
> > > > messages resulting in open failures of database files.
> > >
> > > Could you please help to apply below patch and dump the log?
> > >
> > > From: Chao Yu <chao@kernel.org>
> > > Subject: [PATCH] f2fs: fix to do sanity check for inline inode
> > >
> > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > ---
> > > fs/f2fs/f2fs.h | 7 +++++++
> > > fs/f2fs/inode.c | 11 +++++++----
> > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 0f8c426aed50..13a9212d6cb6 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -4159,6 +4159,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
> > > */
> > > static inline bool f2fs_post_read_required(struct inode *inode)
> > > {
> > > + /*
> > > + * used by sanity_check_inode(), when disk layout fields has not
> > > + * been synchronized to inmem fields.
> > > + */
> > > + if (file_is_encrypt(inode) || file_is_verity(inode) ||
> > > + F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
> > > + return true;
> > > return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
> > > f2fs_compressed_file(inode);
> > > }
> > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > index 02630c17da93..a98614a24ad0 100644
> > > --- a/fs/f2fs/inode.c
> > > +++ b/fs/f2fs/inode.c
> > > @@ -276,11 +276,14 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
> > > }
> > > }
> > >
> > > - if (f2fs_has_inline_data(inode) &&
> > > - (!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
> > > + if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
> > > set_sbi_flag(sbi, SBI_NEED_FSCK);
> > > - f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
> > > - __func__, inode->i_ino, inode->i_mode);
> > > + f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) reason(%d, %llu, %ld, %d, %d, %lu) should not have inline_data, run fsck to fix",
> > > + __func__, inode->i_ino, inode->i_mode,
> > > + f2fs_is_atomic_file(inode),
> > > + i_size_read(inode), MAX_INLINE_DATA(inode),
> > > + file_is_encrypt(inode), file_is_verity(inode),
> > > + F2FS_I(inode)->i_flags & F2FS_COMPR_FL);
> > > return false;
> > > }
> > >
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2022-05-13 21:35 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-28 2:49 [f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode Chao Yu
2022-04-28 2:49 ` Chao Yu
2022-05-04 21:28 ` [f2fs-dev] " Jaegeuk Kim
2022-05-04 21:28 ` Jaegeuk Kim
2022-05-05 2:49 ` [f2fs-dev] " Chao Yu
2022-05-05 2:49 ` Chao Yu
2022-05-05 3:31 ` [f2fs-dev] " Jaegeuk Kim
2022-05-05 3:31 ` Jaegeuk Kim
2022-05-05 14:33 ` [f2fs-dev] " Chao Yu
2022-05-05 14:33 ` Chao Yu
2022-05-08 13:52 ` [f2fs-dev] " Chao Yu
2022-05-08 13:52 ` Chao Yu
2022-05-09 16:57 ` Jaegeuk Kim
2022-05-09 16:57 ` Jaegeuk Kim
2022-05-13 21:35 ` Jaegeuk Kim [this message]
2022-05-13 21:35 ` Jaegeuk Kim
2022-05-13 21:07 ` Jaegeuk Kim
2022-05-13 21:07 ` Jaegeuk Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yn7PH6BDBSDtiaZV@google.com \
--to=jaegeuk@kernel.org \
--cc=chao@kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=yanming@tju.edu.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.