client: set metadata["root"] from mount method when it's called with … #12505

Merged
merged 1 commit into from Dec 20, 2016

Projects

None yet

4 participants

@jtlayton
Contributor
jtlayton commented Dec 15, 2016 edited

…a pathname

Currently, we only set the root properly in the config file or the
--client_metadata command line option. If a userland client program
tries to call ceph_mount with a pathname, it's not being properly
set.

Since we already hold the mutex, we can just update it directly.

Signed-off-by: Jeff Layton jlayton@redhat.com

@jtlayton
Contributor
jtlayton commented Dec 15, 2016 edited

Test run last night with this seems to have mostly passed, with some failures due to unrelated reasons (AFAICT):

http://pulpito.ceph.com/jlayton-2016-12-15_00:30:20-fs-wip-jlayton-submount---basic-smithi/

@liewegas liewegas added this to the kraken milestone Dec 15, 2016
@gregsfortytwo
Member

This obviously fixes it one way around, but as you mentioned on the ticket the MDS-side thing is a bit wonky. Do we need to zap the mds-side check entirely? (I'm worried about older clients, for instance.)

@jtlayton
Contributor
jtlayton commented Dec 15, 2016 edited

Yeah, maybe. I really don't quite get the purpose of that check. Note that the first thing we do after creating the session is a GETATTR of the "root". Presumably that would also fail if you don't have caps for it, even if we removed this check. I don't really see the harm in allowing session creation to succeed as long as we ensure that that session can't get access to the parts of the tree where it doesn't have the necessary caps. We have to do that anyway or a malicious client could "mount" one part of the tree and start working in another.

So yeah, let me see about just removing that check from the MDS.

@jtlayton
Contributor
jtlayton commented Dec 16, 2016 edited

Ok, added a patch to eliminate the "root" check in session setup code. It works correctly in my testing. If I limit the credentials to a subtree then mounts of "/" still fail with -EACCES.

Now, that said... @jcsp added that check relatively recently (commit 4932cb9, though it has gone through several changes since). We should definitely get his ACK on this before we merge it.

I have this patch building in ceph-ci now and I'll plan to run it through the fs suite.

@jtlayton jtlayton requested a review from jcsp Dec 16, 2016
@jtlayton jtlayton client: set metadata["root"] from mount method when it's called with …
…a pathname

Currently, we only set the root properly config file or the
--client_metadata command line option. If a userland client program
tries to call ceph_mount with a pathname, it's not being properly
set.

Since we already hold the mutex, we can just update it directly.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
9f88100
@jtlayton
Contributor

@jcsp says that we do need to have the MDS track and enforce "root", as that info is used in session eviction. So, I dropped the mds patch, and we'll just plan to backport the client-side fix to jewel (and maybe add some workarounds in samba and nfs-ganesha to tie us over).

@jcsp
jcsp approved these changes Dec 19, 2016 View changes
@liewegas liewegas merged commit 34ab000 into master Dec 20, 2016

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
@liewegas liewegas deleted the wip-jlayton-submount branch Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment