Merge pull request ceph#57591 from aclamk/wip-aclamk-denc-compat-check

common, os/bluestore: Fix lack of checking for compat on DENC_START

Reviewed-by: Radoslaw Zarzynski <[email protected]>

doc/dev/release-checklists.rst

@@ -98,6 +98,7 @@ Code cleanup
   `ceph_release_t::*`)
 - [ ] search code for `require_osd_release`
 - [ ] search code for `min_mon_release`
+- [ ] check include/denc.h if DENC_START macro still needs reference to squid
 
 QA suite
 --------

src/CMakeLists.txt

@@ -70,6 +70,10 @@ configure_file(
   ${CMAKE_SOURCE_DIR}/src/ceph_ver.h.in.cmake
   ${CMAKE_BINARY_DIR}/src/include/ceph_ver.h
   @ONLY)
+configure_file(
+  ${CMAKE_SOURCE_DIR}/src/ceph_release.h.in.cmake
+  ${CMAKE_BINARY_DIR}/src/include/ceph_release.h
+  @ONLY)
 
 add_definitions(
   -DHAVE_CONFIG_H

src/ceph_release.h.in.cmake

@@ -0,0 +1,8 @@
+#ifndef CEPH_RELEASE_H
+#define CEPH_RELEASE_H
+
+#define CEPH_RELEASE @CEPH_RELEASE@
+#define CEPH_RELEASE_NAME "@CEPH_RELEASE_NAME@"
+#define CEPH_RELEASE_TYPE "@CEPH_RELEASE_TYPE@"
+
+#endif

src/ceph_ver.h.in.cmake

@@ -3,8 +3,7 @@
 
 #define CEPH_GIT_VER @CEPH_GIT_VER@
 #define CEPH_GIT_NICE_VER "@CEPH_GIT_NICE_VER@"
-#define CEPH_RELEASE @CEPH_RELEASE@
-#define CEPH_RELEASE_NAME "@CEPH_RELEASE_NAME@"
-#define CEPH_RELEASE_TYPE "@CEPH_RELEASE_TYPE@"
+
+#include "ceph_release.h"
 
 #endif

src/include/denc.h

@@ -51,6 +51,9 @@
 
 #include "common/convenience.h"
 #include "common/error_code.h"
+#include "common/likely.h"
+#include "ceph_release.h"
+#include "include/rados.h"
 
 template<typename T, typename=void>
 struct denc_traits {
@@ -1781,6 +1784,13 @@ inline std::enable_if_t<traits::supported && !traits::featured> decode_nohead(
 // wrappers for DENC_{START,FINISH} for inter-version
 // interoperability.
 
+[[maybe_unused]] static void denc_compat_throw(
+  const char* _pretty_function_, uint8_t code_v,
+  uint8_t struct_v, uint8_t struct_compat) {
+  throw ::ceph::buffer::malformed_input("Decoder at '" + std::string(_pretty_function_) +
+    "' v=" + std::to_string(code_v)+ " cannot decode v=" + std::to_string(struct_v) +
+    " minimal_decoder=" + std::to_string(struct_compat));
+}
 #define DENC_HELPERS							\
   /* bound_encode */							\
   static void _denc_start(size_t& p,					\
@@ -1818,8 +1828,11 @@ inline std::enable_if_t<traits::supported && !traits::featured> decode_nohead(
 			  __u8 *struct_compat,				\
 			  char **start_pos,				\
 			  uint32_t *struct_len) {			\
+    __u8 code_v = *struct_v;						\
     denc(*struct_v, p);							\
     denc(*struct_compat, p);						\
+    if (unlikely(code_v < *struct_compat))				\
+      denc_compat_throw(__PRETTY_FUNCTION__, code_v, *struct_v, *struct_compat);\
     denc(*struct_len, p);						\
     *start_pos = const_cast<char*>(p.get_pos());			\
   }									\
@@ -1840,11 +1853,38 @@ inline std::enable_if_t<traits::supported && !traits::featured> decode_nohead(
 // Helpers for versioning the encoding.  These correspond to the
 // {ENCODE,DECODE}_{START,FINISH} macros.
 
+// DENC_START interface suggests it is checking compatibility,
+// but the feature was unimplemented until SQUID.
+// Due to -2 compatibility rule we cannot bump up compat until U____ release.
+// SQUID=19 T____=20 U____=21
+
 #define DENC_START(v, compat, p)					\
   __u8 struct_v = v;							\
   __u8 struct_compat = compat;						\
   char *_denc_pchar;							\
   uint32_t _denc_u32;							\
+  static_assert(CEPH_RELEASE >= (CEPH_RELEASE_SQUID /*19*/ + 2) || compat == 1);	\
+  _denc_start(p, &struct_v, &struct_compat, &_denc_pchar, &_denc_u32);	\
+  do {
+
+// For the only type that is with compat 2: unittest.
+#define DENC_START_COMPAT_2(v, compat, p)				\
+  __u8 struct_v = v;							\
+  __u8 struct_compat = compat;						\
+  char *_denc_pchar;							\
+  uint32_t _denc_u32;							\
+  static_assert(CEPH_RELEASE >= (CEPH_RELEASE_SQUID /*19*/ + 2) || compat == 2);	\
+  _denc_start(p, &struct_v, &struct_compat, &_denc_pchar, &_denc_u32);	\
+  do {
+
+// For osd_reqid_t which cannot be upgraded at all.
+// We used it to communicate with clients and now we cannot safely upgrade.
+#define DENC_START_OSD_REQID(v, compat, p)				\
+  __u8 struct_v = v;							\
+  __u8 struct_compat = compat;						\
+  char *_denc_pchar;							\
+  uint32_t _denc_u32;							\
+  static_assert(compat == 2, "osd_reqid_t cannot be upgraded");		\
   _denc_start(p, &struct_v, &struct_compat, &_denc_pchar, &_denc_u32);	\
   do {
 

src/include/encoding.h

@@ -1466,7 +1466,11 @@ decode(std::array<T, N>& v, bufferlist::const_iterator& p)
 #define ENCODE_FINISH(bl) ENCODE_FINISH_NEW_COMPAT(bl, 0)
 
 #define DECODE_ERR_OLDVERSION(func, v, compatv)					\
-  (std::string(func) + " no longer understand old encoding version " #v " < " + std::to_string(compatv))
+  (std::string(func) + " no longer understands old encoding version " #v " < " + std::to_string(compatv))
+
+#define DECODE_ERR_NO_COMPAT(func, code_v, v, compatv)					\
+  ("Decoder at '" + std::string(func) + "' v=" + std::to_string(code_v) +		\
+  " cannot decode v=" + std::to_string(v) + " minimal_decoder=" + std::to_string(compatv))
 
 #define DECODE_ERR_PAST(func) \
   (std::string(func) + " decode past end of struct encoding")
@@ -1494,7 +1498,7 @@ decode(std::array<T, N>& v, bufferlist::const_iterator& p)
   decode(struct_v, bl);						\
   decode(struct_compat, bl);						\
   if (v < struct_compat)						\
-    throw ::ceph::buffer::malformed_input(DECODE_ERR_OLDVERSION(__PRETTY_FUNCTION__, v, struct_compat)); \
+    throw ::ceph::buffer::malformed_input(DECODE_ERR_NO_COMPAT(__PRETTY_FUNCTION__, v, struct_v, struct_compat)); \
   __u32 struct_len;							\
   decode(struct_len, bl);						\
   if (struct_len > bl.get_remaining())					\
@@ -1512,7 +1516,7 @@ decode(std::array<T, N>& v, bufferlist::const_iterator& p)
     __u8 struct_compat;							\
     decode(struct_compat, bl);					\
     if (v < struct_compat)						\
-      throw ::ceph::buffer::malformed_input(DECODE_ERR_OLDVERSION(__PRETTY_FUNCTION__, v, struct_compat)); \
+      throw ::ceph::buffer::malformed_input(DECODE_ERR_NO_COMPAT(__PRETTY_FUNCTION__, v, struct_v, struct_compat)); \
   } else if (skip_v) {							\
     if (bl.get_remaining() < skip_v)					\
       throw ::ceph::buffer::malformed_input(DECODE_ERR_PAST(__PRETTY_FUNCTION__)); \
@@ -1554,8 +1558,8 @@ decode(std::array<T, N>& v, bufferlist::const_iterator& p)
     __u8 struct_compat;							\
     decode(struct_compat, bl);						\
     if (v < struct_compat)						\
-      throw ::ceph::buffer::malformed_input(DECODE_ERR_OLDVERSION(	\
-	__PRETTY_FUNCTION__, v, struct_compat));			\
+      throw ::ceph::buffer::malformed_input(DECODE_ERR_NO_COMPAT(	\
+	__PRETTY_FUNCTION__, v, struct_v, struct_compat));		\
   }									\
   unsigned struct_end = 0;						\
   if (struct_v >= lenv) {						\

src/osd/osd_types.h

@@ -164,7 +164,7 @@ struct osd_reqid_t {
   {}
 
   DENC(osd_reqid_t, v, p) {
-    DENC_START(2, 2, p);
+    DENC_START_OSD_REQID(2, 2, p);
     denc(v.name, p);
     denc(v.tid, p);
     denc(v.inc, p);

src/test/encoding.cc

@@ -327,7 +327,7 @@ TEST(EncodingException, Macros) {
   } tests[] = {
     {
       DECODE_ERR_OLDVERSION(__PRETTY_FUNCTION__, 100, 200),
-      fmt::format("{} no longer understand old encoding version 100 < 200: Malformed input",
+      fmt::format("{} no longer understands old encoding version 100 < 200: Malformed input",
                   __PRETTY_FUNCTION__)
     },
     {

src/test/test_denc.cc

@@ -352,23 +352,37 @@ struct foo_t {
 };
 WRITE_CLASS_DENC_BOUNDED(foo_t)
 
-struct foo2_t {
-  int32_t c = 0;
-  uint64_t d = 123;
+struct foo2_accept1_t {
+  int32_t a = 0;
+  uint64_t b = 123;
+  int32_t c = -1; // uninitialized for v1
 
-  DENC(foo2_t, v, p) {
-    DENC_START(1, 1, p);
-    ::denc(v.c, p);
-    ::denc(v.d, p);
+  DENC(foo2_accept1_t, v, p) {
+    DENC_START(2, 1, p);
+    ::denc(v.a, p);
+    ::denc(v.b, p);
+    if (struct_v >= 2) {
+      ::denc(v.c, p);
+    }
     DENC_FINISH(p);
   }
+};
+WRITE_CLASS_DENC_BOUNDED(foo2_accept1_t)
 
-  friend bool operator==(const foo2_t& l, const foo2_t& r) {
-    return l.c == r.c && l.d == r.d;
+struct foo2_only2_t {
+  int32_t a = 0;
+  uint64_t b = 123;
+  uint32_t c = 55;
+
+  DENC(foo2_only2_t, v, p) {
+    DENC_START_COMPAT_2(2, 2, p);
+    ::denc(v.a, p);
+    ::denc(v.b, p);
+    ::denc(v.c, p);
+    DENC_FINISH(p);
   }
 };
-WRITE_CLASS_DENC_BOUNDED(foo2_t)
-
+WRITE_CLASS_DENC_BOUNDED(foo2_only2_t)
 
 struct bar_t {
   int32_t a = 0;
@@ -741,3 +755,42 @@ TEST(denc, no_copy_if_segmented_and_lengthy)
     ASSERT_EQ(CEPH_PAGE_SIZE * 2, Legacy::n_decode);
   }
 }
+
+TEST(denc, compat_allows)
+{
+  foo_t v1;
+  v1.a = 5001; v1.b = 6002;
+  size_t s = 0;
+  denc(v1, s);
+  bufferlist bl;
+  {
+    auto app = bl.get_contiguous_appender(s);
+    denc(v1, app);
+  }
+
+  foo2_accept1_t v2;
+  v2.a = 111; v2.b = 111; v2.c = 111;
+  auto bpi = bl.front().begin();
+  denc(v2, bpi);
+  ASSERT_EQ(v1.a, v2.a);
+  ASSERT_EQ(v1.b, v2.b);
+  ASSERT_EQ(111, v2.c);
+}
+
+TEST(denc, compat_disallows)
+{
+  foo2_only2_t v2;
+  v2.a = 5001; v2.b = 6002; v2.c = 7003;
+  size_t s = 0;
+  denc(v2, s);
+  bufferlist bl;
+  {
+    auto app = bl.get_contiguous_appender(s);
+    denc(v2, app);
+  }
+
+  foo_t v1;
+  v1.a = 111; v1.b = 111;
+  auto bpi = bl.front().begin();
+  ASSERT_ANY_THROW(denc(v1,bpi));
+}