lkml.org 
[lkml]   [2006]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] fdset's leakage
Andrew,

>>Another patch from Alexey Kuznetsov fixing memory leak in alloc_fdtable().
>>
>>[PATCH] fdset's leakage
>>
>>When found, it is obvious. nfds calculated when allocating fdsets
>>is rewritten by calculation of size of fdtable, and when we are
>>unlucky, we try to free fdsets of wrong size.
>>
>>Found due to OpenVZ resource management (User Beancounters).
>>
>>Signed-Off-By: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
>>Signed-Off-By: Kirill Korotaev <dev@openvz.org>
>>
>>
>>diff -urp linux-2.6-orig/fs/file.c linux-2.6/fs/file.c
>>--- linux-2.6-orig/fs/file.c 2006-07-10 12:10:51.000000000 +0400
>>+++ linux-2.6/fs/file.c 2006-07-10 14:47:01.000000000 +0400
>>@@ -277,11 +277,13 @@ static struct fdtable *alloc_fdtable(int
>> } while (nfds <= nr);
>> new_fds = alloc_fd_array(nfds);
>> if (!new_fds)
>>- goto out;
>>+ goto out2;
>> fdt->fd = new_fds;
>> fdt->max_fds = nfds;
>> fdt->free_files = NULL;
>> return fdt;
>>+out2:
>>+ nfds = fdt->max_fdset;
>> out:
>> if (new_openset)
>> free_fdset(new_openset, nfds);
>
>
> OK, that was a simple fix. And if we need this fix backported to 2.6.17.x
> then it'd be best to go with the simple fix.
>
> And I think we do need to backport this to 2.6.17.x because NR_OPEN can be
> really big, and vmalloc() is not immortal.
>
> But the code in there is really sick. In all cases we do:
>
> free_fdset(foo->open_fds, foo->max_fdset);
> free_fdset(foo->close_on_exec, foo->max_fdset);
>
> How much neater and more reliable would it be to do:
>
> free_fdsets(foo);
>
> ?
agree. should I prepare a patch?

> Also,
>
> nfds = NR_OPEN_DEFAULT;
> /*
> * Expand to the max in easy steps, and keep expanding it until
> * we have enough for the requested fd array size.
> */
> do {
> #if NR_OPEN_DEFAULT < 256
> if (nfds < 256)
> nfds = 256;
> else
> #endif
> if (nfds < (PAGE_SIZE / sizeof(struct file *)))
> nfds = PAGE_SIZE / sizeof(struct file *);
> else {
> nfds = nfds * 2;
> if (nfds > NR_OPEN)
> nfds = NR_OPEN;
> }
> } while (nfds <= nr);
>
>
> That's going to take a long time to compute if nr > NR_OPEN. I just fixed
> a similar infinite loop in this function. Methinks this
>
> nfds = max(NR_OPEN_DEFAULT, 256);
> nfds = max(nfds, PAGE_SIZE/sizeof(struct file *));
> nfds = max(nfds, round_up_pow_of_two(nr + 1));
> nfds = min(nfds, NR_OPEN);
>
> is clearer and less buggy. I _think_ it's also equivalent (as long as
> NR_OPEN>256). But please check my logic.
Yeah, I also noticed these nasty loops but was too lazy to bother :)
Too much crap for my nerves :)

Your logic looks fine for me. Do we have already round_up_pow_of_two() function or
should we create it as something like:
unsinged long round_up_pow_of_two(unsigned long x)
{
unsigned long res = 1 << BITS_PER_LONG;
while (res > x)
res >>= 1;
}
return res << 1;
}

or maybe using:
n = find_first_bit(x);
return res = 1 << n;
(though it depends on endianness IMHO)
?

Thanks,
Kirill

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

\
 
 \ /
  Last update: 2006-07-11 11:07    [W:0.159 / U:1.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site