[PATCH 1/3] xfs: check _btree_check_block value

[PATCH 1/3] xfs: check _btree_check_block value

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Check the _btree_check_block return value for the firstrec and lastrec
functions, since we have the ability to signal that the repositioning
did not succeed.

Fixes-coverity-id: 114067
Fixes-coverity-id: 114068
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_btree.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 4da85ff..e0bcc4a 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -728,7 +728,8 @@ xfs_btree_firstrec(
 	 * Get the block pointer for this level.
 	 */
 	block = xfs_btree_get_block(cur, level, &bp);
-	xfs_btree_check_block(cur, block, level, bp);
+	if (xfs_btree_check_block(cur, block, level, bp))
+		return 0;
 	/*
 	 * It's empty, there is no such record.
 	 */
@@ -757,7 +758,8 @@ xfs_btree_lastrec(
 	 * Get the block pointer for this level.
 	 */
 	block = xfs_btree_get_block(cur, level, &bp);
-	xfs_btree_check_block(cur, block, level, bp);
+	if (xfs_btree_check_block(cur, block, level, bp))
+		return 0;
 	/*
 	 * It's empty, there is no such record.
 	 */

[PATCH 2/3] xfs: set firstfsb to NULLFSBLOCK before feeding it to _bmapi_write

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

We must initialize the firstfsb parameter to _bmapi_write so that it
doesn't incorrectly treat stack garbage as a restriction on which AGs
it can search for free space.

Fixes-coverity-id: 1402025
Fixes-coverity-id: 1415167
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c |    2 +-
 fs/xfs/xfs_reflink.c     |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 935adde..8c4ee60 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -6508,7 +6508,7 @@ xfs_bmap_finish_one(
 	xfs_filblks_t			*blockcount,
 	xfs_exntst_t			state)
 {
-	xfs_fsblock_t			firstfsb;
+	xfs_fsblock_t			firstfsb = NULLFSBLOCK;
 	int				error = 0;
 
 	trace_xfs_bmap_deferred(tp->t_mountp,
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index ab2270a..d9b3d57 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -329,7 +329,7 @@ xfs_reflink_convert_cow_extent(
 	xfs_filblks_t			count_fsb,
 	struct xfs_defer_ops		*dfops)
 {
-	xfs_fsblock_t			first_block;
+	xfs_fsblock_t			first_block = NULLFSBLOCK;
 	int				nimaps = 1;
 
 	if (imap->br_state == XFS_EXT_NORM)

Re: [PATCH 2/3] xfs: set firstfsb to NULLFSBLOCK before feeding it to _bmapi_write

[Brian Foster]

On Tue, Jul 18, 2017 at 11:24:22AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We must initialize the firstfsb parameter to _bmapi_write so that it
> doesn't incorrectly treat stack garbage as a restriction on which AGs
> it can search for free space.
> 
> Fixes-coverity-id: 1402025
> Fixes-coverity-id: 1415167
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c |    2 +-
>  fs/xfs/xfs_reflink.c     |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 935adde..8c4ee60 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -6508,7 +6508,7 @@ xfs_bmap_finish_one(
>  	xfs_filblks_t			*blockcount,
>  	xfs_exntst_t			state)
>  {
> -	xfs_fsblock_t			firstfsb;
> +	xfs_fsblock_t			firstfsb = NULLFSBLOCK;
>  	int				error = 0;
>  

Shouldn't firstfsb be tied to the transaction lifetime in (at least)
this case? It is used in the allocator to control things like AG locking
order. Here, it looks like we could potentially do multiple unmaps in a
single transaction without carrying the firstfsb state across..?

FWIW this patch still looks fine either way:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	trace_xfs_bmap_deferred(tp->t_mountp,
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index ab2270a..d9b3d57 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -329,7 +329,7 @@ xfs_reflink_convert_cow_extent(
>  	xfs_filblks_t			count_fsb,
>  	struct xfs_defer_ops		*dfops)
>  {
> -	xfs_fsblock_t			first_block;
> +	xfs_fsblock_t			first_block = NULLFSBLOCK;
>  	int				nimaps = 1;
>  
>  	if (imap->br_state == XFS_EXT_NORM)
> 
> --
> 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

Re: [PATCH 2/3] xfs: set firstfsb to NULLFSBLOCK before feeding it to _bmapi_write

[Darrick J. Wong]

On Wed, Jul 19, 2017 at 09:20:22AM -0400, Brian Foster wrote:
> On Tue, Jul 18, 2017 at 11:24:22AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > We must initialize the firstfsb parameter to _bmapi_write so that it
> > doesn't incorrectly treat stack garbage as a restriction on which AGs
> > it can search for free space.
> > 
> > Fixes-coverity-id: 1402025
> > Fixes-coverity-id: 1415167
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c |    2 +-
> >  fs/xfs/xfs_reflink.c     |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 935adde..8c4ee60 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -6508,7 +6508,7 @@ xfs_bmap_finish_one(
> >  	xfs_filblks_t			*blockcount,
> >  	xfs_exntst_t			state)
> >  {
> > -	xfs_fsblock_t			firstfsb;
> > +	xfs_fsblock_t			firstfsb = NULLFSBLOCK;
> >  	int				error = 0;
> >  
> 
> Shouldn't firstfsb be tied to the transaction lifetime in (at least)
> this case? It is used in the allocator to control things like AG locking
> order.

That is correct...

> Here, it looks like we could potentially do multiple unmaps in a
> single transaction without carrying the firstfsb state across..?

...but keep in mind that we call xfs_bmap_finish_one once per
transaction and roll transactions until the work is done.  I'll add a
comment here to that effect:

/*
 * firstfsb is tied to the transaction lifetime and is used to ensure
 * correct AG locking order and schedule work item continuations.
 * XFS_BUI_MAX_FAST_EXTENTS (== 1) restricts us to only making one bmap
 * call per transaction, so it should be safe as a local variable.
 */

> FWIW this patch still looks fine either way:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Thank you for the review!

--D
> 
> >  	trace_xfs_bmap_deferred(tp->t_mountp,
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index ab2270a..d9b3d57 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -329,7 +329,7 @@ xfs_reflink_convert_cow_extent(
> >  	xfs_filblks_t			count_fsb,
> >  	struct xfs_defer_ops		*dfops)
> >  {
> > -	xfs_fsblock_t			first_block;
> > +	xfs_fsblock_t			first_block = NULLFSBLOCK;
> >  	int				nimaps = 1;
> >  
> >  	if (imap->br_state == XFS_EXT_NORM)
> > 
> > --
> > 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

Re: [PATCH 2/3] xfs: set firstfsb to NULLFSBLOCK before feeding it to _bmapi_write

[Brian Foster]

On Wed, Jul 19, 2017 at 08:39:50AM -0700, Darrick J. Wong wrote:
> On Wed, Jul 19, 2017 at 09:20:22AM -0400, Brian Foster wrote:
> > On Tue, Jul 18, 2017 at 11:24:22AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > We must initialize the firstfsb parameter to _bmapi_write so that it
> > > doesn't incorrectly treat stack garbage as a restriction on which AGs
> > > it can search for free space.
> > > 
> > > Fixes-coverity-id: 1402025
> > > Fixes-coverity-id: 1415167
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_bmap.c |    2 +-
> > >  fs/xfs/xfs_reflink.c     |    2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > > index 935adde..8c4ee60 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap.c
> > > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > > @@ -6508,7 +6508,7 @@ xfs_bmap_finish_one(
> > >  	xfs_filblks_t			*blockcount,
> > >  	xfs_exntst_t			state)
> > >  {
> > > -	xfs_fsblock_t			firstfsb;
> > > +	xfs_fsblock_t			firstfsb = NULLFSBLOCK;
> > >  	int				error = 0;
> > >  
> > 
> > Shouldn't firstfsb be tied to the transaction lifetime in (at least)
> > this case? It is used in the allocator to control things like AG locking
> > order.
> 
> That is correct...
> 
> > Here, it looks like we could potentially do multiple unmaps in a
> > single transaction without carrying the firstfsb state across..?
> 
> ...but keep in mind that we call xfs_bmap_finish_one once per
> transaction and roll transactions until the work is done.  I'll add a
> comment here to that effect:
> 
> /*
>  * firstfsb is tied to the transaction lifetime and is used to ensure
>  * correct AG locking order and schedule work item continuations.
>  * XFS_BUI_MAX_FAST_EXTENTS (== 1) restricts us to only making one bmap
>  * call per transaction, so it should be safe as a local variable.
>  */
> 

Ok, got it. One transaction per dfp and one unmap per dfp, therefore one
unmap per transaction. Thanks!

Brian

> > FWIW this patch still looks fine either way:
> > 
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> Thank you for the review!
> 
> --D
> > 
> > >  	trace_xfs_bmap_deferred(tp->t_mountp,
> > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > > index ab2270a..d9b3d57 100644
> > > --- a/fs/xfs/xfs_reflink.c
> > > +++ b/fs/xfs/xfs_reflink.c
> > > @@ -329,7 +329,7 @@ xfs_reflink_convert_cow_extent(
> > >  	xfs_filblks_t			count_fsb,
> > >  	struct xfs_defer_ops		*dfops)
> > >  {
> > > -	xfs_fsblock_t			first_block;
> > > +	xfs_fsblock_t			first_block = NULLFSBLOCK;
> > >  	int				nimaps = 1;
> > >  
> > >  	if (imap->br_state == XFS_EXT_NORM)
> > > 
> > > --
> > > 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
> --
> 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 3/3] xfs: check _alloc_read_agf buffer pointer before using

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In some circumstances, _alloc_read_agf can return an error code of zero
but also a null AGF buffer pointer.  Check for this and jump out.

Fixes-coverity-id: 1415250
Fixes-coverity-id: 1415320
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_refcount.c |    4 ++++
 fs/xfs/xfs_reflink.c         |    2 ++
 2 files changed, 6 insertions(+)


diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 900ea23..45b1c3b 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1638,6 +1638,10 @@ xfs_refcount_recover_cow_leftovers(
 	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
 	if (error)
 		goto out_trans;
+	if (!agbp) {
+		error = -ENOMEM;
+		goto out_trans;
+	}
 	cur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno, NULL);
 
 	/* Find all the leftover CoW staging extents. */
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index d9b3d57..f45fbf0 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -170,6 +170,8 @@ xfs_reflink_find_shared(
 	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
 	if (error)
 		return error;
+	if (!agbp)
+		return -ENOMEM;
 
 	cur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno, NULL);
 

Re: [PATCH 3/3] xfs: check _alloc_read_agf buffer pointer before using

[Brian Foster]

On Tue, Jul 18, 2017 at 11:24:28AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In some circumstances, _alloc_read_agf can return an error code of zero
> but also a null AGF buffer pointer.  Check for this and jump out.
> 

It looks like this is only possible in trylock cases. Otherwise (and
unless I'm missing something), it should always return a buffer or
error.

This is circuitous regardless and so seems fine if it shuts up a
coverity warning:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> Fixes-coverity-id: 1415250
> Fixes-coverity-id: 1415320
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_refcount.c |    4 ++++
>  fs/xfs/xfs_reflink.c         |    2 ++
>  2 files changed, 6 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index 900ea23..45b1c3b 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -1638,6 +1638,10 @@ xfs_refcount_recover_cow_leftovers(
>  	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
>  	if (error)
>  		goto out_trans;
> +	if (!agbp) {
> +		error = -ENOMEM;
> +		goto out_trans;
> +	}
>  	cur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno, NULL);
>  
>  	/* Find all the leftover CoW staging extents. */
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index d9b3d57..f45fbf0 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -170,6 +170,8 @@ xfs_reflink_find_shared(
>  	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
>  	if (error)
>  		return error;
> +	if (!agbp)
> +		return -ENOMEM;
>  
>  	cur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno, NULL);
>  
> 
> --
> 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

Re: [PATCH 3/3] xfs: check _alloc_read_agf buffer pointer before using

[Darrick J. Wong]

On Wed, Jul 19, 2017 at 09:20:32AM -0400, Brian Foster wrote:
> On Tue, Jul 18, 2017 at 11:24:28AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In some circumstances, _alloc_read_agf can return an error code of zero
> > but also a null AGF buffer pointer.  Check for this and jump out.
> > 
> 
> It looks like this is only possible in trylock cases. Otherwise (and
> unless I'm missing something), it should always return a buffer or
> error.
> 
> This is circuitous regardless and so seems fine if it shuts up a
> coverity warning:

<shrug> I argue it's also defensive, in case we ever /do/ change the
semantics to allow more "zero return and no bp" cases, then these parts
won't suddenly start blowing up.

I think the Coverity analysis is just plain wrong (it claims that we can
somehow corrupt return values to end up with a zero return having
started with -EIO) but I did spot the trylock case and figured we could
be defensive about that.

--D

> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > Fixes-coverity-id: 1415250
> > Fixes-coverity-id: 1415320
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_refcount.c |    4 ++++
> >  fs/xfs/xfs_reflink.c         |    2 ++
> >  2 files changed, 6 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> > index 900ea23..45b1c3b 100644
> > --- a/fs/xfs/libxfs/xfs_refcount.c
> > +++ b/fs/xfs/libxfs/xfs_refcount.c
> > @@ -1638,6 +1638,10 @@ xfs_refcount_recover_cow_leftovers(
> >  	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
> >  	if (error)
> >  		goto out_trans;
> > +	if (!agbp) {
> > +		error = -ENOMEM;
> > +		goto out_trans;
> > +	}
> >  	cur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno, NULL);
> >  
> >  	/* Find all the leftover CoW staging extents. */
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index d9b3d57..f45fbf0 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -170,6 +170,8 @@ xfs_reflink_find_shared(
> >  	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
> >  	if (error)
> >  		return error;
> > +	if (!agbp)
> > +		return -ENOMEM;
> >  
> >  	cur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno, NULL);
> >  
> > 
> > --
> > 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

Re: [PATCH 1/3] xfs: check _btree_check_block value

[Brian Foster]

On Tue, Jul 18, 2017 at 11:24:16AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the _btree_check_block return value for the firstrec and lastrec
> functions, since we have the ability to signal that the repositioning
> did not succeed.
> 
> Fixes-coverity-id: 114067
> Fixes-coverity-id: 114068
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_btree.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index 4da85ff..e0bcc4a 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -728,7 +728,8 @@ xfs_btree_firstrec(
>  	 * Get the block pointer for this level.
>  	 */
>  	block = xfs_btree_get_block(cur, level, &bp);
> -	xfs_btree_check_block(cur, block, level, bp);
> +	if (xfs_btree_check_block(cur, block, level, bp))
> +		return 0;
>  	/*
>  	 * It's empty, there is no such record.
>  	 */
> @@ -757,7 +758,8 @@ xfs_btree_lastrec(
>  	 * Get the block pointer for this level.
>  	 */
>  	block = xfs_btree_get_block(cur, level, &bp);
> -	xfs_btree_check_block(cur, block, level, bp);
> +	if (xfs_btree_check_block(cur, block, level, bp))
> +		return 0;
>  	/*
>  	 * It's empty, there is no such record.
>  	 */
> 
> --
> 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