| Message ID | 20200421085445.5731-2-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] fs: Avoid leaving freed inode on dirty list | expand |
hi, > evict() can race with writeback_sb_inodes() and so > list_empty(&inode->i_io_list) check can race with list_move() from > redirty_tail() possibly resulting in list_empty() returning false and ^^^^^^^^^^^^^^^ returning true? if (!list_empty(&inode->i_io_list)) inode_io_list_del(inode); so "!list_empty(&inode->i_io_list)" returns false, and will not remove inode for wb->b_dirty list. > thus we end up leaving freed inode in wb->b_dirty list leading to > use-after-free issues. > > Fix the problem by using list_empty_careful() check and add assert that > inode's i_io_list is empty in clear_inode() to catch the problem earlier > in the future. From list_empty_careful()'s comments, using list_empty_careful() without synchronization can only be safe if the only activity that can happen to the list entry is list_del_init(), but list_move() does not use list_del_init(). static inline void list_move(struct list_head *list, struct list_head *head) { __list_del_entry(list); list_add(list, head); } So I wonder whether list_empty(&inode->i_io_list) check in evict() can race with list_move() from redirty_tail()? Regards, Xiaoguang Wang > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/inode.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 93d9252a00ab..a73c8a7aa71a 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -534,6 +534,7 @@ void clear_inode(struct inode *inode) > BUG_ON(!(inode->i_state & I_FREEING)); > BUG_ON(inode->i_state & I_CLEAR); > BUG_ON(!list_empty(&inode->i_wb_list)); > + BUG_ON(!list_empty(&inode->i_io_list)); > /* don't need i_lock here, no concurrent mods to i_state */ > inode->i_state = I_FREEING | I_CLEAR; > } > @@ -559,7 +560,13 @@ static void evict(struct inode *inode) > BUG_ON(!(inode->i_state & I_FREEING)); > BUG_ON(!list_empty(&inode->i_lru)); > > - if (!list_empty(&inode->i_io_list)) > + /* > + * We are the only holder of the inode so it cannot be marked dirty. > + * Flusher thread won't start new writeback but there can be still e.g. > + * redirty_tail() running from writeback_sb_inodes(). So we have to be > + * careful to remove inode from dirty/io list in all the cases. > + */ > + if (!list_empty_careful(&inode->i_io_list)) > inode_io_list_del(inode); > > inode_sb_list_del(inode); >
On Sat 25-04-20 14:42:03, Xiaoguang Wang wrote: > hi, > > > evict() can race with writeback_sb_inodes() and so > > list_empty(&inode->i_io_list) check can race with list_move() from > > redirty_tail() possibly resulting in list_empty() returning false and > ^^^^^^^^^^^^^^^ > returning true? > if (!list_empty(&inode->i_io_list)) > inode_io_list_del(inode); > so "!list_empty(&inode->i_io_list)" returns false, and will not remove > inode for wb->b_dirty list. Yeah, right. I'll fix the mistake in the changelog. Thanks for noticing. > > thus we end up leaving freed inode in wb->b_dirty list leading to > > use-after-free issues. > > > > Fix the problem by using list_empty_careful() check and add assert that > > inode's i_io_list is empty in clear_inode() to catch the problem earlier > > in the future. > From list_empty_careful()'s comments, using list_empty_careful() without > synchronization can only be safe if the only activity that can happen to the > list entry is list_del_init(), but list_move() does not use list_del_init(). > > static inline void list_move(struct list_head *list, struct list_head *head) > { > __list_del_entry(list); > list_add(list, head); > } > > So I wonder whether list_empty(&inode->i_io_list) check in evict() can > race with list_move() from redirty_tail()? list_empty() check can race with list_move() but I don't think the outcome of the racy check can ever be that the list is empty... Thinking about it again, I'm not sure how even the list_empty() check could give false positive because during the list_move() sequence, I don't think head->next == head is ever true. So maybe this patch isn't needed at all (except for the added BUG_ON() which is useful). Honza > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/inode.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/fs/inode.c b/fs/inode.c > > index 93d9252a00ab..a73c8a7aa71a 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -534,6 +534,7 @@ void clear_inode(struct inode *inode) > > BUG_ON(!(inode->i_state & I_FREEING)); > > BUG_ON(inode->i_state & I_CLEAR); > > BUG_ON(!list_empty(&inode->i_wb_list)); > > + BUG_ON(!list_empty(&inode->i_io_list)); > > /* don't need i_lock here, no concurrent mods to i_state */ > > inode->i_state = I_FREEING | I_CLEAR; > > } > > @@ -559,7 +560,13 @@ static void evict(struct inode *inode) > > BUG_ON(!(inode->i_state & I_FREEING)); > > BUG_ON(!list_empty(&inode->i_lru)); > > - if (!list_empty(&inode->i_io_list)) > > + /* > > + * We are the only holder of the inode so it cannot be marked dirty. > > + * Flusher thread won't start new writeback but there can be still e.g. > > + * redirty_tail() running from writeback_sb_inodes(). So we have to be > > + * careful to remove inode from dirty/io list in all the cases. > > + */ > > + if (!list_empty_careful(&inode->i_io_list)) > > inode_io_list_del(inode); > > inode_sb_list_del(inode); > >
diff --git a/fs/inode.c b/fs/inode.c index 93d9252a00ab..a73c8a7aa71a 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -534,6 +534,7 @@ void clear_inode(struct inode *inode) BUG_ON(!(inode->i_state & I_FREEING)); BUG_ON(inode->i_state & I_CLEAR); BUG_ON(!list_empty(&inode->i_wb_list)); + BUG_ON(!list_empty(&inode->i_io_list)); /* don't need i_lock here, no concurrent mods to i_state */ inode->i_state = I_FREEING | I_CLEAR; } @@ -559,7 +560,13 @@ static void evict(struct inode *inode) BUG_ON(!(inode->i_state & I_FREEING)); BUG_ON(!list_empty(&inode->i_lru)); - if (!list_empty(&inode->i_io_list)) + /* + * We are the only holder of the inode so it cannot be marked dirty. + * Flusher thread won't start new writeback but there can be still e.g. + * redirty_tail() running from writeback_sb_inodes(). So we have to be + * careful to remove inode from dirty/io list in all the cases. + */ + if (!list_empty_careful(&inode->i_io_list)) inode_io_list_del(inode); inode_sb_list_del(inode);
evict() can race with writeback_sb_inodes() and so list_empty(&inode->i_io_list) check can race with list_move() from redirty_tail() possibly resulting in list_empty() returning false and thus we end up leaving freed inode in wb->b_dirty list leading to use-after-free issues. Fix the problem by using list_empty_careful() check and add assert that inode's i_io_list is empty in clear_inode() to catch the problem earlier in the future. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/inode.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)