[BREAKING] fs/vfs: Separate file descriptors from file descriptions #16499

Donny9 · 2025-06-09T12:25:36Z

Summary

This PR is a rework of the NuttX file descriptor implementation. The
goal is two-fold:

Improve POSIX compliance. The old implementation tied file description
to inode only, not the file struct. POSIX however dictates otherwise.
Fix a bug with descriptor duplication (dup2() and dup3()). There is
an existing race condition with this POSIX API that currently results
in a kernel side crash.
The crash occurs when a partially open / closed file descriptor is
duplicated. The reason for the crash is that even if the descriptor is
closed, the file might still be in use by the kernel (due to e.g. ongoing
write to file). The open file data is changed by file_dup3() and this
causes a crash in the device / drivers themselves as they lose access to
the inode and private data.

The fix is done by separating struct file into file and file descriptor
structs. The file struct can live on even if the descriptor is closed,
fixing the crash. This also fixes the POSIX issue, as two descriptors
can now point to the same file.

The implementation of this PR is based on the modifications made in #16361.
Thank you @pussuw

DEPENDS ON: apache/nuttx-apps#3091

Impact

Remove the FS_REFCOUNT config because reference counting is a very necessary feature that is required in many scenarios to ensure stability.
Rename the functions fs_getfilep, fs_putfilep, and fs_reffilep to file_get, file_put, and file_ref respectively, to unify the naming convention to file_xxx, such as file_open, file_ioctl, etc.
Introduce a new fd (file descriptor) structure. If multiple file descriptors (fds) are in a dup relationship, they can share the same file entity to implement the dup functionality.
Modify the functions nx_close_from_tcb, nx_open_from_tcb, nx_dup2_from_tcb, and nx_dup3_from_tcb to fdlist_open, fdlist_close, fdlist_dup2, and fdlist_dup3 respectively, to uniformly operate on file descriptors using fdlist.

Testing

monkey(24 hours) and CI

So that the same code can be used with and without spinlocks. Signed-off-by: Ville Juven <ville.juven@unikie.com>

When a new pseudofile is created, the inode reference count needs to be bumped to protect the node. Signed-off-by: Ville Juven <ville.juven@unikie.com>

Currently the code is dumped into one massive file; fs_files. Move the different logical parts into their own files. Signed-off-by: Ville Juven <ville.juven@unikie.com> Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

Previously, this config was added to ensure that the size of the struct file remained unchanged, thereby preventing the Flash memory of resource-constrained MCUs from being unnecessarily increased. However, we have now refactored the relationship between struct fd and struct file, reducing their memory footprint in both Flash and RAM. Consequently, this config can be removed. Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

1. The call to file_close_without_clear in file_dup3 does not clear the tag information, so there is no need to back it up. 2. file_dup3 don't need to copy tag information, tag is only valid for fd. Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

Donny9 · 2025-06-09T12:26:03Z

@jkivilin @pussuw please review

include/nuttx/fs/ioctl.h

fs/inode/fs_files.c

fs/vfs/fs_fcntl.c

fs/vfs/fs_ioctl.c

include/nuttx/fs/fs.h

fs/vfs/fs_dup.c

fs/vfs/fs_dup3.c

sched/task/task_spawnparms.c

fs/vfs/fs_fcntl.c

fs/inode/fs_files.c

Donny9 · 2025-06-10T10:37:49Z

@jlaitine
The changes made this time are indeed quite significant, involving modifications to the core relationship between file descriptors and file structures. We've made over 140 adjustments internally, and including testing, it took us a week to complete.

Our original intention was always to improve the patch as much as possible. The new PR has been revised following @pussuw the previous approach, and also maintained the original contributor as the owner of the patch.

Donny9 · 2025-06-10T10:38:45Z

Fix the race condition issues pointed out by @xiaoxiang781216 and then this is fine to merge by me. I will not review this any longer as this is not my code anymore.

The new patch still follow your code base, but detail is refine since FS is a very important subsystem and this patch series change the life cycle management, so it's always good to have more expert to review and refine the patch.

Yes, thanks for spending a substantial amount of time doing the review. I think this is ready to merge now. There is a practical reason why I cannot spend any more time with NuttX as I will be moving on to a different line of work very soon. This means this shall be my last contribution (of any kind) to this project.

@pussuw Thank you. I'll continue to maintain it and address any issues that arise in the future until everything is working perfectly.

jerpelea

