xfs
[Top] [All Lists]

Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setatt

To: Jesper Juhl <jj@xxxxxxxxxxxxx>
Subject: Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize()
From: Raghavendra D Prabhu <raghu.prabhu13@xxxxxxxxx>
Date: Mon, 6 Feb 2012 14:41:00 +0530
Cc: xfs@xxxxxxxxxxx, xfs-masters@xxxxxxxxxxx, Ben Myers <bpm@xxxxxxx>, Alex Elder <elder@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :x-operating-system:x-editor:user-agent; bh=wco52hggb4qrj2TiQM9xGFW4aj7xc8Pq5zBYl11dms0=; b=fpsvOPY1vymVgg/zzWeozuiXn+hn8doAU+XK04a2pW539X5DnGXk3Y5sSow6W8Mz7N KgDZZzomWIWEdI/Ijhry9IwREFMdkGKgF5OTBmvrhfzTzopbZtvwpR2Tr+8MuIYsuxBm zE2zg4iBTHVK2w/CZ82TtvxwF2O79fnZpy/q4=
In-reply-to: <alpine.LNX.2.00.1202052220390.32529@xxxxxxxxxxxxxxxxxxxxxxxxx>
Mail-followup-to: Jesper Juhl <jj@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx, xfs-masters@xxxxxxxxxxx, Ben Myers <bpm@xxxxxxx>, Alex Elder <elder@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx
References: <alpine.LNX.2.00.1202052220390.32529@xxxxxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-12-30)
Hi,


* On Sun, Feb 05, 2012 at 10:23:44PM +0100, Jesper Juhl <jj@xxxxxxxxxxxxx> 
wrote:
In xfs_setattr_nonsize(), xfs_trans_alloc() gets its memory from
_xfs_trans_alloc() which gets it from kmem_zone_zalloc() which may
fail and return NULL. So this:

        tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_NOT_SIZE);

may result in a NULL 'tp'.
If it does, then the call:

        error = xfs_trans_reserve(tp, 0, XFS_ICHANGE_LOG_RES(mp), 0, 0, 0);

with a NULL 'tp' will explode, since xfs_trans_reserve() dereferences
its first argument unconditionally.

And if the memory allocation for 'tp' goes well (and thus
xfs_trans_reserve() does not explode) then we may leak the memory
allocated to 'tp' if xfs_trans_reserve() returns error.

I believe this patch should fix both issues, but I'm not intimate with
the XFS code at all, so there can easily be something I overlooked or
something that should be done differently than what I did.

Signed-off-by: Jesper Juhl <jj@xxxxxxxxxxxxx>
---
fs/xfs/xfs_iops.c |    7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)

Note:
 Please review carefully before applying.
 Especially since I don't currently have any XFS filesystems to test
 this on, nor any clear idea of a good way to actually test this if I
 had. So this patch is compile tested only on my end.

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ab30253..194c9d7 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -575,9 +575,14 @@ xfs_setattr_nonsize(
        }

        tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_NOT_SIZE);
+       if (!tp)
+               goto out_dqrele;
+
        error = xfs_trans_reserve(tp, 0, XFS_ICHANGE_LOG_RES(mp), 0, 0, 0);
-       if (error)
+       if (error) {
+               xfs_trans_cancel(tp, 0);
                goto out_dqrele;
+       }

        xfs_ilock(ip, XFS_ILOCK_EXCL);

--
1.7.9


Please CC me on replies.

--
Jesper Juhl <jj@xxxxxxxxxxxxx>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs

The first one won't be triggered because kmem_zone_alloc (the last one in call chain) checks for
    if (ptr || (flags & (KM_MAYFAIL|KM_NOSLEEP)))

whereas xfs_trans_alloc calls _xfs_trans_alloc with KM_SLEEP, also all other callers of _xfs_trans_alloc call it with KM_SLEEP (except one which calls with KM_NOFS), so it looks like we are safe there, it keeps spinning till it finds mem.


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;



Regards,
--
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

Attachment: pgpkkXGHHJg6K.pgp
Description: PGP signature

<Prev in Thread] Current Thread [Next in Thread>