Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] tracefs: eventfs_iterate() returns with SRCU read lock held on is_freed bail-out --
commit 2fa07daf3577d3718d95f99a986545de113bc3c8 Author: Steven Rostedt <[email protected]> ms/eventfs: Use children field for rcu head and add memory barriers This commit overlays the rcu_head with the children list head instead of the list head in struct eventfs_inode. It adds an smp_wmb() after setting is_freed in free_ei(). It also adds an smp_rmb() and an is_freed check at the top of the children iteration 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 [ ... ] > @@ -645,6 +655,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)) { > > + /* > + * If the ei is being freed, then the ei->children may be > + * being used as the rcu list, which means the next element > + * may be garbage. The ei->is_free is set before switching > + * the ei->children over to ei->rcu. The read memory barrier > + * here makes sure the ei_child is read before is_free is > + * updated. > + * > + * Matches the smp_wmb() in free_ei() > + */ > + smp_rmb(); > + if (ei->is_freed) > + return -EINVAL; > + [Severity: Medium] Can this early return leave eventfs_srcu read-locked? eventfs_iterate() takes the SRCU read lock near the top: idx = srcu_read_lock(&eventfs_srcu); Every other exit path in the function uses goto out or goto out_dec, which reach srcu_read_unlock(&eventfs_srcu, idx). The bare return -EINVAL here bypasses it. If a readdir on an eventfs directory races with free_ei() setting ei->is_freed, the iterator returns with the reader still registered on eventfs_srcu. Grace periods on that domain then cannot complete. call_srcu() callbacks such as free_ei_rcu, queued from release_ei(), would not run. That would leak eventfs_inodes along with their names and entry_attrs. Any synchronize_srcu() or srcu_barrier() on eventfs_srcu could also hang. Should this be ret = -EINVAL; goto out; instead? The later commit in this series, "ms/eventfs: fix SRCU read lock leak in eventfs_iterate()", changes this branch to do that, so the end state of the series is correct. As posted, this commit is still broken and affects bisectability. > if (c > 0) { > c--; > continue; -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=2 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
