Re: [PATCH RFC 4/4] UNFINISHED mm, fs: use kmem_cache_charge() in path_openat()

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

On Fri, Mar 01, 2024 at 09:51:18AM -0800, Linus Torvalds wrote:

> Right. I think the natural and logical way to deal with this is to
> just say "we account when we add the file to the fdtable".
> 
> IOW, just have fd_install() do it. That's the really natural point,
> and also makes it very logical why alloc_empty_file_noaccount()
> wouldn't need to do the GFP_KERNEL_ACCOUNT.

We can have the same file occuring in many slots of many descriptor tables,
obviously.  So it would have to be a flag (in ->f_mode?) set by it, for
"someone's already charged for it", or you'll end up with really insane
crap on each fork(), dup(), etc.

But there's also MAP_ANON with its setup_shmem_file(), with the resulting
file not going into descriptor tables at all, and that's not a rare thing.

> > - I don't know how to properly unwind the accounting failure case. It
> >   seems like a new case because when we succeed the open, there's no
> >   further error path at least in path_openat().
> 
> Yeah, let me think about this part. Becasue fd_install() is the right
> point, but that too does not really allow for error handling.
> 
> Yes, we could close things and fail it, but it really is much too late
> at this point.

That as well.  For things like O_CREAT even do_dentry_open() would be too
late for unrolls.

> What I *think* I'd want for this case is
> 
>  (a) allow the accounting to go over by a bit
> 
>  (b) make sure there's a cheap way to ask (before) about "did we go
> over the limit"
> 
> IOW, the accounting never needed to be byte-accurate to begin with,
> and making it fail (cheaply and early) on the next file allocation is
> fine.
> 
> Just make it really cheap. Can we do that?

That might be reasonable, but TBH I would rather combine that with
do_dentry_open()/alloc_file() (i.e. the places where we set FMODE_OPENED)
as places to do that, rather than messing with fd_install().

How does the following sound?
	* those who allocate empty files mark them if they are intended
to be kernel-internal (see below for how to get the information there)
	* memcg charge happens when we set FMODE_OPENED, provided that
struct file instance is not marked kernel-internal.
	* exceeding the limit => pretend we'd succeeded and fail the
next allocation.

As for how to get the information down there...  We have 6 functions
where "allocate" and "mark it opened" callchains converge -
alloc_file() (pipe(2) et.al., mostly), path_openat() (normal opens,
but also filp_open() et.al.), dentry_open(), kernel_file_open(),
kernel_tmpfile_open(), dentry_create().  The last 3 are all
kernel-internal; dentry_open() might or might not be.

For path_openat() we can add a bit somewhere in struct open_flags;
the places where we set struct open_flags up would be the ones that
might need to be annotated.  That's
	file_open_name()
	file_open_root()
	do_sys_openat2() (definitely userland)
	io_openat2() (ditto)
	sys_uselib() (ditto)
	do_open_execat() (IMO can be considered userland in all cases)

For alloc_file() it's almost always userland.  IMO things like
dma_buf_export() and setup_shmem_file() should be charged.

So it's a matter of propagating the information to dentry_open(),
file_open_name() and file_open_root().  That's about 70 callers
to annotate, including filp_open() and file_open_root_mnt() into
the mix.  <greps>  61, actually, and from the quick look it
seems that most of them are really obvious...

Comments?