From: Chao Yu <chao@kernel.org> To: Yangtao Li <frank.li@vivo.com>, jaegeuk@kernel.org Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev] [PATCH] f2fs: fix ctx->pos in f2fs_read_inline_dir() Date: Sat, 17 Jul 2021 16:56:01 +0800 [thread overview] Message-ID: <2d651236-40de-2e7b-d6c6-9a18ce9a25ff@kernel.org> (raw) In-Reply-To: <20210717034955.344408-1-frank.li@vivo.com> On 2021/7/17 11:49, Yangtao Li wrote: > I recently found a case where de->name_len is 0 in f2fs_fill_dentries() easily reproduced, > and finally set the fsck flag. > > Thread A Thread B > > f2fs_readdir > f2fs_read_inline_dir > ctx->pos = d.max > f2fs_add_dentry > f2fs_add_inline_entry > do_convert_inline_dir > f2fs_add_regular_entry > f2fs_readdir > f2fs_fill_dentries > set_sbi_flag(sbi, SBI_NEED_FSCK) > > Process A opens the folder, and has been reading without closing it. During this period, > Process B created a file under the folder (occupying multiple f2fs_dir_entry, exceeding > the d.max of the inline dir). After creation, process A uses the d.max of inline dir to > read it again, and it will read that de->name_len is 0. Nice catch! > > And returning early in f2fs_read_inline_dir will cause the process to be unable to see > the changes before reopening the file. > > So don't return early and remove the modification of ctx->pos in f2fs_read_inline_dir(). > > Signed-off-by: Yangtao Li <frank.li@vivo.com> > --- > fs/f2fs/inline.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > index 56a20d5c15da..fc6551139a3d 100644 > --- a/fs/f2fs/inline.c > +++ b/fs/f2fs/inline.c > @@ -729,9 +729,6 @@ int f2fs_read_inline_dir(struct file *file, struct dir_context *ctx, > > make_dentry_ptr_inline(inode, &d, inline_dentry); > > - if (ctx->pos == d.max) > - return 0; > - > ipage = f2fs_get_node_page(F2FS_I_SB(inode), inode->i_ino); > if (IS_ERR(ipage)) > return PTR_ERR(ipage); > @@ -747,8 +744,6 @@ int f2fs_read_inline_dir(struct file *file, struct dir_context *ctx, > make_dentry_ptr_inline(inode, &d, inline_dentry); > > err = f2fs_fill_dentries(ctx, &d, 0, fstr); After this function, ctx->pos will point to start position of first free slot after last dir_entry, we can't guarantee that the free slot won't be used in above race condition, right? Moreover, w/o inline conversion, the race condition still can happen as below, right? dir_entry1: A dir_entry2: B dir_entry3: C free slot: _ Before: AAAABBBB___ ^ Thread B delete dir_entry2, and create dir_entry3. After: AAAACCCCC__ ^ Thanks, > - if (!err) > - ctx->pos = d.max; > > f2fs_put_page(ipage, 0); > return err < 0 ? err : 0; >
WARNING: multiple messages have this Message-ID (diff)
From: Chao Yu <chao@kernel.org> To: Yangtao Li <frank.li@vivo.com>, jaegeuk@kernel.org Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev] [PATCH] f2fs: fix ctx->pos in f2fs_read_inline_dir() Date: Sat, 17 Jul 2021 16:56:01 +0800 [thread overview] Message-ID: <2d651236-40de-2e7b-d6c6-9a18ce9a25ff@kernel.org> (raw) In-Reply-To: <20210717034955.344408-1-frank.li@vivo.com> On 2021/7/17 11:49, Yangtao Li wrote: > I recently found a case where de->name_len is 0 in f2fs_fill_dentries() easily reproduced, > and finally set the fsck flag. > > Thread A Thread B > > f2fs_readdir > f2fs_read_inline_dir > ctx->pos = d.max > f2fs_add_dentry > f2fs_add_inline_entry > do_convert_inline_dir > f2fs_add_regular_entry > f2fs_readdir > f2fs_fill_dentries > set_sbi_flag(sbi, SBI_NEED_FSCK) > > Process A opens the folder, and has been reading without closing it. During this period, > Process B created a file under the folder (occupying multiple f2fs_dir_entry, exceeding > the d.max of the inline dir). After creation, process A uses the d.max of inline dir to > read it again, and it will read that de->name_len is 0. Nice catch! > > And returning early in f2fs_read_inline_dir will cause the process to be unable to see > the changes before reopening the file. > > So don't return early and remove the modification of ctx->pos in f2fs_read_inline_dir(). > > Signed-off-by: Yangtao Li <frank.li@vivo.com> > --- > fs/f2fs/inline.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > index 56a20d5c15da..fc6551139a3d 100644 > --- a/fs/f2fs/inline.c > +++ b/fs/f2fs/inline.c > @@ -729,9 +729,6 @@ int f2fs_read_inline_dir(struct file *file, struct dir_context *ctx, > > make_dentry_ptr_inline(inode, &d, inline_dentry); > > - if (ctx->pos == d.max) > - return 0; > - > ipage = f2fs_get_node_page(F2FS_I_SB(inode), inode->i_ino); > if (IS_ERR(ipage)) > return PTR_ERR(ipage); > @@ -747,8 +744,6 @@ int f2fs_read_inline_dir(struct file *file, struct dir_context *ctx, > make_dentry_ptr_inline(inode, &d, inline_dentry); > > err = f2fs_fill_dentries(ctx, &d, 0, fstr); After this function, ctx->pos will point to start position of first free slot after last dir_entry, we can't guarantee that the free slot won't be used in above race condition, right? Moreover, w/o inline conversion, the race condition still can happen as below, right? dir_entry1: A dir_entry2: B dir_entry3: C free slot: _ Before: AAAABBBB___ ^ Thread B delete dir_entry2, and create dir_entry3. After: AAAACCCCC__ ^ Thanks, > - if (!err) > - ctx->pos = d.max; > > f2fs_put_page(ipage, 0); > return err < 0 ? err : 0; > _______________________________________________ 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:[~2021-07-17 8:56 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-17 3:49 [PATCH] f2fs: fix ctx->pos in f2fs_read_inline_dir() Yangtao Li 2021-07-17 3:49 ` [f2fs-dev] " Yangtao Li 2021-07-17 8:56 ` Chao Yu [this message] 2021-07-17 8:56 ` Chao Yu 2021-07-17 13:25 ` 李扬韬 2021-07-17 13:25 ` 李扬韬 2021-07-17 14:30 ` Chao Yu 2021-07-17 14:30 ` Chao Yu
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=2d651236-40de-2e7b-d6c6-9a18ce9a25ff@kernel.org \ --to=chao@kernel.org \ --cc=frank.li@vivo.com \ --cc=jaegeuk@kernel.org \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-kernel@vger.kernel.org \ /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: linkBe 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.