* [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() @ 2012-02-05 21:23 Jesper Juhl 2012-02-06 6:23 ` Dave Chinner 2012-02-06 9:11 ` Raghavendra D Prabhu 0 siblings, 2 replies; 8+ messages in thread From: Jesper Juhl @ 2012-02-05 21:23 UTC (permalink / raw) To: xfs; +Cc: xfs-masters, linux-kernel, Ben Myers, Alex Elder 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@chaosbits.net> --- 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@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() 2012-02-05 21:23 [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() Jesper Juhl @ 2012-02-06 6:23 ` Dave Chinner 2012-02-06 20:44 ` Jesper Juhl 2012-02-06 9:11 ` Raghavendra D Prabhu 1 sibling, 1 reply; 8+ messages in thread From: Dave Chinner @ 2012-02-06 6:23 UTC (permalink / raw) To: Jesper Juhl; +Cc: xfs, xfs-masters, linux-kernel, Ben Myers, Alex Elder On Sun, Feb 05, 2012 at 10:23:44PM +0100, Jesper Juhl 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. xfs_trans_alloc() can't fail. It will sleep forever until the memory allocation succeeds. There's 35 other places in XFS where this xfs_trans_alloc/ xfs_trans_reserve pattern occurs - none of them check whether tp is null, either. > 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. yes, that's a problem, though will only happen if a filesystem shutdown occurs between the start of the function and that check. > > 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. Only need to fix the leak of tp. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() 2012-02-06 6:23 ` Dave Chinner @ 2012-02-06 20:44 ` Jesper Juhl 2012-07-20 19:28 ` Jesper Juhl 0 siblings, 1 reply; 8+ messages in thread From: Jesper Juhl @ 2012-02-06 20:44 UTC (permalink / raw) To: Dave Chinner Cc: xfs, xfs-masters, linux-kernel, Ben Myers, Alex Elder, Raghavendra D Prabhu On Mon, 6 Feb 2012, Dave Chinner wrote: > On Sun, Feb 05, 2012 at 10:23:44PM +0100, Jesper Juhl 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. > > xfs_trans_alloc() can't fail. It will sleep forever until the memory > allocation succeeds. > > There's 35 other places in XFS where this xfs_trans_alloc/ > xfs_trans_reserve pattern occurs - none of them check whether tp is > null, either. > > > 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. > > yes, that's a problem, though will only happen if a filesystem > shutdown occurs between the start of the function and that check. > > > > > 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. > > Only need to fix the leak of tp. > Ok. Thank you for the detailed explanation. I believe the patch below should do the trick. From: Jesper Juhl <jj@chaosbits.net> Date: Sun, 5 Feb 2012 22:11:30 +0100 Subject: [PATCH] XFS: Fix mem leak in xfs_setattr_nonsize() If the memory allocation for 'tp' goes well then we will leak the memory allocated to 'tp' if xfs_trans_reserve() returns error. Signed-off-by: Jesper Juhl <jj@chaosbits.net> --- fs/xfs/xfs_iops.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index ab30253..2fc1600 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -576,8 +576,10 @@ xfs_setattr_nonsize( tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_NOT_SIZE); 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 -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() 2012-02-06 20:44 ` Jesper Juhl @ 2012-07-20 19:28 ` Jesper Juhl 0 siblings, 0 replies; 8+ messages in thread From: Jesper Juhl @ 2012-07-20 19:28 UTC (permalink / raw) To: Dave Chinner Cc: xfs, xfs-masters, linux-kernel, Ben Myers, Alex Elder, Raghavendra D Prabhu Ping ? This issue seems to still be present. Any chance we could merge the patch below? /Jesper On Mon, 6 Feb 2012, Jesper Juhl wrote: > On Mon, 6 Feb 2012, Dave Chinner wrote: > > > On Sun, Feb 05, 2012 at 10:23:44PM +0100, Jesper Juhl 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. > > > > xfs_trans_alloc() can't fail. It will sleep forever until the memory > > allocation succeeds. > > > > There's 35 other places in XFS where this xfs_trans_alloc/ > > xfs_trans_reserve pattern occurs - none of them check whether tp is > > null, either. > > > > > 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. > > > > yes, that's a problem, though will only happen if a filesystem > > shutdown occurs between the start of the function and that check. > > > > > > > > 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. > > > > Only need to fix the leak of tp. > > > Ok. > Thank you for the detailed explanation. > I believe the patch below should do the trick. > > > From: Jesper Juhl <jj@chaosbits.net> > Date: Sun, 5 Feb 2012 22:11:30 +0100 > Subject: [PATCH] XFS: Fix mem leak in xfs_setattr_nonsize() > > If the memory allocation for 'tp' goes well then we will leak the > memory allocated to 'tp' if xfs_trans_reserve() returns error. > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> > --- > fs/xfs/xfs_iops.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index ab30253..2fc1600 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -576,8 +576,10 @@ xfs_setattr_nonsize( > > tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_NOT_SIZE); > 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); > > -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() 2012-02-05 21:23 [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() Jesper Juhl 2012-02-06 6:23 ` Dave Chinner @ 2012-02-06 9:11 ` Raghavendra D Prabhu 2012-02-06 20:51 ` Jesper Juhl 1 sibling, 1 reply; 8+ messages in thread From: Raghavendra D Prabhu @ 2012-02-06 9:11 UTC (permalink / raw) To: Jesper Juhl; +Cc: xfs, xfs-masters, Ben Myers, Alex Elder, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3289 bytes --] Hi, * On Sun, Feb 05, 2012 at 10:23:44PM +0100, Jesper Juhl <jj@chaosbits.net> 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@chaosbits.net> >--- > 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@chaosbits.net> 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@oss.sgi.com >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 #2: Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() 2012-02-06 9:11 ` Raghavendra D Prabhu @ 2012-02-06 20:51 ` Jesper Juhl 2012-02-06 21:27 ` Dave Chinner 2012-02-07 22:41 ` Raghavendra D Prabhu 0 siblings, 2 replies; 8+ messages in thread From: Jesper Juhl @ 2012-02-06 20:51 UTC (permalink / raw) To: Raghavendra D Prabhu Cc: xfs, xfs-masters, Ben Myers, Alex Elder, linux-kernel, Dave Chinner On Mon, 6 Feb 2012, Raghavendra D Prabhu wrote: > Hi, > > > * On Sun, Feb 05, 2012 at 10:23:44PM +0100, Jesper Juhl <jj@chaosbits.net> > 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@chaosbits.net> > > --- > > 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. > > [...] > > 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. > Good. > > 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. 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. If it is *not* significant then your patch is probably better since that means one less thing done while holding a lock. But I don't know enough XFS details to say which it is, so I'll leave it to someone else to pick the best patch of the two for this. -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() 2012-02-06 20:51 ` Jesper Juhl @ 2012-02-06 21:27 ` Dave Chinner 2012-02-07 22:41 ` Raghavendra D Prabhu 1 sibling, 0 replies; 8+ messages in thread From: Dave Chinner @ 2012-02-06 21:27 UTC (permalink / raw) To: Jesper Juhl Cc: Raghavendra D Prabhu, xfs, xfs-masters, Ben Myers, Alex Elder, linux-kernel 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@fromorbit.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Re: [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() 2012-02-06 20:51 ` Jesper Juhl 2012-02-06 21:27 ` Dave Chinner @ 2012-02-07 22:41 ` Raghavendra D Prabhu 1 sibling, 0 replies; 8+ messages in thread From: Raghavendra D Prabhu @ 2012-02-07 22:41 UTC (permalink / raw) To: Jesper Juhl Cc: xfs, xfs-masters, Ben Myers, Alex Elder, linux-kernel, Dave Chinner [-- Attachment #1: Type: text/plain, Size: 4505 bytes --] Hi, * On Mon, Feb 06, 2012 at 09:51:54PM +0100, Jesper Juhl <jj@chaosbits.net> wrote: >On Mon, 6 Feb 2012, Raghavendra D Prabhu wrote: > >> Hi, >> >> >> * On Sun, Feb 05, 2012 at 10:23:44PM +0100, Jesper Juhl <jj@chaosbits.net> >> 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@chaosbits.net> >> > --- >> > 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. >> > >[...] >> >> 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. >> >Good. > >> >> 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. > >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. >If it is *not* significant then your patch is probably better since that >means one less thing done while holding a lock. > >But I don't know enough XFS details to say which it is, so I'll leave it >to someone else to pick the best patch of the two for this. > > >-- >Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ >Don't top-post http://www.catb.org/jargon/html/T/top-post.html >Plain text mails only, please. > Thanks, I noticed it a few moments after I posted it :) but I needed to know the reason behind unlock before cancel pattern which was provided by David Chinner. Regards, -- Raghavendra Prabhu GPG Id : 0xD72BE977 Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977 www: wnohang.net [-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-07-20 19:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-02-05 21:23 [PATCH][RFC] XFS: Fix mem leak and possible NULL deref in xfs_setattr_nonsize() Jesper Juhl 2012-02-06 6:23 ` Dave Chinner 2012-02-06 20:44 ` Jesper Juhl 2012-07-20 19:28 ` Jesper Juhl 2012-02-06 9:11 ` Raghavendra D Prabhu 2012-02-06 20:51 ` Jesper Juhl 2012-02-06 21:27 ` Dave Chinner 2012-02-07 22:41 ` Raghavendra D Prabhu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).