Copilot commented on code in PR #21754:
URL: https://github.com/apache/nuttx/pull/21754#discussion_r4547573856
##########
fs/shm/shmfs.c:
##########
@@ -479,6 +479,59 @@ static int shmfs_unmap_area(FAR struct task_group_s *group,
return ret;
}
+/****************************************************************************
+ * Name: shmfs_munmap_partial
+ *
+ * Description:
+ * Shrink the mapping, or split it in two when the middle is unmapped.
+ * Each resulting mapping holds one inode reference. The object's pages
+ * stay allocated until its last mapping is removed.
+ *
+ ****************************************************************************/
+
+static int shmfs_munmap_partial(FAR struct task_group_s *group,
+ FAR struct mm_map_entry_s *entry,
+ FAR void *start, size_t length)
+{
+ FAR char *entry_end = (FAR char *)entry->vaddr + entry->length;
+ FAR char *unmap_end = (FAR char *)start + length;
+ off_t head = (FAR char *)start - (FAR char *)entry->vaddr;
+ off_t tail = entry_end - unmap_end;
Review Comment:
In kernel builds, `shmfs_unmap_area()` removes whole pages, but the
surviving entries use byte-based boundaries. With 4 KiB pages, `munmap(base,
1)` on an 8 KiB mapping leaves a tail at `base + 1`, inside the removed page;
unmapping that tail then violates `up_shmdt()`'s alignment assertion. Validate
start alignment and use a page-rounded removal interval for both unmapping and
bookkeeping, including full-unmap cleanup when rounding removes the entire
mapping.
##########
fs/mmap/fs_mmap.c:
##########
@@ -155,8 +155,14 @@ static int file_mmap_(FAR struct file *filep, FAR void
*start,
* in memory.
*/
+ /* A writable MAP_PRIVATE mapping of a file gets a private copy from
+ * rammap() below; a driver's own mmap() defines its own semantics.
+ */
+
if (filep->f_inode &&
- filep->f_inode->u.i_ops->mmap != NULL)
+ filep->f_inode->u.i_ops->mmap != NULL &&
+ ((flags & MAP_SHARED) != 0 || (prot & PROT_WRITE) == 0 ||
+ INODE_IS_DRIVER(filep->f_inode)))
Review Comment:
This also switches writable private tmpfs mappings from `tmpfs_unmap()` to
`unmap_rammap()`, whose partial-unmap path resizes and records the remaining
allocation using the number of bytes removed rather than the offset of `start`.
For example, unmapping the last 4 KiB of a 12 KiB mapping now leaves a recorded
length of 4 KiB instead of 8 KiB, discarding a still-live portion of the
mapping. Correct `unmap_rammap()` to retain the prefix length before routing
these mappings through it.
##########
fs/shm/shmfs.c:
##########
@@ -479,6 +479,59 @@ static int shmfs_unmap_area(FAR struct task_group_s *group,
return ret;
}
+/****************************************************************************
+ * Name: shmfs_munmap_partial
+ *
+ * Description:
+ * Shrink the mapping, or split it in two when the middle is unmapped.
+ * Each resulting mapping holds one inode reference. The object's pages
+ * stay allocated until its last mapping is removed.
+ *
+ ****************************************************************************/
+
+static int shmfs_munmap_partial(FAR struct task_group_s *group,
+ FAR struct mm_map_entry_s *entry,
+ FAR void *start, size_t length)
+{
+ FAR char *entry_end = (FAR char *)entry->vaddr + entry->length;
+ FAR char *unmap_end = (FAR char *)start + length;
+ off_t head = (FAR char *)start - (FAR char *)entry->vaddr;
+ off_t tail = entry_end - unmap_end;
+ int ret;
+
+ ret = shmfs_unmap_area(group, start, length);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ if (head > 0 && tail > 0)
+ {
+ struct mm_map_entry_s tailmap = *entry;
+
+ tailmap.vaddr = unmap_end;
+ tailmap.length = tail;
+ tailmap.offset = entry->offset + head + length;
+
+ entry->length = head;
+
+ inode_addref((FAR struct inode *)entry->priv.p);
+ return mm_map_add(get_group_mm(group), &tailmap);
Review Comment:
If `mm_map_add()` fails, the middle has already been unmapped, the original
entry shortened, and an inode reference added. This leaves the still-mapped
tail untracked and leaks its inode reference. Reserve the tail entry before
changing the mapping, and remove that reservation if unmapping fails.
##########
fs/mmap/fs_mmap.c:
##########
@@ -155,8 +155,14 @@ static int file_mmap_(FAR struct file *filep, FAR void
*start,
* in memory.
*/
+ /* A writable MAP_PRIVATE mapping of a file gets a private copy from
+ * rammap() below; a driver's own mmap() defines its own semantics.
+ */
+
if (filep->f_inode &&
- filep->f_inode->u.i_ops->mmap != NULL)
+ filep->f_inode->u.i_ops->mmap != NULL &&
+ ((flags & MAP_SHARED) != 0 || (prot & PROT_WRITE) == 0 ||
+ INODE_IS_DRIVER(filep->f_inode)))
Review Comment:
Routing writable private tmpfs/shm mappings through `rammap()` changes the
descriptor's file position because that helper seeks and reads without
restoring it. Mapping all four bytes of a tmpfs file positioned at byte 1 now
leaves it at EOF, so the next read uses the wrong offset. Make the RAM-copy
path preserve the file position on both success and error paths.
--
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]
