casaroli opened a new pull request, #21468:
URL: https://github.com/apache/nuttx/pull/21468
## Summary
Three fixes in the stat and ioctl paths of the VFS. Each stands on its own.
- **fs/vfs: Fill a local stat buffer and copy it out at the end.** `stat()`,
`lstat()`, `fstat()`, `statfs()` and `fstatfs()` wrote the caller's buffer
field by field while they held an inode reference, the temporary path buffer of
`inode_search_setup()` or a file reference, and a file system fills only some
of the fields (`binfs_stat()` sets six; no file system sets `st_dev`). Now
these functions fill a buffer on the kernel stack, clear it first, and copy it
to the caller's buffer with one `memcpy()` after the references are released.
So an unset field is 0, not what the buffer held before, and the caller's
buffer changes only on success.
- **fs/vfs/fs_ioctl.c: Add blank lines after declarations.** Whitespace
only; the file had three nxstyle errors and checkpatch checks the whole file.
- **fs/vfs: Read the FIONBIO argument before the file reference is taken.**
`ioctl(fd, FIONBIO, &value)` passes a pointer to an `int`. The driver reads the
`int`, and `file_vioctl()` reads it again after the driver ioctl, while
`nx_vioctl()` holds a reference to the file. Another thread can change the
`int` between the two reads, and then the driver and `f_oflags` do not agree.
Now `nx_vioctl()` reads the `int` once before `file_get2()` and gives the
driver and `file_vioctl()` a pointer to the kernel copy, so no user memory is
read with the reference held. Kernel callers of `file_ioctl()` do not change.
This is separate from #20418 ("copy in ioctls that carry user pointers"):
#20418 copies nested ioctl structures in the system call wrapper; this commit
changes only `nx_vioctl()`, which #20418 does not touch.
## Impact
- The stat family and FIONBIO behave the same, except that the stat buffer
is no longer changed when the call fails, and fields a file system does not set
now read 0.
- Cost: one `struct stat` or `struct statfs` on the stack of the five
functions (80 bytes in `nx_stat()` and `nx_fstat()`, 48 in `statfs()` and
`fstatfs()`, arm64 `-fstack-usage`).
- Generic: flat, protected and kernel builds, every architecture. No ABI
change. Not a breaking change.
## Testing
Test program `statfill.c`: the buffer is filled with 0xa5 before each call.
After a successful call `st_dev` and `st_rdev`, which these file systems do not
set, must not read 0xa5. After a failed call the buffer must not change.
qemu-armv8a:nsh (flat, HVF) with `CONFIG_FS_BINFS` and `CONFIG_FS_LINKS`,
master 6b9b216ff30:
| Case | master | this PR |
|---|---|---|
| binfs, `/bin`: `stat()`, `lstat()` | FAIL: `st_dev` and `st_rdev` are 0xa5
| PASS |
| binfs, `/bin/statfill`: `stat()`, `lstat()`, `fstat()` | FAIL: `st_dev`
and `st_rdev` are 0xa5 | PASS |
| failed `stat()` of a soft link with no target | FAIL: the buffer changes |
PASS: the buffer does not change |
| pseudo file system, `/dev`, `/dev/zero` | PASS | PASS |
qemu-armv8a:knsh (kernel, HVF): `statfill.c` on v9fs and `/dev/zero`, and
`statleak.c` (ten calls of each function after other system calls: `st_dev` and
`st_rdev` stay 0), PASS on master and with this PR. v9fs clears the buffer
itself, so these do not show the change.
ostest exits with status 0 on qemu-armv8a:knsh, qemu-armv8a:nsh and on the
flat configuration above.
No test shows the FIONBIO commit: a test needs a second thread that changes
the `int` between the two reads, at the right time. The change is from reading
the code.
Builds with `EXTRAFLAGS="-Wno-cpp -Werror"`, each OK and with `fs_stat.c`
compiled: qemu-armv8a:knsh, qemu-armv8a:nsh, rv-virt:knsh64, rv-virt:pnsh64,
mps2-an500:knsh, lm3s6965-ek:qemu-protected, sim:nsh.
`tools/checkpatch.sh -c -u -m -g` passes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
