Messages in this thread Patch in this message | | | Date | Sat, 3 Oct 1998 14:30:32 -0400 (EDT) | From | Alexander Viro <> | Subject | Patch for leak in acct.c [2.1.120+] |
| |
Hi. There are two struct file leaks in kernel/acct.c. One of them leaves a write-opened file (i.e. no clean reboot). Way to reproduce: turn the accounting on (e.g. with acct("/tmp/foo");), leave less than 2% of free space on that filesystem to suspend accounting and call acct() with any argument (e.g. acct(NULL)). struct file for the /tmp/foo will be lost. Reason: acct.c uses the same flag both for disabled/enabled and suspended/active. Situation with accounting enabled but suspended is treated as 'disabled'. Another (less nasty) leak: if sys_acct() proceeds to obtain dentry, get a free struct file but fails to get write permission or fails on file->f_op->open() it doesn't return the struct file, only decrements f_count. On the next call that struct file is completely forgotten. Patch (obvious) follows: ----------------------------------------------------------------------------- --- acct.c.old Sat Oct 3 11:50:33 1998 +++ acct.c Sat Oct 3 12:26:23 1998 @@ -16,6 +16,10 @@ * * (C) Copyright 1995 - 1997 Marco van Wieringen - ELM Consultancy B.V. * + * Plugged two leaks. 1) It didn't return acct_file into the free_filps if + * the file happened to be read-only. 2) If the accounting was suspended + * due to the lack of space it happily allowed to reopen it and completely + * lost the old acct_file. 3/10/98, Al Viro. */ #include <linux/config.h> @@ -123,17 +127,25 @@ goto out; if (name == (char *)NULL) { - if (acct_active) { - acct_process(0); + if (acct_file) { + /* fput() may block, so just in case... */ + struct file *tmp = acct_file; + if (acct_active) + acct_process(0); del_timer(&acct_timer); acct_active = 0; acct_needcheck = 0; - fput(acct_file); + acct_file = NULL; + fput(tmp); } error = 0; goto out; } else { - if (!acct_active) { + /* + * We can't rely on acct_active - it might be disabled + * due to the lack of space. + */ + if (!acct_file) { tmp = getname(name); error = PTR_ERR(tmp); if (IS_ERR(tmp)) @@ -181,11 +193,17 @@ } put_write_access(acct_file->f_dentry->d_inode); } - acct_file->f_count--; + /* decrementing f_count is _not_ enough */ + put_filp(acct_file); + acct_file = NULL; } else error = -EUSERS; dput(dentry); } else + /* + * NB: in this case FreeBSD just closes acct_file + * and opens new one. Maybe it's better behavior... + */ error = -EBUSY; } out: ----------------------------------------------------------------------------- HTH. HAND. Al -- "You're one of those condescending Unix computer users!" "Here's a nickel, kid. Get yourself a better computer" - Dilbert.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |