[PATCH 1/3] xfs: Do not unshare ranges beyond EOF

[PATCH 1/3] xfs: Do not unshare ranges beyond EOF

[Julian Sun]

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"
 #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);
 
-- 
2.39.2

Re: [PATCH 1/3] xfs: Do not unshare ranges beyond EOF

[Christoph Hellwig]

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.

Re: [PATCH 1/3] xfs: Do not unshare ranges beyond EOF

[Julian Sun]

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,
-- 
Julian Sun <sunjunchao2870@gmail.com>