| Message ID | 20240920122621.215397-1-sunjunchao2870@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] xfs: Do not unshare ranges beyond EOF | expand |
On Fri, Sep 20, 2024 at 08:26:21PM +0800, Julian Sun wrote: > Attempting to unshare extents beyond EOF will trigger > the need zeroing case, which in turn triggers a warning. > Therefore, let's skip the unshare process if extents are > beyond EOF. > > Reported-and-tested-by: syzbot+296b1c84b9cbf306e5a0@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=296b1c84b9cbf306e5a0 > Fixes: 32a38a499104 ("iomap: use write_begin to read pages to unshare") > Inspired-by: Dave Chinner <david@fromorbit.com> > Signed-off-by: Julian Sun <sunjunchao2870@gmail.com> > --- > fs/xfs/xfs_reflink.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c > index 6fde6ec8092f..65509ff6aba0 100644 > --- a/fs/xfs/xfs_reflink.c > +++ b/fs/xfs/xfs_reflink.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2016 Oracle. All Rights Reserved. > * Author: Darrick J. Wong <darrick.wong@oracle.com> > */ > +#include "linux/fs.h" This really should not be needed (and is the wrong way to include non-local headers anyway). > #include "xfs.h" > #include "xfs_fs.h" > #include "xfs_shared.h" > @@ -1669,6 +1670,9 @@ xfs_reflink_unshare( > > if (!xfs_is_reflink_inode(ip)) > return 0; > + /* don't try to unshare any ranges beyond EOF. */ > + if (offset + len > i_size_read(inode)) > + len = i_size_read(inode) - offset; So i_size is a byte granularity value, but later on iomap_file_unshare operates on blocks. If you reduce the value like this here this means we can't ever unshare the last block of a file if the file size is not block aligned, which feels odd.
Christoph Hellwig <hch@infradead.org> 于2024年9月20日周五 22:23写道： > > On Fri, Sep 20, 2024 at 08:26:21PM +0800, Julian Sun wrote: > > Attempting to unshare extents beyond EOF will trigger > > the need zeroing case, which in turn triggers a warning. > > Therefore, let's skip the unshare process if extents are > > beyond EOF. > > > > Reported-and-tested-by: syzbot+296b1c84b9cbf306e5a0@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=296b1c84b9cbf306e5a0 > > Fixes: 32a38a499104 ("iomap: use write_begin to read pages to unshare") > > Inspired-by: Dave Chinner <david@fromorbit.com> > > Signed-off-by: Julian Sun <sunjunchao2870@gmail.com> > > --- > > fs/xfs/xfs_reflink.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c > > index 6fde6ec8092f..65509ff6aba0 100644 > > --- a/fs/xfs/xfs_reflink.c > > +++ b/fs/xfs/xfs_reflink.c > > @@ -3,6 +3,7 @@ > > * Copyright (C) 2016 Oracle. All Rights Reserved. > > * Author: Darrick J. Wong <darrick.wong@oracle.com> > > */ > > +#include "linux/fs.h" > > This really should not be needed (and is the wrong way to include > non-local headers anyway). Yes, it was added automatically by vscode... I will recheck the patch before sending it. > > > #include "xfs.h" > > #include "xfs_fs.h" > > #include "xfs_shared.h" > > @@ -1669,6 +1670,9 @@ xfs_reflink_unshare( > > > > if (!xfs_is_reflink_inode(ip)) > > return 0; > > + /* don't try to unshare any ranges beyond EOF. */ > > + if (offset + len > i_size_read(inode)) > > + len = i_size_read(inode) - offset; > > So i_size is a byte granularity value, but later on iomap_file_unshare > operates on blocks. If you reduce the value like this here this means > we can't ever unshare the last block of a file if the file size is > not block aligned, which feels odd. Got it, will fix it in patch v2. Thanks for your review and explantion. > Thanks,
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c index 6fde6ec8092f..65509ff6aba0 100644 --- a/fs/xfs/xfs_reflink.c +++ b/fs/xfs/xfs_reflink.c @@ -3,6 +3,7 @@ * Copyright (C) 2016 Oracle. All Rights Reserved. * Author: Darrick J. Wong <darrick.wong@oracle.com> */ +#include "linux/fs.h" #include "xfs.h" #include "xfs_fs.h" #include "xfs_shared.h" @@ -1669,6 +1670,9 @@ xfs_reflink_unshare( if (!xfs_is_reflink_inode(ip)) return 0; + /* don't try to unshare any ranges beyond EOF. */ + if (offset + len > i_size_read(inode)) + len = i_size_read(inode) - offset; trace_xfs_reflink_unshare(ip, offset, len);
Attempting to unshare extents beyond EOF will trigger the need zeroing case, which in turn triggers a warning. Therefore, let's skip the unshare process if extents are beyond EOF. Reported-and-tested-by: syzbot+296b1c84b9cbf306e5a0@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=296b1c84b9cbf306e5a0 Fixes: 32a38a499104 ("iomap: use write_begin to read pages to unshare") Inspired-by: Dave Chinner <david@fromorbit.com> Signed-off-by: Julian Sun <sunjunchao2870@gmail.com> --- fs/xfs/xfs_reflink.c | 4 ++++ 1 file changed, 4 insertions(+)