linux-kernel - Re: [PATCH RFC 4/4] UNFINISHED mm, fs: use kmem_cache_charge() in path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240324022731.GR538574@ZenIV>
Date: Sun, 24 Mar 2024 02:27:31 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Vlastimil Babka <vbabka@...e.cz>, Josh Poimboeuf <jpoimboe@...nel.org>,
	Jeff Layton <jlayton@...nel.org>,
	Chuck Lever <chuck.lever@...cle.com>, Kees Cook <kees@...nel.org>,
	Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>,
	David Rientjes <rientjes@...gle.com>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Roman Gushchin <roman.gushchin@...ux.dev>,
	Hyeonggon Yoo <42.hyeyoo@...il.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Michal Hocko <mhocko@...nel.org>,
	Shakeel Butt <shakeelb@...gle.com>,
	Muchun Song <muchun.song@...ux.dev>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	cgroups@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH RFC 4/4] UNFINISHED mm, fs: use kmem_cache_charge() in
 path_openat()

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?