Skip to content

fix[dfs_v1]: prevent vnode ref underflow and double release on close/fd release #11119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wdfk-prog wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix[dfs_v1]: prevent vnode ref underflow and double release on close/fd release #11119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions components/dfs/dfs_v1/src/dfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,13 +412,16 @@ void fdt_fd_release(struct dfs_fdtable* fdt, int fd)
if (fd_slot->ref_count == 0)
{
struct dfs_vnode *vnode = fd_slot->vnode;
fd_slot->vnode = RT_NULL;
if (vnode)
{
vnode->ref_count--;
if (vnode->ref_count > 0)
{
vnode->ref_count--;
}
Comment on lines +418 to +421
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Concurrency/并发]: Insufficient synchronization between dfs_file_close and fdt_fd_release / dfs_file_close 和 fdt_fd_release 之间的同步不足

English: The dfs_file_close function uses dfs_fm_lock() (vnode manager lock) while fdt_fd_release uses dfs_file_lock() (fd table lock). These are two different locks protecting different resources. This creates a race condition where:

  1. Thread A calls dfs_file_close, decrements vnode->ref_count to 0, and starts cleaning up
  2. Thread B calls fdt_fd_release concurrently and also tries to decrement the same vnode's ref_count
  3. Both threads might try to free the same vnode, or one might access freed memory

The guard if (vnode->ref_count > 0) at line 418 only partially mitigates this - if the race occurs after the check but before the decrement, underflow can still happen.

中文：dfs_file_close 函数使用 dfs_fm_lock()（vnode 管理器锁），而 fdt_fd_release 使用 dfs_file_lock()（fd 表锁）。这是两个不同的锁保护不同的资源。这会创建一个竞争条件：

  1. 线程 A 调用 dfs_file_close，将 vnode->ref_count 递减到 0，并开始清理
  2. 线程 B 同时调用 fdt_fd_release，也尝试递减同一个 vnode 的 ref_count
  3. 两个线程可能都尝试释放同一个 vnode，或者一个可能访问已释放的内存

第 418 行的保护条件 if (vnode->ref_count > 0) 只能部分缓解这个问题 - 如果竞争发生在检查之后但递减之前，仍然可能发生下溢。

A proper fix would require using the same lock (likely dfs_fm_lock) in both functions when accessing vnode ref_count.

Copilot uses AI. Check for mistakes.
if(vnode->ref_count == 0)
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Code Style/代码风格]: Missing space after 'if' keyword / 'if' 关键字后缺少空格

English: According to RT-Thread coding standards, there should be a space between 'if' and the opening parenthesis. The code should be if (vnode->ref_count == 0) instead of if(vnode->ref_count == 0).

中文：根据 RT-Thread 编码标准，'if' 和左括号之间应该有一个空格。代码应该是 if (vnode->ref_count == 0) 而不是 if(vnode->ref_count == 0)。

Suggested change
if(vnode->ref_count == 0)
if (vnode->ref_count == 0)

Copilot uses AI. Check for mistakes.
{
rt_free(vnode);
fd_slot->vnode = RT_NULL;
}
Comment on lines +418 to 425
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug/错误]: Potential vnode double-free vulnerability / 潜在的 vnode 双重释放漏洞

English: While the guard if (vnode->ref_count > 0) prevents underflow, the logic here is problematic. If fdt_fd_release is called when the vnode was already freed by dfs_file_close, this could lead to use-after-free. The vnode might have already been freed in dfs_file_close at line 363, but fd_slot->vnode still holds a dangling pointer until line 415 sets it to NULL. Additionally, freeing the vnode here at line 424 without proper cleanup (removing from hash list, freeing path/fullpath) is incomplete compared to the cleanup in dfs_file_close. This inconsistency suggests a design flaw in managing vnode lifecycle across two different code paths.

