Bug 197045 - renameat2 rename_noreplace
Summary: renameat2 rename_noreplace
Status: RESOLVED INVALID
Alias: None
Product: File System
Classification: Unclassified
Component: ext4 (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: fs_ext4@kernel-bugs.osdl.org
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-26 16:27 UTC by Martin Salbba
Modified: 2017-10-25 08:59 UTC (History)
3 users (show)

See Also:
Kernel Version: 4.4.0, 4.11.0
Subsystem:
Regression: No
Bisected commit-id:


Attachments
Test case showing the problem (317 bytes, text/x-csrc)
2017-10-24 12:28 UTC, Elvis Stansvik
Details

Description Martin Salbba 2017-09-26 16:27:28 UTC
on ext4 renameat2 fails in conjunction with RENAME_NOREPLACE, if the target file is not present

renameat2(AT_FDCWD, "rename-test-a", AT_FDCWD, "rename-test-c", RENAME_NOREPLACE) = -1 EINVAL (Invalid argument)

if the target file is present, functionality is ok
Comment 1 Martin Salbba 2017-09-26 16:42:20 UTC
works with brtfs and xfs
Comment 2 Elvis Stansvik 2017-10-24 12:28:18 UTC
Created attachment 260359 [details]
Test case showing the problem

A test case, run with a.out <srcfile> <dstfile>
Comment 3 Elvis Stansvik 2017-10-24 12:29:42 UTC
I'm hit by this on 4.10 a well (the default kernel on Ubuntu 16.04).

I attached a full test case showing.

Example run:


> [estan@newton renameat2tc]$ gcc renameat2.c 
> [estan@newton renameat2tc]$ touch foo
> [estan@newton renameat2tc]$ ./a.out foo bar
> Invalid argument
> [estan@newton renameat2tc]$ uname -a
> Linux newton 4.10.0-37-generic #41~16.04.1-Ubuntu SMP Fri Oct 6 22:42:59 UTC
> 2017 x86_64 x86_64 x86_64 GNU/Linux
> [estan@newton renameat2tc]$ mount | grep " / "
> /dev/sda1 on / type ext4 (rw,noatime,discard,errors=remount-ro,data=ordered)

The reason I discovered it was that Qt recently started using this syscall, which in turn made Qt Creator (a Qt application) fail to write its config files.
Comment 4 Thiago Macieira 2017-10-24 17:41:33 UTC
Martin: can you confirm you did see these issues on 4.4 and 4.11? Elvis is reporting he did not see the issue on 4.4.

By any chance, are you also on Ubuntu?
Comment 5 Elvis Stansvik 2017-10-24 19:22:10 UTC
Martin: I've just discovered that I only see this problem when on my eCryptfs encrypted home directory, not when testing in e.g. /tmp (unencrypted). Are you also using eCryptfs?
Comment 6 Elvis Stansvik 2017-10-24 19:31:12 UTC
So I guess I'm hitting this: https://github.com/torvalds/linux/blob/master/fs/ecryptfs/inode.c#L592

Seems rename flags are not supported on ecryptfs, plain and simple?
Comment 7 Elvis Stansvik 2017-10-24 19:35:28 UTC
Here's the patch that added the if (flags) return EINVAL; for ecryptfs:

https://patchwork.kernel.org/patch/9295699/

So I guess it's just expected that it won't work on ecryptfs.
Comment 8 Elvis Stansvik 2017-10-24 19:41:56 UTC
Though I wonder if that is correct, or if it was hastily added by mistake for ecryptfs as part of that mass porting to rename2.. Maybe it would just work if ecryptfs_rename simply forwarded the flags to vfs_rename without checking them..? Someone who knows the code will have to answer.
Comment 9 Elvis Stansvik 2017-10-24 20:25:57 UTC
Scratch that, it's obviously not that easy.
Comment 10 Thiago Macieira 2017-10-24 21:03:06 UTC
No, it seems that non-local filesystems need specific code to guarante RENAME_NOREPLACE, so the EINVAL error is legitimate.
Comment 11 Theodore Tso 2017-10-24 21:14:18 UTC
I've confirmed that the test case works just fine on plain ext4, using the 4.13 based kernel.  I've also confirmed that it works just fine if you use ext4 encryption.  (I suggest that Ubuntu users file a feature request bug to Ubuntu to ditch ecryptfs and switch to ext4 encryption.)

I can also confirm that ecryptfs != ext4, and that discussing this here has approximately zero chance of it being noticed by the ecryptfs maintainer.   Since Tyler works at Canonical, filing a Launchpad bug is most likely to get someone to look at it.
Comment 12 Thiago Macieira 2017-10-24 21:35:32 UTC
Thanks Ted.

For the record, I'm applying a fix to the Qt code, since the error is legitimate for non-local fs: https://codereview.qt-project.org/209385
Comment 13 Elvis Stansvik 2017-10-24 21:42:06 UTC
Indeed, I'll file an LP bug and try to ping Tyler. Who knows, perhaps they're already planning to switch to encrypted ext4 (thanks for testing that Ted).

And thanks to Thiago for being so quick with the Qt fix (I ran into this behavior by way of Qt).
Comment 14 Theodore Tso 2017-10-25 08:59:28 UTC
Michael Halcrow (the original author of ecryptfs, and who has jokingly referred to his work on ext4 encryption as his apology for ecryptfs :-) and I have talked to Tyler about Ubuntu switching to ext4 encryption.   This is really going to be a matter of prioritizing work at Canonical; so user feature requests (especially requests from paying users :-) would be helpful.

Note You need to log in before you can comment on or make changes to this bug.