please add a commit message to
0ec77cf
and change the title to
tools/pynuttx: update fs.py base on new structure fd and file

Donny9 · 2025-06-10T14:30:02Z

please add a commit message to 0ec77cf and change the title to tools/pynuttx: update fs.py base on new structure fd and file

Done~

jerpelea

please add a commit message explaining the change

xiaoxiang781216 · 2025-06-11T07:44:46Z

please add a commit message explaining the change

could you point out which patch need more info? @jerpelea

jerpelea · 2025-06-11T07:46:06Z

please add a commit message explaining the change

could you point out which patch need more info? @jerpelea

4aaeb80

update locked to f_locked Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

old: fs_getfilep, fs_putfilep, fs_reffilep new: file_get, file_put, file_ref Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

This patch is a rework of the NuttX file descriptor implementation. The goal is two-fold: 1. Improve POSIX compliance. The old implementation tied file description to inode only, not the file struct. POSIX however dictates otherwise. 2. Fix a bug with descriptor duplication (dup2() and dup3()). There is an existing race condition with this POSIX API that currently results in a kernel side crash. The crash occurs when a partially open / closed file descriptor is duplicated. The reason for the crash is that even if the descriptor is closed, the file might still be in use by the kernel (due to e.g. ongoing write to file). The open file data is changed by file_dup3() and this causes a crash in the device / drivers themselves as they lose access to the inode and private data. The fix is done by separating struct file into file and file descriptor structs. The file struct can live on even if the descriptor is closed, fixing the crash. This also fixes the POSIX issue, as two descriptors can now point to the same file. Signed-off-by: Ville Juven <ville.juven@unikie.com> Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

Update the Python script based on the PR "Separate file descriptors from file descriptions" in fs/vfs. Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

Donny9 · 2025-06-11T15:17:18Z

please add a commit message explaining the change

could you point out which patch need more info? @jerpelea

4aaeb80

Done~

pussuw and others added 5 commits June 9, 2025 18:37

nuttx/spinlock: Define empty macros for spin_lock/unlock

ea15bb9

So that the same code can be used with and without spinlocks. Signed-off-by: Ville Juven <ville.juven@unikie.com>

fs/pseudofs: Add missing bumping of inode refcount

11ef71d

When a new pseudofile is created, the inode reference count needs to be bumped to protect the node. Signed-off-by: Ville Juven <ville.juven@unikie.com>

nuttx/fs: Reorganize the code for close, dup, et. al.

14f5c48

Currently the code is dumped into one massive file; fs_files. Move the different logical parts into their own files. Signed-off-by: Ville Juven <ville.juven@unikie.com> Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

pussuw reviewed Jun 9, 2025

View reviewed changes

include/nuttx/fs/ioctl.h Show resolved Hide resolved

Donny9 force-pushed the fs_fdlist_rewrite branch from d6a0913 to 5bd3a75 Compare June 9, 2025 12:51

github-actions bot added the Area: Drivers Drivers issues label Jun 9, 2025

Donny9 force-pushed the fs_fdlist_rewrite branch from 5bd3a75 to 2e0ba72 Compare June 9, 2025 12:55

pussuw reviewed Jun 9, 2025

View reviewed changes

fs/inode/fs_files.c Outdated Show resolved Hide resolved

Donny9 force-pushed the fs_fdlist_rewrite branch from 2e0ba72 to 6b00ab4 Compare June 9, 2025 13:08

pussuw reviewed Jun 9, 2025

View reviewed changes

fs/vfs/fs_fcntl.c Outdated Show resolved Hide resolved

pussuw reviewed Jun 9, 2025

View reviewed changes

fs/vfs/fs_ioctl.c Outdated Show resolved Hide resolved

Donny9 force-pushed the fs_fdlist_rewrite branch from 6b00ab4 to 132bbdb Compare June 9, 2025 14:56

xiaoxiang781216 reviewed Jun 9, 2025

View reviewed changes

include/nuttx/fs/fs.h Outdated Show resolved Hide resolved

fs/vfs/fs_dup.c Outdated Show resolved Hide resolved

fs/vfs/fs_dup3.c Outdated Show resolved Hide resolved

sched/task/task_spawnparms.c Outdated Show resolved Hide resolved

fs/vfs/fs_fcntl.c Outdated Show resolved Hide resolved

xiaoxiang781216 reviewed Jun 9, 2025

View reviewed changes

xiaoxiang781216 mentioned this pull request Jun 9, 2025

