Patch for leak in acct.c [2.1.120+]

Alexander Viro (viro@math.psu.edu)
Sat, 3 Oct 1998 14:30:32 -0400 (EDT)

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/