[PATCH v2] xfs: do not unshare any blocks beyond eof

[PATCH v2] xfs: do not unshare any blocks 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 blocks are
beyond EOF.

This patch passed the xfstests using './check -g quick', without
causing any additional failure

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_iomap.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 72c981e3dc92..81a0514b8652 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -976,6 +976,7 @@ xfs_buffered_write_iomap_begin(
 	int			error = 0;
 	unsigned int		lockmode = XFS_ILOCK_EXCL;
 	u64			seq;
+	xfs_fileoff_t eof_fsb;
 
 	if (xfs_is_shutdown(mp))
 		return -EIO;
@@ -1016,6 +1017,13 @@ xfs_buffered_write_iomap_begin(
 	if (eof)
 		imap.br_startoff = end_fsb; /* fake hole until the end */
 
+	/* Don't try to unshare any blocks beyond EOF. */
+	eof_fsb = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
+	if (flags & IOMAP_UNSHARE && end_fsb > eof_fsb) {
+		xfs_trim_extent(&imap, offset_fsb, eof_fsb - offset_fsb);
+		end_fsb = eof_fsb;
+	}
+
 	/* We never need to allocate blocks for zeroing or unsharing a hole. */
 	if ((flags & (IOMAP_UNSHARE | IOMAP_ZERO)) &&
 	    imap.br_startoff > offset_fsb) {
@@ -1030,7 +1038,6 @@ xfs_buffered_write_iomap_begin(
 	 */
 	if ((flags & IOMAP_ZERO) && imap.br_startoff <= offset_fsb &&
 	    isnullstartblock(imap.br_startblock)) {
-		xfs_fileoff_t eof_fsb = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
 
 		if (offset_fsb >= eof_fsb)
 			goto convert_delay;
-- 
2.39.2

Re: [PATCH v2] xfs: do not unshare any blocks beyond eof

[Darrick J. Wong]

[cc linux-xfs]

On Fri, Sep 27, 2024 at 02:53:44PM +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 blocks are
> beyond EOF.
> 
> This patch passed the xfstests using './check -g quick', without
> causing any additional failure
> 
> 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_iomap.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 72c981e3dc92..81a0514b8652 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -976,6 +976,7 @@ xfs_buffered_write_iomap_begin(

I'm unsure about why this correction is in
xfs_buffered_write_iomap_begin.  If extent size hints are enabled, this
function delegates to xfs_direct_write_iomap_begin, which means that
this isn't a complete fix.

Shouldn't it suffice to clamp offset/len in xfs_reflink_unshare?

--D

>  	int			error = 0;
>  	unsigned int		lockmode = XFS_ILOCK_EXCL;
>  	u64			seq;
> +	xfs_fileoff_t eof_fsb;
>  
>  	if (xfs_is_shutdown(mp))
>  		return -EIO;
> @@ -1016,6 +1017,13 @@ xfs_buffered_write_iomap_begin(
>  	if (eof)
>  		imap.br_startoff = end_fsb; /* fake hole until the end */
>  
> +	/* Don't try to unshare any blocks beyond EOF. */
> +	eof_fsb = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
> +	if (flags & IOMAP_UNSHARE && end_fsb > eof_fsb) {
> +		xfs_trim_extent(&imap, offset_fsb, eof_fsb - offset_fsb);
> +		end_fsb = eof_fsb;
> +	}
> +
>  	/* We never need to allocate blocks for zeroing or unsharing a hole. */
>  	if ((flags & (IOMAP_UNSHARE | IOMAP_ZERO)) &&
>  	    imap.br_startoff > offset_fsb) {
> @@ -1030,7 +1038,6 @@ xfs_buffered_write_iomap_begin(
>  	 */
>  	if ((flags & IOMAP_ZERO) && imap.br_startoff <= offset_fsb &&
>  	    isnullstartblock(imap.br_startblock)) {
> -		xfs_fileoff_t eof_fsb = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
>  
>  		if (offset_fsb >= eof_fsb)
>  			goto convert_delay;
> -- 
> 2.39.2
> 

Re: [PATCH v2] xfs: do not unshare any blocks beyond eof

[Julian Sun]

Hi, Darrick. Thanks for your review and comments.

Darrick J. Wong <djwong@kernel.org> 于2024年9月28日周六 12:44写道：
>
> [cc linux-xfs]

Sorry for missing the CC to the XFS mailing list...
>
> On Fri, Sep 27, 2024 at 02:53:44PM +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 blocks are
> > beyond EOF.
> >
> > This patch passed the xfstests using './check -g quick', without
> > causing any additional failure
> >
> > 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_iomap.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > index 72c981e3dc92..81a0514b8652 100644
> > --- a/fs/xfs/xfs_iomap.c
> > +++ b/fs/xfs/xfs_iomap.c
> > @@ -976,6 +976,7 @@ xfs_buffered_write_iomap_begin(
>
> I'm unsure about why this correction is in
> xfs_buffered_write_iomap_begin.  If extent size hints are enabled, this
> function delegates to xfs_direct_write_iomap_begin, which means that
> this isn't a complete fix.

My understanding is that xfs_get_extsz_hint() return a nonzero value
only involving realtime dev/inodes, right?
If that is true, and reflink is not supproted with realtime devices,
then fallocate unshare will directly return 0 within
xfs_reflink_unshare() for rt dev/inodes. Furthermore,
xfs_get_extsz_hint() will always return zero for non-rt dev/inodes,
which means that fallocate unshare will never enter
xfs_direct_write_iomap_begin().

I reviewed the code for xfs_direct_write_iomap_begin(), and there is
no handling of IOMAP_UNSHARE, just as xfs_buffered_write_iomap_begin()
has done. Perhaps this is the reason?
>
> Shouldn't it suffice to clamp offset/len in xfs_reflink_unshare?

Possible makes sense. However, as Christoph mentioned here[1] where I
did this in xfs_reflink_unshare(), we should consider the last block
if file size is not block aligned. IMO, it's more elegant to do it in
iomap_begin() callback...

If there's any misunderstanding or if I missed something, please let
me know, thanks!

[1]: https://lore.kernel.org/linux-xfs/Zu2FWuonuO97Q6V8@infradead.org/
>
> --D
>
> >       int                     error = 0;
> >       unsigned int            lockmode = XFS_ILOCK_EXCL;
> >       u64                     seq;
> > +     xfs_fileoff_t eof_fsb;
> >
> >       if (xfs_is_shutdown(mp))
> >               return -EIO;
> > @@ -1016,6 +1017,13 @@ xfs_buffered_write_iomap_begin(
> >       if (eof)
> >               imap.br_startoff = end_fsb; /* fake hole until the end */
> >
> > +     /* Don't try to unshare any blocks beyond EOF. */
> > +     eof_fsb = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
> > +     if (flags & IOMAP_UNSHARE && end_fsb > eof_fsb) {
> > +             xfs_trim_extent(&imap, offset_fsb, eof_fsb - offset_fsb);
> > +             end_fsb = eof_fsb;
> > +     }
> > +
> >       /* We never need to allocate blocks for zeroing or unsharing a hole. */
> >       if ((flags & (IOMAP_UNSHARE | IOMAP_ZERO)) &&
> >           imap.br_startoff > offset_fsb) {
> > @@ -1030,7 +1038,6 @@ xfs_buffered_write_iomap_begin(
> >        */
> >       if ((flags & IOMAP_ZERO) && imap.br_startoff <= offset_fsb &&
> >           isnullstartblock(imap.br_startblock)) {
> > -             xfs_fileoff_t eof_fsb = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
> >
> >               if (offset_fsb >= eof_fsb)
> >                       goto convert_delay;
> > --
> > 2.39.2
> >


Thanks,
-- 
Julian Sun <sunjunchao2870@gmail.com>

Re: [PATCH v2] xfs: do not unshare any blocks beyond eof

[Dave Chinner]

On Fri, Sep 27, 2024 at 02:53:44PM +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 blocks are
> beyond EOF.
> 
> This patch passed the xfstests using './check -g quick', without
> causing any additional failure
> 
> 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_iomap.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 72c981e3dc92..81a0514b8652 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -976,6 +976,7 @@ xfs_buffered_write_iomap_begin(
>  	int			error = 0;
>  	unsigned int		lockmode = XFS_ILOCK_EXCL;
>  	u64			seq;
> +	xfs_fileoff_t eof_fsb;
>  
>  	if (xfs_is_shutdown(mp))
>  		return -EIO;
> @@ -1016,6 +1017,13 @@ xfs_buffered_write_iomap_begin(
>  	if (eof)
>  		imap.br_startoff = end_fsb; /* fake hole until the end */
>  
> +	/* Don't try to unshare any blocks beyond EOF. */
> +	eof_fsb = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
> +	if (flags & IOMAP_UNSHARE && end_fsb > eof_fsb) {
> +		xfs_trim_extent(&imap, offset_fsb, eof_fsb - offset_fsb);
> +		end_fsb = eof_fsb;
> +	}

No. The EOF check/limiting needs to be at a higher level - probably
in xfs_falloc_unshare_range() because the existing
xfs_falloc_newsize() call implements the wrong file size growing
semantics for UNSHARE...

-Dave.
-- 
Dave Chinner
david@fromorbit.com