On Wed, Feb 05, 2020 at 08:39:32AM +1100, Dave Chinner wrote:
> On Tue, Feb 04, 2020 at 03:06:36PM +0800, Zorro Lang wrote:
> > This patch fixes below KASAN report. The xfs_attr3_node_inactive()
> > gets 'child_bp' at there:
> > error = xfs_trans_get_buf(*trans, mp->m_ddev_targp,
> > child_blkno,
> > XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0,
> > &child_bp);
> > if (error)
> > return error;
> > error = bp->b_error;
> >
> > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by:
> > xfs_trans_brelse(*trans, bp);
>
> ....
> > ---
> > fs/xfs/xfs_attr_inactive.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> > index bbfa6ba84dcd..26230d150bf2 100644
> > --- a/fs/xfs/xfs_attr_inactive.c
> > +++ b/fs/xfs/xfs_attr_inactive.c
> > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive(
> > &child_bp);
> > if (error)
> > return error;
> > - error = bp->b_error;
> > + error = child_bp->b_error;
> > if (error) {
> > xfs_trans_brelse(*trans, child_bp);
> > return error;
>
> Isn't this dead code now? i.e. any error that occurs on the buffer
> during a xfs_trans_get_buf() call is returned directly and so it's
> caught by the "if (error)" check. Hence this whole child_bp->b_error
> check can be removed, right?
Thanks, by looking into the xfs_trans_get_buf() code, I think you're right. Sorry
I didn't recognise that before.
But when should we check the bp->b_error? and when's it not necessary?
In other words, when XFS set the bp->b_error? Looks like it's set in some *verify*
functions and ioend time?
Thanks,
Zorro
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
