Skip to content

Commit 5b2b117

Browse files
committed
Merge tag 'trace-v6.7-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace
Pull tracing fixes from Steven Rostedt:: "Eventfs fixes: - With the usage of simple_recursive_remove() recommended by Al Viro, the code should not be calling "d_invalidate()" itself. Doing so is causing crashes. The code was calling d_invalidate() on the race of trying to look up a file while the parent was being deleted. This was detected, and the added dentry was having d_invalidate() called on it, but the deletion of the directory was also calling d_invalidate() on that same dentry. - A fix to not free the eventfs_inode (ei) until the last dput() was called on its ei->dentry made the ei->dentry exist even after it was marked for free by setting the ei->is_freed. But code elsewhere still was checking if ei->dentry was NULL if ei->is_freed is set and would trigger WARN_ON if that was the case. That's no longer true and there should not be any warnings when it is true. - Use GFP_NOFS for allocations done under eventfs_mutex. The eventfs_mutex can be taken on file system reclaim, make sure that allocations done under that mutex do not trigger file system reclaim. - Clean up code by moving the taking of inode_lock out of the helper functions and into where they are needed, and not use the parameter to know to take it or not. It must always be held but some callers of the helper function have it taken when they were called. - Warn if the inode_lock is not held in the helper functions. - Warn if eventfs_start_creating() is called without a parent. As eventfs is underneath tracefs, all files created will have a parent (the top one will have a tracefs parent). Tracing update: - Add Mathieu Desnoyers as an official reviewer of the tracing subsystem" * tag 'trace-v6.7-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace: MAINTAINERS: TRACING: Add Mathieu Desnoyers as Reviewer eventfs: Make sure that parent->d_inode is locked in creating files/dirs eventfs: Do not allow NULL parent to eventfs_start_creating() eventfs: Move taking of inode_lock into dcache_dir_open_wrapper() eventfs: Use GFP_NOFS for allocation when eventfs_mutex is held eventfs: Do not invalidate dentry in create_file/dir_dentry() eventfs: Remove expectation that ei->is_freed means ei->dentry == NULL
2 parents d2da77f + 76d9eaf commit 5b2b117

File tree

3 files changed

+31
-48
lines changed

3 files changed

+31
-48
lines changed

MAINTAINERS

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22079,6 +22079,7 @@ F: drivers/watchdog/tqmx86_wdt.c
2207922079
TRACING
2208022080
M: Steven Rostedt <[email protected]>
2208122081
M: Masami Hiramatsu <[email protected]>
22082+
R: Mathieu Desnoyers <[email protected]>
2208222083
2208322084
2208422085
S: Maintained

fs/tracefs/event_inode.c

Lines changed: 26 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -27,16 +27,16 @@
2727
/*
2828
* eventfs_mutex protects the eventfs_inode (ei) dentry. Any access
2929
* to the ei->dentry must be done under this mutex and after checking
30-
* if ei->is_freed is not set. The ei->dentry is released under the
31-
* mutex at the same time ei->is_freed is set. If ei->is_freed is set
32-
* then the ei->dentry is invalid.
30+
* if ei->is_freed is not set. When ei->is_freed is set, the dentry
31+
* is on its way to being freed after the last dput() is made on it.
3332
*/
3433
static DEFINE_MUTEX(eventfs_mutex);
3534

3635
/*
3736
* The eventfs_inode (ei) itself is protected by SRCU. It is released from
3837
* its parent's list and will have is_freed set (under eventfs_mutex).
39-
* After the SRCU grace period is over, the ei may be freed.
38+
* After the SRCU grace period is over and the last dput() is called
39+
* the ei is freed.
4040
*/
4141
DEFINE_STATIC_SRCU(eventfs_srcu);
4242

