[PATCH] xfs: fix attr leaf header freemap.size underflow

[PATCH] xfs: fix attr leaf header freemap.size underflow

[Brian Foster]

The leaf format xattr addition helper xfs_attr3_leaf_add_work()
adjusts the block freemap in a couple places. The first update drops
the size of the freemap that the caller had already selected to
place the xattr name/value data. Before the function returns, it
also checks whether the entries array has encroached on a freemap
range by virtue of the new entry addition. This is necessary because
the entries array grows from the start of the block (but end of the
block header) towards the end of the block while the name/value data
grows from the end of the block in the opposite direction. If the
associated freemap is already empty, however, size is zero and the
subtraction underflows the field and causes corruption.

This is reproduced rarely by generic/070. The observed behavior is
that a smaller sized freemap is aligned to the end of the entries
list, several subsequent xattr additions land in larger freemaps and
the entries list expands into the smaller freemap until it is fully
consumed and then underflows. Note that it is not otherwise a
corruption for the entries array to consume an empty freemap because
the nameval list (i.e. the firstused pointer in the xattr header)
starts beyond the end of the corrupted freemap.

Update the freemap size modification to account for the fact that
the freemap entry can be empty and thus stale.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 85ec5945d29f..86155260d8b9 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -1510,7 +1510,9 @@ xfs_attr3_leaf_add_work(
 	for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
 		if (ichdr->freemap[i].base == tmp) {
 			ichdr->freemap[i].base += sizeof(xfs_attr_leaf_entry_t);
-			ichdr->freemap[i].size -= sizeof(xfs_attr_leaf_entry_t);
+			ichdr->freemap[i].size -=
+				min_t(uint16_t, ichdr->freemap[i].size,
+						sizeof(xfs_attr_leaf_entry_t));
 		}
 	}
 	ichdr->usedbytes += xfs_attr_leaf_entsize(leaf, args->index);
-- 
2.20.1

Re: [PATCH] xfs: fix attr leaf header freemap.size underflow

[Darrick J. Wong]

On Fri, Nov 15, 2019 at 06:41:37AM -0500, Brian Foster wrote:
> The leaf format xattr addition helper xfs_attr3_leaf_add_work()
> adjusts the block freemap in a couple places. The first update drops
> the size of the freemap that the caller had already selected to
> place the xattr name/value data. Before the function returns, it
> also checks whether the entries array has encroached on a freemap
> range by virtue of the new entry addition. This is necessary because
> the entries array grows from the start of the block (but end of the
> block header) towards the end of the block while the name/value data
> grows from the end of the block in the opposite direction. If the
> associated freemap is already empty, however, size is zero and the
> subtraction underflows the field and causes corruption.
> 
> This is reproduced rarely by generic/070. The observed behavior is
> that a smaller sized freemap is aligned to the end of the entries
> list, several subsequent xattr additions land in larger freemaps and
> the entries list expands into the smaller freemap until it is fully
> consumed and then underflows. Note that it is not otherwise a
> corruption for the entries array to consume an empty freemap because
> the nameval list (i.e. the firstused pointer in the xattr header)
> starts beyond the end of the corrupted freemap.
> 
> Update the freemap size modification to account for the fact that
> the freemap entry can be empty and thus stale.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Hm.  freemap.size == 0 means the freemap entry is stale and therefore
anything looking for free space in the leaf will ignore the entry, right?

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

(Urk, there are still a lot of ASSERT-on-metadata in the dir/attr
code...)

--D

> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 85ec5945d29f..86155260d8b9 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1510,7 +1510,9 @@ xfs_attr3_leaf_add_work(
>  	for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
>  		if (ichdr->freemap[i].base == tmp) {
>  			ichdr->freemap[i].base += sizeof(xfs_attr_leaf_entry_t);
> -			ichdr->freemap[i].size -= sizeof(xfs_attr_leaf_entry_t);
> +			ichdr->freemap[i].size -=
> +				min_t(uint16_t, ichdr->freemap[i].size,
> +						sizeof(xfs_attr_leaf_entry_t));
>  		}
>  	}
>  	ichdr->usedbytes += xfs_attr_leaf_entsize(leaf, args->index);
> -- 
> 2.20.1
> 

Re: [PATCH] xfs: fix attr leaf header freemap.size underflow

[Brian Foster]

On Fri, Nov 15, 2019 at 10:59:55AM -0800, Darrick J. Wong wrote:
> On Fri, Nov 15, 2019 at 06:41:37AM -0500, Brian Foster wrote:
> > The leaf format xattr addition helper xfs_attr3_leaf_add_work()
> > adjusts the block freemap in a couple places. The first update drops
> > the size of the freemap that the caller had already selected to
> > place the xattr name/value data. Before the function returns, it
> > also checks whether the entries array has encroached on a freemap
> > range by virtue of the new entry addition. This is necessary because
> > the entries array grows from the start of the block (but end of the
> > block header) towards the end of the block while the name/value data
> > grows from the end of the block in the opposite direction. If the
> > associated freemap is already empty, however, size is zero and the
> > subtraction underflows the field and causes corruption.
> > 
> > This is reproduced rarely by generic/070. The observed behavior is
> > that a smaller sized freemap is aligned to the end of the entries
> > list, several subsequent xattr additions land in larger freemaps and
> > the entries list expands into the smaller freemap until it is fully
> > consumed and then underflows. Note that it is not otherwise a
> > corruption for the entries array to consume an empty freemap because
> > the nameval list (i.e. the firstused pointer in the xattr header)
> > starts beyond the end of the corrupted freemap.
> > 
> > Update the freemap size modification to account for the fact that
> > the freemap entry can be empty and thus stale.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> 
> Hm.  freemap.size == 0 means the freemap entry is stale and therefore
> anything looking for free space in the leaf will ignore the entry, right?
> 

Yep, at least that's my understanding from the code in the caller that
explicitly checks for and skips freemaps where size == 0.

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

Thanks!

Brian

> (Urk, there are still a lot of ASSERT-on-metadata in the dir/attr
> code...)
> 
> --D
> 
> > ---
> >  fs/xfs/libxfs/xfs_attr_leaf.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index 85ec5945d29f..86155260d8b9 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -1510,7 +1510,9 @@ xfs_attr3_leaf_add_work(
> >  	for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
> >  		if (ichdr->freemap[i].base == tmp) {
> >  			ichdr->freemap[i].base += sizeof(xfs_attr_leaf_entry_t);
> > -			ichdr->freemap[i].size -= sizeof(xfs_attr_leaf_entry_t);
> > +			ichdr->freemap[i].size -=
> > +				min_t(uint16_t, ichdr->freemap[i].size,
> > +						sizeof(xfs_attr_leaf_entry_t));
> >  		}
> >  	}
> >  	ichdr->usedbytes += xfs_attr_leaf_entsize(leaf, args->index);
> > -- 
> > 2.20.1
> > 
>