On Fri, May 01, 2020 at 06:40:09PM +0200, Christoph Hellwig wrote:
> On Fri, May 01, 2020 at 11:57:24AM -0400, Brian Foster wrote:
> > > if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> > > - return __this_address;
> > > - return xfs_attr_shortform_verify(ip);
> > > + fa = __this_address;
> > > + else
> > > + fa = xfs_attr_shortform_verify(ip);
> > > +
> > > + if (fa) {
> > > + xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> > > + ip->i_afp->if_u1.if_data, ip->i_afp->if_bytes, fa);
> > > + return -EFSCORRUPTED;
> >
> > This explicitly makes !ip->i_afp one of the handled corruption cases for
> > XFS_DINODE_FMT_LOCAL, but then attempts to access it anyways. Otherwise
> > seems Ok modulo the comments on the previous patch...
>
> No, it keeps the existing bogus behavior, just making the bug more
> obvious by moving the bits closer together :(
>
The associated code replaced in this patch checks the attr fork pointer:
- ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
- xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
- ifp ? ifp->if_u1.if_data : NULL,
- ifp ? ifp->if_bytes : 0, fa);
Brian
> That being said this is getting fixed later in the series.
>
