On Mon, 2021-10-18 at 10:47 +0200, Miklos Szeredi wrote: > On Sun, 17 Oct 2021 at 22:53, Kevin Locke <kevin@xxxxxxxxxxxxxxx> wrote: >> With 5.15-rc5 or torvalds master (d999ade1cc86), attempting to rename >> a file fails with -EINVAL on an overlayfs mount with a lower >> filesystem that returns -EINVAL for ioctl(FS_IOC_GETFLAGS). >> >> [...] >> >> This issue does not occur on 5.14. I've bisected the regression to >> 72db82115d2b. > > This is clearly a regression. Not trivial how far the fix should go, though. > > One option is to just ignore all errors from ovl_copy_fileattr(), > which would solve this and similar issues. However that would result > in missing the cases when the attributes were really meant to be > copied up, but failed to do so for some reason. > > If vfs_fileattr_get() fails with ENOIOCTLCMD or ENOTTY on lower, that > obviously means we need to return success (lower fs does not support > fileattr). As ntfs-3g seems to return EINVAL that needs to be added > too. I agree. I like your approach. Especially for ENOIOCTLCMD/ENOTTY. Since ntfs-3g returns EINVAL, that seems necessary to avoid logspam for now. Do you think it would make sense for me to suggest returning ENOIOCTLCMD to the ntfs-3g project? It seems more appropriate and consistent with other filesystems to me, but I'm certainly not an expert: https://github.com/tuxera/ntfs-3g/blob/2021.8.22/libntfs-3g/ioctl.c#L415 I didn't see any other errors from in-tree filesystems, but who knows errors other FUSE projects may return. > More interesting question is what to do with get/set failures on > upper. My feeling is that for now we should try to return errors > (even ENOTTY), but should print a warning in the kernel log. If that > turns out to regress some use cases, then that needs to fixed as well. Does that mean all copy-up operations would fail for an upper which does not support file attributes, or only ones where the file being copied has attributes set? I'm a little concerned about debugability, since the failure modes may not be obvious to users. Although the log warnings would help with that. Would it be possible to refuse to mount an upper which doesn't support file attrs over a lower which does? At least the failure would be immediate and obvious. If a use case is found, perhaps a mount option to control file attr behavior cold be added at that point? Just thinking out loud. > Untested patch attached. Works great for me. Tested-by: Kevin Locke <kevin@xxxxxxxxxxxxxxx> Thanks for the fast investigation and fix! Kevin
