On 11/12/26 02:16, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
to consider:
- [High] tracefs: eventfs_iterate() returns -EINVAL without srcu_read_unlock()
- [Medium] tracefs: free_ei() WARN_ON_ONCE fires on uninitialised children list
--
Commit 02dabdf68279c061e3542bcb12e5c799c6b10fbf
Author: Steven Rostedt <[email protected]>
ms/eventfs: Use children field for rcu head and add memory barriers
This commit moves the rcu_head into a union with the children list instead
of the list field. It adds a smp_wmb() in free_ei() after setting is_freed.
It also adds a smp_rmb() and an is_freed check at the top of the SRCU
children loop in eventfs_iterate().
Link: https://patch.msgid.link/20260808094215.4252430d@robin
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
[ ... ]
@@ -124,7 +124,17 @@ static inline void put_ei(struct eventfs_inode *ei)
static inline void free_ei(struct eventfs_inode *ei)
{
if (ei) {
+ /* The ei should have no children if it is being freed. */
+ WARN_ON_ONCE(!list_empty(&ei->children));
[Severity: Medium]
Can this WARN_ON_ONCE() fire spuriously on an eventfs_inode whose children
list was never initialized?
In eventfs_create_events_dir(), ei comes from kzalloc(), so
ei->children.next is NULL. If tracefs_get_inode() fails:
eventfs_create_events_dir()
inode = tracefs_get_inode(dentry->d_sb);
if (unlikely(!inode))
goto fail;
...
fail:
cleanup_ei() / free_ei()
this happens before INIT_LIST_HEAD(&ei->children) runs. list_empty() tests
head->next == head, which is false for NULL, so
WARN_ON_ONCE(!list_empty(&ei->children)) triggers although there are no
children. On systems with panic_on_warn this would be a panic.
This is fixed later in the series by 9d0d1df3c54a ("Initialize ei->children
and ei->list in init_ei()"), which moves the INIT_LIST_HEAD calls into
init_ei(). The warning is incorrect in this commit alone.
ei->is_freed = 1;
[ ... ]
@@ -647,6 +657,20 @@ static int eventfs_iterate(struct file *file, struct
dir_context *ctx)
list_for_each_entry_srcu(ei_child, &ei->children, list,
srcu_read_lock_held(&eventfs_srcu)) {
+ /*
[ ... ]
+ smp_rmb();
+ if (ei->is_freed)
+ return -EINVAL;
[Severity: High]
Does this early return leak the SRCU read lock?
eventfs_iterate() takes idx = srcu_read_lock(&eventfs_srcu) at the top, and
the only srcu_read_unlock() is at the out: label:
eventfs_iterate()
idx = srcu_read_lock(&eventfs_srcu);
...
list_for_each_entry_srcu(...) {
smp_rmb();
if (ei->is_freed)
return -EINVAL; <- bypasses out:
...
out:
srcu_read_unlock(&eventfs_srcu, idx);
When a concurrent eventfs_remove_rec()/free_ei() sets is_freed during a
getdents(), this path is reachable. The eventfs_srcu grace period then
cannot complete, so the call_srcu() callbacks queued by release_ei() never
run. That would leak the eventfs inodes, names and entry_attrs, and could
hang any synchronize_srcu() or srcu_barrier() on that domain.
Would "ret = -EINVAL; goto out;" be the right form here?
This is fixed later in the series by 64fdb88769c6 ("ms/eventfs: fix SRCU
read lock leak in eventfs_iterate()"), which replaces the bare return with
that form. The regression is present in this commit, which affects
bisectability.
if (c > 0) {
c--;
continue;
[ ... ]
Both issues are present only at this step (3/5), and are fixed later in
the series: the SRCU leak by 4/5 and the WARN by 5/5. Tree 6.12.y has
the same split (1b95382da85c, then 9e8999106959). The patches are kept
verbatim so they match 6.12.y exactly.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
