linux-kernel - Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr

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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 7 Feb 2012 08:27:54 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Jesper Juhl <jj@...osbits.net>
Cc:	Raghavendra D Prabhu <raghu.prabhu13@...il.com>, xfs@....sgi.com,
	xfs-masters@....sgi.com, Ben Myers <bpm@....com>,
	Alex Elder <elder@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in
 xfs_setattr_nonsize()

On Mon, Feb 06, 2012 at 09:51:54PM +0100, Jesper Juhl wrote:
> On Mon, 6 Feb 2012, Raghavendra D Prabhu wrote:
> > As far as second one is concerned, looks fine, though this one should also do
> > the same.
> > 
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index ab30253..d331f5b 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -730,9 +730,9 @@ xfs_setattr_nonsize(
> >         return 0;
> > 
> > out_trans_cancel:
> > -       xfs_trans_cancel(tp, 0);
> >         xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > out_dqrele:
> > +       xfs_trans_cancel(tp, 0);
> >         xfs_qm_dqrele(udqp);
> >         xfs_qm_dqrele(gdqp);
> >         return error;
> > 
> 
> Thank you for the feedback.
> 
> I worry about the fact that this suddenly calls xfs_trans_cancel() without 
> holding the lock. I don't know if that's actually significant though. 

You're right to worry about it, because it is significant.

The transaction needs to be cancelled before we unlock the inode
because the transaction cancel cleans up state on the inode if the
inode has been joined to the transaction.  Unlocking the inode
before the transaction is cancelled means some other transaction can
lock the inode and join it to a new transaction before the old one
is cleaned up. Then Bad Stuff Happens.

IOWs, the above change is not safe to make.

> If it *is* significant, then I think the patch I just submitted in reply to 
> Dave Chinner is better since there we do the alloc and cancel before even 
> taking the lock at all in the leaky case and all other case have 
> identical behaviour as before.

I'll go check it out.

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/