On 7/12/2021 10:47 AM, Vivek Goyal wrote: > On Mon, Jul 12, 2021 at 11:41:06AM -0400, J. Bruce Fields wrote: >> On Mon, Jul 12, 2021 at 10:02:47AM -0400, Vivek Goyal wrote: >>> On Fri, Jul 09, 2021 at 04:10:16PM -0400, Bruce Fields wrote: >>>> On Fri, Jul 9, 2021 at 1:59 PM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: >>>>> nfs seems to have some issues. >>>> I'm not sure what the expected behavior is for nfs. All I have for >>>> now is some generic troubleshooting ideas, sorry: >>>> >>>>> - I can set user.foo xattr on symlink and query it back using xattr name. >>>>> >>>>> getfattr -h -n user.foo foo-link.txt >>>>> >>>>> But when I try to dump all xattrs on this file, user.foo is being >>>>> filtered out it looks like. Not sure why. >>>> Logging into the server and seeing what's set there could help confirm >>>> whether it's the client or server that's at fault. (Or watching the >>>> traffic in wireshark; there are GET/SET/LISTXATTR ops that should be >>>> easy to spot.) >>>> >>>>> - I can't set "user.foo" xattr on a device node on nfs and I get >>>>> "Permission denied". I am assuming nfs server is returning this. >>>> Wireshark should tell you whether it's the server or client doing that. >>>> >>>> The RFC is https://datatracker.ietf.org/doc/html/rfc8276, and I don't >>>> see any explicit statement about what the server should do in the case >>>> of symlinks or device nodes, but I do see "Any regular file or >>>> directory may have a set of extended attributes", so that was clearly >>>> the assumption. Also, NFS4ERR_WRONG_TYPE is listed as a possible >>>> error return for the xattr ops. But on a quick skim I don't see any >>>> explicit checks in the nfsd code, so I *think* it's just relying on >>>> the vfs for any file type checks. >>> Hi Bruce, >>> >>> Thanks for the response. I am just trying to do set a user.foo xattr on >>> a device node on nfs. >>> >>> setfattr -n "user.foo" -v "bar" /mnt/nfs/test-dev >>> >>> and I get -EACCESS. >>> >>> I put some printk() statements and EACCESS is being returned from here. >>> >>> nfs4_xattr_set_nfs4_user() { >>> if (!nfs_access_get_cached(inode, current_cred(), &cache, true)) { >>> if (!(cache.mask & NFS_ACCESS_XAWRITE)) { >>> return -EACCES; >>> } >>> } >>> } >>> >>> Value of cache.mask=0xd at the time of error. >> Looks like 0xd is what the server returns to access on a device node >> with mode bits rw- for the caller. >> >> Commit c11d7fd1b317 "nfsd: take xattr bits into account for permission >> checks" added the ACCESS_X* bits for regular files and directories but >> not others. >> >> But you don't want to determine permission from the mode bits anyway, >> you want it to depend on the owner, > Thinking more about this part. Current implementation of my patch is > effectively doing both the checks. It checks that you are owner or > have CAP_FOWNER in xattr_permission() and then goes on to call > inode_permission(). And that means file mode bits will also play a > role. If caller does not have write permission on the file, it will > be denied setxattr(). > > If I don't call inode_permission(), and just return 0 right away for > file owner (for symlinks and special files), then just being owner > is enough to write user.* xattr. And then even security modules will > not get a chance to block that operation. That isn't going to fly. SELinux and Smack don't rely on ownership as a criteria for access. Being the owner of a symlink conveys no special privilege. The LSM must be consulted to determine if the module's policy allows the access. > IOW, if you are owner of > a symlink or special file, you can write as many user.* xattr as you > like and except quota does not look like anything else can block > it. I am wondering if this approach is ok? > > > >> so I guess we should be calling >> xattr_permission somewhere if we want that behavior. >> The RFC assumes user xattrs are for regular files and directories, >> without, as far as I can tell, actually explicitly forbidding them on >> other objects. We should also raise this with the working group if we >> want to increase the chances that you'll get the behavior you want on >> non-Linux servers. > Ok. I am hoping once this patch merges in some form, then I can > follow it up with relevant working group. > >> The "User extended attributes" section of the xattr(7) man page will >> need updating. > Agreed. I will take care of that in a separate patch. > > Right now, I am not too sure if being owner should be the only check > and I should skip calling inode_permission() entirely or not. > > Thanks > Vivek > >> --b. >>
