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

[Patrick Donnelly <pdonnell@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2931 bytes --]

I started work on a patch and it is largely in agreement with what
Ilya suggested below.

On Tue, Dec 2, 2025 at 6:30 AM Ilya Dryomov <idryomov@gmail.com> wrote:
> 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.

We should match the ceph-fuse client behavior.

Attached is the patch (I've not built) which roughly gets us there.
The missing bit will be the "*" glob matching.

In summary, we should definitely start decoding `fs_name` from the
MDSMap and do strict authorizations checks against it. Note that the
`--mds_namespace` should only be used for selecting the file system to
mount and nothing else. It's possible no mds_namespace is specified
but the kernel will mount the only file system that exists which may
have name "foo".


--
Patrick Donnelly, Ph.D.
He / Him / His
Red Hat Partner Engineer
IBM, Inc.
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D

[-- Attachment #2: fs_name.patch --]
[-- Type: application/x-patch, Size: 2299 bytes --]