Why is it that in fs/nfsd/vfs.c only vfs_mknod() and vfs_rename() are

surrounded by mnt_want_write/mnt_drop_write, and not the other

operations (vfs_create, vfs_mkdir, vfs_symlink, ...)?
I noticed this while looking at the AppArmor patches, which need to

pass the vfsmount down to the security module.  And I'm wondering, why

can't mnt_want_write() and mnt_drop_write() be done _inside_ vfs_foo()?
I know there are a few cases, where filesystems call vfs_foo()

internally, where the vfsmount isn't available, but I think the proper

solution is just to fix those places, and not recurse back into the

VFS (which is AFAICS in all those cases totally unnecessary anyway).

This would make everybody happy, no?
Miklos

--

Apparmor can go play with itself.  The proper fix is to lift the LSM nonsense

into callers and leave vfs_...() alone; vfsmounts should *not* be passed

there at all, with the exception of vfs_follow_link() which gets the full

nameidata.

--

Maybe.  I know precious little about this security thing, so I won't

argue about it's merits or faults.  But:
 a) I have a hunch that the security guys wouldn't like to see the

 order between permission() and security_foo() changed.
 b) I fail to see how moving functionality to callers would improve
Why?
Miklos

--

It's their problem.  Adjusting LSM methods, if needed, is up to LSM

maintainers, whenever moving the hooks or code around those become

convenient for kernel proper.  According to Linus, IIRC.
Especially since in this case they want to change prototypes anyway, so the

churn is not an issue and having the hook called earlier is very unlikely to
Because filesystem shouldn't _care_ where it is mounted.  Anything

vfsmount-dependent belongs to upper layers.  The same goes for passing

nameidata to fs methods, BTW - again, ->follow_link() is an obvious

legitimate exception.

--

Nobody wants to send vfsmounts to the filesystem.  But vfs_...() are

still part of the "upper layer", not the filesystem, so I'm not

convinced yet.  For example:
-extern int vfs_mkdir(struct inode *, struct dentry *, int);

+extern int vfs_mkdir(const struct path *, struct dentry *, int);
There's one caller of vfs_mkdir that can't do this: cgroup_clone().

But that can call cgroup_mkdir() instead.
And having the vfsmount available within vfs_...() functions means,

that the mnt_want_write() check can be moved inside, which means that

callers get simpler and less likely to be buggy.  Those are all

advantages IMO, regardless of any security module issues.
Miklos

--

Or we can introduce another set of exported functions (path_mkdir(),

...), and leave vfs_...() alone.  And then the only question is if

LSM's can live with ordering change.
Miklos

--

I really don't see the point of new helpers; especially since one doesn't

have to _have_ vfsmount to use the old ones and since we don't have a lot

of users of each of those to start with.
As for the apparmor and friends...  I'm far past the point of trying to

give them feedback, seeing how any such feedback is ignored, if not worse

(the ugliness of some of the suggested kludges is bloody astonishing - e.g.

some guy proposed to stuff a reference to vfsmount into task_struct, etc.)

--

Traditionally we have syscalls, and nfsd.  Both of them want the

security checks, and I think nfsd wants the read-only mount checking

as well, but I'm not entirely sure.  Maybe we can handle that by just

making nfsd acquire a write-ref on the mount and keep it while it's

exported.
Then there's ecryptfs and unionfs, which probably need neither, but it

wouldn't hurt to do them anyway.
Still, even if there are only two callers, then moving stuff to up

doesn't make any sense.  Passing down a struct path is free for the

syscall case, it doesn't consume any stack space or extra CPU.  Do

please tell, why would that be such a bad thing?
Miklos

--

The only question for me would be where the current r/o checks are

happening (IS_RDONLY()).  I generally based my patches on replacing

those calls.  
-- Dave
--

In may_create()/may_delete() on parent directory.  So that one needs

audit of all callers, unless Al can be convinced that moving those

checks into the VFS makes sense.
Miklos

--

Because we'd been that way before; see the shitpiles around ->lookup()

getting nameidata, etc.  You'll end up with some callers passing NULL

as ->mnt since they don't have anything better to pass, some stuff

called *from* the damn thing caring to check for ->mnt being NULL,

some stuff not caring about what ->mnt is at all and some assuming

that it's not NULL.  Which will lead to exploding combinations that

won't be noticed until somebody steps into such config.
As for the vfsmount-dependent checks (and any kind of MAC, while we are

at it)...  They belong to callers, exactly because different callers may

want different (amount of) checks.

--

Right, we do want to prevent that happening.
And for example moving read-only mount checks inside vfs_...() would
And we end up random callers forgetting some of the checks, like we

have now with nfsd.  Not good at all.
I think it's still a lot better all the checks are always done, even

if not strictly necessary for a certain caller, than if the caller has

to make sure the necessary ones do get done.
Assuming of course, that all valid users _do_ have the vfsmount

available, which I think is true.  If you have a counterexample,

please let us know.
If not all (but most) callers have the vfsmount available, then a new

helper makes sense.
If there was only one caller which needed a certain check, then moving

that into the caller would be the right thing of course.  But that's

not the case here.
Miklos

--

TOMOYO is one of AppArmor's friends, and I am the guy who proposed to pass

I'm not asking to pass the vfsmount parameter to individual filesystem's "vfs functions".

I'm asking to pass the vfsmount parameter to the "vfs *helper* functions".

So, filesytem will not care where it is mounted

even if the vfsmount is passed to "vfs *helper* functions".

The vfs helper functions are designed to aggregate common checks (permission checks,

inode_operations->foo existence checks etc.) to avoid scattering same code everywhere

At least, calls to vfs helper functions from userland code

_do_ have the vfsmount available because they are called immediately after the name resolution.
Calls to vfs helper functions from kernel code does not always have the vfsmount available,

but that's beyond what the LSM can do.

We must trust kernel code, because kernel code can bypass the LSM check

if the kernel code is malicious enough to directly call "vfs functions" instead of

calling "vfs helper functions".

(Or, more simply, kernel code can rewrite the call to the LSM check to no-op

like funny workaround for vmsplice's vulnerability).
I think attempt to receive the vfsmount from kernel code won't help guaranteeing that

LSM's security checks are always performed.

Routes to access "vfs functions" from kernel code is undeterminable.

In other words, LSM can't guarantee that LSM's security checks are always performed

against the kernel code regardless of the security model.
But, at least, LSM can guarantee that LSM's security checks are always performed

against the userland code regardless of the security model.

Routes to access "vfs functions" from userland code is determinable.
So, making the vfsmount available to LSM make sense.

I don't care if the vfsmount is unavailable when the vfs helper function call was

issued from kernel code.

--