Merge PR ceph#57192 into main

* refs/pull/57192/head:
	PendingReleaseNotes: add note on the client incompatibility health warning and feature bit
	doc/cephfs: add client_mds_auth_caps client feature bit
	doc/cephfs: add missing client feature bits
	doc/cephfs: document MDS_CLIENTS_BROKEN_ROOTSQUASH health error
	qa: add tests for MDS_CLIENTS_BROKEN_ROOTSQUASH
	mds: raise health warning if client lacks feature for root_squash
	mon/MDSMonitor: add note about missing metadata inclusion
	mds: check relevant caps for fs include root_squash
	mds: refactor out fs_name match in MDSAuthCaps
	qa: test for root_squash with multiple caps
	qa: pass kwargs to mount from remount
	qa: simplify update_attrs and only update relevant keys
	client: allow overriding client features

Reviewed-by: Xiubo Li <[email protected]>
Reviewed-by: Rishabh Dave <[email protected]>

Author: batrick

PendingReleaseNotes

@@ -186,6 +186,13 @@ CephFS: Disallow delegating preallocated inode ranges to clients. Config
   confirmation flag when some MDSs exhibit health warning MDS_TRIM or
   MDS_CACHE_OVERSIZED. This is to prevent accidental MDS failover causing
   further delays in recovery.
+* CephFS: fixes to the implementation of the ``root_squash`` mechanism enabled
+  via cephx ``mds`` caps on a client credential require a new client feature
+  bit, ``client_mds_auth_caps``. Clients using credentials with ``root_squash``
+  without this feature will trigger the MDS to raise a HEALTH_ERR on the
+  cluster, MDS_CLIENTS_BROKEN_ROOTSQUASH. See the documentation on this warning
+  and the new feature bit for more information.
+
 
 >=18.0.0
 

doc/cephfs/administration.rst

@@ -306,31 +306,47 @@ Clients that are missing newly added features will be evicted automatically.
 
 Here are the current CephFS features and first release they came out:
 
-+------------------+--------------+-----------------+
-| Feature          | Ceph release | Upstream Kernel |
-+==================+==============+=================+
-| jewel            | jewel        | 4.5             |
-+------------------+--------------+-----------------+
-| kraken           | kraken       | 4.13            |
-+------------------+--------------+-----------------+
-| luminous         | luminous     | 4.13            |
-+------------------+--------------+-----------------+
-| mimic            | mimic        | 4.19            |
-+------------------+--------------+-----------------+
-| reply_encoding   | nautilus     | 5.1             |
-+------------------+--------------+-----------------+
-| reclaim_client   | nautilus     | N/A             |
-+------------------+--------------+-----------------+
-| lazy_caps_wanted | nautilus     | 5.1             |
-+------------------+--------------+-----------------+
-| multi_reconnect  | nautilus     | 5.1             |
-+------------------+--------------+-----------------+
-| deleg_ino        | octopus      | 5.6             |
-+------------------+--------------+-----------------+
-| metric_collect   | pacific      | N/A             |
-+------------------+--------------+-----------------+
-| alternate_name   | pacific      | PLANNED         |
-+------------------+--------------+-----------------+
++----------------------------+--------------+-----------------+
+| Feature                    | Ceph release | Upstream Kernel |
++============================+==============+=================+
+| jewel                      | jewel        | 4.5             |
++----------------------------+--------------+-----------------+
+| kraken                     | kraken       | 4.13            |
++----------------------------+--------------+-----------------+
+| luminous                   | luminous     | 4.13            |
++----------------------------+--------------+-----------------+
+| mimic                      | mimic        | 4.19            |
++----------------------------+--------------+-----------------+
+| reply_encoding             | nautilus     | 5.1             |
++----------------------------+--------------+-----------------+
+| reclaim_client             | nautilus     | N/A             |
++----------------------------+--------------+-----------------+
+| lazy_caps_wanted           | nautilus     | 5.1             |
++----------------------------+--------------+-----------------+
+| multi_reconnect            | nautilus     | 5.1             |
++----------------------------+--------------+-----------------+
+| deleg_ino                  | octopus      | 5.6             |
++----------------------------+--------------+-----------------+
+| metric_collect             | pacific      | N/A             |
++----------------------------+--------------+-----------------+
+| alternate_name             | pacific      | 6.5             |
++----------------------------+--------------+-----------------+
+| notify_session_state       | quincy       | 5.19            |
++----------------------------+--------------+-----------------+
+| op_getvxattr               | quincy       | 6.0             |
++----------------------------+--------------+-----------------+
+| 32bits_retry_fwd           | reef         | 6.6             |
++----------------------------+--------------+-----------------+
+| new_snaprealm_info         | reef         | UNKNOWN         |
++----------------------------+--------------+-----------------+
+| has_owner_uidgid           | reef         | 6.6             |
++----------------------------+--------------+-----------------+
+| client_mds_auth_caps       | squid+bp     | PLANNED         |
++----------------------------+--------------+-----------------+
+
+..
+    Comment: use `git describe --tags --abbrev=0 <commit>` to lookup release
+
 
 CephFS Feature Descriptions
 
@@ -388,6 +404,15 @@ Clients can send performance metric to MDS if MDS support this feature.
 Clients can set and understand "alternate names" for directory entries. This is
 to be used for encrypted file name support.
 
+::
+
+    client_mds_auth_caps
+
+To effectively implement ``root_squash`` in a client's ``mds`` caps, the client
+must understand that it is enforcing ``root_squash`` and other cap metadata.
+Clients without this feature are in danger of dropping updates to files.  It is
+recommend to set this feature bit.
+
 
 Global settings
 ---------------

doc/cephfs/health-messages.rst

@@ -252,3 +252,20 @@ other daemons, please see :ref:`health-checks`.
     dirty data for cap revokes). If ``defer_client_eviction_on_laggy_osds`` is
     set to true (default true), client eviction will not take place and thus
     this health warning will be generated.
+
+``MDS_CLIENTS_BROKEN_ROOTSQUASH``
+---------------------------------
+  Message
+    "X client(s) with broken root_squash implementation (MDS_CLIENTS_BROKEN_ROOTSQUASH)"
+
+  Description
+    A bug was discovered in root_squash which would potentially lose changes made by a
+    client restricted with root_squash caps. The fix required a change to the protocol
+    and a client upgrade is required.
+
+    This is a HEALTH_ERR warning because of the danger of inconsistency and lost
+    data. It is recommended to either upgrade your clients, discontinue using
+    root_squash in the interim, or silence the warning if desired.
+
+    To evict and permanently block broken clients from connecting to the
+    cluster, set the ``required_client_feature`` bit ``client_mds_auth_caps``.

qa/tasks/cephfs/mount.py

@@ -544,30 +544,21 @@ def _verify_attrs(self, **kwargs):
                 raise RuntimeError('value of attributes should be either str '
                                    f'or None. {k} - {v}')
 
