Re: [Regression] ovl: rename(2) EINVAL if lower doesn't support fileattrs

[Kevin Locke <kevin@kevinlocke.name>]

On Mon, 2021-10-18 at 10:47 +0200, Miklos Szeredi wrote:
> On Sun, 17 Oct 2021 at 22:53, Kevin Locke <kevin@kevinlocke.name> 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@kevinlocke.name>

Thanks for the fast investigation and fix!
Kevin