Project

General

Profile

Bug #46084

client: supplying ceph_fsetxattr with no value unsets xattr

Added by John Mulligan 5 months ago. Updated 4 months ago.

Status:
Resolved
Priority:
Normal
Category:
Correctness/Safety
Target version:
% Done:

0%

Source:
Development
Tags:
Backport:
octopus,nautilus
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Component(FS):
Client, libcephfs
Labels (FS):
Pull request ID:
Crash signature:

Description

While working on [1] I noticed an unexpected behavior when trying to use ceph_fsetxattr with an empty (null) value. I expected it to set an xattr with a name only. However, the call appears to remove the xattr entirely rather than clearing the value.

I tested this with `setfattr -n user.foo <file>` on the same fs mounted via fuse and it behaves as expected.

I saw the following line in the source that looked suspicious:

int Client::_do_setxattr(Inode *in, const char *name, const void *value,
size_t size, int flags, const UserPerm& perms) {
int xattr_flags = 0;
if (!value)
xattr_flags |= CEPH_XATTR_REMOVE; // <------

Of course, I'm no expert on the ceph internals.

While the use case for a value-less xattr isn't great, I could argue that having just the name can be used a boolean for some cases. Plus, it seems inconsistent with the way VFS xattr support works. Since removexattr appears to have it's own code path it seems a bit odd to me that there's a need to unset the xattr via a ceph_*setxattr path.

[1] - https://github.com/ceph/go-ceph/issues/247


Related issues

Copied to CephFS - Backport #46409: nautilus: client: supplying ceph_fsetxattr with no value unsets xattr Resolved
Copied to CephFS - Backport #46410: octopus: client: supplying ceph_fsetxattr with no value unsets xattr Resolved

History

#1 Updated by John Mulligan 5 months ago

Forgot to mention this: I tested this on both an octopus build and a nautilus build.

#2 Updated by Patrick Donnelly 5 months ago

Interesting design choice here. Is it causing issues for some application of yours? I'm inclined not to change the behavior in case it breaks some applciation using libcephfs.

#3 Updated by Jeff Layton 5 months ago

  • Assignee set to Jeff Layton

#4 Updated by Jeff Layton 5 months ago

Looks like a legit bug. The Linux kernel does this in __vfs_setxattr:

        if (size == 0)
                value = "";  /* empty EA, do not remove */

...so basically if you set size to 0 on setxattr() syscall, you should get an xattr that is set but is zero-length. I think we'll probably want userland ceph client to do the same.

#5 Updated by John Mulligan 5 months ago

Patrick Donnelly wrote:

Interesting design choice here. Is it causing issues for some application of yours? I'm inclined not to change the behavior in case it breaks some applciation using libcephfs.

Not directly, as my "application" is a wrapper around the Ceph APIs in Go (golang). However, I certainly found the current behavior unexpected given my prior experience using xattrs on Linux (kernel/libc/etc). I filed the issue preemptively assuming the consumers of our library could also be surprised by this behavior.

If the ceph team wants the function to continue behaving the same for some applications, to avoid breaking old applications, perhaps a new flag to the function to get more standard behavior would be appropriate? In our layer we could always set the flag if we are given a zero-length value.

#6 Updated by Patrick Donnelly 5 months ago

  • Subject changed from Supplying ceph_fsetxattr with no value unsets xattr to client: supplying ceph_fsetxattr with no value unsets xattr
  • Status changed from New to Triaged
  • Assignee changed from Jeff Layton to Sidharth Anupkrishnan
  • Target version set to v16.0.0
  • Source set to Development
  • Backport set to octopus,nautilus

Jeff suggests the behavior should be to convert the nullptr to an empty string, to match the behavior of the kernel client.

Sidharth, please make the change.

#7 Updated by Jeff Layton 5 months ago

The kernel seems to key its behavior on the size parameter. When it's 0, the pointer passed in is ignored and an empty string is used instead. I expect that when you pass in a NULL pointer with a non-zero size parameter, you probably get back EINVAL, but you probably ought to test that on a local fs (xfs or ext4).

#8 Updated by Sidharth Anupkrishnan 5 months ago

  • Pull request ID set to 35725

#9 Updated by Sidharth Anupkrishnan 5 months ago

Jeff Layton wrote:

The kernel seems to key its behavior on the size parameter. When it's 0, the pointer passed in is ignored and an empty string is used instead. I expect that when you pass in a NULL pointer with a non-zero size parameter, you probably get back EINVAL, but you probably ought to test that on a local fs (xfs or ext4).

Good Point. I think we should handle that case too. As for the ceph userland client's current behaviour, when the posix_acl xattr is set then posix_acl_check() in _do_setxattr() catches this and makes the function return EINVAL. If that xattr is not set, then we don't handle it at all. I will add handling of this in the PR.

#10 Updated by Sidharth Anupkrishnan 5 months ago

Sidharth Anupkrishnan wrote:

Jeff Layton wrote:

The kernel seems to key its behavior on the size parameter. When it's 0, the pointer passed in is ignored and an empty string is used instead. I expect that when you pass in a NULL pointer with a non-zero size parameter, you probably get back EINVAL, but you probably ought to test that on a local fs (xfs or ext4).

Good Point. I think we should handle that case too. As for the ceph userland client's current behaviour, when the posix_acl xattr is set then posix_acl_check() in _do_setxattr() catches this and makes the function return EINVAL. If that xattr is not set, then we don't handle it at all. I will add handling of this in the PR.

Also tested it out in my local fs (ext4). Supplying NULL value with a non zero size does indeed give EINVAL.

#11 Updated by Zheng Yan 5 months ago

Jeff Layton wrote:

Looks like a legit bug. The Linux kernel does this in __vfs_setxattr:

[...]

...so basically if you set size to 0 on setxattr() syscall, you should get an xattr that is set but is zero-length. I think we'll probably want userland ceph client to do the same.

If I remember right. Old kernel does not do this. this change was introduced by

commit 6c6ef9f26e598fb977f60935e109cd5b266c941a
Author: Andreas Gruenbacher <>
Date: Thu Sep 29 17:48:44 2016 +0200

xattr: Stop calling {get,set,remove}xattr inode operations

#12 Updated by Patrick Donnelly 5 months ago

  • Status changed from Triaged to Pending Backport

#13 Updated by Nathan Cutler 5 months ago

  • Copied to Backport #46409: nautilus: client: supplying ceph_fsetxattr with no value unsets xattr added

#14 Updated by Nathan Cutler 5 months ago

  • Copied to Backport #46410: octopus: client: supplying ceph_fsetxattr with no value unsets xattr added

#15 Updated by Nathan Cutler 4 months ago

  • Status changed from Pending Backport to Resolved

While running with --resolve-parent, the script "backport-create-issue" noticed that all backports of this issue are in status "Resolved" or "Rejected".

Also available in: Atom PDF