linux-kernel - Re: [PATCH] xfs: call xfs_idestroy_fork() in xfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 27 Apr 2015 08:56:25 +1000
From:	Dave Chinner <david@...morbit.com>
To:	Brian Foster <bfoster@...hat.com>
Cc:	Waiman Long <Waiman.Long@...com>, linux-kernel@...r.kernel.org,
	xfs@....sgi.com
Subject: Re: [PATCH] xfs: call xfs_idestroy_fork() in xfs_ilock() critical
 section

On Fri, Apr 24, 2015 at 07:57:33AM -0400, Brian Foster wrote:
> On Fri, Apr 24, 2015 at 08:08:23AM +1000, Dave Chinner wrote:
> > On Thu, Apr 23, 2015 at 08:21:50AM -0400, Brian Foster wrote:
> > > On Thu, Apr 23, 2015 at 09:17:58AM +1000, Dave Chinner wrote:
> > > > @@ -410,11 +418,12 @@ xfs_attr_inactive(xfs_inode_t *dp)
> > > > +	lock_mode = XFS_ILOCK_EXCL;
> > > > +	cancel_flags = XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT;
> > > > +	xfs_ilock(dp, lock_mode);
> > > >  
> > > >  	/*
> > > >  	 * No need to make quota reservations here. We expect to release some
> > > > @@ -423,28 +432,36 @@ xfs_attr_inactive(xfs_inode_t *dp)
> > > >  	xfs_trans_ijoin(trans, dp, 0);
> > > >  
> > > >  	/*
> > > > -	 * Decide on what work routines to call based on the inode size.
> > > > +	 * It's unlikely we've raced with an attribute fork creation, but check
> > > > +	 * anyway just in case.
> > > >  	 */
> > > > -	if (!xfs_inode_hasattr(dp) ||
> > > > -	    dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> > > > -		error = 0;
> > > > -		goto out;
> > > > +	if (!XFS_IFORK_Q(dp))
> > > > +		goto out_cancel;
> > > 
> > > What about attribute fork creation would cause di_forkoff == 0 if that
> > > wasn't the case above? Do you mean to say a potential race with
> > > attribute fork destruction?
> > 
> > atrtibute fork creation will never leave di_forkoff == 0. See
> > xfs_attr_shortform_bytesfit() as a guideline for the min/max fork
> > offset at attribute fork creation time.
> > 
> > The race I'm talking about is the fact we check for an attr fork,
> > then drop the lock, do the trans reserve and then grab the lock
> > again. The inode could have changed in that time, so we need to
> > check again. It's extremely unlikely that the inode has changed due
> > to the fact it is in the ->evict path and can't be referenced by the
> > VFS again until it's in a reclaimable state. Hence it is only
> > internal filesystem stuff that could modify it, which I don't think
> > can happen. So, leave the check, mark the race as unlikely to occur.
> 
> The check seems fine to me. I'm referring to the comment above: "It's
> unlikely we've raced with an attribute fork creation, ..." 

Oh, ok, I missed that. I'll fix it.

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/