extend the lseek on dir #245

briteny-pwn · 2025-10-27T16:36:41Z

No description provided.

Copilot

Pull Request Overview

This PR extends the lseek operation to support directory file descriptors in addition to regular files. The changes implement directory-specific seek handling with stricter validation and security constraints.

Key Changes:

Added directory type detection and separate handling logic for directories vs. regular files
Implemented SEEK_SET support for directories with offset validation (max i64::MAX)
Added SEEK_CUR support for directories in overlayfs with overflow protection

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File	Description
project/libfuse-fs/src/passthrough/async_io.rs	Adds directory type checking and implements `SEEK_SET` for directories with bounds validation, while maintaining existing file seek behavior
project/libfuse-fs/src/overlayfs/async_io.rs	Removes FIXME comment and implements comprehensive directory seek support including handle verification, `SEEK_SET`, and `SEEK_CUR` operations with overflow checks

project/libfuse-fs/src/passthrough/async_io.rs

Copilot · 2025-10-28T01:25:15Z

project/libfuse-fs/src/passthrough/async_io.rs

+                    let res = unsafe {
+                        libc::lseek64(
+                            file.as_raw_fd(),
+                            offset as libc::off64_t,
+                            libc::SEEK_SET,
+                        )
+                    };


The unsafe block lacks a // SAFETY: comment explaining why this operation is safe. Per coding guidelines (ID 1000000), all unsafe blocks must include justification. Add a comment explaining that the file descriptor is valid (held by HandleData), offset has been validated, and the operation is FFI-safe.

project/libfuse-fs/src/overlayfs/async_io.rs

Copilot · 2025-10-28T01:25:16Z

project/libfuse-fs/src/overlayfs/async_io.rs

+                        rh.layer
+                            .lseek(req, rh.inode, rh.handle.load(Ordering::Relaxed), offset, whence)
+                            .await
+                    },


[nitpick] Inconsistent comma usage in match arms. The SEEK_SET and SEEK_CUR arms have trailing commas, but the default case _ => on line 974 does not. For consistency and to follow Rust best practices, all match arms should have trailing commas.

project/libfuse-fs/src/overlayfs/async_io.rs

Ricky-Daxia · 2025-10-28T02:38:07Z

project/libfuse-fs/src/overlayfs/async_io.rs

