| Message ID | 20220801190311.65703-1-sherry.yang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] xfs: initialize error in xfs_defer_finish_one | expand |
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.
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
> 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
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 >
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);
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(-)