Bug #46084
closedclient: supplying ceph_fsetxattr with no value unsets xattr
0%
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.
Updated by John Mulligan almost 4 years ago
Forgot to mention this: I tested this on both an octopus build and a nautilus build.
Updated by Patrick Donnelly almost 4 years 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.
Updated by Jeff Layton almost 4 years 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.
Updated by John Mulligan almost 4 years 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.
Updated by Patrick Donnelly almost 4 years 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.
Updated by Jeff Layton almost 4 years 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).
Updated by Sidharth Anupkrishnan almost 4 years ago
- Pull request ID set to 35725
Updated by Sidharth Anupkrishnan almost 4 years 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.
Updated by Sidharth Anupkrishnan almost 4 years 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.
Updated by Zheng Yan almost 4 years 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 <agruenba@redhat.com>
Date: Thu Sep 29 17:48:44 2016 +0200
xattr: Stop calling {get,set,remove}xattr inode operationsUpdated by Patrick Donnelly almost 4 years ago
- Status changed from Triaged to Pending Backport
Updated by Nathan Cutler almost 4 years ago
- Copied to Backport #46409: nautilus: client: supplying ceph_fsetxattr with no value unsets xattr added
Updated by Nathan Cutler almost 4 years ago
- Copied to Backport #46410: octopus: client: supplying ceph_fsetxattr with no value unsets xattr added
Updated by Nathan Cutler over 3 years 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".