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; [ ... ] -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=3 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
