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

[Tycho Andersen <tycho@tycho.pizza>]

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