-    def update_attrs(self, client_id=None, client_keyring_path=None,
-                     client_remote=None, hostfs_mntpt=None, cephfs_name=None,
-                     cephfs_mntpt=None):
-        if not (client_id or client_keyring_path or client_remote or
-                cephfs_name or cephfs_mntpt or hostfs_mntpt):
-            return
-
-        self._verify_attrs(client_id=client_id,
-                           client_keyring_path=client_keyring_path,
-                           hostfs_mntpt=hostfs_mntpt, cephfs_name=cephfs_name,
-                           cephfs_mntpt=cephfs_mntpt)
-
-        if client_id:
-            self.client_id = client_id
-        if client_keyring_path:
-            self.client_keyring_path = client_keyring_path
-        if client_remote:
-            self.client_remote = client_remote
-        if hostfs_mntpt:
-            self.hostfs_mntpt = hostfs_mntpt
-        if cephfs_name:
-            self.cephfs_name = cephfs_name
-        if cephfs_mntpt:
-            self.cephfs_mntpt = cephfs_mntpt
+    def update_attrs(self, **kwargs):
+        verify_keys = [
+          'client_id',
+          'client_keyring_path',
+          'hostfs_mntpt',
+          'cephfs_name',
+          'cephfs_mntpt',
+        ]
+
+        self._verify_attrs(**{key: kwargs[key] for key in verify_keys if key in kwargs})
+
+        for k in verify_keys:
+            v = kwargs.get(k)
+            if v is not None:
+                setattr(self, k, v)
 
     def remount(self, **kwargs):
         """
@@ -590,7 +581,7 @@ def remount(self, **kwargs):
 
         self.update_attrs(**kwargs)
 
-        retval = self.mount(mntopts=mntopts, check_status=check_status)
+        retval = self.mount(mntopts=mntopts, check_status=check_status, **kwargs)
         # avoid this scenario (again): mount command might've failed and
         # check_status might have silenced the exception, yet we attempt to
         # wait which might lead to an error.

qa/tasks/cephfs/test_admin.py

@@ -1478,6 +1478,101 @@ def test_multifs_single_path_rootsquash(self):
         self.captester2.conduct_neg_test_for_chown_caps()
         self.captester2.conduct_neg_test_for_truncate_caps()
 
+    def test_multifs_rootsquash_nofeature(self):
+        """
+        That having root_squash on one fs doesn't prevent access to others.
+        """
+
+        if not isinstance(self.mount_a, FuseMount):
+            self.skipTest("only FUSE client has CEPHFS_FEATURE_MDS_AUTH_CAPS "
+                          "needed to enforce root_squash MDS caps")
+
+        self.fs1 = self.fs
+        self.fs2 = self.mds_cluster.newfs('testcephfs2')
+
+        self.mount_a.umount_wait()
+
+        # Authorize client to fs1
+        FS_AUTH_CAPS = (('/', 'rw'),)
+        self.fs1.authorize(self.client_id, FS_AUTH_CAPS)
+
+        FS_AUTH_CAPS = (('/', 'rw', 'root_squash'),)
+        keyring = self.fs2.authorize(self.client_id, FS_AUTH_CAPS)
+
+        CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK = 21
+        # all but CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK
+        features = ",".join([str(i) for i in range(CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK)])
+        mntargs = [f"--client_debug_inject_features={features}"]
+
+        # should succeed
+        with self.assert_cluster_log("report clients with broken root_squash", present=False):
+            keyring_path = self.mount_a.client_remote.mktemp(data=keyring)
+            self.mount_a.remount(client_id=self.client_id, client_keyring_path=keyring_path, mntargs=mntargs, cephfs_name=self.fs1.name)
+
+        captester = CapTester(self.mount_a, '/')
+        captester.conduct_pos_test_for_read_caps()
+        captester.conduct_pos_test_for_open_caps()
+
+    def test_rootsquash_nofeature(self):
+        """
+        That having root_squash on an fs without the feature bit raises a HEALTH_ERR warning.
+        """
+
+        if not isinstance(self.mount_a, FuseMount):
+            self.skipTest("only FUSE client has CEPHFS_FEATURE_MDS_AUTH_CAPS "
+                          "needed to enforce root_squash MDS caps")
+
+        self.mount_a.umount_wait()
+        self.mount_b.umount_wait()
+
+        FS_AUTH_CAPS = (('/', 'rw', 'root_squash'),)
+        keyring = self.fs.authorize(self.client_id, FS_AUTH_CAPS)
+
+        CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK = 21
+        # all but CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK
+        features = ",".join([str(i) for i in range(CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK)])
+        mntargs = [f"--client_debug_inject_features={features}"]
+
+        # should succeed
+        with self.assert_cluster_log("with broken root_squash implementation"):
+            keyring_path = self.mount_a.client_remote.mktemp(data=keyring)
+            self.mount_a.remount(client_id=self.client_id, client_keyring_path=keyring_path, mntargs=mntargs, cephfs_name=self.fs.name)
+            self.wait_for_health("MDS_CLIENTS_BROKEN_ROOTSQUASH", 60)
+            self.assertFalse(self.mount_a.is_blocked())
+
+        self.mount_a.umount_wait()
+        self.wait_for_health_clear(60)
+
+    def test_rootsquash_nofeature_evict(self):
+        """
+        That having root_squash on an fs without the feature bit can be evicted.
+        """
+
+        if not isinstance(self.mount_a, FuseMount):
+            self.skipTest("only FUSE client has CEPHFS_FEATURE_MDS_AUTH_CAPS "
+                          "needed to enforce root_squash MDS caps")
+
+        self.mount_a.umount_wait()
+        self.mount_b.umount_wait()
+
+        FS_AUTH_CAPS = (('/', 'rw', 'root_squash'),)
+        keyring = self.fs.authorize(self.client_id, FS_AUTH_CAPS)
+
+        CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK = 21
+        # all but CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK
+        features = ",".join([str(i) for i in range(CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK)])
+        mntargs = [f"--client_debug_inject_features={features}"]
+
+        # should succeed
+        keyring_path = self.mount_a.client_remote.mktemp(data=keyring)
+        self.mount_a.remount(client_id=self.client_id, client_keyring_path=keyring_path, mntargs=mntargs, cephfs_name=self.fs.name)
+        self.wait_for_health("MDS_CLIENTS_BROKEN_ROOTSQUASH", 60)
+
+        self.fs.required_client_features("add", "client_mds_auth_caps")
+        self.wait_for_health_clear(60)
+        self.assertTrue(self.mount_a.is_blocked())
+
+
     def test_single_path_rootsquash_issue_56067(self):
         """
         That a FS client using root squash MDS caps allows non-root user to write data

src/client/Client.cc

@@ -392,6 +392,12 @@ Client::Client(Messenger *m, MonClient *mc, Objecter *objecter_)
   if (cct->_conf->client_acl_type == "posix_acl")
     acl_type = POSIX_ACL;
 
