[PATCH v1] xfs: initialize error in xfs_defer_finish_one

[PATCH v1] xfs: initialize error in xfs_defer_finish_one

[Sherry Yang]

Path through non-void function 'xfs_defer_finish_one' may return error
uninitialized if no iteration of 'list_for_each_safe' occurs. Fix this
by initializing error.

Fixes: bb47d79750f1 ("xfs: refactor xfs_defer_finish_noroll")
Cc: stable@vger.kernel.org
Signed-off-by: Sherry Yang <sherry.yang@oracle.com>
---
 fs/xfs/libxfs/xfs_defer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 5a321b783398..3188712ff34e 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -468,7 +468,7 @@ xfs_defer_finish_one(
 	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
 	struct xfs_btree_cur		*state = NULL;
 	struct list_head		*li, *n;
-	int				error;
+	int				error = 0;
 
 	trace_xfs_defer_pending_finish(tp->t_mountp, dfp);
 
-- 
2.31.1

Re: [PATCH v1] xfs: initialize error in xfs_defer_finish_one

[Dave Chinner]

On Mon, Aug 01, 2022 at 12:03:11PM -0700, Sherry Yang wrote:
> Path through non-void function 'xfs_defer_finish_one' may return error
> uninitialized if no iteration of 'list_for_each_safe' occurs. Fix this
> by initializing error.

I didn't think this situation was possible - how do we get deferred
work queued with no work items on it?

If we can return an uninitialised error from xfs_defer_finish_one()
because of an empty queued work, then something else has gone wrong
earlier in the work deferral process. If this can actually happen,
then we need to fix whatever is creating the empty work rather than
paper over it by initialising the error being returned for empty
works...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1] xfs: initialize error in xfs_defer_finish_one

[Darrick J. Wong]

On Tue, Aug 02, 2022 at 06:49:02AM +1000, Dave Chinner wrote:
> On Mon, Aug 01, 2022 at 12:03:11PM -0700, Sherry Yang wrote:
> > Path through non-void function 'xfs_defer_finish_one' may return error
> > uninitialized if no iteration of 'list_for_each_safe' occurs. Fix this
> > by initializing error.
> 
> I didn't think this situation was possible - how do we get deferred
> work queued with no work items on it?
> 
> If we can return an uninitialised error from xfs_defer_finish_one()
> because of an empty queued work, then something else has gone wrong
> earlier in the work deferral process. If this can actually happen,
> then we need to fix whatever is creating the empty work rather than
> paper over it by initialising the error being returned for empty
> works...

/me bets this is a response to a static checker that doesn't know that
list_empty(&dfp->dfp_work) == false in all circumstances.  It's not
possible for tp->t_dfops to contain an xfs_defer_pending with no work
items.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH v1] xfs: initialize error in xfs_defer_finish_one

[Sherry Yang]

> On Aug 2, 2022, at 9:31 PM, Darrick J. Wong <djwong@kernel.org> wrote:
> 
> On Tue, Aug 02, 2022 at 06:49:02AM +1000, Dave Chinner wrote:
>> On Mon, Aug 01, 2022 at 12:03:11PM -0700, Sherry Yang wrote:
>>> Path through non-void function 'xfs_defer_finish_one' may return error
>>> uninitialized if no iteration of 'list_for_each_safe' occurs. Fix this
>>> by initializing error.
>> 
>> I didn't think this situation was possible - how do we get deferred
>> work queued with no work items on it?
>> 
>> If we can return an uninitialised error from xfs_defer_finish_one()
>> because of an empty queued work, then something else has gone wrong
>> earlier in the work deferral process. If this can actually happen,
>> then we need to fix whatever is creating the empty work rather than
>> paper over it by initialising the error being returned for empty
>> works...
> 
> /me bets this is a response to a static checker that doesn't know that
> list_empty(&dfp->dfp_work) == false in all circumstances.  It's not
> possible for tp->t_dfops to contain an xfs_defer_pending with no work
> items.

Hi Darrick, 

You’re correct. This is a false positive bug detected by our static code 
analysis tool. Sorry for the noise.

Sherry
> 
> --D
> 
>> Cheers,
>> 
>> Dave.
>> -- 
>> Dave Chinner
>> david@fromorbit.com

Re: [PATCH v1] xfs: initialize error in xfs_defer_finish_one

[Darrick J. Wong]

On Wed, Aug 03, 2022 at 11:42:21PM +0000, Sherry Yang wrote:
> 
> > On Aug 2, 2022, at 9:31 PM, Darrick J. Wong <djwong@kernel.org> wrote:
> > 
> > On Tue, Aug 02, 2022 at 06:49:02AM +1000, Dave Chinner wrote:
> >> On Mon, Aug 01, 2022 at 12:03:11PM -0700, Sherry Yang wrote:
> >>> Path through non-void function 'xfs_defer_finish_one' may return error
> >>> uninitialized if no iteration of 'list_for_each_safe' occurs. Fix this
> >>> by initializing error.
> >> 
> >> I didn't think this situation was possible - how do we get deferred
> >> work queued with no work items on it?
> >> 
> >> If we can return an uninitialised error from xfs_defer_finish_one()
> >> because of an empty queued work, then something else has gone wrong
> >> earlier in the work deferral process. If this can actually happen,
> >> then we need to fix whatever is creating the empty work rather than
> >> paper over it by initialising the error being returned for empty
> >> works...
> > 
> > /me bets this is a response to a static checker that doesn't know that
> > list_empty(&dfp->dfp_work) == false in all circumstances.  It's not
> > possible for tp->t_dfops to contain an xfs_defer_pending with no work
> > items.
> 
> Hi Darrick, 
> 
> You’re correct. This is a false positive bug detected by our static code 
> analysis tool. Sorry for the noise.

Well, thank /you/ for running smatch/sparse/whatever on the XFS code
base.  Let us know if you find any other oddities, since it does tend to
find things every now and then. :)

--D

> Sherry
> > 
> > --D
> > 
> >> Cheers,
> >> 
> >> Dave.
> >> -- 
> >> Dave Chinner
> >> david@fromorbit.com
>