fs/vfs: Separate file descriptors from file descriptions #16361

Closed

Donny9 force-pushed the fs_fdlist_rewrite branch from 132bbdb to 6bb7093 Compare June 9, 2025 15:40

pussuw changed the title ~~fs/vfs: New separate file descriptors from file descriptions~~ fs/vfs: separate file descriptors from file descriptions Jun 9, 2025

pussuw changed the title ~~fs/vfs: separate file descriptors from file descriptions~~ fs/vfs: Separate file descriptors from file descriptions Jun 9, 2025

xiaoxiang781216 reviewed Jun 9, 2025

View reviewed changes

fs/inode/fs_files.c Outdated Show resolved Hide resolved

fs/inode/fs_files.c Show resolved Hide resolved

jerpelea approved these changes Jun 10, 2025

View reviewed changes

jerpelea requested changes Jun 10, 2025

View reviewed changes

Donny9 force-pushed the fs_fdlist_rewrite branch from 0ec77cf to 4aaeb80 Compare June 10, 2025 14:29

pussuw approved these changes Jun 10, 2025

View reviewed changes

xiaoxiang781216 approved these changes Jun 10, 2025

View reviewed changes

xiaoxiang781216 requested a review from jerpelea June 10, 2025 15:02

jerpelea requested changes Jun 11, 2025

View reviewed changes

Donny9 and others added 4 commits June 11, 2025 23:16

fs/file: unify prefix f_ for member locked

53b0fb6

update locked to f_locked Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

fs/file: unify prefix about file_xxx api, like file_open, file_ioctl

c320b48

old: fs_getfilep, fs_putfilep, fs_reffilep new: file_get, file_put, file_ref Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

tools/pynuttx: update fs.py base on new structure fd and file

9b13fa4

Update the Python script based on the PR "Separate file descriptors from file descriptions" in fs/vfs. Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>

Donny9 force-pushed the fs_fdlist_rewrite branch from 4aaeb80 to 9b13fa4 Compare June 11, 2025 15:16

xiaoxiang781216 requested a review from jerpelea June 12, 2025 02:01

xiaoxiang781216 approved these changes Jun 12, 2025

View reviewed changes

jerpelea approved these changes Jun 12, 2025

View reviewed changes

pussuw approved these changes Jun 12, 2025

View reviewed changes

xiaoxiang781216 merged commit 7927c8d into apache:master Jun 12, 2025
11 of 39 checks passed

Donny9 mentioned this pull request Jun 13, 2025

driver/optee: fix compile break #16526

Merged

fdcavalcanti mentioned this pull request Jun 13, 2025

[BUG] Failing QEMU tests after #16499 #16531

Closed

1 task

cederom changed the title ~~fs/vfs: Separate file descriptors from file descriptions~~ [BREAKING] fs/vfs: Separate file descriptors from file descriptions Jun 13, 2025

Donny9 mentioned this pull request Jun 14, 2025

fs/fcntl: using ioctl to implement FIOGCLEX/FIOCLEX/FIONCLEX #16536

Merged

SPRESENSE mentioned this pull request Jun 20, 2025

[BUG] Cannot close a driver that created a thread with open #16560

Closed

1 task

eren-terzioglu mentioned this pull request Jun 23, 2025

[BUG] ESP32-S3 boot issue #16600

Closed

1 task

jerpelea mentioned this pull request Jun 27, 2025

{bp-3091} testing/fs: update api name fs_getfilep/fs_putfilep to file_get/file_put apache/nuttx-apps#3113

Closed

[BREAKING] fs/vfs: Separate file descriptors from file descriptions #16499

[BREAKING] fs/vfs: Separate file descriptors from file descriptions #16499

Uh oh!

Conversation

Donny9 commented Jun 9, 2025 • edited by cederom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Impact

Testing

Uh oh!

Donny9 commented Jun 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Donny9 commented Jun 10, 2025

Uh oh!

Donny9 commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerpelea left a comment

Choose a reason for hiding this comment

Uh oh!

Donny9 commented Jun 10, 2025

Uh oh!

jerpelea left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 commented Jun 11, 2025

Uh oh!

jerpelea commented Jun 11, 2025

Uh oh!

Donny9 commented Jun 11, 2025

Uh oh!

Uh oh!

Uh oh!

Donny9 commented Jun 9, 2025 •

edited by cederom

Loading

Donny9 commented Jun 10, 2025 •

edited

Loading