From: dparmar18 <dparmar@redhat.com>
Date: Mon, 26 Sep 2022 11:18:23 +0000 (+0530)
Subject: mds/Server: do not allow -ve reclaim flags to cause client eviction
X-Git-Tag: v16.2.13~116^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=3bbbe0d2947ef13819d44f67b3650a2bca1f8799;p=ceph.git

mds/Server: do not allow -ve reclaim flags to cause client eviction

Server::handle_client_reclaim uses get_flags() whose return type is uint32_t,
so if a negative value(int) is provided for reclaim flag, it gets converted
to a positive value(unsigned int) and this leads to incorrect evaluation of
reclaim flag.

So, for instance if -1 is sent as reclaim flag, 4294967295(typecased to unsigned int)
would be the value evaluated with MClientReclaim::FLAG_FINISH and thus wrongly
calls Server::finish_reclaim_session instead of Server::reclaim_session, which
leads to unexepected client eviction.

This can fixed if we check the signed value of flag before calling out anything
by typecasting the value of m->get_flags() to int32_t.
(reason typecasting to int32_t is get_flags() returning value of type uint32_t).

Fixes: https://tracker.ceph.com/issues/57359
Signed-off-by: Dhairya Parmar <dparmar@redhat.com>
(cherry picked from commit c0390f528e0b80ee494c3916e4d1ca6cad6f3c77)
---

diff --git a/src/mds/Server.cc b/src/mds/Server.cc
index 4912b3babd7..55e7028e055 100644
--- a/src/mds/Server.cc
+++ b/src/mds/Server.cc
@@ -503,6 +503,7 @@ void Server::finish_reclaim_session(Session *session, const ref_t<MClientReclaim
 void Server::handle_client_reclaim(const cref_t<MClientReclaim> &m)
 {
   Session *session = mds->get_session(m);
+  uint32_t flags = m->get_flags();
   dout(3) << __func__ <<  " " << *m << " from " << m->get_source() << dendl;
   ceph_assert(m->is_a_client()); // should _not_ come from an mds!
 
@@ -522,7 +523,15 @@ void Server::handle_client_reclaim(const cref_t<MClientReclaim> &m)
     return;
   }
 
-  if (m->get_flags() & MClientReclaim::FLAG_FINISH) {
+  if (flags & MClientReclaim::FLAG_FINISH) {
+    if (flags ^ MClientReclaim::FLAG_FINISH) {
+      dout(0) << __func__ << " client specified FLAG_FINISH with other flags."
+                             " Other flags:" << flags << dendl;
+      auto reply = make_message<MClientReclaimReply>(0);
+      reply->set_result(-CEPHFS_EINVAL);
+      mds->send_message_client(reply, session);
+      return;
+    }
     finish_reclaim_session(session);
   } else {
     reclaim_session(session, m);
diff --git a/src/test/client/ops.cc b/src/test/client/ops.cc
index 1cb78625e43..e6e25135e23 100644
--- a/src/test/client/ops.cc
+++ b/src/test/client/ops.cc
@@ -35,5 +35,11 @@ TEST_F(TestClient, CheckZeroReclaimFlag) {
   ASSERT_EQ(client->check_unknown_reclaim_flag(0), true);
 }
 TEST_F(TestClient, CheckUnknownReclaimFlag) {
+  ASSERT_EQ(client->check_unknown_reclaim_flag(2), true);
+}
+TEST_F(TestClient, CheckNegativeReclaimFlagUnmasked) {
   ASSERT_EQ(client->check_unknown_reclaim_flag(-1 & ~MClientReclaim::FLAG_FINISH), true);
 }
+TEST_F(TestClient, CheckNegativeReclaimFlag) {
+  ASSERT_EQ(client->check_unknown_reclaim_flag(-1), true);
+}