rofiles-fuse: Fix lchown() (and pro-actively NOFOLLOW)

[alexlarsson]

If you lchown("symlink") then we were incorrectly trying to chown the
symlink target, rather than the symlink itself. In particular, this cause
cp -a to fail for a broken symlink. Additionally, it was using the
symlink target when verifying writability, rather than the symlink
itself.

To fix this, we just pass AT_SYMLINK_NOFOLLOW in these cases.

In general, the kernel itself will always resolve any symlinks for us
before calling into the fuse backend, so we should really never do any
symlink following in the fuse fs itself. So, we pro-actively add
NOFOLLOW flags to a few other places:

truncate:
In reality this will never be hit, because
the kernel will resolve symlinks before calling us.
chmod:
Theoretically enable ownership setting on a symlink. This is
not implemented on current kernels, but if it ever is, this
will be required.
access:
It seems the current fuse implementation never calls this
(faccessat w/AT_SYMLINK_NOFOLLOW never reaches the fuse fs)
but if this ever is implemented this is the correct behaviour.

[cgwalters]

This is tricky; not all *at() ops support this flag. In particular man-pages-4.09-3.fc26.noarch says:

   AT_SYMLINK_NOFOLLOW
         If pathname is a symbolic link, do not dereference it: instead operate on the link itself.  This flag is not currently implemented.

There was a thread about this recently...

https://marc.info/?l=linux-kernel&m=148830147803162&w=2
https://marc.info/?l=linux-fsdevel&m=149193779929561&w=2

[cgwalters]

So we probably need to simulate this via openat(...O_NOFOLLOW), ignore ELOOP (meaning symlinks), and for regular files use fchmod() on the fd.

[alexlarsson]

I think that is overdoing it. Currently (as in the commit message) the kernel will never emit the fuse chmod operation on a symlink, because the kernel does not support it. If, later the kernel starts implementing this it might. However, if it does, then the fchmodat call is likely to work, so the code as it will work.

[cgwalters]

But today the glibc syscall wrapper returns EINVAL if any flags are set.

I wrote #1141 and verified that this patch causes the tests to fail.

[cgwalters]

So let's drop the flag and add a comment with links to the above linux-fsdevel threads?

[alexlarsson]

Hmm, i ran into issues with this patch. Without it I get:

strace /usr/bin/install -c gnome-autogen.sh '/usr/bin'
...
open("gnome-autogen.sh", O_RDONLY)      = 3
fstat(3, {st_mode=S_IFREG|0755, st_size=12093, ...}) = 0
open("/usr/bin/gnome-autogen.sh", O_WRONLY|O_CREAT|O_EXCL, 0600) = 4
fstat(4, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
fadvise64(3, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
mmap(NULL, 139264, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fbfd79cb000
read(3, "#!/bin/sh\n# Run this to generate"..., 131072) = 12093
write(4, "#!/bin/sh\n# Run this to generate"..., 12093) = 12093
read(3, "", 131072)                     = 0
fchmod(4, 0600)                         = 0
close(4)                                = 0
close(3)                                = 0

But with the patch i get:

strace /usr/bin/install -c gnome-autogen.sh '/usr/bin'
...
open("gnome-autogen.sh", O_RDONLY)      = 3
fstat(3, {st_mode=S_IFREG|0755, st_size=12093, ...}) = 0
open("/usr/bin/gnome-autogen.sh", O_WRONLY|O_CREAT|O_EXCL, 0600) = 4
fstat(4, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
fadvise64(3, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
mmap(NULL, 139264, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f8c9d975000
read(3, "#!/bin/sh\n# Run this to generate"..., 131072) = 12093
write(4, "#!/bin/sh\n# Run this to generate"..., 12093) = 12093
read(3, "", 131072)                     = 0
fchmod(4, 0600)                         = -1 EOPNOTSUPP (Operation not supported)

So, maybe the patch has issues with fchmod.

[alexlarsson]

Maybe the safest approach is to only change the parts that we can verify fixes things in the current kernel, rather than pre-empty later kernel changes.

[cgwalters]

@alexlarsson See my comments above.

[cgwalters]

OK, I reworked this patch and extended the test suite to cover a lot of this.

@alexlarsson can you take a look? Feel free to r+ 3c906f9 if it looks good.

[alexlarsson]

Looks good to me and fixes my usecases
@rh-atomic-bot r+ 3c906f9

[rh-atomic-bot]

@alexlarsson: 🔑 Insufficient privileges: Collaborator required

[alexlarsson]

@rh-atomic-bot r+ 3c906f9

[rh-atomic-bot]

⌛ Testing commit 3c906f9 with merge 08eaf66...

[rh-atomic-bot]

☀️ Test successful - status-atomicjenkins
Approved by: alexlarsson
Pushing 08eaf66 to master...