Sorry, couldn't get to this after my PTO. Comments inline. On Tue, Aug 26, 2025 at 12:02 AM Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> wrote: > > On Mon, 2025-08-25 at 00:18 +0530, khiremat@xxxxxxxxxx wrote: > > From: Kotresh HR <khiremat@xxxxxxxxxx> > > > > The mds auth caps check should also validate the > > fsname along with the associated caps. Not doing > > so would result in applying the mds auth caps of > > one fs on to the other fs in a multifs ceph cluster. > > The bug causes multiple issues w.r.t user > > authentication, following is one such example. > > > > Steps to Reproduce (on vstart cluster): > > 1. Create two file systems in a cluster, say 'fsname1' and 'fsname2' > > 2. Authorize read only permission to the user 'client.usr' on fs 'fsname1' > > $ceph fs authorize fsname1 client.usr / r > > 3. Authorize read and write permission to the same user 'client.usr' on fs 'fsname2' > > $ceph fs authorize fsname2 client.usr / rw > > 4. Update the keyring > > $ceph auth get client.usr >> ./keyring > > > > With above permssions for the user 'client.usr', following is the > > expectation. > > a. The 'client.usr' should be able to only read the contents > > and not allowed to create or delete files on file system 'fsname1'. > > b. The 'client.usr' should be able to read/write on file system 'fsname2'. > > > > But, with this bug, the 'client.usr' is allowed to read/write on file > > system 'fsname1'. See below. > > > > 5. Mount the file system 'fsname1' with the user 'client.usr' > > $sudo bin/mount.ceph usr@.fsname1=/ /kmnt_fsname1_usr/ > > 6. Try creating a file on file system 'fsname1' with user 'client.usr'. This > > should fail but passes with this bug. > > $touch /kmnt_fsname1_usr/file1 > > 7. Mount the file system 'fsname1' with the user 'client.admin' and create a > > file. > > $sudo bin/mount.ceph admin@.fsname1=/ /kmnt_fsname1_admin > > $echo "data" > /kmnt_fsname1_admin/admin_file1 > > 8. Try removing an existing file on file system 'fsname1' with the user > > 'client.usr'. This shoudn't succeed but succeeds with the bug. > > $rm -f /kmnt_fsname1_usr/admin_file1 > > > > For more information, please take a look at the corresponding mds/fuse patch > > and tests added by looking into the tracker mentioned below. > > > > v2: Fix a possible null dereference in doutc > > v3: Don't store fsname from mdsmap, validate against > > ceph_mount_options's fsname and use it > > > > URL: https://tracker.ceph.com/issues/72167 > > Signed-off-by: Kotresh HR <khiremat@xxxxxxxxxx> > > --- > > fs/ceph/mds_client.c | 10 ++++++++++ > > fs/ceph/mdsmap.c | 14 +++++++++++++- > > 2 files changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > index ce0c129f4651..638a12626432 100644 > > --- a/fs/ceph/mds_client.c > > +++ b/fs/ceph/mds_client.c > > @@ -5680,11 +5680,21 @@ static int ceph_mds_auth_match(struct ceph_mds_client *mdsc, > > u32 caller_uid = from_kuid(&init_user_ns, cred->fsuid); > > u32 caller_gid = from_kgid(&init_user_ns, cred->fsgid); > > struct ceph_client *cl = mdsc->fsc->client; > > + const char *fs_name = mdsc->fsc->mount_options->mds_namespace; > > const char *spath = mdsc->fsc->mount_options->server_path; > > bool gid_matched = false; > > u32 gid, tlen, len; > > int i, j; > > > > The doutc is debug output and it will never be shown without enabling it. So, it > will be completely enough to place the doutc one time for both cases here. > ok. makes sense. I will get this done in the next patch version. > > + if (auth->match.fs_name && strcmp(auth->match.fs_name, fs_name)) { > > + doutc(cl, "fsname check failed fs_name=%s match.fs_name=%s\n", > > + fs_name, auth->match.fs_name); > > + return 0; > > If the check is failed, then it sounds to me that we need to show an error > message here and return error code: > > pr_err_client(<error message>); > return -EINVAL; ???? > > Am I correct here? No. The code should continue to validate the next fsname in the mds cap and see if it matches. > > > + } else { > > + doutc(cl, "fsname check passed fs_name=%s match.fs_name=%s\n", > > + fs_name, auth->match.fs_name ? auth->match.fs_name : ""); > > + } > > + > > doutc(cl, "match.uid %lld\n", auth->match.uid); > > if (auth->match.uid != MDS_AUTH_UID_ANY) { > > if (auth->match.uid != caller_uid) > > diff --git a/fs/ceph/mdsmap.c b/fs/ceph/mdsmap.c > > index 8109aba66e02..44f435351daa 100644 > > --- a/fs/ceph/mdsmap.c > > +++ b/fs/ceph/mdsmap.c > > @@ -356,7 +356,19 @@ struct ceph_mdsmap *ceph_mdsmap_decode(struct ceph_mds_client *mdsc, void **p, > > /* enabled */ > > ceph_decode_8_safe(p, end, m->m_enabled, bad_ext); > > /* fs_name */ > > - ceph_decode_skip_string(p, end, bad_ext); > > + const char *mds_namespace = mdsc->fsc->mount_options->mds_namespace; > > + u32 fsname_len; > > I am afraid we could have compiler warnings for such C declarations. Let's have > all declarations in the beginning of scope: > > if (mdsmap_ev >= 8) { > const char *mds_namespace = mdsc->fsc->mount_options->mds_namespace; > u32 fsname_len; > > /* enabled */ > ceph_decode_8_safe(p, end, m->m_enabled, bad_ext); > /* fs_name */ > <rest logic> > } > Sure, I will fix this up in the next patch version. > > + ceph_decode_32_safe(p, end, fsname_len, bad_ext); > > + > > + void *sp = *p; > > What the point to introduce sp variable but not to use p pointer directly? Any > particular reason? > No. It was overlooked. I will address this in the next patch version. > > + if (!(mds_namespace && > > + strlen(mds_namespace) == fsname_len && > > + !strncmp(mds_namespace, (char *)sp, fsname_len))) { > > Frankly speaking, I think to introduce a static inline function for this check > could make the code cleaner. I mean something like this: > > if (fsname_mismatch()) { > <complain> > goto bad; > } Makes sense. I will address this in the next patch version. > > > + pr_warn_client(cl, "fsname doesn't match\n"); > > What's about sharing the mismatched names? > > pr_warn_client(cl, "fsname %s doesn't match to mds_namespace %s\n"); > I was doubtful initially that this code can get executed before the mds_namespace is initialized and also if fsname can be null. I think this can be done as pr_warn_client handles NULL gracefully? > Thanks, > Slava. > > > + goto bad; > > + } > > + // skip fsname after validation > > + ceph_decode_skip_n(p, end, fsname_len, bad); > > } > > /* damaged */ > > if (mdsmap_ev >= 9) { >
