[PATCH] NR_RESERVED_FILES broken in 2.4 too

From: Szabolcs Szakacsits (szaka@f-secure.com)
Date: Sun Dec 10 2000 - 11:52:59 EST


On Thu, 7 Dec 2000, Szabolcs Szakacsits wrote:
> On Thu, 7 Dec 2000, Tigran Aivazian wrote:
> > On Thu, 7 Dec 2000, Szabolcs Szakacsits wrote:
> > > Read the whole get_empty_filp function, especially this part,
> > I have read the whole function, including the above code, of course. The
> > new_one label has nothing to do with freelists -- it adds the file to the
> > anon_list, where the new arrivales from the slab cache go. The goto
> > new_one above is there simply to initialize the structure with sane
> > initial values
> OK, 2.2 has put_inuse(f); instead of putting it to anon_list, so 2.4
> seems ok.

Back to common sense ;) Nevertheless what you wrote additionally
get_empty_filp returns an allocated file struct that gets to be used.
So ignoring your four emails arguing kernel is ok, I downloaded
2.4-test11-pre7 and tried it out.

root# echo 1024 > /proc/sys/fs/file-max

Unpatched kernel,

user% ./fd-exhaustion # e.g. while(1) open("/dev/null",...);
root# cat /proc/sys/fs/file-nr
cat: /proc/sys/fs/file-nr: Too many open files in system

The above happens even with increased NR_RESERVED_FILES to 96 [no
wonder, get_empty_filp is broken].

With the patch below,

user% ./fd-exhaustion
root# cat /proc/sys/fs/file-nr
946 0 1024
 or
1024 78 1024
 or
something that also works

The patch also has a fix not to allocate potentially more file
structs than NR_FILES on SMP.

Unfortunately NR_RESERVED_FILES needs to be increased to be useful
[i.e. e.g. to make ssh|login+ps|kill work for superuser]. Other way
would be to more aggressively free unused file structs if kernel is
short on free fd's.

> > There are even books (Understanding the Linux
> > Kernel by Bovet et all) which describe this freelist in the
> > current context so your patch will require updates to the books.

Checked this part of the book, ok for 2.0 but not for 2.[24].

        Szaka

diff -ur linux-2.4.0-test12-pre7/fs/file_table.c linux/fs/file_table.c
--- linux-2.4.0-test12-pre7/fs/file_table.c Fri Dec 8 08:17:12 2000
+++ linux/fs/file_table.c Sun Dec 10 17:05:55 2000
@@ -32,39 +32,36 @@
 {
         static int old_max = 0;
         struct file * f;
+ int total_free;

         file_list_lock();
- if (files_stat.nr_free_files > NR_RESERVED_FILES) {
- used_one:
- f = list_entry(free_list.next, struct file, f_list);
- list_del(&f->f_list);
- files_stat.nr_free_files--;
- new_one:
- memset(f, 0, sizeof(*f));
- atomic_set(&f->f_count,1);
- f->f_version = ++event;
- f->f_uid = current->fsuid;
- f->f_gid = current->fsgid;
- list_add(&f->f_list, &anon_list);
- file_list_unlock();
- return f;
- }
- /*
- * Use a reserved one if we're the superuser
- */
- if (files_stat.nr_free_files && !current->euid)
- goto used_one;
- /*
- * Allocate a new one if we're below the limit.
- */
- if (files_stat.nr_files < files_stat.max_files) {
+ total_free = files_stat.max_files - files_stat.nr_files + files_stat.nr_free_files;
+ if (total_free > NR_RESERVED_FILES || (total_free && !current->euid)) {
+ if (files_stat.nr_free_files) {
+ /* used_one */
+ f = list_entry(free_list.next, struct file, f_list);
+ list_del(&f->f_list);
+ files_stat.nr_free_files--;
+ new_one:
+ memset(f, 0, sizeof(*f));
+ atomic_set(&f->f_count,1);
+ f->f_version = ++event;
+ f->f_uid = current->fsuid;
+ f->f_gid = current->fsgid;
+ list_add(&f->f_list, &anon_list);
+ file_list_unlock();
+ return f;
+ }
+ /*
+ * Allocate a new one if we're below the limit.
+ */
+ files_stat.nr_files++;
                 file_list_unlock();
                 f = kmem_cache_alloc(filp_cachep, SLAB_KERNEL);
                 file_list_lock();
- if (f) {
- files_stat.nr_files++;
+ if (f)
                         goto new_one;
- }
+ files_stat.nr_files--;
                 /* Big problems... */
                 printk("VFS: filp allocation failed\n");

diff -ur linux-2.4.0-test12-pre7/include/linux/fs.h linux/include/linux/fs.h
--- linux-2.4.0-test12-pre7/include/linux/fs.h Fri Dec 8 15:06:55 2000
+++ linux/include/linux/fs.h Sun Dec 10 17:37:52 2000
@@ -57,7 +57,7 @@
 extern int leases_enable, dir_notify_enable, lease_break_time;

 #define NR_FILE 8192 /* this can well be larger on a larger system */
-#define NR_RESERVED_FILES 10 /* reserved for root */
+#define NR_RESERVED_FILES 96 /* reserved for root */
 #define NR_SUPER 256

 #define MAY_EXEC 1

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Fri Dec 15 2000 - 21:00:19 EST