On 23.07.24 16:43, Steven Rostedt wrote:
> On Fri, 19 Jul 2024 22:47:01 +0200
> Mathias Krause <mini...@grsecurity.net> wrote:
>
>> Beside the obvious bug, I noticed the following (not fixing the issue,
>> tho):
>>
>> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
>> index 5d88c184f0fc..687ad0a26458 100644
>> --- a/fs/tracefs/event_inode.c
>> +++ b/fs/tracefs/event_inode.c
>> @@ -112,7 +112,7 @@ static void release_ei(struct kref *ref)
>> entry->release(entry->name, ei->data);
>> }
>>
>> - call_rcu(&ei->rcu, free_ei_rcu);
>> + call_srcu(&eventfs_srcu, &ei->rcu, free_ei_rcu);
>> }
>
> This should be fixed too. Care to send a patch for this as well?
Sure, will do.
>
> It use to need RCU but then everything was switched over to SRCU. This was
> just leftover.
SRCU usage came up with commit 63940449555e ("eventfs: Implement eventfs
lookup, read, open functions") and following, was further extended just
to get almost completely nuked by [1] earlier this year.
[1]
https://lore.kernel.org/linux-trace-kernel/20240131184918.945345...@goodmis.org/
>
>>
>> static inline void put_ei(struct eventfs_inode *ei)
>> @@ -735,7 +735,9 @@ struct eventfs_inode *eventfs_create_dir(const char
>> *name, struct eventfs_inode
>>
>> /* Was the parent freed? */
>> if (list_empty(&ei->list)) {
>> + mutex_lock(&eventfs_mutex);
>> cleanup_ei(ei);
>> + mutex_unlock(&eventfs_mutex);
>
> Why do you think this is needed? The ei is not on the list and has not been
> made visible. It was just allocated but the parent it was going to be
> attached to is about to be freed.
I have no strong understanding of the code, just reading into what the
context told me the rules should be, which would be on one hand the
following comment...:
/*
* The eventfs_inode (ei) itself is protected by SRCU. It is released from
* its parent's list and will have is_freed set (under eventfs_mutex).
* After the SRCU grace period is over and the last dput() is called
* the ei is freed.
*/
...and on the other the common pattern, mostly complying to the rule of
first taking the eventfs_mutex, then checking 'is_freed' for a given ei
-- supposedly implying, it can only be set under that very same mutex.
As cleanup_ei() is just a glorified free_ei() which sets ei->is_freed to
1, I was implying the lack of taking eventfs_mutex is a bug. But looking
further for the precondition, getting 'ei' unchained again after it was
put to the parent's children list, I can find eventfs_remove_rec() which
is only ever called under eventfs_mutex and does:
list_del(&ei->list);
free_ei(ei);
So you're right and I wasn't paying close enough attention and got
mislead by cleanup_ei() also setting ei->is_freed. But as it should
already be 1 at that point, no bug here.
Thanks,
Mathias
