Re: [PATCH] ceph: fix kernel crash in ceph_open()

[Ilya Dryomov <idryomov@gmail.com>]

On Mon, Dec 1, 2025 at 9:04 PM Viacheslav Dubeyko <Slava.Dubeyko@ibm.com> wrote:
>
> On Thu, 2025-11-27 at 13:03 +0530, Kotresh Hiremath Ravishankar wrote:
> > On Tue, Nov 25, 2025 at 2:42 AM Viacheslav Dubeyko
> > <Slava.Dubeyko@ibm.com> wrote:
> > >
> > > On Tue, 2025-11-25 at 00:48 +0530, Kotresh Hiremath Ravishankar wrote:
> > > > On Fri, Nov 21, 2025 at 1:47 AM Viacheslav Dubeyko
> > > > <Slava.Dubeyko@ibm.com> wrote:
> > > > >
> > > > > On Thu, 2025-11-20 at 19:50 +0530, Kotresh Hiremath Ravishankar wrote:
> > > > > > Hi All,
> > > > > >
> > > > > > I think the patch is necessary and fixes the crash. There is no harm
> > > > > > in taking this patch as it behaves like an old kernel with this
> > > > > > particular scenario.
> > > > > >
> > > > > > When does the issue happen:
> > > > > >    - The issue happens only when the old mount syntax is used where
> > > > > > passing the file system name is optional in which case, it chooses the
> > > > > > default mds namespace but doesn't get filled in the
> > > > > > mdsc->fsc->mount_options->mds_namespace.
> > > > > >    - Along with the above, the mount user should be non admin.
> > > > > > Does it break the earlier fix ?
> > > > > >    - Not fully!!! Though the open does succeed, the subsequent
> > > > > > operation like write would get EPERM. I am not exactly able to
> > > > > > recollect but this was discussed before writing the fix 22c73d52a6d0
> > > > > > ("ceph: fix multifs mds auth caps issue"), it's guarded by another
> > > > > > check before actual operation like write.
> > > > > >
> > > > > > I think there are a couple of options to fix this cleanly.
> > > > > >  1. Use the default fsname when
> > > > > > mdsc->fsc->mount_options->mds_namespace is NULL during comparison.
> > > > > >  2. Mandate passing the fsname with old syntax ?
> > > > > >
> > > > >
> > > > > Anyway, we should be ready operate correctly if fsname or/and auth-
> > > > > > match.fs_name are NULL. And if we need to make the fix more cleanly, then we
> > > > > can introduce another patch with nicer fix.
> > > > >
> > > > > I am not completely sure how default fsname can be applicable here. If I
> > > > > understood the CephFS mount logic correctly, then fsname can be NULL during some
> > > > > initial steps. But, finally, we will have the real fsname for comparison. But I
> > > > > don't know if it's right of assuming that fsname == NULL is equal to fsname ==
> > > > > default_name.
> > > >
> > > > We are pretty sure fsname is NULL only if the old mount syntax is used
> > > > without providing the
> > > > fsname in the optional arg. I believe kclient knows the fsname that's
> > > > mounted somewhere in this case ?
> > > > I am not sure though. If so, it can be used. If not, then can we rely
> > > > on what mds sends as part
> > > > of the mdsmap?
> > > >
> > > >
>
> <skipped>
>
> > >
> > > > >
> > > > > And I am not sure that we can mandate anyone to use the old syntax. If there is
> > > > > some other opportunity, then someone could use it. But, maybe, I am missing the
> > > > > point. :) What do you mean by "Mandate passing the fsname with old syntax"?
> > > >
> > > > In the old mount syntax, the fsname is passed as on optional argument
> > > > using 'mds_namespace'.
> > > > I was suggesting to mandate it if possible. But I guess it breaks
> > > > backward compatibility.
> > > >
> > > > >
> > > > >
> > >
> > > We had a private discussion with Ilya. Yes, he also mentioned the breaking of
> > > backward compatibility for the case of mandating passing the fsname with old
> > > syntax. He believes that: "Use the default fsname when mdsc->fsc->mount_options-
> > > > mds_namespace is NULL during comparison seems like a sensible approach to me".
> > >
> > >
>
> OK. So, what finally should we consider like a right solution/fix here?

Hi Slava,

I think the right solution would be a patch that establishes
consistency with the userspace client.  What does ceph-fuse do when
--client_fs option isn't passed?  It's the exact equivalent of
mds_namespace mount option (--client_mds_namespace is what it used to
be named), so the kernel client just needs to be made to do exactly the
same.

After taking a deeper look I doubt that using the default fs_name for
the comparison would be sufficient and not prone to edge cases.  First,
even putting the NULL dereference aside, both the existing check by
Kotresh

    if (auth->match.fs_name && strcmp(auth->match.fs_name, fs_name))
        /* mismatch */

and your proposed check

    if (!fs_name1 || !fs_name2)
        /* match */

    if (strcmp(fs_name1, fs_name2))
        /* mismatch */

aren't equivalent to

  bool match_fs(std::string_view target_fs) const {
    return fs_name == target_fs || fs_name.empty() || fs_name == "*";
  }

in src/mds/MDSAuthCaps.h -- "*" isn't handled at all.

Second, I'm not following a reason to only "validate" fs_name against
mds_namespace option in ceph_mdsmap_decode().  Why not hold onto it and
actually use it in ceph_mds_auth_match() for the comparison as done in
src/client/Client.cc?

int Client::mds_check_access(std::string& path, const UserPerm& perms, int mask)
{
  ...
  std::string_view fs_name = mdsmap->get_fs_name();   <---------
  for (auto& s: cap_auths) {
    ...
    if (s.match.match(fs_name, path, perms.uid(), perms.gid(), &gid_list)) {
      /* match */

AFAIU the default fs_name would come into the picture only in case of
a super ancient cluster with prior to mdsmap v8 encoding.

I haven't really looked at this code before, so it's possible that
there are other things that are missing/inconsistent here.  I'd ask
that the final patch is formally reviewed by Venky and Patrick as
they were the approvers on https://github.com/ceph/ceph/pull/64550
in userspace.

Thanks,

                Ilya