Merge pull request ceph#55945 from lxbsz/wip-64679-new

vshankar · web-flow · commit cf514170fb52 · 2024-06-28T22:42:17.000+05:30
mds/client: return -ENODATA when xattr doesn't exist for removexattr

Reviewed-by: Venky Shankar &lt;vshankar@redhat.com&gt;
diff --git a/qa/workunits/libcephfs/test.sh b/qa/workunits/libcephfs/test.sh
@@ -7,5 +7,6 @@ ceph_test_libcephfs_lazyio
 ceph_test_libcephfs_newops
 ceph_test_libcephfs_suidsgid
 ceph_test_libcephfs_snapdiff
+ceph_test_libcephfs_vxattr
 
 exit 0
diff --git a/src/client/Client.cc b/src/client/Client.cc
@@ -13868,7 +13868,7 @@ int Client::_do_setxattr(Inode *in, const char *name, const void *value,
 
   int xattr_flags = 0;
   if (!value)
-    xattr_flags |= CEPH_XATTR_REMOVE;
+    xattr_flags |= CEPH_XATTR_REMOVE | CEPH_XATTR_REMOVE2;
   if (flags & XATTR_CREATE)
     xattr_flags |= CEPH_XATTR_CREATE;
   if (flags & XATTR_REPLACE)
@@ -13926,6 +13926,7 @@ int Client::_setxattr(Inode *in, const char *name, const void *value,
       mode_t new_mode = in->mode;
       if (value) {
 	int ret = posix_acl_equiv_mode(value, size, &new_mode);
+	ldout(cct, 3) << __func__ << "(" << in->ino << ", \"" << name << "\") = " << ret << dendl;
 	if (ret < 0)
 	  return ret;
 	if (ret == 0) {
@@ -13975,6 +13976,11 @@ int Client::_setxattr(Inode *in, const char *name, const void *value,
       ret = -CEPHFS_EOPNOTSUPP;
   }
 
+  if ((!strcmp(name, ACL_EA_ACCESS) ||
+      !strcmp(name, ACL_EA_DEFAULT)) &&
+      ret == -CEPHFS_ENODATA)
+    ret = 0;
+
   return ret;
 }
 
@@ -14063,7 +14069,7 @@ int Client::ll_setxattr(Inode *in, const char *name, const void *value,
 
   vinodeno_t vino = _get_vino(in);
 
-  ldout(cct, 3) << __func__ << " " << vino << " " << name << " size " << size << dendl;
+  ldout(cct, 3) << __func__ << " " << vino << " " << name << " size " << size << " value " << !!value << dendl;
   tout(cct) << __func__ << std::endl;
   tout(cct) << vino.ino.val << std::endl;
   tout(cct) << name << std::endl;
diff --git a/src/include/ceph_fs.h b/src/include/ceph_fs.h
@@ -488,6 +488,7 @@ int ceph_flags_sys2wire(int flags);
  */
 #define CEPH_XATTR_CREATE  (1 << 0)
 #define CEPH_XATTR_REPLACE (1 << 1)
+#define CEPH_XATTR_REMOVE2 (1 << 30)
 #define CEPH_XATTR_REMOVE  (1 << 31)
 
 /*
diff --git a/src/mds/Server.cc b/src/mds/Server.cc
@@ -6534,6 +6534,11 @@ int Server::xattr_validate(CInode *cur, const InodeStoreBase::xattr_map_const_pt
       return -CEPHFS_ENODATA;
     }
 
+    if ((flags & CEPH_XATTR_REMOVE2) && !(xattrs && xattrs->count(mempool::mds_co::string(xattr_name)))) {
+      dout(10) << "setxattr '" << xattr_name << "' XATTR_REMOVE2 and CEPHFS_ENODATA on " << *cur << dendl;
+      return -CEPHFS_ENODATA;
+    }
+
     return 0;
   }
 
@@ -6743,7 +6748,7 @@ void Server::handle_client_setxattr(const MDRequestRef& mdr)
   pi.inode->change_attr++;
   pi.inode->xattr_version++;
 
-  if ((flags & CEPH_XATTR_REMOVE)) {
+  if ((flags & (CEPH_XATTR_REMOVE | CEPH_XATTR_REMOVE2))) {
     std::invoke(handler->removexattr, this, cur, pi.xattrs, xattr_op);
   } else {
     std::invoke(handler->setxattr, this, cur, pi.xattrs, xattr_op);
diff --git a/src/test/libcephfs/CMakeLists.txt b/src/test/libcephfs/CMakeLists.txt
@@ -10,7 +10,6 @@ if(WITH_LIBCEPHFS)
     main.cc
     deleg.cc
     monconfig.cc
-    vxattr.cc
   )
   target_link_libraries(ceph_test_libcephfs
     ceph-common
@@ -50,6 +49,21 @@ if(WITH_LIBCEPHFS)
   install(TARGETS ceph_test_libcephfs_suidsgid
     DESTINATION ${CMAKE_INSTALL_BINDIR})
 
+  add_executable(ceph_test_libcephfs_vxattr
+    vxattr.cc
+    main.cc
+  )
+  target_link_libraries(ceph_test_libcephfs_vxattr
+    ceph-common
+    cephfs
+    librados
+    ${UNITTEST_LIBS}
+    ${EXTRALIBS}
+    ${CMAKE_DL_LIBS}
+    )
+  install(TARGETS ceph_test_libcephfs_vxattr
+    DESTINATION ${CMAKE_INSTALL_BINDIR})
+
   add_executable(ceph_test_libcephfs_newops
     main.cc
     newops.cc
diff --git a/src/test/libcephfs/test.cc b/src/test/libcephfs/test.cc
@@ -3520,39 +3520,6 @@ TEST(LibCephFS, SetMountTimeout) {
   ceph_shutdown(cmount);
 }
 
-TEST(LibCephFS, FsCrypt) {
-  struct ceph_mount_info *cmount;
-  ASSERT_EQ(ceph_create(&cmount, NULL), 0);
-  ASSERT_EQ(ceph_conf_read_file(cmount, NULL), 0);
-  ASSERT_EQ(0, ceph_conf_parse_env(cmount, NULL));
-  ASSERT_EQ(ceph_mount(cmount, NULL), 0);
-
-  char test_xattr_file[NAME_MAX];
-  sprintf(test_xattr_file, "test_fscrypt_%d", getpid());
-  int fd = ceph_open(cmount, test_xattr_file, O_RDWR|O_CREAT, 0666);
-  ASSERT_GT(fd, 0);
-
-  ASSERT_EQ(0, ceph_fsetxattr(cmount, fd, "ceph.fscrypt.auth", "foo", 3, CEPH_XATTR_CREATE));
-  ASSERT_EQ(0, ceph_fsetxattr(cmount, fd, "ceph.fscrypt.file", "foo", 3, CEPH_XATTR_CREATE));
-
-  char buf[64];
-  ASSERT_EQ(3, ceph_fgetxattr(cmount, fd, "ceph.fscrypt.auth", buf, sizeof(buf)));
-  ASSERT_EQ(3, ceph_fgetxattr(cmount, fd, "ceph.fscrypt.file", buf, sizeof(buf)));
-  ASSERT_EQ(0, ceph_close(cmount, fd));
-
-  ASSERT_EQ(0, ceph_unmount(cmount));
-  ASSERT_EQ(0, ceph_mount(cmount, NULL));
-
-  fd = ceph_open(cmount, test_xattr_file, O_RDWR, 0666);
-  ASSERT_GT(fd, 0);
-  ASSERT_EQ(3, ceph_fgetxattr(cmount, fd, "ceph.fscrypt.auth", buf, sizeof(buf)));
-  ASSERT_EQ(3, ceph_fgetxattr(cmount, fd, "ceph.fscrypt.file", buf, sizeof(buf)));
-
-  ASSERT_EQ(0, ceph_close(cmount, fd));
-  ASSERT_EQ(0, ceph_unmount(cmount));
-  ceph_shutdown(cmount);
-}
-
 TEST(LibCephFS, SnapdirAttrs) {
   struct ceph_mount_info *cmount;
   ASSERT_EQ(ceph_create(&cmount, NULL), 0);
diff --git a/src/test/libcephfs/vxattr.cc b/src/test/libcephfs/vxattr.cc
@@ -383,3 +383,75 @@ TEST(LibCephFS, GetAndSetDirRandom) {
 
   ceph_shutdown(cmount);
 }
+
+TEST(LibCephFS, FsCrypt) {
+  struct ceph_mount_info *cmount;
+  ASSERT_EQ(ceph_create(&cmount, NULL), 0);
+  ASSERT_EQ(ceph_conf_read_file(cmount, NULL), 0);
+  ASSERT_EQ(0, ceph_conf_parse_env(cmount, NULL));
+  ASSERT_EQ(ceph_mount(cmount, NULL), 0);
+
+  char test_xattr_file[NAME_MAX];
+  sprintf(test_xattr_file, "test_fscrypt_%d", getpid());
+  int fd = ceph_open(cmount, test_xattr_file, O_RDWR|O_CREAT, 0666);
+  ASSERT_GT(fd, 0);
+
+  ASSERT_EQ(0, ceph_fsetxattr(cmount, fd, "ceph.fscrypt.auth", "foo", 3, XATTR_CREATE));
+  ASSERT_EQ(0, ceph_fsetxattr(cmount, fd, "ceph.fscrypt.file", "foo", 3, XATTR_CREATE));
+
+  char buf[64];
+  ASSERT_EQ(3, ceph_fgetxattr(cmount, fd, "ceph.fscrypt.auth", buf, sizeof(buf)));
+  ASSERT_EQ(3, ceph_fgetxattr(cmount, fd, "ceph.fscrypt.file", buf, sizeof(buf)));
+  ASSERT_EQ(0, ceph_close(cmount, fd));
+
+  ASSERT_EQ(0, ceph_unmount(cmount));
+  ASSERT_EQ(0, ceph_mount(cmount, NULL));
+
+  fd = ceph_open(cmount, test_xattr_file, O_RDWR, 0666);
+  ASSERT_GT(fd, 0);
+  ASSERT_EQ(3, ceph_fgetxattr(cmount, fd, "ceph.fscrypt.auth", buf, sizeof(buf)));
+  ASSERT_EQ(3, ceph_fgetxattr(cmount, fd, "ceph.fscrypt.file", buf, sizeof(buf)));
+
+  ASSERT_EQ(0, ceph_close(cmount, fd));
+  ASSERT_EQ(0, ceph_unmount(cmount));
+  ceph_shutdown(cmount);
+}
+
+#define ACL_EA_ACCESS  "system.posix_acl_access"
+#define ACL_EA_DEFAULT "system.posix_acl_default"
+
+TEST(LibCephFS, Removexattr) {
+  struct ceph_mount_info *cmount;
+  ASSERT_EQ(ceph_create(&cmount, NULL), 0);
+  ASSERT_EQ(ceph_conf_read_file(cmount, NULL), 0);
+  ASSERT_EQ(0, ceph_conf_parse_env(cmount, NULL));
+  ASSERT_EQ(ceph_mount(cmount, NULL), 0);
+
+  char test_xattr_file[NAME_MAX];
+  sprintf(test_xattr_file, "test_removexattr_%d", getpid());
+  int fd = ceph_open(cmount, test_xattr_file, O_RDWR|O_CREAT, 0666);
+  ASSERT_GT(fd, 0);
+
+  // remove xattr
+  ASSERT_EQ(-CEPHFS_ENODATA, ceph_fremovexattr(cmount, fd, "user.remove.xattr"));
+  ASSERT_EQ(0, ceph_fsetxattr(cmount, fd, "user.remove.xattr", "foo", 3, XATTR_CREATE));
+  ASSERT_EQ(0, ceph_fremovexattr(cmount, fd, "user.remove.xattr"));
+
+  // remove xattr via setxattr & XATTR_REPLACE
+  ASSERT_EQ(-CEPHFS_ENODATA, ceph_fsetxattr(cmount, fd, "user.remove.xattr", nullptr, 0, XATTR_REPLACE));
+  ASSERT_EQ(0, ceph_fsetxattr(cmount, fd, "user.remove.xattr", "foo", 3, XATTR_CREATE));
+  ASSERT_EQ(0, ceph_fsetxattr(cmount, fd, "user.remove.xattr", nullptr, 0, XATTR_REPLACE));
+
+  // ACL_EA_ACCESS and ACL_EA_DEFAULT are special and will always return success.
+  // If the corresponding attributes exist already the first one will remove it
+  // and the second one will remove the non-existing acl attributes.
+  ASSERT_EQ(0, ceph_fremovexattr(cmount, fd, ACL_EA_ACCESS));
+  ASSERT_EQ(0, ceph_fremovexattr(cmount, fd, ACL_EA_ACCESS));
+  ASSERT_EQ(0, ceph_fremovexattr(cmount, fd, ACL_EA_DEFAULT));
+  ASSERT_EQ(0, ceph_fremovexattr(cmount, fd, ACL_EA_DEFAULT));
+
+  ASSERT_EQ(0, ceph_close(cmount, fd));
+  ASSERT_EQ(0, ceph_unmount(cmount));
+  ceph_shutdown(cmount);
+}
+
