[PATCH] xfs: don't set bt_nr_sectors to a negative number

[PATCH] xfs: don't set bt_nr_sectors to a negative number

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

xfs_daddr_t is a signed type, which means that xfs_buf_map_verify is
using a signed comparison.  This causes problems if bt_nr_sectors is
never overridden (e.g. in the case of an xfbtree for rmap btree repairs)
because even daddr 0 can't pass the verifier test in that case.

Define an explicit max constant and set the initial bt_nr_sectors to a
positive value.

Found by xfs/422.

Cc: <stable@vger.kernel.org> # v6.18-rc1
Fixes: 42852fe57c6d2a ("xfs: track the number of blocks in each buftarg")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_buf.h |    1 +
 fs/xfs/xfs_buf.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 8fa7bdf59c9110..e25cd2a160f31c 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -22,6 +22,7 @@ extern struct kmem_cache *xfs_buf_cache;
  */
 struct xfs_buf;
 
+#define XFS_BUF_DADDR_MAX	((xfs_daddr_t) S64_MAX)
 #define XFS_BUF_DADDR_NULL	((xfs_daddr_t) (-1LL))
 
 #define XBF_READ	 (1u << 0) /* buffer intended for reading from device */
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 773d959965dc29..47edf3041631bb 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1751,7 +1751,7 @@ xfs_init_buftarg(
 	const char			*descr)
 {
 	/* The maximum size of the buftarg is only known once the sb is read. */
-	btp->bt_nr_sectors = (xfs_daddr_t)-1;
+	btp->bt_nr_sectors = XFS_BUF_DADDR_MAX;
 
 	/* Set up device logical sector size mask */
 	btp->bt_logical_sectorsize = logical_sectorsize;

Re: [PATCH] xfs: don't set bt_nr_sectors to a negative number

[Christoph Hellwig]

On Mon, Oct 13, 2025 at 09:33:10AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> xfs_daddr_t is a signed type, which means that xfs_buf_map_verify is
> using a signed comparison.  This causes problems if bt_nr_sectors is
> never overridden (e.g. in the case of an xfbtree for rmap btree repairs)
> because even daddr 0 can't pass the verifier test in that case.
> 
> Define an explicit max constant and set the initial bt_nr_sectors to a
> positive value.

Looks good:

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

Re: [PATCH] xfs: don't set bt_nr_sectors to a negative number

[Nirjhar Roy (IBM)]

On Mon, 2025-10-13 at 09:33 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> xfs_daddr_t is a signed type, which means that xfs_buf_map_verify is
> using a signed comparison.  This causes problems if bt_nr_sectors is
> never overridden (e.g. in the case of an xfbtree for rmap btree repairs)
> because even daddr 0 can't pass the verifier test in that case.
Okay so the check "if (map->bm_bn < 0 || map->bm_bn >= btp->bt_nr_sectors) {" will be true of the
default value of btp->bt_nr_sectors = -1 and the verifier will fail(incorrectly), right?
Why would we not want to override bt_nr_sectors? If there is device, then shouldn't it always have a
buffer target with a certain number of bt_nr_sectors?
--NR
> 
> Define an explicit max constant and set the initial bt_nr_sectors to a
> positive value.
> 
> Found by xfs/422.
> 
> Cc: <stable@vger.kernel.org> # v6.18-rc1
> Fixes: 42852fe57c6d2a ("xfs: track the number of blocks in each buftarg")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
>  fs/xfs/xfs_buf.h |    1 +
>  fs/xfs/xfs_buf.c |    2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 8fa7bdf59c9110..e25cd2a160f31c 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -22,6 +22,7 @@ extern struct kmem_cache *xfs_buf_cache;
>   */
>  struct xfs_buf;
>  
> +#define XFS_BUF_DADDR_MAX	((xfs_daddr_t) S64_MAX)
>  #define XFS_BUF_DADDR_NULL	((xfs_daddr_t) (-1LL))
>  
>  #define XBF_READ	 (1u << 0) /* buffer intended for reading from device */
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 773d959965dc29..47edf3041631bb 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1751,7 +1751,7 @@ xfs_init_buftarg(
>  	const char			*descr)
>  {
>  	/* The maximum size of the buftarg is only known once the sb is read. */
> -	btp->bt_nr_sectors = (xfs_daddr_t)-1;
> +	btp->bt_nr_sectors = XFS_BUF_DADDR_MAX;
>  
>  	/* Set up device logical sector size mask */
>  	btp->bt_logical_sectorsize = logical_sectorsize;

Re: [PATCH] xfs: don't set bt_nr_sectors to a negative number

[Darrick J. Wong]

On Tue, Oct 14, 2025 at 12:17:30PM +0530, Nirjhar Roy (IBM) wrote:
> On Mon, 2025-10-13 at 09:33 -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > xfs_daddr_t is a signed type, which means that xfs_buf_map_verify is
> > using a signed comparison.  This causes problems if bt_nr_sectors is
> > never overridden (e.g. in the case of an xfbtree for rmap btree repairs)
> > because even daddr 0 can't pass the verifier test in that case.
> Okay so the check "if (map->bm_bn < 0 || map->bm_bn >= btp->bt_nr_sectors) {" will be true of the
> default value of btp->bt_nr_sectors = -1 and the verifier will fail(incorrectly), right?
> Why would we not want to override bt_nr_sectors? If there is device, then shouldn't it always have a
> buffer target with a certain number of bt_nr_sectors?

Online repair creates tmpfs files in which to stage repairs, and uses
the xfbtree buftarg so that it can build a replacement rmapbt in a tmpfs
file.  I guess xfbtree should be setting bt_nr_sectors to (max pagecache
size / 512) but in practicality nobody should ever have a 16TB rmap
btree on 32-bit or an 8EB rmap btree on 64-bit.

--D

> --NR
> > 
> > Define an explicit max constant and set the initial bt_nr_sectors to a
> > positive value.
> > 
> > Found by xfs/422.
> > 
> > Cc: <stable@vger.kernel.org> # v6.18-rc1
> > Fixes: 42852fe57c6d2a ("xfs: track the number of blocks in each buftarg")
> > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_buf.h |    1 +
> >  fs/xfs/xfs_buf.c |    2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > index 8fa7bdf59c9110..e25cd2a160f31c 100644
> > --- a/fs/xfs/xfs_buf.h
> > +++ b/fs/xfs/xfs_buf.h
> > @@ -22,6 +22,7 @@ extern struct kmem_cache *xfs_buf_cache;
> >   */
> >  struct xfs_buf;
> >  
> > +#define XFS_BUF_DADDR_MAX	((xfs_daddr_t) S64_MAX)
> >  #define XFS_BUF_DADDR_NULL	((xfs_daddr_t) (-1LL))
> >  
> >  #define XBF_READ	 (1u << 0) /* buffer intended for reading from device */
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index 773d959965dc29..47edf3041631bb 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -1751,7 +1751,7 @@ xfs_init_buftarg(
> >  	const char			*descr)
> >  {
> >  	/* The maximum size of the buftarg is only known once the sb is read. */
> > -	btp->bt_nr_sectors = (xfs_daddr_t)-1;
> > +	btp->bt_nr_sectors = XFS_BUF_DADDR_MAX;
> >  
> >  	/* Set up device logical sector size mask */
> >  	btp->bt_logical_sectorsize = logical_sectorsize;
> 
> 

Re: [PATCH] xfs: don't set bt_nr_sectors to a negative number

[Nirjhar Roy (IBM)]

On 10/14/25 23:50, Darrick J. Wong wrote:
> On Tue, Oct 14, 2025 at 12:17:30PM +0530, Nirjhar Roy (IBM) wrote:
>> On Mon, 2025-10-13 at 09:33 -0700, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <djwong@kernel.org>
>>>
>>> xfs_daddr_t is a signed type, which means that xfs_buf_map_verify is
>>> using a signed comparison.  This causes problems if bt_nr_sectors is
>>> never overridden (e.g. in the case of an xfbtree for rmap btree repairs)
>>> because even daddr 0 can't pass the verifier test in that case.
>> Okay so the check "if (map->bm_bn < 0 || map->bm_bn >= btp->bt_nr_sectors) {" will be true of the
>> default value of btp->bt_nr_sectors = -1 and the verifier will fail(incorrectly), right?
>> Why would we not want to override bt_nr_sectors? If there is device, then shouldn't it always have a
>> buffer target with a certain number of bt_nr_sectors?
> Online repair creates tmpfs files in which to stage repairs, and uses
> the xfbtree buftarg so that it can build a replacement rmapbt in a tmpfs
> file.  I guess xfbtree should be setting bt_nr_sectors to (max pagecache
> size / 512) but in practicality nobody should ever have a 16TB rmap
> btree on 32-bit or an 8EB rmap btree on 64-bit.

Okay, that makes sense. Thank you for the explanation.

--NR

> --D
>
>> --NR
>>> Define an explicit max constant and set the initial bt_nr_sectors to a
>>> positive value.
>>>
>>> Found by xfs/422.
>>>
>>> Cc: <stable@vger.kernel.org> # v6.18-rc1
>>> Fixes: 42852fe57c6d2a ("xfs: track the number of blocks in each buftarg")
>>> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
>>> ---
>>>   fs/xfs/xfs_buf.h |    1 +
>>>   fs/xfs/xfs_buf.c |    2 +-
>>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
>>> index 8fa7bdf59c9110..e25cd2a160f31c 100644
>>> --- a/fs/xfs/xfs_buf.h
>>> +++ b/fs/xfs/xfs_buf.h
>>> @@ -22,6 +22,7 @@ extern struct kmem_cache *xfs_buf_cache;
>>>    */
>>>   struct xfs_buf;
>>>   
>>> +#define XFS_BUF_DADDR_MAX	((xfs_daddr_t) S64_MAX)
>>>   #define XFS_BUF_DADDR_NULL	((xfs_daddr_t) (-1LL))
>>>   
>>>   #define XBF_READ	 (1u << 0) /* buffer intended for reading from device */
>>> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
>>> index 773d959965dc29..47edf3041631bb 100644
>>> --- a/fs/xfs/xfs_buf.c
>>> +++ b/fs/xfs/xfs_buf.c
>>> @@ -1751,7 +1751,7 @@ xfs_init_buftarg(
>>>   	const char			*descr)
>>>   {
>>>   	/* The maximum size of the buftarg is only known once the sb is read. */
>>> -	btp->bt_nr_sectors = (xfs_daddr_t)-1;
>>> +	btp->bt_nr_sectors = XFS_BUF_DADDR_MAX;
>>>   
>>>   	/* Set up device logical sector size mask */
>>>   	btp->bt_logical_sectorsize = logical_sectorsize;
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore