linux-kernel - Re: [PATCH 1/2] locks: let the caller free file

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]

Message-ID: <20101104014024.GA22498@fieldses.org>
Date:	Wed, 3 Nov 2010 21:40:24 -0400
From:	"J. Bruce Fields" <bfields@...ldses.org>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	Arnd Bergmann <arnd@...db.de>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Bryan Schumaker <bjschuma@...app.com>,
	linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] locks: let the caller free file_lock on ->setlease
 failure

On Wed, Nov 03, 2010 at 04:41:48PM -0400, J. Bruce Fields wrote:
> On Sun, Oct 31, 2010 at 08:35:10AM -0400, Christoph Hellwig wrote:
> > Index: linux-2.6/fs/locks.c
> > ===================================================================
> > --- linux-2.6.orig/fs/locks.c	2010-10-31 07:10:07.649004084 -0400
> > +++ linux-2.6/fs/locks.c	2010-10-31 07:34:10.102255587 -0400
> > @@ -1428,8 +1425,9 @@ int generic_setlease(struct file *filp,
> >  		goto out;
> >  
> >  	if (my_before != NULL) {
> > -		*flp = *my_before;
> >  		error = lease->fl_lmops->fl_change(my_before, arg);
> > +		if (!error)
> > +			*flp = *my_before;
> 
> Argh, missed this: we're leaking the passed-in lease in this case.

We could do something like this.

The irritating thing is that the only lease user I understand is the
nfsd code, and it doesn't want this lease-merging behavior; the only
reason that fl_change is there is so it can just turn this case into an
error every time.

And I have no idea what the requirements are of any other users: do
leases behave like this on purpose, or was it just an arbitrary choice,
and does anyone depend on it now?

In the end maybe it would be better just to leave leases as they are and
define a new lock type for nfsd.

We'd probably have to do that eventually anyway, and it'd save me trying
to guess what the lease semantics are supposed to be....

--b.

>From f5c6d51ae638af213d8bda31504f4b2287b8a801 Mon Sep 17 00:00:00 2001
From: J. Bruce Fields <bfields@...hat.com>
Date: Wed, 3 Nov 2010 16:49:44 -0400
Subject: [PATCH 1/2] locks: fix leak on merging leases

We must also free the passed-in lease in the case it wasn't used because
an existing lease was upgrade/downgraded or already existed.

Note the nfsd caller doesn't care because it's fl_change callback
returns an error in those cases.

Signed-off-by: J. Bruce Fields <bfields@...hat.com>
---
 fs/locks.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 65765cb..61c22f7 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1504,7 +1504,7 @@ static int do_fcntl_delete_lease(struct file *filp)
 
 static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
 {
-	struct file_lock *fl;
+	struct file_lock *fl, *ret;
 	struct fasync_struct *new;
 	struct inode *inode = filp->f_path.dentry->d_inode;
 	int error;
@@ -1518,6 +1518,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
 		locks_free_lock(fl);
 		return -ENOMEM;
 	}
+	ret = fl;
 	lock_flocks();
 	error = __vfs_setlease(filp, arg, &fl);
 	if (error) {
@@ -1525,6 +1526,8 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
 		locks_free_lock(fl);
 		goto out_free_fasync;
 	}
+	if (ret != fl)
+		locks_free_lock(fl);
 
 	/*
 	 * fasync_insert_entry() returns the old entry if any.
@@ -1532,7 +1535,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
 	 * inserted it into the fasync list. Clear new so that
 	 * we don't release it here.
 	 */
-	if (!fasync_insert_entry(fd, filp, &fl->fl_fasync, new))
+	if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new))
 		new = NULL;
 
 	if (error < 0) {
-- 
1.7.1

--
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/