中文：虽然保护条件 if (vnode->ref_count > 0) 可以防止下溢，但这里的逻辑存在问题。如果在 vnode 已被 dfs_file_close 释放后调用 fdt_fd_release，这可能导致释放后使用。vnode 可能已在 dfs_file_close 的第 363 行被释放，但 fd_slot->vnode 仍持有悬空指针，直到第 415 行将其设置为 NULL。此外，在第 424 行释放 vnode 时没有适当的清理（从哈希列表中删除、释放 path/fullpath），与 dfs_file_close 中的清理相比是不完整的。这种不一致性表明在两个不同代码路径中管理 vnode 生命周期存在设计缺陷。

The correct approach should ensure that vnode cleanup is performed in only one place with proper synchronization, or the cleanup in both places should be identical and complete.

Copilot uses AI. Check for mistakes.
}
rt_free(fd_slot);
Expand Down
7 changes: 4 additions & 3 deletions components/dfs/dfs_v1/src/dfs_file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ int dfs_file_close(struct dfs_file *fd)
dfs_fm_lock();
vnode = fd->vnode;

if (vnode->ref_count <= 0)
if (vnode == NULL || vnode->ref_count <= 0)
{
dfs_fm_unlock();
return -ENXIO;
Comment on lines +340 to 343
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug/错误]: NULL check added but early return prevents vnode cleanup / 添加了 NULL 检查但提前返回阻止了 vnode 清理

English: While adding vnode == NULL check is good defensive programming, returning -ENXIO when vnode is NULL causes a potential resource leak. If vnode is somehow NULL but fd->ref_count == 1, the function returns early without decrementing fd->ref_count or cleaning up the fd structure. This could lead to fd resources not being properly released. Consider whether this is an assertion-worthy condition instead, since vnode should never be NULL when fd->ref_count == 1 in a correctly functioning system.

中文：虽然添加 vnode == NULL 检查是良好的防御性编程，但当 vnode 为 NULL 时返回 -ENXIO 会导致潜在的资源泄漏。如果 vnode 不知何故为 NULL 但 fd->ref_count == 1，函数会提前返回，而不递减 fd->ref_count 或清理 fd 结构。这可能导致 fd 资源无法正确释放。考虑这是否应该是断言级别的条件，因为在正常运行的系统中，当 fd->ref_count == 1 时 vnode 不应该为 NULL。

Copilot uses AI. Check for mistakes.
Expand All @@ -348,11 +348,12 @@ int dfs_file_close(struct dfs_file *fd)
result = vnode->fops->close(fd);
}

if (vnode->ref_count == 1)
vnode->ref_count--;
fd->vnode = NULL;
if (vnode->ref_count == 0)
Comment on lines +351 to +353
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug/错误]: Reference count decrement and vnode cleanup order may cause race condition / 引用计数递减和vnode清理顺序可能导致竞争条件

English: The current implementation decrements vnode->ref_count and sets fd->vnode = NULL before checking if ref_count == 0. This creates a race condition window where another thread could access the vnode with ref_count=0 but before it's cleaned up. The check should be performed on the original ref_count value before decrementing, not after.

中文：当前实现在检查 ref_count == 0 之前先递减 vnode->ref_count 并设置 fd->vnode = NULL。这会创建一个竞争条件窗口，其中另一个线程可能在清理之前访问 ref_count=0 的 vnode。应该在递减之前对原始 ref_count 值执行检查，而不是之后。

The logic should be:

  1. Check if vnode->ref_count == 1 (last reference)
  2. If yes, perform cleanup (remove from hash, free memory)
  3. Otherwise, just decrement ref_count
  4. Finally, set fd->vnode = NULL

逻辑应该是：

  1. 检查 vnode->ref_count == 1（最后一个引用）
  2. 如果是，执行清理（从哈希中删除，释放内存）
  3. 否则，只需递减 ref_count
  4. 最后，设置 fd->vnode = NULL

Copilot uses AI. Check for mistakes.
{
/* remove from hash */
rt_list_remove(&vnode->list);
fd->vnode = NULL;

if (vnode->path != vnode->fullpath)
{
Expand Down
Loading