Re: Leak in nlmsvc_testlock for async GETFL case
On Mon, Jan 14, 2008 at 09:26:06PM -0700, Matthew Wilcox wrote: > On Mon, Jan 14, 2008 at 03:44:19PM -0500, J. Bruce Fields wrote: > > Thanks! I've queued it up for 2.6.25. > > Hi Bruce, > > I haven't had as much time to play with de-BKL-ising fs/locks.c as I > would like, so fixing that for 2.6.25 is probably out of the question, > but here are two janitorial patches that hopefully can be applied and > will make the next steps easier. They make sense all by themselves, > even if I don't get back to this project for a few months. OK, thanks. Hopefully we will get back to this--it'll be nice to finally make progress on it. --b. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Leak in nlmsvc_testlock for async GETFL case
On Mon, Jan 14, 2008 at 03:44:19PM -0500, J. Bruce Fields wrote: > Thanks! I've queued it up for 2.6.25. Hi Bruce, I haven't had as much time to play with de-BKL-ising fs/locks.c as I would like, so fixing that for 2.6.25 is probably out of the question, but here are two janitorial patches that hopefully can be applied and will make the next steps easier. They make sense all by themselves, even if I don't get back to this project for a few months. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Leak in nlmsvc_testlock for async GETFL case
On Fri, Jan 11, 2008 at 09:57:35PM -0500, Oleg Drokin wrote: > Hello! > > On Nov 29, 2007, at 2:08 PM, J. Bruce Fields wrote: > >> On Thu, Nov 29, 2007 at 01:46:04PM -0500, Oleg Drokin wrote: >>> Hello! >>> >>> Per our discussion, I am resending this patch that fixes a leak in >>> nlmsvc_testlock. It is addition to another leak fixing patch you >>> already have. Without the patch, there is a leakage of nlmblock >>> structure refcount that holds a reference nlmfile structure, that >>> holds a reference to struct file, when async GETFL is used >>> (-EINPROGRESS return from file_ops->lock()), and also in some error >>> cases >> Thanks for the fix! Looks right to me. Yes, somehow I missed this >> one >> when you sent it privately. Applied and pushed out to >> git://linux-nfs.org/~bfields/linux.git nfs-server-stable >> and I'll submit it for 2.6.25. > > After playing around that code a bit more, I figured out the leak was > not > completely fixed by that first patch, the case where there is > conflicting > lock passed in by callback still leaks block reference. > This simple incremental fix (against your current tree) takes care of > that. > > Signed-off-by: Oleg Drokin <[EMAIL PROTECTED]> Thanks! I've queued it up for 2.6.25. --b. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Leak in nlmsvc_testlock for async GETFL case
Hello! On Nov 29, 2007, at 2:08 PM, J. Bruce Fields wrote: On Thu, Nov 29, 2007 at 01:46:04PM -0500, Oleg Drokin wrote: Hello! Per our discussion, I am resending this patch that fixes a leak in nlmsvc_testlock. It is addition to another leak fixing patch you already have. Without the patch, there is a leakage of nlmblock structure refcount that holds a reference nlmfile structure, that holds a reference to struct file, when async GETFL is used (-EINPROGRESS return from file_ops->lock()), and also in some error cases Thanks for the fix! Looks right to me. Yes, somehow I missed this one when you sent it privately. Applied and pushed out to git://linux-nfs.org/~bfields/linux.git nfs-server-stable and I'll submit it for 2.6.25. After playing around that code a bit more, I figured out the leak was not completely fixed by that first patch, the case where there is conflicting lock passed in by callback still leaks block reference. This simple incremental fix (against your current tree) takes care of that. Signed-off-by: Oleg Drokin <[EMAIL PROTECTED]> I guess there is no way to safely include patches in messages in apple mail, so the patch is still attached. nlmblock-leak_fix-2-incr.diff Description: Binary data
Re: Leak in nlmsvc_testlock for async GETFL case
Hello!
On Dec 3, 2007, at 12:00 PM, Felix Blyakher wrote:
Per our discussion, I am resending this patch that fixes a leak
in nlmsvc_testlock.
It is addition to another leak fixing patch you already have.
Without the patch, there is a leakage of nlmblock structure
refcount that holds a
reference nlmfile structure, that holds a reference to struct
file, when async GETFL
is used (-EINPROGRESS return from file_ops->lock()), and also in
some error cases.
> @@ -502,15 +509,19 @@
>}
>else {
>nlmsvc_unlink_block(block);
> - return nlm_granted;
> + ret = nlm_granted;
> +goto out;
>}
Do we really need to release block with 'goto out' here?
Yes, absolutely.
nlmsvc_unlink_block() is doing it internally in the following
call chain:
...
And on a last reference 'block' will be freed in nlmsvc_free_block.
Are you sure we have an extra reference here and won't go through
nlmsvc_free_block?
It releases reference that was obtained when lock was linked into a
queue by
nlmsvc_insert_block (which is indicated by B_QUEUED flag in the
nlmblock)
And then we need to release another reference that we got from block
lookup.
This path you pointed out is definitely leaking, it is a success path
for
async GETFL handling so I tested this case.
Bye,
Oleg
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Leak in nlmsvc_testlock for async GETFL case
On Nov 29, 2007, at 12:46 PM, Oleg Drokin wrote:
Hello!
Per our discussion, I am resending this patch that fixes a leak
in nlmsvc_testlock.
It is addition to another leak fixing patch you already have.
Without the patch, there is a leakage of nlmblock structure
refcount that holds a
reference nlmfile structure, that holds a reference to struct
file, when async GETFL
is used (-EINPROGRESS return from file_ops->lock()), and also in
some error cases.
Bye,
Oleg
> @@ -502,15 +509,19 @@
>}
>else {
>nlmsvc_unlink_block(block);
> - return nlm_granted;
> + ret = nlm_granted;
> +goto out;
>}
Do we really need to release block with 'goto out' here?
nlmsvc_unlink_block() is doing it internally in the following
call chain:
nlmsvc_unlink_block
nlmsvc_remove_block
nlmsvc_release_block
And on a last reference 'block' will be freed in nlmsvc_free_block.
Are you sure we have an extra reference here and won't go through
nlmsvc_free_block?
Felix
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Leak in nlmsvc_testlock for async GETFL case
On Thu, Nov 29, 2007 at 01:46:04PM -0500, Oleg Drokin wrote: > Hello! > >Per our discussion, I am resending this patch that fixes a leak in >nlmsvc_testlock. It is addition to another leak fixing patch you >already have. Without the patch, there is a leakage of nlmblock >structure refcount that holds a reference nlmfile structure, that >holds a reference to struct file, when async GETFL is used >(-EINPROGRESS return from file_ops->lock()), and also in some error >cases. Thanks for the fix! Looks right to me. Yes, somehow I missed this one when you sent it privately. Applied and pushed out to git://linux-nfs.org/~bfields/linux.git nfs-server-stable and I'll submit it for 2.6.25. Minor nit: your editor is messing up the whitespace; keep the indents all tabs. Also, for future patches, could you see Documentation/SubmittingPatches? In particular, see "12) Sign your work". And it'd be better to inline the patch if at all possible. Thanks again. --b. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