+  if (auto str = cct->_conf->client_debug_inject_features; !str.empty()) {
+    myfeatures = feature_bitset_t(str);
+  } else {
+    myfeatures = feature_bitset_t(CEPHFS_FEATURES_CLIENT_SUPPORTED);
+  }
+
   lru.lru_set_midpoint(cct->_conf->client_cache_mid);
 
   // file handles
@@ -2354,7 +2360,7 @@ MetaSessionRef Client::_open_mds_session(mds_rank_t mds)
 
   auto m = make_message<MClientSession>(CEPH_SESSION_REQUEST_OPEN);
   m->metadata = metadata;
-  m->supported_features = feature_bitset_t(CEPHFS_FEATURES_CLIENT_SUPPORTED);
+  m->supported_features = myfeatures;
   m->metric_spec = feature_bitset_t(CEPHFS_METRIC_FEATURES_ALL);
   session->con->send_message2(std::move(m));
   return session;

src/client/Client.h

@@ -1936,6 +1936,8 @@ class Client : public Dispatcher, public md_config_obs_t {
   uint64_t nr_write_request = 0;
 
   std::vector<MDSCapAuth> cap_auths;
+
+  feature_bitset_t myfeatures;
 };
 
 /**

src/common/options/mds-client.yaml.in

@@ -251,6 +251,14 @@ options:
   default: 0
   services:
   - mds_client
+- name: client_debug_inject_features
+  type: str
+  level: dev
+  services:
+  - mds_client
+  flags:
+  - startup
+  with_legacy: true
 - name: client_max_inline_size
   type: size
   level: dev

src/mds/Beacon.cc

@@ -486,6 +486,30 @@ void Beacon::notify_health(MDSRank const *mds)
     health.metrics.push_back(m);
   }
 
+  // Report a health warning if clients have broken root_squash
+  if (auto c = mds->sessionmap.num_broken_root_squash_clients(); c > 0) {
+    std::vector<MDSHealthMetric> metrics;
+
+    for (auto&& session : mds->sessionmap.get_broken_root_squash_clients()) {
+      CachedStackStringStream css;
+      *css << "Client " << session->get_human_name() << " has broken root_squash implementation";
+      MDSHealthMetric m(MDS_HEALTH_CLIENTS_BROKEN_ROOTSQUASH, HEALTH_ERR, css->strv());
+      m.metadata["client_id"] = stringify(session->get_client());
+      metrics.emplace_back(std::move(m));
+    }
+
+    if (metrics.size() <= (size_t)g_conf()->mds_health_summarize_threshold) {
+      health.metrics.insert(std::end(health.metrics), std::make_move_iterator(std::begin(metrics)), std::make_move_iterator(std::end(metrics)));
+    } else {
+      CachedStackStringStream css;
+      *css << "There are " << c << " clients with broken root_squash implementations";
+      dout(20) << css->strv() << dendl;
+      MDSHealthMetric m(MDS_HEALTH_CLIENTS_BROKEN_ROOTSQUASH, HEALTH_ERR, css->strv());
+      m.metadata["client_count"] = stringify(c);
+      health.metrics.push_back(std::move(m));
+    }
+  }
+
   // Report if we have significantly exceeded our cache size limit
   if (mds->mdcache->cache_overfull()) {
     CachedStackStringStream css;

src/mds/MDSAuthCaps.h

@@ -158,6 +158,10 @@ struct MDSCapMatch {
   bool match_path(std::string_view target_path) const;
   std::string to_string();
 
+  bool match_fs(std::string_view target_fs) const {
+    return fs_name == target_fs || fs_name.empty() || fs_name == "*";
+  }
+
   void encode(ceph::buffer::list& bl) const {
     ENCODE_START(1, 1, bl);
     encode(uid, bl);
@@ -276,8 +280,7 @@ class MDSAuthCaps
     }
 
     for (const MDSCapGrant &g : grants) {
-      if (g.match.fs_name == fs_name || g.match.fs_name.empty() ||
-	  g.match.fs_name == "*") {
+      if (g.match.match_fs(fs_name)) {
 	if (mask & MAY_READ && g.spec.allow_read()) {
 	  return true;
 	}
@@ -300,10 +303,12 @@ class MDSAuthCaps
     }
   }
 
-  bool root_squash_in_caps() const {
-    for (const MDSCapGrant &g : grants) {
-      if (g.match.root_squash) {
-        return true;
+  bool root_squash_in_caps(std::string_view fs_name) const {
+    for (const MDSCapGrant& g : grants) {
+      if (g.match.match_fs(fs_name)) {
+        if (g.match.root_squash) {
+          return true;
+        }
       }
     }
     return false;