Created attachment 253056 [details] DISPOSABLE tests with the desired behavior Move the conversation started on GitHub pull request 1329 over to the proper channel. Issue ----- # rm -f /boot/efi/foo* rm: /boot/efi/foo*: Invalid argument Given /boot/efi is an MS-DOS file system. Tests ----- Attached is a *disposable* patch with the tests that yield the desired output. Other operating systems should produce a similar output, as documented originally in the pull request. The proposed patch in the pull request fails the last test: # touch /boot/efi/A.DAT # mv /boot/efi/A.DAT /boot/efi/B*.DAT mv: rename /boot/efi/A.DAT to /boot/efi/B*.DAT: Invalid argument
The patch that has been discussed as GitHub pull request 1329 (https://github.com/freebsd/freebsd-src/pull/1329) will be attached to this PR. It has an undesirable side effect: the mv command returns "No such file or directory" instead of "Invalid argument" when the target file name is invalid: # touch /boot/efi/A.DAT # mv /boot/efi/A.DAT /boot/efi/B*.DAT mv: rename /boot/efi/A.DAT to /boot/efi/B*.DAT: No such file or directory This is caused by namei() being called with the target filename and that invokes the patched msdosfs_lookup() and thus gets and returns the changed error code. But since the patch affects namei() and functions that call it, more vnode operations will return ENOENT instead of ```EINVAL`` with this patch (when given an invalid MSDOS file name on an MSDOSFS file system). The rename(2) man page mentions both EINVAL and ENOENT, but neither description mentions the case of an invalid target file name: [ENOENT] A component of the from path does not exist, or a path prefix of to does not exist. [EINVAL] The from argument is a parent directory of to, or an attempt is made to rename ‘.’ or ‘..’. Maybe the case of a target name not allowed on some file system should be added, e.g. one of: [EINVAL] The from argument is a parent directory of to, or an attempt is made to rename ‘.’ or ‘..’, or the target filename is invalid. [ENOENT] A component of the from path does not exist, a path prefix of to does not exist, or the target filename is invalid. If changing the return code of rename() (and possibly other system calls that return the error code of a failed namei() call unmodiified) is acceptable for this specific error case, I'll commit the patch to -CURRENT, but I'm not sure whether POLA will allow merging to -STABLE.
Created attachment 253060 [details] Make MSDOSFS return ENOENT instead of EINVAL for lookup of invalid name
I do not think that this is the right change (not even a fix). First, it is a hack over the place where the behavior occur. If really wanted this, the msdosfs_lookup_ino() should convert 0 from unix2dosfn() result to ENOENT instead of EINVAL. Second, there is nothing wrong with returning EINVAL for lookups for operations like rename: it is indeed EINVAL according to it core meaning. You could try to take the cnp->cn_nameiop into account when converting the unix2dosfn() result zero into the errno. So lookups for OPEN and DELETE might indeed start returning ENOENT for non-representable msdosfs names if somebody wants to spent efforts on this non-issue.
Created attachment 253069 [details] Patch that makes the tests pass First of all, thank you both for the insightful information provided. As someone who is not familiar with the internals, this is much appreciated. If I understood comment #3, the patch attached will make the tests pass. 1. rm -f /boot/efi/foo* (returns nothing, no error) 2. ls -l /boot/efi/foo* (returns ENOENT) 2. mv /boot/efi/A.DAT /boot/efi/B*.DAT (returns EINVAL) However, I feel there are many other things not being considered? What I did not understand is if this should be considered a bug after all?
(In reply to Jose Luis Duran from comment #4) This is not strictly a bug, but some improvement for someone. The proper patch should also return EINVAL for CREATE, not only RENAME, IMO.
Created attachment 253070 [details] msdosfs: Return EINVAL only on CREATE or RENAME (In reply to Konstantin Belousov from comment #5) Amazing, thank you! The following tests were performed: 1. rm -f /boot/efi/foo* (no error) 2. ls -l /boot/efi/foo* (ENOENT) 3. touch /boot/efi/A.DAT && mv /boot/efi/A.DAT /boot/efi/B*.DAT (EINVAL) 4. touch /boot/efi/B*.DAT (EINVAL)
Can you send me git format-patch -formatted patch by mail, with proper metadata, most importantly the 'author' set?
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=0b2c159c8faa3ca155412a1b2e57b3d6bcf91880 commit 0b2c159c8faa3ca155412a1b2e57b3d6bcf91880 Author: Jose Luis Duran <jlduran@gmail.com> AuthorDate: 2024-08-25 14:50:53 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2024-08-25 14:50:53 +0000 msdosfs: return ENOENT if looking up invalid name for OPEN or DELETE and keep reporting EINVAL for CREATE or RENAME lookups. The reasoning is that non-corrupted filesystem cannot have invalid dirents anyway, so returning ENOENT is more in-line with the natural behavior. PR: 281033 MFC after: 1 week sys/fs/msdosfs/msdosfs_lookup.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Created attachment 253098 [details] rename.2: Extend EINVAL's description Per se@'s suggestion, update the documentation of rename(2) regarding EINVAL. Not changing the documentation of ENOENT, because we changed it to return only on OPEN or DELETE. Nothing interesting on: https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html For open(2), [ENOENT] is fine IMO: "A component of the path name that must exist dot not exist." I would like to avoid adding the word invalid to the description of ENOENT. I also could not find anything substantial on: https://pubs.opengroup.org/onlinepubs/9799919799/functions/open.html Suggestions?
'is invalid on the target filesystem' or like. Note that the same wording for EINVAL should be useful for openat(2) as well, because open(O_CREAT) is really seen as CREATE by VOP_LOOKUP(). The better place to handle patches review is phabricator.
(In reply to Konstantin Belousov from comment #10) https://reviews.freebsd.org/D46450
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=33f58ac0795b2b02593ad0a8bf8a1ea24c1dc5e1 commit 33f58ac0795b2b02593ad0a8bf8a1ea24c1dc5e1 Author: Jose Luis Duran <jlduran@gmail.com> AuthorDate: 2024-08-26 01:32:14 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2024-08-27 22:09:33 +0000 rename(2): Extend EINVAL's description On some file systems, the last component of the destination path can contain invalid characters and return EINVAL. PR: 281033 Differential revision: https://reviews.freebsd.org/D46450 MFC after: 1 week lib/libsys/rename.2 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=02c23c3b5ac9c9c1115c8b892034bb5d67b05c06 commit 02c23c3b5ac9c9c1115c8b892034bb5d67b05c06 Author: Jose Luis Duran <jlduran@gmail.com> AuthorDate: 2024-08-27 02:23:14 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2024-08-27 22:09:33 +0000 open(2): Extend EINVAL's description If O_CREAT is specified, the last component of the path argument can contain invalid characters, and return EINVAL on some file systems. PR: 281033 Differential revision: https://reviews.freebsd.org/D46450 MFC after: 1 week lib/libsys/open.2 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=f7eb6b134db8fed6a6851b6a3d00a2ea0e11eadd commit f7eb6b134db8fed6a6851b6a3d00a2ea0e11eadd Author: Jose Luis Duran <jlduran@gmail.com> AuthorDate: 2024-08-27 02:23:14 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2024-09-01 00:53:36 +0000 open(2): Extend EINVAL's description PR: 281033 (cherry picked from commit 02c23c3b5ac9c9c1115c8b892034bb5d67b05c06) lib/libc/sys/open.2 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=65ae1d493e314f5270cc92315cea0bb2e1afa3b0 commit 65ae1d493e314f5270cc92315cea0bb2e1afa3b0 Author: Jose Luis Duran <jlduran@gmail.com> AuthorDate: 2024-08-25 14:50:53 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2024-09-01 00:53:36 +0000 msdosfs: return ENOENT if looking up invalid name for OPEN or DELETE PR: 281033 (cherry picked from commit 0b2c159c8faa3ca155412a1b2e57b3d6bcf91880) sys/fs/msdosfs/msdosfs_lookup.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=97ccb41979bfee6d5bf35886b310e6c9bc7e8eed commit 97ccb41979bfee6d5bf35886b310e6c9bc7e8eed Author: Jose Luis Duran <jlduran@gmail.com> AuthorDate: 2024-08-26 01:32:14 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2024-09-01 00:53:36 +0000 rename(2): Extend EINVAL's description PR: 281033 (cherry picked from commit 33f58ac0795b2b02593ad0a8bf8a1ea24c1dc5e1) lib/libc/sys/rename.2 | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
I believe this can be closed now. Thank you!