On Tue, 2 Apr 2024 at 14:00, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> 1) location of that hook is wrong. It's really "how do we catch
> file creation that does not come through open() - yes, you can use
> mknod(2) for that". It should've been after the call of vfs_create(),
> not the entire switch. LSM folks have a disturbing fondness of inserting
> hooks in various places, but IMO this one has no business being where
> they'd placed it. Bikeshedding regarding the name/arguments/etc. for
> that thing is, IMO, not interesting...
Hmm. I guess that's right - for a non-file node, there's nothing that
the security layer can really check after-the-fact anyway.
It's not like you can attest the contents of a character device or whatever...
> 2) the only ->mknod() instance in the tree that tries to leave
> dentry unhashed negative on success is CIFS (and only one case in it).
> From conversation with CIFS folks it's actually cheaper to instantiate
> in that case as well - leaving instantiation to the next lookup will
> cost several extra roundtrips for no good reason.
Ack.
> 3) documentation (in vfs.rst) is way too vague. The actual
> rules are
> * ->create() must instantiate on success
> * ->mkdir() is allowed to return unhashed negative on success and
> it might be forced to do so in some cases. If a caller of vfs_mkdir()
> wants the damn thing positive, it should account for such possibility and do
> a lookup. Normal callers don't care; see e.g. nfsd and overlayfs for example
> of those that do.
> * ->mknod() is interesting - historically it had been "may leave
> unhashed negative", but e.g. unix_bind() expected that it won't do so;
> the reason it didn't blow up for CIFS is that this case (SFU) of their mknod()
> does not support FIFOs and sockets anyway. Considering how few instances
> try to make use of that option and how it doesn't actually save them
> anything, I would prefer to declare that ->mknod() should act as ->create().
> * ->symlink() - not sure; there are instances that make use of that
> option (coda and hostfs). OTOH, the only callers of vfs_symlink() that
> care either way are nfsd and overlayfs, and neither is usable with coda
> or hostfs... Could go either way, but we need to say it clearly in the
> docs, whichever way we choose.
Fair enough.
Anyway, it does sound like maybe the minimal fix would be just that
"move it into the
case 0: case S_IFREG:
path".
Although if somebody already has the cifs patch to just do the
d_instantiate() for mknod, that might be even better.
I will leave this in more competent hands for now.
Let the bike-shedding commence,
Linus
