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 b6a13360138495d4eaa422bc939f68ed3b38f222..8b5b06720755d6982597fbcecf55bf7743aa398d 100644 (file)
--- a/src/mds/Server.cc
+++ b/src/mds/Server.cc
@@ -506,6 +506,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!
 
@@ -525,7 +526,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 1cb78625e43808bccd828a2b892af804eee40ec5..e6e25135e236498cb177422f38796c0dfd9fa5cd 100644 (file)
--- 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);
+}