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 <[email protected]>
---
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 <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
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
[email protected]
Hi,
* On Sun, Feb 05, 2012 at 10:23:44PM +0100, Jesper Juhl <[email protected]> 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 <[email protected]>
>---
> 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 <[email protected]> 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
>[email protected]
>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
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 <[email protected]>
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 <[email protected]>
---
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 <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
On Mon, 6 Feb 2012, Raghavendra D Prabhu wrote:
> Hi,
>
>
> * On Sun, Feb 05, 2012 at 10:23:44PM +0100, Jesper Juhl <[email protected]>
> 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 <[email protected]>
> > ---
> > 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 <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
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
[email protected]
Hi,
* On Mon, Feb 06, 2012 at 09:51:54PM +0100, Jesper Juhl <[email protected]> wrote:
>On Mon, 6 Feb 2012, Raghavendra D Prabhu wrote:
>
>> Hi,
>>
>>
>> * On Sun, Feb 05, 2012 at 10:23:44PM +0100, Jesper Juhl <[email protected]>
>> 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 <[email protected]>
>> > ---
>> > 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 <[email protected]> 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