Re: [PATCH]: Brown paper bag in fs/file.c?

[Dipankar Sarma <dipankar@in.ibm.com>]

On Wed, Sep 14, 2005 at 11:31:33AM -0700, David S. Miller wrote:
> 
> The bug is that free_fd_array() takes a "num" argument, but when
> calling it from __free_fdtable() we're instead passing in the size in
> bytes (ie. "num * sizeof(struct file *)").

Yes it is a bug. I think I messed it up while merging newer
changes with an older version where I was using size in bytes
to optimize.

> 
> How come this doesn't crash things for people?  Perhaps I'm missing
> something.  fs/vmalloc.c should bark very loudly if we call it with a
> non-vmalloc area address, since that is what would happen if we pass a
> kmalloc() SLAB object address to vfree().
> 
> I think I know what might be happening.  If the miscalculation means
> that we kfree() the embedded fdarray, that would actually work just
> fine, and free up the fdtable.  I guess if the SLAB redzone stuff were
> enabled for these caches, it would trigger when something like this
> happens.

__free_fdtable() is used only when the fdarray/fdset are vmalloced
(use of the workqueue) or there is a race between two expand_files().
That might be why we haven't seen this cause any explicit problem
so far.

This would be an appropriate patch - (untested). I will update
as soon as testing is done.

Thanks
Dipankar



Fixes the fdtable freeing in the case of vmalloced fdset/arrays.

Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com>
--


 fs/file.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff -puN fs/file.c~files-fix-fdtable-free fs/file.c
--- linux-2.6.14-rc1-fd/fs/file.c~files-fix-fdtable-free	2005-09-15 00:36:03.000000000 +0530
+++ linux-2.6.14-rc1-fd-dipankar/fs/file.c	2005-09-15 00:39:46.000000000 +0530
@@ -69,13 +69,9 @@ void free_fd_array(struct file **array, 
 
 static void __free_fdtable(struct fdtable *fdt)
 {
-	int fdset_size, fdarray_size;
-
-	fdset_size = fdt->max_fdset / 8;
-	fdarray_size = fdt->max_fds * sizeof(struct file *);
-	free_fdset(fdt->open_fds, fdset_size);
-	free_fdset(fdt->close_on_exec, fdset_size);
-	free_fd_array(fdt->fd, fdarray_size);
+	free_fdset(fdt->open_fds, fdt->max_fdset);
+	free_fdset(fdt->close_on_exec, fdt->max_fdset);
+	free_fd_array(fdt->fd, fdt->max_fds);
 	kfree(fdt);
 }
 

_