Re: [RFC 4/6] misc cgroup: introduce an fd counter

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Nov 07, 2023 at 05:26:45PM -0700, Tycho Andersen wrote:

> +	if (!charge_current_fds(newf, count_open_files(new_fdt)))
> +		return newf;

Are you sure that on configs that are not cgroup-infested compiler
will figure out that count_open_files() would have no side effects
and doesn't need to be evaluated?

Incidentally, since you are adding your charge/uncharge stuff on each
allocation/freeing, why not simply maintain an accurate counter, cgroup or
no cgroup?  IDGI...  Make it an inlined helper right there in fs/file.c,
doing increment/decrement and, conditional upon config, calling
the cgroup side of things.  No need to look at fdt, etc. outside
of fs/file.c either - the counter can be picked right from the
files_struct...

>  static void __put_unused_fd(struct files_struct *files, unsigned int fd)
>  {
>  	struct fdtable *fdt = files_fdtable(files);
> +	if (test_bit(fd, fdt->open_fds))
> +		uncharge_current_fds(files, 1);

Umm...  Just where do we call it without the bit in ->open_fds set?
Any such caller would be a serious bug; suppose you are trying to
call __put_unused_fd(files, N) while N is not in open_fds.  Just before
your call another thread grabs a descriptor and picks N.  Resulting
state won't be pretty, especially if right *after* your call the
third thread also asks for a descriptor - and also gets N.

Sure, you have an exclusion on ->file_lock, but AFAICS all callers
are under it and in all callers except for put_unused_fd() we
have just observed a non-NULL file reference in ->fd[N]; that
would *definitely* be a hard constraint violation if it ever
happened with N not in ->open_fds at that moment.

So the only possibility would be a broken caller of put_unused_fd(),
and any such would be a serious bug.

Details, please - have you ever observed that?

BTW, what about the locking hierarchy?  In the current tree ->files_lock
nests inside of everything; what happens with your patches in place?