On Fri, 2025-09-12 at 12:25 +0530, Kotresh Hiremath Ravishankar wrote: > On Fri, Sep 12, 2025 at 12:46 AM Viacheslav Dubeyko > <Slava.Dubeyko@xxxxxxx> wrote: > > > > On Thu, 2025-09-11 at 15:02 +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 > > > v4: Code refactor, better warning message and > > > fix possible compiler warning > > > > > > URL: https://tracker.ceph.com/issues/72167 > > > Signed-off-by: Kotresh HR <khiremat@xxxxxxxxxx> > > > --- > > > fs/ceph/mds_client.c | 8 ++++++++ > > > fs/ceph/mdsmap.c | 13 ++++++++++++- > > > fs/ceph/super.c | 14 -------------- > > > fs/ceph/super.h | 14 ++++++++++++++ > > > 4 files changed, 34 insertions(+), 15 deletions(-) > > > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > > index 179130948041..97f9de888713 100644 > > > --- a/fs/ceph/mds_client.c > > > +++ b/fs/ceph/mds_client.c > > > @@ -5689,11 +5689,19 @@ 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; > > > > > > + doutc(cl, "fsname check fs_name=%s match.fs_name=%s\n", > > > + fs_name, auth->match.fs_name ? auth->match.fs_name : ""); > > > + if (auth->match.fs_name && strcmp(auth->match.fs_name, fs_name)) { > > > + /* fsname check failed, try next one */ > > > > I am still thinking that "check failed" sounds too strong here. Names simply > > don't match to each others and we need to check next one. But "check failed" > > sounds like error condition but not the normal flow. Potentially, I used the > > word "fail" too frequently for commenting the error conditions. :) > > Yeah, 'fsname mismatch, try next one' is a better choice. Please feel free > to change that while you submit. > Sounds good. > > > > > + return 0; > > > + } > > > + > > > 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..de457470d07c 100644 > > > --- a/fs/ceph/mdsmap.c > > > +++ b/fs/ceph/mdsmap.c > > > @@ -353,10 +353,21 @@ struct ceph_mdsmap *ceph_mdsmap_decode(struct ceph_mds_client *mdsc, void **p, > > > __decode_and_drop_type(p, end, u8, bad_ext); > > > } > > > if (mdsmap_ev >= 8) { > > > + u32 fsname_len; > > > /* enabled */ > > > ceph_decode_8_safe(p, end, m->m_enabled, bad_ext); > > > /* fs_name */ > > > - ceph_decode_skip_string(p, end, bad_ext); > > > + ceph_decode_32_safe(p, end, fsname_len, bad_ext); > > > + > > > + /* validate fsname against mds_namespace */ > > > + if (!namespace_equals(mdsc->fsc->mount_options, (const char *)*p, fsname_len)) { > > > + pr_warn_client(cl, "fsname %*pE doesn't match mds_namespace %s\n", > > > + (int)fsname_len, (char *)*p, > > > + mdsc->fsc->mount_options->mds_namespace); > > > + goto bad; > > > + } > > > + // skip fsname after validation > > > + ceph_decode_skip_n(p, end, fsname_len, bad); > > > } > > > /* damaged */ > > > if (mdsmap_ev >= 9) { > > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c > > > index 0e6787501b2f..2c6c45b2056d 100644 > > > --- a/fs/ceph/super.c > > > +++ b/fs/ceph/super.c > > > @@ -246,20 +246,6 @@ static void canonicalize_path(char *path) > > > path[j] = '\0'; > > > } > > > > > > -/* > > > - * Check if the mds namespace in ceph_mount_options matches > > > - * the passed in namespace string. First time match (when > > > - * ->mds_namespace is NULL) is treated specially, since > > > - * ->mds_namespace needs to be initialized by the caller. > > > - */ > > > -static int namespace_equals(struct ceph_mount_options *fsopt, > > > - const char *namespace, size_t len) > > > -{ > > > - return !(fsopt->mds_namespace && > > > - (strlen(fsopt->mds_namespace) != len || > > > - strncmp(fsopt->mds_namespace, namespace, len))); > > > -} > > > - > > > static int ceph_parse_old_source(const char *dev_name, const char *dev_name_end, > > > struct fs_context *fc) > > > { > > > diff --git a/fs/ceph/super.h b/fs/ceph/super.h > > > index 8cba1ce3b8b0..bb992f12e3b0 100644 > > > --- a/fs/ceph/super.h > > > +++ b/fs/ceph/super.h > > > @@ -104,6 +104,20 @@ struct ceph_mount_options { > > > struct fscrypt_dummy_policy dummy_enc_policy; > > > }; > > > > > > +/* > > > + * Check if the mds namespace in ceph_mount_options matches > > > + * the passed in namespace string. First time match (when > > > + * ->mds_namespace is NULL) is treated specially, since > > > + * ->mds_namespace needs to be initialized by the caller. > > > + */ > > > +static inline int namespace_equals(struct ceph_mount_options *fsopt, > > > + const char *namespace, size_t len) > > > +{ > > > + return !(fsopt->mds_namespace && > > > + (strlen(fsopt->mds_namespace) != len || > > > + strncmp(fsopt->mds_namespace, namespace, len))); > > > +} > > > + > > > /* mount state */ > > > enum { > > > CEPH_MOUNT_MOUNTING, > > > > Looks good. > > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> > > > > Let me run xfstests on your patch. I'll be back with the results ASAP. > > The xfstests has finished successfully. I don't see any regression. Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> Thanks, Slava.
