xiaoxiang781216 commented on code in PR #19599:
URL: https://github.com/apache/nuttx/pull/19599#discussion_r3698691364
##########
fs/vfs/fs_unlink.c:
##########
@@ -88,6 +88,14 @@ int nx_unlink(FAR const char *pathname)
if (INODE_IS_MOUNTPT(inode) && inode->u.i_mops)
{
+ inode_rlock();
Review Comment:
why not move the lock into inode_checksearchpath
##########
fs/vfs/fs_stat.c:
##########
@@ -105,6 +105,15 @@ static int stat_recursive(FAR const char *path,
inode = desc.node;
DEBUGASSERT(inode != NULL);
+ inode_rlock();
Review Comment:
ditto
##########
fs/vfs/fs_open.c:
##########
@@ -170,17 +170,29 @@ static int file_vopen(FAR struct file *filep, FAR const
char *path,
}
#endif
- /* Validate operation support and pseudo-filesystem permissions */
+ /* Enforce directory search (X_OK) on ancestors / mount gates, then
+ * validate open modes. Hold the inode tree read lock so modes cannot
+ * race with concurrent chmod of parents for pseudo nodes.
+ */
+
+ inode_rlock();
Review Comment:
ditto
##########
fs/vfs/fs_mkdir.c:
##########
@@ -100,6 +100,15 @@ int mkdir(const char *pathname, mode_t mode)
goto errout_with_inode;
}
+ inode_rlock();
Review Comment:
ditto
##########
fs/inode/fs_inode.c:
##########
@@ -214,71 +214,117 @@ void inode_runlock(void)
}
/****************************************************************************
- * Name: inode_checkperm
+ * Name: inode_permission
*
* Description:
- * Check 'inode' for 'amode' access on pseudo-filesystem inodes.
- * NULL 'inode' (root) and mountpoints are exempt.
- *
- * Input Parameters:
- * inode - Inode to check, or NULL for a root-level path
- * amode - Access mode bitmask (R_OK / W_OK / X_OK)
- *
- * Returned Value:
- * Zero (OK) on success, or -EACCES if permission is denied.
+ * Generic access-mode check against an inode's stored owner/group/mode.
+ * Applies to pseudoFS nodes and to mountpoint inodes (whose i_mode gates
+ * traversal into the mounted filesystem). Optional mountpt_operations
+ * .permission may expose the same policy for in-volume paths; the VFS
+ * does not call that hook for mount-crossing.
*
****************************************************************************/
+int inode_permission(FAR struct inode *inode, int amode)
+{
+#ifdef CONFIG_FS_PERMISSION
+ if (inode == NULL)
+ {
+ return OK;
+ }
+
+ return fs_checkmode(inode->i_owner, inode->i_group, inode->i_mode, amode);
+#else
+ UNUSED(inode);
+ UNUSED(amode);
+ return OK;
+#endif
+}
+
/****************************************************************************
- * Name: inode_checkperm
+ * Name: inode_checksearchpath
+ *
+ * Description:
+ * Require X_OK on every ancestor of 'inode', and on 'inode' itself when
+ * it is a directory or mountpoint that must be traversed.
+ *
****************************************************************************/
-int inode_checkperm(FAR struct inode *inode, int amode)
+int inode_checksearchpath(FAR struct inode *inode)
Review Comment:
inode_checkpathperm
##########
fs/inode/fs_inode.c:
##########
@@ -214,71 +214,117 @@ void inode_runlock(void)
}
/****************************************************************************
- * Name: inode_checkperm
+ * Name: inode_permission
*
* Description:
- * Check 'inode' for 'amode' access on pseudo-filesystem inodes.
- * NULL 'inode' (root) and mountpoints are exempt.
- *
- * Input Parameters:
- * inode - Inode to check, or NULL for a root-level path
- * amode - Access mode bitmask (R_OK / W_OK / X_OK)
- *
- * Returned Value:
- * Zero (OK) on success, or -EACCES if permission is denied.
+ * Generic access-mode check against an inode's stored owner/group/mode.
+ * Applies to pseudoFS nodes and to mountpoint inodes (whose i_mode gates
+ * traversal into the mounted filesystem). Optional mountpt_operations
+ * .permission may expose the same policy for in-volume paths; the VFS
+ * does not call that hook for mount-crossing.
*
****************************************************************************/
+int inode_permission(FAR struct inode *inode, int amode)
Review Comment:
why change the function name
##########
fs/vfs/fs_rename.c:
##########
@@ -516,6 +521,15 @@ int rename(FAR const char *oldpath, FAR const char
*newpath)
oldinode = olddesc.node;
DEBUGASSERT(oldinode != NULL);
+ inode_rlock();
Review Comment:
ditto
##########
fs/mount/fs_mount.c:
##########
@@ -403,6 +403,22 @@ int nx_mount(FAR const char *source, FAR const char
*target,
inode_release(mountpt_inode);
goto errout_with_lock;
}
+
+ /* Require search on ancestors and write on the mount target. */
+
+ ret = inode_checksearchpath(mountpt_inode);
+ if (ret < 0)
+ {
+ inode_release(mountpt_inode);
+ goto errout_with_lock;
+ }
+
+ ret = inode_permission(mountpt_inode, W_OK);
Review Comment:
ditto
##########
fs/vfs/fs_readlink.c:
##########
@@ -94,6 +94,15 @@ ssize_t readlink(FAR const char *path, FAR char *buf, size_t
bufsize)
node = desc.node;
DEBUGASSERT(node != NULL);
+ inode_rlock();
Review Comment:
ditto
##########
fs/inode/fs_inodereserve.c:
##########
@@ -229,7 +229,15 @@ int inode_reserve(FAR const char *path,
#ifdef CONFIG_FS_PERMISSION
if (parent != NULL)
{
- ret = inode_checkperm(parent, W_OK | X_OK);
+ /* Traverse ancestors (X_OK), then require write on the parent. */
+
+ ret = inode_checksearchpath(parent);
+ if (ret < 0)
+ {
+ goto errout_with_search;
+ }
+
+ ret = inode_permission(parent, W_OK);
Review Comment:
ditto
##########
fs/vfs/fs_rmdir.c:
##########
@@ -91,6 +91,15 @@ int rmdir(FAR const char *pathname)
if (INODE_IS_MOUNTPT(inode) && inode->u.i_mops)
{
+ inode_rlock();
Review Comment:
ditto
##########
fs/mount/fs_umount2.c:
##########
@@ -110,9 +110,24 @@ int nx_umount2(FAR const char *target, unsigned int flags)
* performed, or a negated error code on a failure.
*/
- /* Hold the semaphore through the unbind logic */
+ /* Hold the inode tree lock across permission checks and unbind,
+ * matching mount()'s check-under-lock pattern.
+ */
inode_lock();
+
+ ret = inode_checksearchpath(mountpt_inode);
+ if (ret < 0)
+ {
+ goto errout_with_lock;
+ }
+
+ ret = inode_permission(mountpt_inode, W_OK);
Review Comment:
why not add a new api or flag to merge
inode_checksearchpath/inode_permission into one call?
##########
fs/vfs/fs_chstat.c:
##########
@@ -71,6 +71,15 @@ static int chstat_recursive(FAR const char *path,
inode = desc.node;
DEBUGASSERT(inode != NULL);
+ inode_rlock();
Review Comment:
ditto
##########
Documentation/implementation/index.rst:
##########
@@ -16,6 +16,7 @@ Implementation Details
device_nodes.rst
drivers_design.rst
file_descriptors.rst
+ fs_permission_iface.rst
Review Comment:
file_permission.rst
##########
fs/vfs/fs_statfs.c:
##########
@@ -110,6 +110,15 @@ int statfs(FAR const char *path, FAR struct statfs *buf)
inode = desc.node;
DEBUGASSERT(inode != NULL);
+ inode_rlock();
Review Comment:
ditto
##########
fs/inode/fs_inoderemove.c:
##########
@@ -82,7 +82,14 @@ static FAR struct inode *inode_unlink(FAR const char *path)
#ifdef CONFIG_FS_PERMISSION
if (desc.parent != NULL)
{
- ret = inode_checkperm(desc.parent, W_OK);
+ ret = inode_checksearchpath(desc.parent);
+ if (ret < 0)
+ {
+ inode = NULL;
+ goto errout;
+ }
+
+ ret = inode_permission(desc.parent, W_OK);
Review Comment:
ditto
--
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]