@@ -95,7 +95,7 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,
9595
if (!(dentry->d_inode->i_mode & S_IFDIR)) {
9696
if (!ei->entry_attrs) {
9797
ei->entry_attrs = kzalloc(sizeof(*ei->entry_attrs) * ei->nr_entries,
98-
GFP_KERNEL);
98+
GFP_NOFS);
9999
if (!ei->entry_attrs) {
100100
ret = -ENOMEM;
101101
goto out;
@@ -326,7 +326,8 @@ create_file_dentry(struct eventfs_inode *ei, int idx,
326326
struct eventfs_attr *attr = NULL;
327327
struct dentry **e_dentry = &ei->d_children[idx];
328328
struct dentry *dentry;
329-
bool invalidate = false;
329+
330+
WARN_ON_ONCE(!inode_is_locked(parent->d_inode));
330331

331332
mutex_lock(&eventfs_mutex);
332333
if (ei->is_freed) {
@@ -348,15 +349,8 @@ create_file_dentry(struct eventfs_inode *ei, int idx,
348349

349350
mutex_unlock(&eventfs_mutex);
350351

351-
/* The lookup already has the parent->d_inode locked */
352-
if (!lookup)
353-
inode_lock(parent->d_inode);
354-
355352
dentry = create_file(name, mode, attr, parent, data, fops);
356353

357-
if (!lookup)
358-
inode_unlock(parent->d_inode);
359-
360354
mutex_lock(&eventfs_mutex);
361355

362356
if (IS_ERR_OR_NULL(dentry)) {
@@ -365,12 +359,14 @@ create_file_dentry(struct eventfs_inode *ei, int idx,
365359
* created the dentry for this e_dentry. In which case
366360
* use that one.
367361
*
368-
* Note, with the mutex held, the e_dentry cannot have content
369-
* and the ei->is_freed be true at the same time.
362+
* If ei->is_freed is set, the e_dentry is currently on its
363+
* way to being freed, don't return it. If e_dentry is NULL
364+
* it means it was already freed.
370365
*/
371-
dentry = *e_dentry;
372-
if (WARN_ON_ONCE(dentry && ei->is_freed))
366+
if (ei->is_freed)
373367
dentry = NULL;
368+
else
369+
dentry = *e_dentry;
374370
/* The lookup does not need to up the dentry refcount */
375371
if (dentry && !lookup)
376372
dget(dentry);
@@ -387,17 +383,14 @@ create_file_dentry(struct eventfs_inode *ei, int idx,
387383
* Otherwise it means two dentries exist with the same name.
388384
*/
389385
WARN_ON_ONCE(!ei->is_freed);
390-
invalidate = true;
386+
dentry = NULL;
391387
}
392388
mutex_unlock(&eventfs_mutex);
393389

394-
if (invalidate)
395-
d_invalidate(dentry);
396-
397-
if (lookup || invalidate)
390+
if (lookup)
398391
dput(dentry);
399392

400-
return invalidate ? NULL : dentry;
393+
return dentry;
401394
}
402395

403396
/**
@@ -437,9 +430,10 @@ static struct dentry *
437430
create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
438431
struct dentry *parent, bool lookup)
439432
{
440-
bool invalidate = false;
441433
struct dentry *dentry = NULL;
442434

435+
WARN_ON_ONCE(!inode_is_locked(parent->d_inode));
436+
443437
mutex_lock(&eventfs_mutex);
444438
if (pei->is_freed || ei->is_freed) {
445439
mutex_unlock(&eventfs_mutex);
@@ -456,15 +450,8 @@ create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
456450
}
457451
mutex_unlock(&eventfs_mutex);
458452

459-
/* The lookup already has the parent->d_inode locked */
460-
if (!lookup)
461-
inode_lock(parent->d_inode);
462-
463453
dentry = create_dir(ei, parent);
464454

465-
if (!lookup)
466-
inode_unlock(parent->d_inode);
467-
468455
mutex_lock(&eventfs_mutex);
469456

470457
if (IS_ERR_OR_NULL(dentry) && !ei->is_freed) {
@@ -473,8 +460,8 @@ create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
473460
* created the dentry for this e_dentry. In which case
474461
* use that one.
475462
*
476-
* Note, with the mutex held, the e_dentry cannot have content
477-
* and the ei->is_freed be true at the same time.
463+
* If ei->is_freed is set, the e_dentry is currently on its
464+
* way to being freed.
478465
*/
479466
dentry = ei->dentry;
480467
if (dentry && !lookup)
@@ -493,16 +480,14 @@ create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
493480
* Otherwise it means two dentries exist with the same name.
494481
*/
495482
WARN_ON_ONCE(!ei->is_freed);
496-
invalidate = true;
483+
dentry = NULL;
497484
}
498485
mutex_unlock(&eventfs_mutex);
499-
if (invalidate)
500-
d_invalidate(dentry);
501486

502-
if (lookup || invalidate)
487+
if (lookup)
503488
dput(dentry);
504489

505-
return invalidate ? NULL : dentry;
490+
return dentry;
506491
}
507492

508493
/**
@@ -632,7 +617,7 @@ static int add_dentries(struct dentry ***dentries, struct dentry *d, int cnt)
632617
{
633618
struct dentry **tmp;
634619

635-
tmp = krealloc(*dentries, sizeof(d) * (cnt + 2), GFP_KERNEL);
620+
tmp = krealloc(*dentries, sizeof(d) * (cnt + 2), GFP_NOFS);
636621
if (!tmp)
637622
return -1;
638623
tmp[cnt] = d;
@@ -698,6 +683,7 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
698683
return -ENOMEM;
699684
}
700685

686+
inode_lock(parent->d_inode);
701687
list_for_each_entry_srcu(ei_child, &ei->children, list,
702688
srcu_read_lock_held(&eventfs_srcu)) {
703689
d = create_dir_dentry(ei, ei_child, parent, false);
@@ -730,6 +716,7 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
730716
cnt++;
731717
}
732718
}
719+
inode_unlock(parent->d_inode);
733720
srcu_read_unlock(&eventfs_srcu, idx);
734721
ret = dcache_dir_open(inode, file);
735722

fs/tracefs/inode.c

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -509,20 +509,15 @@ struct dentry *eventfs_start_creating(const char *name, struct dentry *parent)
509509
struct dentry *dentry;
510510
int error;
511511

512+
/* Must always have a parent. */
513+
if (WARN_ON_ONCE(!parent))
514+
return ERR_PTR(-EINVAL);
515+
512516
error = simple_pin_fs(&trace_fs_type, &tracefs_mount,
513517
&tracefs_mount_count);
514518
if (error)
515519
return ERR_PTR(error);
516520

517-
/*
518-
* If the parent is not specified, we create it in the root.
519-
* We need the root dentry to do this, which is in the super
520-
* block. A pointer to that is in the struct vfsmount that we
521-
* have around.
522-
*/
523-
if (!parent)
524-
parent = tracefs_mount->mnt_root;
525-
526521
if (unlikely(IS_DEADDIR(parent->d_inode)))
527522
dentry = ERR_PTR(-ENOENT);
528523
else

0 commit comments

Comments
 (0)