* [PATCH] fs: do not push freeing inode to b_dirty_time list
@ 2022-11-13 15:24 Svyatoslav Feldsherov
2022-11-14 10:46 ` Jan Kara
0 siblings, 1 reply; 9+ messages in thread
From: Svyatoslav Feldsherov @ 2022-11-13 15:24 UTC (permalink / raw)
To: Alexander Viro, Lukas Czerner, Theodore Ts'o, Jan Kara
Cc: syzbot+6ba92bd00d5093f7e371, oferz, linux-fsdevel, linux-kernel,
Svyatoslav Feldsherov
After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode
already has I_DIRTY_INODE") writeiback_single_inode can push inode with
I_DIRTY_TIME set to b_dirty_time list. In case of freeing inode with
I_DIRTY_TIME set this can happened after deletion of inode io_list at
evict. Stack trace is following.
evict
fat_evict_inode
fat_truncate_blocks
fat_flush_inodes
writeback_inode
sync_inode_metadata
writeback_single_inode
This will lead to use after free in flusher thread.
Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE")
Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com
Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com>
---
fs/fs-writeback.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 443f83382b9b..31c93cbdb3fe 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1718,7 +1718,7 @@ static int writeback_single_inode(struct inode *inode,
*/
if (!(inode->i_state & I_DIRTY_ALL))
inode_cgwb_move_to_attached(inode, wb);
- else if (!(inode->i_state & I_SYNC_QUEUED)) {
+ else if (!(inode->i_state & (I_SYNC_QUEUED | I_FREEING))) {
if ((inode->i_state & I_DIRTY))
redirty_tail_locked(inode, wb);
else if (inode->i_state & I_DIRTY_TIME) {
--
2.38.1.431.g37b22c650d-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] fs: do not push freeing inode to b_dirty_time list
2022-11-13 15:24 [PATCH] fs: do not push freeing inode to b_dirty_time list Svyatoslav Feldsherov
@ 2022-11-14 10:46 ` Jan Kara
2022-11-14 17:43 ` Svyatoslav Feldsherov
0 siblings, 1 reply; 9+ messages in thread
From: Jan Kara @ 2022-11-14 10:46 UTC (permalink / raw)
To: Svyatoslav Feldsherov
Cc: Alexander Viro, Lukas Czerner, Theodore Ts'o, Jan Kara,
syzbot+6ba92bd00d5093f7e371, oferz, linux-fsdevel, linux-kernel
On Sun 13-11-22 17:24:39, Svyatoslav Feldsherov wrote:
> After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode
> already has I_DIRTY_INODE") writeiback_single_inode can push inode with
> I_DIRTY_TIME set to b_dirty_time list. In case of freeing inode with
> I_DIRTY_TIME set this can happened after deletion of inode io_list at
> evict. Stack trace is following.
>
> evict
> fat_evict_inode
> fat_truncate_blocks
> fat_flush_inodes
> writeback_inode
> sync_inode_metadata
> writeback_single_inode
>
> This will lead to use after free in flusher thread.
>
> Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE")
> Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com
> Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com>
Thanks for the analysis! I was scratching my head over this syzbot report
for a while and it didn't occur to me somebody could be calling
writeback_single_inode() from the .evict callback.
Also what contributes to the problem is that FAT calls
sync_inode_metadata(inode, 0) so it is not marking this final flush as data
integrity sync and so we happily leave the I_DIRTY_TIME bit set.
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 443f83382b9b..31c93cbdb3fe 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1718,7 +1718,7 @@ static int writeback_single_inode(struct inode *inode,
> */
> if (!(inode->i_state & I_DIRTY_ALL))
> inode_cgwb_move_to_attached(inode, wb);
> - else if (!(inode->i_state & I_SYNC_QUEUED)) {
> + else if (!(inode->i_state & (I_SYNC_QUEUED | I_FREEING))) {
> if ((inode->i_state & I_DIRTY))
> redirty_tail_locked(inode, wb);
> else if (inode->i_state & I_DIRTY_TIME) {
So even calling inode_cgwb_move_to_attached() is not safe when I_FREEING is
already set. So I belive the I_FREEING bit check needs to be before this
whole if block.
I also think we should add some assertions into i_io_list handling
functions to complain if I_FREEING bit is set to catch these problems
earlier which means to be also more careful in __mark_inode_dirty(). But
this is for a separate cleanup.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fs: do not push freeing inode to b_dirty_time list
2022-11-14 10:46 ` Jan Kara
@ 2022-11-14 17:43 ` Svyatoslav Feldsherov
2022-11-14 19:21 ` Jan Kara
0 siblings, 1 reply; 9+ messages in thread
From: Svyatoslav Feldsherov @ 2022-11-14 17:43 UTC (permalink / raw)
To: Jan Kara
Cc: Alexander Viro, Lukas Czerner, Theodore Ts'o,
syzbot+6ba92bd00d5093f7e371, oferz, linux-fsdevel, linux-kernel
Thank you for looking into this!
On Mon, Nov 14, 2022 at 12:46 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 13-11-22 17:24:39, Svyatoslav Feldsherov wrote:
> > After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode
> > already has I_DIRTY_INODE") writeiback_single_inode can push inode with
> > I_DIRTY_TIME set to b_dirty_time list. In case of freeing inode with
> > I_DIRTY_TIME set this can happened after deletion of inode io_list at
> > evict. Stack trace is following.
> >
> > evict
> > fat_evict_inode
> > fat_truncate_blocks
> > fat_flush_inodes
> > writeback_inode
> > sync_inode_metadata
> > writeback_single_inode
> >
> > This will lead to use after free in flusher thread.
> >
> > Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE")
> > Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com
> > Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com>
>
> Thanks for the analysis! I was scratching my head over this syzbot report
> for a while and it didn't occur to me somebody could be calling
> writeback_single_inode() from the .evict callback.
>
> Also what contributes to the problem is that FAT calls
> sync_inode_metadata(inode, 0) so it is not marking this final flush as data
> integrity sync and so we happily leave the I_DIRTY_TIME bit set.
>
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 443f83382b9b..31c93cbdb3fe 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -1718,7 +1718,7 @@ static int writeback_single_inode(struct inode *inode,
> > */
> > if (!(inode->i_state & I_DIRTY_ALL))
> > inode_cgwb_move_to_attached(inode, wb);
> > - else if (!(inode->i_state & I_SYNC_QUEUED)) {
> > + else if (!(inode->i_state & (I_SYNC_QUEUED | I_FREEING))) {
> > if ((inode->i_state & I_DIRTY))
> > redirty_tail_locked(inode, wb);
> > else if (inode->i_state & I_DIRTY_TIME) {
>
> So even calling inode_cgwb_move_to_attached() is not safe when I_FREEING is
> already set. So I belive the I_FREEING bit check needs to be before this
> whole if block.
Agree, let me move the I_FREEING check before this if block.
The commit I am fixing didn't change this codepath, so I suspect there is an
implicit invariant which keeps inode_cgwb_move_to_attached call safe.
But I am 100% in favor of making I_FREEING check explicitly.
>
> I also think we should add some assertions into i_io_list handling
> functions to complain if I_FREEING bit is set to catch these problems
> earlier which means to be also more careful in __mark_inode_dirty(). But
> this is for a separate cleanup.
>
> Honza
Sounds reasonable. Will look into that afterwards.
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
--
Slava
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fs: do not push freeing inode to b_dirty_time list
2022-11-14 17:43 ` Svyatoslav Feldsherov
@ 2022-11-14 19:21 ` Jan Kara
2022-11-14 21:21 ` [PATCH v2] fs: do not update freeing inode io_list Svyatoslav Feldsherov
0 siblings, 1 reply; 9+ messages in thread
From: Jan Kara @ 2022-11-14 19:21 UTC (permalink / raw)
To: Svyatoslav Feldsherov
Cc: Jan Kara, Alexander Viro, Lukas Czerner, Theodore Ts'o,
syzbot+6ba92bd00d5093f7e371, oferz, linux-fsdevel, linux-kernel
On Mon 14-11-22 19:43:54, Svyatoslav Feldsherov wrote:
> Thank you for looking into this!
>
> On Mon, Nov 14, 2022 at 12:46 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Sun 13-11-22 17:24:39, Svyatoslav Feldsherov wrote:
> > > After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode
> > > already has I_DIRTY_INODE") writeiback_single_inode can push inode with
> > > I_DIRTY_TIME set to b_dirty_time list. In case of freeing inode with
> > > I_DIRTY_TIME set this can happened after deletion of inode io_list at
> > > evict. Stack trace is following.
> > >
> > > evict
> > > fat_evict_inode
> > > fat_truncate_blocks
> > > fat_flush_inodes
> > > writeback_inode
> > > sync_inode_metadata
> > > writeback_single_inode
> > >
> > > This will lead to use after free in flusher thread.
> > >
> > > Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE")
> > > Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com
> > > Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com>
> >
> > Thanks for the analysis! I was scratching my head over this syzbot report
> > for a while and it didn't occur to me somebody could be calling
> > writeback_single_inode() from the .evict callback.
> >
> > Also what contributes to the problem is that FAT calls
> > sync_inode_metadata(inode, 0) so it is not marking this final flush as data
> > integrity sync and so we happily leave the I_DIRTY_TIME bit set.
> >
> > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > > index 443f83382b9b..31c93cbdb3fe 100644
> > > --- a/fs/fs-writeback.c
> > > +++ b/fs/fs-writeback.c
> > > @@ -1718,7 +1718,7 @@ static int writeback_single_inode(struct inode *inode,
> > > */
> > > if (!(inode->i_state & I_DIRTY_ALL))
> > > inode_cgwb_move_to_attached(inode, wb);
> > > - else if (!(inode->i_state & I_SYNC_QUEUED)) {
> > > + else if (!(inode->i_state & (I_SYNC_QUEUED | I_FREEING))) {
> > > if ((inode->i_state & I_DIRTY))
> > > redirty_tail_locked(inode, wb);
> > > else if (inode->i_state & I_DIRTY_TIME) {
> >
> > So even calling inode_cgwb_move_to_attached() is not safe when I_FREEING is
> > already set. So I belive the I_FREEING bit check needs to be before this
> > whole if block.
>
> Agree, let me move the I_FREEING check before this if block.
> The commit I am fixing didn't change this codepath, so I suspect there is an
> implicit invariant which keeps inode_cgwb_move_to_attached call safe.
> But I am 100% in favor of making I_FREEING check explicitly.
Actually, as I've looked into fat_evict_inode() I don't see anything making
that safe except for the fact that it may be more difficult for syzbot to
excercise the per-memcg writeback path...
> > I also think we should add some assertions into i_io_list handling
> > functions to complain if I_FREEING bit is set to catch these problems
> > earlier which means to be also more careful in __mark_inode_dirty(). But
> > this is for a separate cleanup.
>
> Sounds reasonable. Will look into that afterwards.
Thanks!
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] fs: do not update freeing inode io_list
2022-11-14 19:21 ` Jan Kara
@ 2022-11-14 21:21 ` Svyatoslav Feldsherov
2022-11-14 21:25 ` Randy Dunlap
2022-11-15 10:55 ` Jan Kara
0 siblings, 2 replies; 9+ messages in thread
From: Svyatoslav Feldsherov @ 2022-11-14 21:21 UTC (permalink / raw)
To: Alexander Viro, Lukas Czerner, Theodore Ts'o, Jan Kara
Cc: syzbot+6ba92bd00d5093f7e371, oferz, linux-fsdevel, linux-kernel,
Svyatoslav Feldsherov
After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode
already has I_DIRTY_INODE") writeiback_single_inode can push inode with
I_DIRTY_TIME set to b_dirty_time list. In case of freeing inode with
I_DIRTY_TIME set this can happened after deletion of inode io_list at
evict. Stack trace is following.
evict
fat_evict_inode
fat_truncate_blocks
fat_flush_inodes
writeback_inode
sync_inode_metadata(inode, sync=0)
writeback_single_inode(inode, wbc) <- wbc->sync_mode == WB_SYNC_NONE
This will lead to use after free in flusher thread.
Similar issue can be triggered if writeback_single_inode in the
stack trace update inode->io_list. Add explicit check to avoid it.
Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE")
Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com
Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com>
---
V1 -> V2:
- address review comments
- skip inode_cgwb_move_to_attached for freeing inode
fs/fs-writeback.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 443f83382b9b..c4aea096689c 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1712,18 +1712,26 @@ static int writeback_single_inode(struct inode *inode,
wb = inode_to_wb_and_lock_list(inode);
spin_lock(&inode->i_lock);
/*
- * If the inode is now fully clean, then it can be safely removed from
- * its writeback list (if any). Otherwise the flusher threads are
- * responsible for the writeback lists.
+ * If the inode is freeing, it's io_list shoudn't be updated
+ * as it can be finally deleted at this moment.
*/
- if (!(inode->i_state & I_DIRTY_ALL))
- inode_cgwb_move_to_attached(inode, wb);
- else if (!(inode->i_state & I_SYNC_QUEUED)) {
- if ((inode->i_state & I_DIRTY))
- redirty_tail_locked(inode, wb);
- else if (inode->i_state & I_DIRTY_TIME) {
- inode->dirtied_when = jiffies;
- inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
+ if (!(inode->i_state & I_FREEING)) {
+ /*
+ * If the inode is now fully clean, then it can be safely
+ * removed from its writeback list (if any). Otherwise the
+ * flusher threads are responsible for the writeback lists.
+ */
+ if (!(inode->i_state & I_DIRTY_ALL))
+ inode_cgwb_move_to_attached(inode, wb);
+ else if (!(inode->i_state & I_SYNC_QUEUED)) {
+ if ((inode->i_state & I_DIRTY))
+ redirty_tail_locked(inode, wb);
+ else if (inode->i_state & I_DIRTY_TIME) {
+ inode->dirtied_when = jiffies;
+ inode_io_list_move_locked(inode,
+ wb,
+ &wb->b_dirty_time);
+ }
}
}
--
2.38.1.431.g37b22c650d-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fs: do not update freeing inode io_list
2022-11-14 21:21 ` [PATCH v2] fs: do not update freeing inode io_list Svyatoslav Feldsherov
@ 2022-11-14 21:25 ` Randy Dunlap
2022-11-15 9:19 ` Svyatoslav Feldsherov
2022-11-15 10:55 ` Jan Kara
1 sibling, 1 reply; 9+ messages in thread
From: Randy Dunlap @ 2022-11-14 21:25 UTC (permalink / raw)
To: Svyatoslav Feldsherov, Alexander Viro, Lukas Czerner,
Theodore Ts'o, Jan Kara
Cc: syzbot+6ba92bd00d5093f7e371, oferz, linux-fsdevel, linux-kernel
Hi--
Please see a small nit below.
On 11/14/22 13:21, Svyatoslav Feldsherov wrote:
> After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode
> already has I_DIRTY_INODE") writeiback_single_inode can push inode with
> I_DIRTY_TIME set to b_dirty_time list. In case of freeing inode with
> I_DIRTY_TIME set this can happened after deletion of inode io_list at
> evict. Stack trace is following.
>
> evict
> fat_evict_inode
> fat_truncate_blocks
> fat_flush_inodes
> writeback_inode
> sync_inode_metadata(inode, sync=0)
> writeback_single_inode(inode, wbc) <- wbc->sync_mode == WB_SYNC_NONE
>
> This will lead to use after free in flusher thread.
>
> Similar issue can be triggered if writeback_single_inode in the
> stack trace update inode->io_list. Add explicit check to avoid it.
>
> Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE")
> Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com
> Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com>
> ---
> V1 -> V2:
> - address review comments
> - skip inode_cgwb_move_to_attached for freeing inode
>
> fs/fs-writeback.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 443f83382b9b..c4aea096689c 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1712,18 +1712,26 @@ static int writeback_single_inode(struct inode *inode,
> wb = inode_to_wb_and_lock_list(inode);
> spin_lock(&inode->i_lock);
> /*
> - * If the inode is now fully clean, then it can be safely removed from
> - * its writeback list (if any). Otherwise the flusher threads are
> - * responsible for the writeback lists.
> + * If the inode is freeing, it's io_list shoudn't be updated
its
> + * as it can be finally deleted at this moment.
> */
> - if (!(inode->i_state & I_DIRTY_ALL))
> - inode_cgwb_move_to_attached(inode, wb);
> - else if (!(inode->i_state & I_SYNC_QUEUED)) {
> - if ((inode->i_state & I_DIRTY))
> - redirty_tail_locked(inode, wb);
> - else if (inode->i_state & I_DIRTY_TIME) {
> - inode->dirtied_when = jiffies;
> - inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
> + if (!(inode->i_state & I_FREEING)) {
> + /*
> + * If the inode is now fully clean, then it can be safely
> + * removed from its writeback list (if any). Otherwise the
> + * flusher threads are responsible for the writeback lists.
> + */
> + if (!(inode->i_state & I_DIRTY_ALL))
> + inode_cgwb_move_to_attached(inode, wb);
> + else if (!(inode->i_state & I_SYNC_QUEUED)) {
> + if ((inode->i_state & I_DIRTY))
> + redirty_tail_locked(inode, wb);
> + else if (inode->i_state & I_DIRTY_TIME) {
> + inode->dirtied_when = jiffies;
> + inode_io_list_move_locked(inode,
> + wb,
> + &wb->b_dirty_time);
> + }
> }
> }
>
--
~Randy
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fs: do not update freeing inode io_list
2022-11-14 21:25 ` Randy Dunlap
@ 2022-11-15 9:19 ` Svyatoslav Feldsherov
0 siblings, 0 replies; 9+ messages in thread
From: Svyatoslav Feldsherov @ 2022-11-15 9:19 UTC (permalink / raw)
To: Randy Dunlap
Cc: Alexander Viro, Lukas Czerner, Theodore Ts'o, Jan Kara,
syzbot+6ba92bd00d5093f7e371, oferz, linux-fsdevel, linux-kernel
Thank you for noticing that!
I will send a fixed patch in 8-10 hours if no other comment will arrive.
On Mon, Nov 14, 2022 at 11:25 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> Hi--
>
> Please see a small nit below.
>
> On 11/14/22 13:21, Svyatoslav Feldsherov wrote:
> > After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode
> > already has I_DIRTY_INODE") writeiback_single_inode can push inode with
> > I_DIRTY_TIME set to b_dirty_time list. In case of freeing inode with
> > I_DIRTY_TIME set this can happened after deletion of inode io_list at
> > evict. Stack trace is following.
> >
> > evict
> > fat_evict_inode
> > fat_truncate_blocks
> > fat_flush_inodes
> > writeback_inode
> > sync_inode_metadata(inode, sync=0)
> > writeback_single_inode(inode, wbc) <- wbc->sync_mode == WB_SYNC_NONE
> >
> > This will lead to use after free in flusher thread.
> >
> > Similar issue can be triggered if writeback_single_inode in the
> > stack trace update inode->io_list. Add explicit check to avoid it.
> >
> > Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE")
> > Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com
> > Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com>
> > ---
> > V1 -> V2:
> > - address review comments
> > - skip inode_cgwb_move_to_attached for freeing inode
> >
> > fs/fs-writeback.c | 30 +++++++++++++++++++-----------
> > 1 file changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 443f83382b9b..c4aea096689c 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -1712,18 +1712,26 @@ static int writeback_single_inode(struct inode *inode,
> > wb = inode_to_wb_and_lock_list(inode);
> > spin_lock(&inode->i_lock);
> > /*
> > - * If the inode is now fully clean, then it can be safely removed from
> > - * its writeback list (if any). Otherwise the flusher threads are
> > - * responsible for the writeback lists.
> > + * If the inode is freeing, it's io_list shoudn't be updated
>
> its
>
> > + * as it can be finally deleted at this moment.
> > */
> > - if (!(inode->i_state & I_DIRTY_ALL))
> > - inode_cgwb_move_to_attached(inode, wb);
> > - else if (!(inode->i_state & I_SYNC_QUEUED)) {
> > - if ((inode->i_state & I_DIRTY))
> > - redirty_tail_locked(inode, wb);
> > - else if (inode->i_state & I_DIRTY_TIME) {
> > - inode->dirtied_when = jiffies;
> > - inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
> > + if (!(inode->i_state & I_FREEING)) {
> > + /*
> > + * If the inode is now fully clean, then it can be safely
> > + * removed from its writeback list (if any). Otherwise the
> > + * flusher threads are responsible for the writeback lists.
> > + */
> > + if (!(inode->i_state & I_DIRTY_ALL))
> > + inode_cgwb_move_to_attached(inode, wb);
> > + else if (!(inode->i_state & I_SYNC_QUEUED)) {
> > + if ((inode->i_state & I_DIRTY))
> > + redirty_tail_locked(inode, wb);
> > + else if (inode->i_state & I_DIRTY_TIME) {
> > + inode->dirtied_when = jiffies;
> > + inode_io_list_move_locked(inode,
> > + wb,
> > + &wb->b_dirty_time);
> > + }
> > }
> > }
> >
>
> --
> ~Randy
--
Slava
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fs: do not update freeing inode io_list
2022-11-14 21:21 ` [PATCH v2] fs: do not update freeing inode io_list Svyatoslav Feldsherov
2022-11-14 21:25 ` Randy Dunlap
@ 2022-11-15 10:55 ` Jan Kara
2022-11-15 20:28 ` Svyatoslav Feldsherov
1 sibling, 1 reply; 9+ messages in thread
From: Jan Kara @ 2022-11-15 10:55 UTC (permalink / raw)
To: Svyatoslav Feldsherov
Cc: Alexander Viro, Lukas Czerner, Theodore Ts'o, Jan Kara,
syzbot+6ba92bd00d5093f7e371, oferz, linux-fsdevel, linux-kernel
On Mon 14-11-22 21:21:55, Svyatoslav Feldsherov wrote:
> After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode
> already has I_DIRTY_INODE") writeiback_single_inode can push inode with
^^^ writeback
> I_DIRTY_TIME set to b_dirty_time list. In case of freeing inode with
> I_DIRTY_TIME set this can happened after deletion of inode io_list at
^^ happen ^^^ deletion of
inode *from i_io_list*
> evict. Stack trace is following.
>
> evict
> fat_evict_inode
> fat_truncate_blocks
> fat_flush_inodes
> writeback_inode
> sync_inode_metadata(inode, sync=0)
> writeback_single_inode(inode, wbc) <- wbc->sync_mode == WB_SYNC_NONE
>
> This will lead to use after free in flusher thread.
>
> Similar issue can be triggered if writeback_single_inode in the
> stack trace update inode->io_list. Add explicit check to avoid it.
^^ inode->i_io_list
> Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE")
> Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com
> Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com>
Besides these gramatical nits the patch looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Thanks!
Honza
> ---
> V1 -> V2:
> - address review comments
> - skip inode_cgwb_move_to_attached for freeing inode
>
> fs/fs-writeback.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 443f83382b9b..c4aea096689c 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1712,18 +1712,26 @@ static int writeback_single_inode(struct inode *inode,
> wb = inode_to_wb_and_lock_list(inode);
> spin_lock(&inode->i_lock);
> /*
> - * If the inode is now fully clean, then it can be safely removed from
> - * its writeback list (if any). Otherwise the flusher threads are
> - * responsible for the writeback lists.
> + * If the inode is freeing, it's io_list shoudn't be updated
> + * as it can be finally deleted at this moment.
> */
> - if (!(inode->i_state & I_DIRTY_ALL))
> - inode_cgwb_move_to_attached(inode, wb);
> - else if (!(inode->i_state & I_SYNC_QUEUED)) {
> - if ((inode->i_state & I_DIRTY))
> - redirty_tail_locked(inode, wb);
> - else if (inode->i_state & I_DIRTY_TIME) {
> - inode->dirtied_when = jiffies;
> - inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
> + if (!(inode->i_state & I_FREEING)) {
> + /*
> + * If the inode is now fully clean, then it can be safely
> + * removed from its writeback list (if any). Otherwise the
> + * flusher threads are responsible for the writeback lists.
> + */
> + if (!(inode->i_state & I_DIRTY_ALL))
> + inode_cgwb_move_to_attached(inode, wb);
> + else if (!(inode->i_state & I_SYNC_QUEUED)) {
> + if ((inode->i_state & I_DIRTY))
> + redirty_tail_locked(inode, wb);
> + else if (inode->i_state & I_DIRTY_TIME) {
> + inode->dirtied_when = jiffies;
> + inode_io_list_move_locked(inode,
> + wb,
> + &wb->b_dirty_time);
> + }
> }
> }
>
> --
> 2.38.1.431.g37b22c650d-goog
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fs: do not update freeing inode io_list
2022-11-15 10:55 ` Jan Kara
@ 2022-11-15 20:28 ` Svyatoslav Feldsherov
0 siblings, 0 replies; 9+ messages in thread
From: Svyatoslav Feldsherov @ 2022-11-15 20:28 UTC (permalink / raw)
To: Jan Kara
Cc: Alexander Viro, Lukas Czerner, Theodore Ts'o,
syzbot+6ba92bd00d5093f7e371, oferz, linux-fsdevel, linux-kernel
Thank you for the review.
I've sent v3 with proposed fixes.
Also tried to be more consistent and use i_io_list in comments and
commit message instead of io_list//io list.
On Tue, Nov 15, 2022 at 12:55 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 14-11-22 21:21:55, Svyatoslav Feldsherov wrote:
> > After commit cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode
> > already has I_DIRTY_INODE") writeiback_single_inode can push inode with
> ^^^ writeback
>
> > I_DIRTY_TIME set to b_dirty_time list. In case of freeing inode with
> > I_DIRTY_TIME set this can happened after deletion of inode io_list at
> ^^ happen ^^^ deletion of
> inode *from i_io_list*
>
> > evict. Stack trace is following.
> >
> > evict
> > fat_evict_inode
> > fat_truncate_blocks
> > fat_flush_inodes
> > writeback_inode
> > sync_inode_metadata(inode, sync=0)
> > writeback_single_inode(inode, wbc) <- wbc->sync_mode == WB_SYNC_NONE
> >
> > This will lead to use after free in flusher thread.
> >
> > Similar issue can be triggered if writeback_single_inode in the
> > stack trace update inode->io_list. Add explicit check to avoid it.
> ^^ inode->i_io_list
>
> > Fixes: cbfecb927f42 ("fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE")
> > Reported-by: syzbot+6ba92bd00d5093f7e371@syzkaller.appspotmail.com
> > Signed-off-by: Svyatoslav Feldsherov <feldsherov@google.com>
>
> Besides these gramatical nits the patch looks good to me. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> Thanks!
>
> Honza
>
> > ---
> > V1 -> V2:
> > - address review comments
> > - skip inode_cgwb_move_to_attached for freeing inode
> >
> > fs/fs-writeback.c | 30 +++++++++++++++++++-----------
> > 1 file changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 443f83382b9b..c4aea096689c 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -1712,18 +1712,26 @@ static int writeback_single_inode(struct inode *inode,
> > wb = inode_to_wb_and_lock_list(inode);
> > spin_lock(&inode->i_lock);
> > /*
> > - * If the inode is now fully clean, then it can be safely removed from
> > - * its writeback list (if any). Otherwise the flusher threads are
> > - * responsible for the writeback lists.
> > + * If the inode is freeing, it's io_list shoudn't be updated
> > + * as it can be finally deleted at this moment.
> > */
> > - if (!(inode->i_state & I_DIRTY_ALL))
> > - inode_cgwb_move_to_attached(inode, wb);
> > - else if (!(inode->i_state & I_SYNC_QUEUED)) {
> > - if ((inode->i_state & I_DIRTY))
> > - redirty_tail_locked(inode, wb);
> > - else if (inode->i_state & I_DIRTY_TIME) {
> > - inode->dirtied_when = jiffies;
> > - inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
> > + if (!(inode->i_state & I_FREEING)) {
> > + /*
> > + * If the inode is now fully clean, then it can be safely
> > + * removed from its writeback list (if any). Otherwise the
> > + * flusher threads are responsible for the writeback lists.
> > + */
> > + if (!(inode->i_state & I_DIRTY_ALL))
> > + inode_cgwb_move_to_attached(inode, wb);
> > + else if (!(inode->i_state & I_SYNC_QUEUED)) {
> > + if ((inode->i_state & I_DIRTY))
> > + redirty_tail_locked(inode, wb);
> > + else if (inode->i_state & I_DIRTY_TIME) {
> > + inode->dirtied_when = jiffies;
> > + inode_io_list_move_locked(inode,
> > + wb,
> > + &wb->b_dirty_time);
> > + }
> > }
> > }
> >
> > --
> > 2.38.1.431.g37b22c650d-goog
> >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
--
Slava
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-15 20:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-13 15:24 [PATCH] fs: do not push freeing inode to b_dirty_time list Svyatoslav Feldsherov
2022-11-14 10:46 ` Jan Kara
2022-11-14 17:43 ` Svyatoslav Feldsherov
2022-11-14 19:21 ` Jan Kara
2022-11-14 21:21 ` [PATCH v2] fs: do not update freeing inode io_list Svyatoslav Feldsherov
2022-11-14 21:25 ` Randy Dunlap
2022-11-15 9:19 ` Svyatoslav Feldsherov
2022-11-15 10:55 ` Jan Kara
2022-11-15 20:28 ` Svyatoslav Feldsherov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).