-        }
+            // Special handling and security restrictions for directory operations
+            let handles = self.handles.lock().await;
+            let handle_data = handles.get(&fh).ok_or_else(|| {


这里为了保留与下面的分支的一致性，请用 let (layer, real_inode, real_handle) = self.find_real_info_from_handle(fh).await?;
这几行实际就是 API 的展开，我们尽量复用原有的代码

project/libfuse-fs/src/passthrough/async_io.rs

Ricky-Daxia · 2025-10-28T02:58:09Z

project/libfuse-fs/src/passthrough/async_io.rs

+        if is_dir {
+            match whence {
+                // 目录只支持 SEEK_SET
+                libc::SEEK_SET => {


为什么overlay层支持了 SEEK_SET 和 SEEK_CUR，但是 passthrough 中只支持 SEEK_SET，是不是漏了一种情况
提交代码前可以测试一下，例如写个小程序，通过 libc::lseek(fd, 10, libc::SEEK_SET); 和 libc::lseek(fd, 0, libc::SEEK_CUR); 来验证一下返回值，这样可以帮助发现 bug

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

Copilot · 2025-10-28T15:41:07Z

project/libfuse-fs/src/passthrough/async_io.rs

+                        }
+                        // Set the new offset using libc::lseek64
+                        let res = unsafe {
+                            libc::lseek64(file.as_raw_fd(), new_offset as libc::off64_t, libc::SEEK_SET)


Similar to SEEK_SET branch, casting u64 to off64_t without explicit validation. Although the check on line 1839 ensures new_offset <= i64::MAX, the cast could be clearer by using new_offset as i64 or validating the range explicitly before the cast.

Suggested change

libc::lseek64(file.as_raw_fd(), new_offset as libc::off64_t, libc::SEEK_SET)

libc::lseek64(file.as_raw_fd(), new_offset as i64 as libc::off64_t, libc::SEEK_SET)

Copilot · 2025-10-28T15:41:07Z

project/libfuse-fs/src/overlayfs/async_io.rs

+                .await
+            {
+                Ok(s) => s,
+                Err(_) => return Err(Error::from_raw_os_error(libc::EBADF).into()),


Returning EBADF for all getattr failures loses the original error context. If the underlying error is something other than a bad file descriptor (e.g., EACCES or EIO), this will mislead the caller. Consider propagating the original error or at least logging it before converting to EBADF.

Suggested change

Err(_) => return Err(Error::from_raw_os_error(libc::EBADF).into()),

Err(e) => {

tracing::warn!(

error = ?e,

"Failed to getattr underlying directory handle in lseek"

);

return Err(e.into());

}

Copilot · 2025-10-28T15:41:07Z

project/libfuse-fs/src/overlayfs/async_io.rs

+                    let current = match layer
+                        .lseek(req, real_inode, real_handle, 0, libc::SEEK_CUR)
+                        .await
+                    {
+                        Ok(r) => r.offset,
+                        Err(_) => return Err(Error::from_raw_os_error(libc::EINVAL).into()),
+                    };


Converting any lseek error to EINVAL discards the original error information. The underlying layer may return more specific errors (e.g., EBADF, ESPIPE) that should be preserved to help diagnose issues. Consider propagating the original error instead of converting all failures to EINVAL.

Copilot · 2025-10-28T15:41:08Z

project/libfuse-fs/src/overlayfs/async_io.rs

+                            .await
+                        {
+                            Ok(_) => Ok(ReplyLSeek { offset: new_offset }),
+                            Err(_) => Err(Error::from_raw_os_error(libc::EINVAL).into()),


Similar to the previous SEEK_CUR error handling, converting all lseek errors to EINVAL loses the original error context. The underlying layer may return specific errors that should be propagated to provide accurate error reporting.

Suggested change

Err(_) => Err(Error::from_raw_os_error(libc::EINVAL).into()),

Err(e) => Err(e.into()),

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

Copilot · 2025-10-30T07:08:27Z

project/libfuse-fs/src/passthrough/async_io.rs

-        if res < 0 {
-            Err(io::Error::last_os_error().into())
+                    // Perform the seek operation using libc::lseek64
+                    // This directly manipulates the file descriptor's position


Missing safety comment for unsafe block. The codebase convention uses // Safe because comments before unsafe blocks (as seen on line 1862). Add: // Safe because this doesn't modify any memory and we check the return value.

Suggested change

// This directly manipulates the file descriptor's position

// This directly manipulates the file descriptor's position

// Safe because this doesn't modify any memory and we check the return value.

Copilot · 2025-10-30T07:08:28Z

project/libfuse-fs/src/passthrough/async_io.rs

+                // SEEK_CUR: move relative to current directory offset
+                x if x == libc::SEEK_CUR as u32 => {
+                    // Get current position using libc::lseek64 with offset 0
+                    let cur = unsafe { libc::lseek64(file.as_raw_fd(), 0, libc::SEEK_CUR) };


Missing safety comment for unsafe block. The codebase convention uses // Safe because comments before unsafe blocks. Add: // Safe because this doesn't modify any memory and we check the return value.

Copilot · 2025-10-30T07:08:28Z

project/libfuse-fs/src/passthrough/async_io.rs

+                        let res = unsafe {
+                            libc::lseek64(file.as_raw_fd(), new_offset as libc::off64_t, libc::SEEK_SET)
+                        };


Missing safety comment for unsafe block. The codebase convention uses // Safe because comments before unsafe blocks. Add: // Safe because this doesn't modify any memory and we check the return value.

Copilot · 2025-10-30T07:08:28Z

project/libfuse-fs/src/passthrough/async_io.rs

+                        if res < 0 {
+                            return Err(io::Error::last_os_error().into());
+                        }
+                        Ok(ReplyLSeek { offset: new_offset })


The returned offset should be res as u64 (the actual result from lseek64), not new_offset. While they should match in the success case, using the kernel's returned value ensures consistency with what was actually set and matches the pattern used elsewhere (lines 1825, 1874).

Suggested change

Ok(ReplyLSeek { offset: new_offset })

Ok(ReplyLSeek { offset: res as u64 })

Copilot · 2025-10-30T07:08:28Z

project/libfuse-fs/src/passthrough/async_io.rs

+        // Check file type to determine appropriate lseek handling
+        let st = stat_fd(data.get_file(), None)?;
+        let is_dir = (st.st_mode & libc::S_IFMT) == libc::S_IFDIR;
+
+        if is_dir {
+            // Directory special handling: support SEEK_SET and SEEK_CUR with bounds checks.
+            // Acquire the lock to get exclusive access
+            let (_guard, file) = data.get_file_mut().await;


The file lock get_file_mut() is acquired after determining file type, but stat_fd already accesses the file. Consider acquiring the lock once before stat_fd to avoid potential TOCTOU issues and eliminate redundant locking for directory operations, matching the pattern used in do_readdir (lines 76-81).

Suggested change

// Check file type to determine appropriate lseek handling

let st = stat_fd(data.get_file(), None)?;

let is_dir = (st.st_mode & libc::S_IFMT) == libc::S_IFDIR;

if is_dir {

// Directory special handling: support SEEK_SET and SEEK_CUR with bounds checks.

// Acquire the lock to get exclusive access

let (_guard, file) = data.get_file_mut().await;

// Acquire the lock to get exclusive access before stat_fd to avoid TOCTOU

let (_guard, file) = data.get_file_mut().await;

// Check file type to determine appropriate lseek handling

let st = stat_fd(&*file, None)?;

let is_dir = (st.st_mode & libc::S_IFMT) == libc::S_IFDIR;

if is_dir {

// Directory special handling: support SEEK_SET and SEEK_CUR with bounds checks.

Copilot · 2025-10-30T07:08:29Z

project/libfuse-fs/src/passthrough/async_io.rs

        } else {
-            Ok(ReplyLSeek { offset: res as u64 })
+            // File seek handling for non-directory files
+            // Acquire the lock to get exclusive access, otherwise it may break do_readdir().


The comment 'otherwise it may break do_readdir()' is misleading in the file (non-directory) path. This comment was copied from the directory branch but doesn't apply here since regular files don't use readdir. Consider rephrasing to: 'Acquire the lock to get exclusive access to the file descriptor'.

Suggested change

// Acquire the lock to get exclusive access, otherwise it may break do_readdir().

// Acquire the lock to get exclusive access to the file descriptor.

Copilot · 2025-10-30T07:08:29Z

project/libfuse-fs/src/overlayfs/async_io.rs

-            error!("lseek on directory");
-            return Err(Error::from_raw_os_error(libc::EINVAL).into());
-        }
+            // Special handling and security restrictions for directory operations.


[nitpick] The comment mentions 'security restrictions' but the code primarily validates file type and delegates operations. Either clarify what specific security concern is addressed (e.g., preventing cross-layer attacks) or remove this phrase to avoid confusion.

Suggested change

// Special handling and security restrictions for directory operations.

// Special handling for directory operations: validate type and delegate appropriately.

Ricky-Daxia

Please use gpg-sign when making commits.

genedna requested a review from Copilot October 28, 2025 01:23

Copilot AI reviewed Oct 28, 2025

View reviewed changes

Ricky-Daxia requested changes Oct 28, 2025

View reviewed changes

genedna requested review from Ricky-Daxia and Copilot October 28, 2025 15:39

Copilot AI reviewed Oct 28, 2025

View reviewed changes

Ricky-Daxia requested a review from Copilot October 30, 2025 07:05

Copilot AI reviewed Oct 30, 2025

View reviewed changes

Ricky-Daxia reviewed Oct 30, 2025

View reviewed changes

briteny-pwn force-pushed the main branch from 5528f1b to 5e7cd98 Compare October 30, 2025 08:49

briteny-pwn changed the title ~~“extend the lseek on dir”~~ extend the lseek on dir Oct 30, 2025

Ricky-Daxia self-requested a review October 30, 2025 10:04

Ricky-Daxia approved these changes Oct 30, 2025

View reviewed changes

genedna added this pull request to the merge queue Oct 30, 2025

Merged via the queue into rk8s-dev:main with commit 6eac0e4 Oct 30, 2025
6 checks passed

extend of lseek on dir

5e7cd98

	libc::lseek64(file.as_raw_fd(), new_offset as libc::off64_t, libc::SEEK_SET)
	libc::lseek64(file.as_raw_fd(), new_offset as i64 as libc::off64_t, libc::SEEK_SET)

-                Err(_) => return Err(Error::from_raw_os_error(libc::EBADF).into()),
+                Err(e) => {
+                    tracing::warn!(
+                        error = ?e,
+                        "Failed to getattr underlying directory handle in lseek"
+                    );
+                    return Err(e.into());
+                }

	Err(_) => Err(Error::from_raw_os_error(libc::EINVAL).into()),
	Err(e) => Err(e.into()),

	// This directly manipulates the file descriptor's position
	// This directly manipulates the file descriptor's position
	// Safe because this doesn't modify any memory and we check the return value.

	Ok(ReplyLSeek { offset: new_offset })
	Ok(ReplyLSeek { offset: res as u64 })

	// Acquire the lock to get exclusive access, otherwise it may break do_readdir().
	// Acquire the lock to get exclusive access to the file descriptor.

	// Special handling and security restrictions for directory operations.
	// Special handling for directory operations: validate type and delegate appropriately.

extend the lseek on dir #245

extend the lseek on dir #245

Conversation

briteny-pwn commented Oct 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ricky-Daxia Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ricky-Daxia Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Ricky-Daxia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Ricky-Daxia Oct 28, 2025 •

edited

Loading