| Message ID | 20180312152156.14034-1-guaneryu@gmail.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2,1/2] ext4: protect i_disksize update by i_data_sem in direct write path | expand |
On Mon, Mar 12, 2018 at 11:21:55PM +0800, Eryu Guan wrote: > i_disksize update should be protected by i_data_sem, by either taking > the lock explicitly or by using ext4_update_i_disksize() helper. But the > i_disksize updates in ext4_direct_IO_write() are not protected at all, > which may be racing with i_disksize updates in writeback path in > delalloc buffer write path. > > This is found by code inspection, and I didn't hit any i_disksize > corruption due to this bug. Thanks to Jan Kara for catching this bug and > suggesting the fix! > > Reported-by: Jan Kara <jack@suse.cz> > Suggested-by: Jan Kara <jack@suse.cz> > Signed-off-by: Eryu Guan <guaneryu@gmail.com> Applied, thanks. - Ted
On Mon 12-03-18 23:21:55, Eryu Guan wrote: > i_disksize update should be protected by i_data_sem, by either taking > the lock explicitly or by using ext4_update_i_disksize() helper. But the > i_disksize updates in ext4_direct_IO_write() are not protected at all, > which may be racing with i_disksize updates in writeback path in > delalloc buffer write path. > > This is found by code inspection, and I didn't hit any i_disksize > corruption due to this bug. Thanks to Jan Kara for catching this bug and > suggesting the fix! > > Reported-by: Jan Kara <jack@suse.cz> > Suggested-by: Jan Kara <jack@suse.cz> > Signed-off-by: Eryu Guan <guaneryu@gmail.com> Looks good. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > > There's no v1 version of this patch. > > It's a bit unfortunate that I have to remove the "ei" definition in this > patch and reintroduce it in patch 2/2, otherwise I got transient > "defined but not used" build warning with only this patch applied. > > fs/ext4/inode.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index c94780075b04..bff44b4a0783 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3658,7 +3658,6 @@ static ssize_t ext4_direct_IO_write(struct kiocb *iocb, struct iov_iter *iter) > { > struct file *file = iocb->ki_filp; > struct inode *inode = file->f_mapping->host; > - struct ext4_inode_info *ei = EXT4_I(inode); > ssize_t ret; > loff_t offset = iocb->ki_pos; > size_t count = iov_iter_count(iter); > @@ -3682,7 +3681,7 @@ static ssize_t ext4_direct_IO_write(struct kiocb *iocb, struct iov_iter *iter) > goto out; > } > orphan = 1; > - ei->i_disksize = inode->i_size; > + ext4_update_i_disksize(inode, inode->i_size); > ext4_journal_stop(handle); > } > > @@ -3790,7 +3789,7 @@ static ssize_t ext4_direct_IO_write(struct kiocb *iocb, struct iov_iter *iter) > if (ret > 0) { > loff_t end = offset + ret; > if (end > inode->i_size) { > - ei->i_disksize = end; > + ext4_update_i_disksize(inode, end); > i_size_write(inode, end); > /* > * We're going to return a positive `ret' > -- > 2.14.3 >
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index c94780075b04..bff44b4a0783 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3658,7 +3658,6 @@ static ssize_t ext4_direct_IO_write(struct kiocb *iocb, struct iov_iter *iter) { struct file *file = iocb->ki_filp; struct inode *inode = file->f_mapping->host; - struct ext4_inode_info *ei = EXT4_I(inode); ssize_t ret; loff_t offset = iocb->ki_pos; size_t count = iov_iter_count(iter); @@ -3682,7 +3681,7 @@ static ssize_t ext4_direct_IO_write(struct kiocb *iocb, struct iov_iter *iter) goto out; } orphan = 1; - ei->i_disksize = inode->i_size; + ext4_update_i_disksize(inode, inode->i_size); ext4_journal_stop(handle); } @@ -3790,7 +3789,7 @@ static ssize_t ext4_direct_IO_write(struct kiocb *iocb, struct iov_iter *iter) if (ret > 0) { loff_t end = offset + ret; if (end > inode->i_size) { - ei->i_disksize = end; + ext4_update_i_disksize(inode, end); i_size_write(inode, end); /* * We're going to return a positive `ret'
i_disksize update should be protected by i_data_sem, by either taking the lock explicitly or by using ext4_update_i_disksize() helper. But the i_disksize updates in ext4_direct_IO_write() are not protected at all, which may be racing with i_disksize updates in writeback path in delalloc buffer write path. This is found by code inspection, and I didn't hit any i_disksize corruption due to this bug. Thanks to Jan Kara for catching this bug and suggesting the fix! Reported-by: Jan Kara <jack@suse.cz> Suggested-by: Jan Kara <jack@suse.cz> Signed-off-by: Eryu Guan <guaneryu@gmail.com> --- There's no v1 version of this patch. It's a bit unfortunate that I have to remove the "ei" definition in this patch and reintroduce it in patch 2/2, otherwise I got transient "defined but not used" build warning with only this patch applied. fs/ext4/inode.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)