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

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

Hi Patrick,

On Tue, 2025-12-02 at 15:21 -0500, Patrick Donnelly wrote:
> 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".
> 
> 

Is the attached patch ready for review and testing? Should we wait the final
version of the patch?

Thanks,
Slava.