A field in files_struct has been used without initialization

A field in files_struct has been used without initialization

[Abd-Alrhman Masalkhi]

Hello Linux community,

I have came acrose the following code in dup_fd()

1	newf = kmem_cache_alloc(files_cachep, GFP_KERNEL);
2	if (!newf)
3		goto out;
4
5	atomic_set(&newf->count, 1);
6
7	spin_lock_init(&newf->file_lock);
8	newf->resize_in_progress = false;
9	init_waitqueue_head(&newf->resize_wait);
10	newf->next_fd = 0;
11	new_fdt = &newf->fdtab;
12	new_fdt->max_fds = NR_OPEN_DEFAULT;
13	new_fdt->close_on_exec = newf->close_on_exec_init;

On line 13 new_fdt->close_on_exec has given the value of
newf->close_on_exec_init, but new_fdt->close_on_exec itself has not
been initialized, is it intended to be like this.

Thanky you very much!

RE: A field in files_struct has been used without initialization

[Abd-Alrhman Masalkhi]

> new_fdt->close_on_exec itself has not been initialized, is it intended
> to be like this.

I meant:

newf->close_on_exec_init itself has not been initialized ...

Re: A field in files_struct has been used without initialization

[Al Viro]

On Thu, Oct 06, 2022 at 12:57:28PM +0200, Abd-Alrhman Masalkhi wrote:
> > new_fdt->close_on_exec itself has not been initialized, is it intended
> > to be like this.
> 
> I meant:
> 
> newf->close_on_exec_init itself has not been initialized ...

Huh? close_on_exec_init is an array, and this assignment stores the address
of its first (and only) element into newf->fdtab.close_on_exec.  So it's
basically
	newf->fdtab.close_on_exec = &newf->close_on_exec_init[0];

->fdtab and ->close_on_exec_init are to be used only if we need no more than
BITS_PER_LONG descriptors.  It's common enough to make avoiding a separate
allocation (and separate cacheline on following the pointer chain) worth
the trouble.

Note that we do not use newf->fdtab directly - we use newf->fdt.

What happens here is
        new_fdt = &newf->fdtab;
	...
	set newf->fdtab contents for the case we need few descriptors

	if we need more
		allocate a separate struct fdtable, bitmaps, etc.
		set the contents of that separate fdtable
		new_fdt = that new fdtable

	copy bitmaps into whatever new_fdt->close_on_exec and
	new_fdt->open_fds are pointing at.

	copy file pointers into whatever new_fdt->fd[] points at.

	set newf->fdt to new_fdt.

The value of newf->close_on_exec_init is simply newf + known constant.
It needs no initialization at all.  The *contents* of the array
it points to is used only if new_fdt remains pointing to newf->fdtab;
in that case it's initialized by
	copy_fd_bitmaps(new_fdt, old_fdt, open_files);

IOW, for few-descriptors case we end up with

newf:
	fdt			points to newf->fdtab
	fdtab.close_on_exec	points to newf->close_on_exec_init[0]
	fdtab.full_fd_bits	points to newf->full_fd_bits_init[0]
	fdtab.open_fds		points to newf->open_fds_init[0]
	fdtab.fd		points to newf->fd_array[0]
	fdtab.max_fds		max capacity; will be BITS_PER_LONG
	close_on_exec_init[0]	hosts the close_on_exec bitmap
	full_fd_bits_init[0]	hosts the full_fd_bits bitmap
	open_fds_init[0]	hosts the open_fds bitmap
	fd_array[]		contains file pointers (BITS_PER_LONG of them)

For more-than-a-few-descriptors case we have

array of pointers (from first kvmalloc() in alloc_fdtable()): contains file pointers
array of unsigned long (from the second kmvalloc() there): hosts the bitmaps
fdtable (allocated in alloc_fdtable()):
	open_fds		points to the beginning of bitmap-hosting array
	close_on_exec		points to the middle third of the same array
	full_fd_bits		points to the last third of the same array
	fd			points to array of struct file pointers.
	max_fds			max capacity, matches the sizes of arrays.
newf:
	fdt			points to fdtable
	fdtab, ..._init and fd_array 	unused

Might be useful to take a piece of paper and draw the picture,
I really don't want to bother with ASCII graphics for that...

RE: A field in files_struct has been used without initialization

[Abd-Alrhman Masalkhi]

Hello Al Viro

> close_on_exec_init is an array, and this assignment stores the address
> of its first (and only) element into newf->fdtab.close_on_exec.  So it's
> basically
>      newf->fdtab.close_on_exec = &newf->close_on_exec_init[0];
>
> ->fdtab and ->close_on_exec_init are to be used only if we need no more than
> BITS_PER_LONG descriptors.  It's common enough to make avoiding a separate
> allocation (and separate cacheline on following the pointer chain) worth
> the trouble
> ...
> ...

Fascinating, thank you for this very informative response. I have learned a
lot from it.

Re: A field in files_struct has been used without initialization

[Al Viro]

On Fri, Oct 07, 2022 at 08:16:11PM +0100, Al Viro wrote:

> array of pointers (from first kvmalloc() in alloc_fdtable()): contains file pointers
> array of unsigned long (from the second kmvalloc() there): hosts the bitmaps
> fdtable (allocated in alloc_fdtable()):
> 	open_fds		points to the beginning of bitmap-hosting array
> 	close_on_exec		points to the middle third of the same array
> 	full_fd_bits		points to the last third of the same array

Rereading that... a bit of clarification: the wording would seem to imply
that these 3 bitmaps are of equal size; they are not - the third one is
smaller.

If fdt->max_fds (table capacity) is equal to N, we have
	* N struct file pointers in fdt->fd[]
	* N bits in fdt->open_fds[] (i.e. N/BITS_PER_LONG unsigned long)
	* N bits in fdt->close_on_exec[] (ditto)
	* N/BITS_PER_LONG bits in fdt->full_fd_bits[]

The meaning of bitmaps:
	bit k set in open_fds[] - descriptor k is in use
	bit k set in close_on_exec[] - descriptor k is to be closed on exec()
	bit k set in full_fd_bits[] - all descriptors covered by open_fds[k]
(i.e. BITS_PER_LONG of them,, starting from k * BITS_PER_LONG) are in use.
In other words, "don't even bother looking for clear bits in open_fds[k];
there won't be any".

->full_fd_bits[] is there purely as a way to speed finding unused descriptors,
along with ->next_fd.  Some processes have an obscene amount of opened
descriptors and linear search through the mostly full bitmap can get painful.
E.g. 4 millions of descriptors would mean half megabyte worth of bitmap
(in ->open_fds[]).  Cheaper to search through 8Kb of ->full_fd_bits, then
check one 64bit word in ->open_fds[]; kinder on the cache as well...