Hi Al,
Thanks for looking. Somehow I also missed CCing you, whoops,
On Wed, Nov 08, 2023 at 04:57:49PM +0000, Al Viro wrote:
> 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...
Thanks, I can re-work it to look like this.
> > 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?
No, I just kept it from the original series. I agree that it should be
safe to drop.
> BTW, what about the locking hierarchy? In the current tree ->files_lock
> nests inside of everything; what happens with your patches in place?
If I understand correctly you're asking about ->files_lock nesting
inside of task_lock()? I tried to make the cgroup side in this patch
do the same thing in the same order. Or am I misunderstanding?
I did test this with some production container traffic and didn't see
anything too strange, but no doubt there are complicated edge cases
here.
Thanks,
Tycho
