fs: fix iomap_bmap position calculation
diff mbox series

Message ID 78937e83-5665-11bf-14a6-7815221bf4d8@redhat.com
State New
Headers show
Series
  • fs: fix iomap_bmap position calculation
Related show

Commit Message

Eric Sandeen Aug. 2, 2018, 8:03 p.m. UTC
The position calculation in iomap_bmap() shifts bno the wrong way,
so we don't progress properly and end up re-mapping block zero
over and over, yielding an unchanging physical block range as the
logical block advances:

# filefrag -Be file
 ext:   logical_offset:     physical_offset: length:   expected: flags:
   0:      0..       0:      21..        21:      1:             merged
   1:      1..       1:      21..        21:      1:         22: merged
Discontinuity: Block 1 is at 21 (was 22)
   2:      2..       2:      21..        21:      1:         22: merged
Discontinuity: Block 2 is at 21 (was 22)
   3:      3..       3:      21..        21:      1:         22: merged

This breaks the FIBMAP interface for anyone using it (XFS), which
in turn breaks LILO, zipl, etc.

Bug-actually-spotted-by: Darrick J. Wong <darrick.wong@oracle.com> 
Fixes: 89eb1906a953 ("iomap: add an iomap-based bmap implementation")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---


--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Darrick J. Wong Aug. 2, 2018, 8:08 p.m. UTC | #1
On Thu, Aug 02, 2018 at 03:03:51PM -0500, Eric Sandeen wrote:
> The position calculation in iomap_bmap() shifts bno the wrong way,
> so we don't progress properly and end up re-mapping block zero
> over and over, yielding an unchanging physical block range as the
> logical block advances:
> 
> # filefrag -Be file
>  ext:   logical_offset:     physical_offset: length:   expected: flags:
>    0:      0..       0:      21..        21:      1:             merged
>    1:      1..       1:      21..        21:      1:         22: merged
> Discontinuity: Block 1 is at 21 (was 22)
>    2:      2..       2:      21..        21:      1:         22: merged
> Discontinuity: Block 2 is at 21 (was 22)
>    3:      3..       3:      21..        21:      1:         22: merged
> 
> This breaks the FIBMAP interface for anyone using it (XFS), which
> in turn breaks LILO, zipl, etc.
> 
> Bug-actually-spotted-by: Darrick J. Wong <darrick.wong@oracle.com> 
> Fixes: 89eb1906a953 ("iomap: add an iomap-based bmap implementation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 77397b5a96ef..0d0bd8845586 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -1443,7 +1443,7 @@ iomap_bmap(struct address_space *mapping, sector_t bno,
>  		const struct iomap_ops *ops)
>  {
>  	struct inode *inode = mapping->host;
> -	loff_t pos = bno >> inode->i_blkbits;
> +	loff_t pos = bno << inode->i_blkbits;
>  	unsigned blocksize = i_blocksize(inode);
>  
>  	if (filemap_write_and_wait(mapping))
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Aug. 2, 2018, 9:02 p.m. UTC | #2
On Thu, Aug 02, 2018 at 03:03:51PM -0500, Eric Sandeen wrote:
> The position calculation in iomap_bmap() shifts bno the wrong way,
> so we don't progress properly and end up re-mapping block zero
> over and over, yielding an unchanging physical block range as the
> logical block advances:
> 
> # filefrag -Be file
>  ext:   logical_offset:     physical_offset: length:   expected: flags:
>    0:      0..       0:      21..        21:      1:             merged
>    1:      1..       1:      21..        21:      1:         22: merged
> Discontinuity: Block 1 is at 21 (was 22)
>    2:      2..       2:      21..        21:      1:         22: merged
> Discontinuity: Block 2 is at 21 (was 22)
>    3:      3..       3:      21..        21:      1:         22: merged
> 
> This breaks the FIBMAP interface for anyone using it (XFS), which
> in turn breaks LILO, zipl, etc.
> 
> Bug-actually-spotted-by: Darrick J. Wong <darrick.wong@oracle.com> 
> Fixes: 89eb1906a953 ("iomap: add an iomap-based bmap implementation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

I coulda sworn I already RVBd this, but perhaps not?

Will test it out, anyway...

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 77397b5a96ef..0d0bd8845586 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -1443,7 +1443,7 @@ iomap_bmap(struct address_space *mapping, sector_t bno,
>  		const struct iomap_ops *ops)
>  {
>  	struct inode *inode = mapping->host;
> -	loff_t pos = bno >> inode->i_blkbits;
> +	loff_t pos = bno << inode->i_blkbits;
>  	unsigned blocksize = i_blocksize(inode);
>  
>  	if (filemap_write_and_wait(mapping))
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Aug. 3, 2018, 3:24 p.m. UTC | #3
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

I'm surprised nothing caught this earlier..
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox series

diff --git a/fs/iomap.c b/fs/iomap.c
index 77397b5a96ef..0d0bd8845586 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -1443,7 +1443,7 @@  iomap_bmap(struct address_space *mapping, sector_t bno,
 		const struct iomap_ops *ops)
 {
 	struct inode *inode = mapping->host;
-	loff_t pos = bno >> inode->i_blkbits;
+	loff_t pos = bno << inode->i_blkbits;
 	unsigned blocksize = i_blocksize(inode);
 
 	if (filemap_write_and_wait(mapping))