Merge tag 'trace-tracefs-v6.10' of git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace

Pull tracefs/eventfs updates from Steven Rostedt:
"Bug fixes:

- The eventfs directories need to have unique inode numbers. Make
sure that they do not get the default file inode number.

- Update the inode uid and gid fields on remount.

When a remount happens where a uid and/or gid is specified, all the
tracefs files and directories should get the specified uid and/or
gid. But this can be sporadic when some uids were assigned already.
There's already a list of inodes that are allocated. Just update
their uid and gid fields at the time of remount.

- Update the eventfs_inodes on remount from the top level "events"
descriptor.

There was a bug where not all the eventfs files or directories
where getting updated on remount. One fix was to clear the
SAVED_UID/GID flags from the inode list during the iteration of the
inodes during the remount. But because the eventfs inodes can be
freed when the last referenced is released, not all the
eventfs_inodes were being updated. This lead to the ownership
selftest to fail if it was run a second time (the first time would
leave eventfs_inodes with no corresponding tracefs_inode).

Instead, for eventfs_inodes, only process the "events"
eventfs_inode from the list iteration, as it is guaranteed to have
a tracefs_inode (it's never freed while the "events" directory
exists). As it has a list of its children, and the children have a
list of their children, just iterate all the eventfs_inodes from
the "events" descriptor and it is guaranteed to get all of them.

- Clear the EVENT_INODE flag from the tracefs_drop_inode() callback.

Currently the EVENTFS_INODE FLAG is cleared in the tracefs_d_iput()
callback. But this is the wrong location. The iput() callback is
called when the last reference to the dentry inode is hit. There
could be a case where two dentry's have the same inode, and the
flag will be cleared prematurely. The flag needs to be cleared when
the last reference of the inode is dropped and that happens in the
inode's drop_inode() callback handler.

Cleanups:

- Consolidate the creation of a tracefs_inode for an eventfs_inode

A tracefs_inode is created for both files and directories of the
eventfs system. It is open coded. Instead, consolidate it into a
single eventfs_get_inode() function call.

- Remove the eventfs getattr and permission callbacks.

The permissions for the eventfs files and directories are updated
when the inodes are created, on remount, and when the user sets
them (via setattr). The inodes hold the current permissions so
there is no need to have custom getattr or permissions callbacks as
they will more likely cause them to be incorrect. The inode's
permissions are updated when they should be updated. Remove the
getattr and permissions inode callbacks.

- Do not update eventfs_inode attributes on creation of inodes.

The eventfs_inodes attribute field is used to store the permissions
of the directories and files for when their corresponding inodes
are freed and are created again. But when the creation of the
inodes happen, the eventfs_inode attributes are recalculated. The
recalculation should only happen when the permissions change for a
given file or directory. Currently, the attribute changes are just
being set to their current files so this is not a bug, but it's
unnecessary and error prone. Stop doing that.

- The events directory inode is created once when the events
directory is created and deleted when it is deleted. It is now
updated on remount and when the user changes the permissions.
There's no need to use the eventfs_inode of the events directory to
store the events directory permissions. But using it to store the
default permissions for the files within the directory that have
not been updated by the user can simplify the code"

* tag 'trace-tracefs-v6.10' of git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace:
eventfs: Do not use attributes for events directory
eventfs: Cleanup permissions in creation of inodes
eventfs: Remove getattr and permission callbacks
eventfs: Consolidate the eventfs_inode update in eventfs_get_inode()
tracefs: Clear EVENT_INODE flag in tracefs_drop_inode()
eventfs: Update all the eventfs_inodes from the events descriptor
tracefs: Update inode permissions on remount
eventfs: Keep the directories from having the same inode number as files

Linus Torvalds 2 years ago 0eb03c7e 6d69b6c1

+128 -167

2 changed files

expand all

tracefs

event_inode.c

inode.c

+100 -149

fs/tracefs/event_inode.c

··· 37 37 38 38 struct eventfs_root_inode { 39 39 struct eventfs_inode ei; 40 - struct inode *parent_inode; 41 40 struct dentry *events_dir; 42 41 }; 43 42 ··· 49 50 /* Just try to make something consistent and unique */ 50 51 static int eventfs_dir_ino(struct eventfs_inode *ei) 51 52 { 52 - if (!ei->ino) 53 + if (!ei->ino) { 53 54 ei->ino = get_next_ino(); 55 + /* Must not have the file inode number */ 56 + if (ei->ino == EVENTFS_FILE_INODE_INO) 57 + ei->ino = get_next_ino(); 58 + } 54 59 55 60 return ei->ino; 56 61 } ··· 210 207 * determined by the parent directory. 211 208 */ 212 209 if (dentry->d_inode->i_mode & S_IFDIR) { 213 - update_attr(&ei->attr, iattr); 210 + /* Just use the inode permissions for the events directory */ 211 + if (!ei->is_events) 212 + update_attr(&ei->attr, iattr); 214 213 215 214 } else { 216 215 name = dentry->d_name.name; ··· 230 225 return ret; 231 226 } 232 227 233 - static void update_events_attr(struct eventfs_inode *ei, struct super_block *sb) 234 - { 235 - struct eventfs_root_inode *rei; 236 - struct inode *parent; 237 - 238 - rei = get_root_inode(ei); 239 - 240 - /* Use the parent inode permissions unless root set its permissions */ 241 - parent = rei->parent_inode; 242 - 243 - if (rei->ei.attr.mode & EVENTFS_SAVE_UID) 244 - ei->attr.uid = rei->ei.attr.uid; 245 - else 246 - ei->attr.uid = parent->i_uid; 247 - 248 - if (rei->ei.attr.mode & EVENTFS_SAVE_GID) 249 - ei->attr.gid = rei->ei.attr.gid; 250 - else 251 - ei->attr.gid = parent->i_gid; 252 - } 253 - 254 - static void set_top_events_ownership(struct inode *inode) 255 - { 256 - struct tracefs_inode *ti = get_tracefs(inode); 257 - struct eventfs_inode *ei = ti->private; 258 - 259 - /* The top events directory doesn't get automatically updated */ 260 - if (!ei || !ei->is_events) 261 - return; 262 - 263 - update_events_attr(ei, inode->i_sb); 264 - 265 - if (!(ei->attr.mode & EVENTFS_SAVE_UID)) 266 - inode->i_uid = ei->attr.uid; 267 - 268 - if (!(ei->attr.mode & EVENTFS_SAVE_GID)) 269 - inode->i_gid = ei->attr.gid; 270 - } 271 - 272 - static int eventfs_get_attr(struct mnt_idmap *idmap, 273 - const struct path *path, struct kstat *stat, 274 - u32 request_mask, unsigned int flags) 275 - { 276 - struct dentry *dentry = path->dentry; 277 - struct inode *inode = d_backing_inode(dentry); 278 - 279 - set_top_events_ownership(inode); 280 - 281 - generic_fillattr(idmap, request_mask, inode, stat); 282 - return 0; 283 - } 284 - 285 - static int eventfs_permission(struct mnt_idmap *idmap, 286 - struct inode *inode, int mask) 287 - { 288 - set_top_events_ownership(inode); 289 - return generic_permission(idmap, inode, mask); 290 - } 291 - 292 228 static const struct inode_operations eventfs_dir_inode_operations = { 293 229 .lookup = eventfs_root_lookup, 294 230 .setattr = eventfs_set_attr, 295 - .getattr = eventfs_get_attr, 296 - .permission = eventfs_permission, 297 231 }; 298 232 299 233 static const struct inode_operations eventfs_file_inode_operations = { ··· 245 301 .llseek = generic_file_llseek, 246 302 }; 247 303 304 + static void eventfs_set_attrs(struct eventfs_inode *ei, bool update_uid, kuid_t uid, 305 + bool update_gid, kgid_t gid, int level) 306 + { 307 + struct eventfs_inode *ei_child; 308 + 309 + /* Update events/<system>/<event> */ 310 + if (WARN_ON_ONCE(level > 3)) 311 + return; 312 + 313 + if (update_uid) { 314 + ei->attr.mode &= ~EVENTFS_SAVE_UID; 315 + ei->attr.uid = uid; 316 + } 317 + 318 + if (update_gid) { 319 + ei->attr.mode &= ~EVENTFS_SAVE_GID; 320 + ei->attr.gid = gid; 321 + } 322 + 323 + list_for_each_entry(ei_child, &ei->children, list) { 324 + eventfs_set_attrs(ei_child, update_uid, uid, update_gid, gid, level + 1); 325 + } 326 + 327 + if (!ei->entry_attrs) 328 + return; 329 + 330 + for (int i = 0; i < ei->nr_entries; i++) { 331 + if (update_uid) { 332 + ei->entry_attrs[i].mode &= ~EVENTFS_SAVE_UID; 333 + ei->entry_attrs[i].uid = uid; 334 + } 335 + if (update_gid) { 336 + ei->entry_attrs[i].mode &= ~EVENTFS_SAVE_GID; 337 + ei->entry_attrs[i].gid = gid; 338 + } 339 + } 340 + 341 + } 342 + 248 343 /* 249 344 * On a remount of tracefs, if UID or GID options are set, then 250 345 * the mount point inode permissions should be used. ··· 293 310 { 294 311 struct eventfs_inode *ei = ti->private; 295 312 296 - if (!ei) 313 + /* Only the events directory does the updates */ 314 + if (!ei || !ei->is_events || ei->is_freed) 297 315 return; 298 316 299 - if (update_uid) 300 - ei->attr.mode &= ~EVENTFS_SAVE_UID; 301 - 302 - if (update_gid) 303 - ei->attr.mode &= ~EVENTFS_SAVE_GID; 304 - 305 - if (!ei->entry_attrs) 306 - return; 307 - 308 - for (int i = 0; i < ei->nr_entries; i++) { 309 - if (update_uid) 310 - ei->entry_attrs[i].mode &= ~EVENTFS_SAVE_UID; 311 - if (update_gid) 312 - ei->entry_attrs[i].mode &= ~EVENTFS_SAVE_GID; 313 - } 317 + eventfs_set_attrs(ei, update_uid, ti->vfs_inode.i_uid, 318 + update_gid, ti->vfs_inode.i_gid, 0); 314 319 } 315 320 316 - /* Return the evenfs_inode of the "events" directory */ 317 - static struct eventfs_inode *eventfs_find_events(struct dentry *dentry) 321 + static void update_inode_attr(struct inode *inode, umode_t mode, 322 + struct eventfs_attr *attr, struct eventfs_root_inode *rei) 318 323 { 319 - struct eventfs_inode *ei; 320 - 321 - do { 322 - // The parent is stable because we do not do renames 323 - dentry = dentry->d_parent; 324 - // ... and directories always have d_fsdata 325 - ei = dentry->d_fsdata; 326 - 327 - /* 328 - * If the ei is being freed, the ownership of the children 329 - * doesn't matter. 330 - */ 331 - if (ei->is_freed) 332 - return NULL; 333 - 334 - // Walk upwards until you find the events inode 335 - } while (!ei->is_events); 336 - 337 - update_events_attr(ei, dentry->d_sb); 338 - 339 - return ei; 340 - } 341 - 342 - static void update_inode_attr(struct dentry *dentry, struct inode *inode, 343 - struct eventfs_attr *attr, umode_t mode) 344 - { 345 - struct eventfs_inode *events_ei = eventfs_find_events(dentry); 346 - 347 - if (!events_ei) 348 - return; 349 - 350 - inode->i_mode = mode; 351 - inode->i_uid = events_ei->attr.uid; 352 - inode->i_gid = events_ei->attr.gid; 353 - 354 - if (!attr) 355 - return; 356 - 357 - if (attr->mode & EVENTFS_SAVE_MODE) 324 + if (attr && attr->mode & EVENTFS_SAVE_MODE) 358 325 inode->i_mode = attr->mode & EVENTFS_MODE_MASK; 326 + else 327 + inode->i_mode = mode; 359 328 360 - if (attr->mode & EVENTFS_SAVE_UID) 329 + if (attr && attr->mode & EVENTFS_SAVE_UID) 361 330 inode->i_uid = attr->uid; 331 + else 332 + inode->i_uid = rei->ei.attr.uid; 362 333 363 - if (attr->mode & EVENTFS_SAVE_GID) 334 + if (attr && attr->mode & EVENTFS_SAVE_GID) 364 335 inode->i_gid = attr->gid; 336 + else 337 + inode->i_gid = rei->ei.attr.gid; 338 + } 339 + 340 + static struct inode *eventfs_get_inode(struct dentry *dentry, struct eventfs_attr *attr, 341 + umode_t mode, struct eventfs_inode *ei) 342 + { 343 + struct eventfs_root_inode *rei; 344 + struct eventfs_inode *pei; 345 + struct tracefs_inode *ti; 346 + struct inode *inode; 347 + 348 + inode = tracefs_get_inode(dentry->d_sb); 349 + if (!inode) 350 + return NULL; 351 + 352 + ti = get_tracefs(inode); 353 + ti->private = ei; 354 + ti->flags |= TRACEFS_EVENT_INODE; 355 + 356 + /* Find the top dentry that holds the "events" directory */ 357 + do { 358 + dentry = dentry->d_parent; 359 + /* Directories always have d_fsdata */ 360 + pei = dentry->d_fsdata; 361 + } while (!pei->is_events); 362 + 363 + rei = get_root_inode(pei); 364 + 365 + update_inode_attr(inode, mode, attr, rei); 366 + 367 + return inode; 365 368 } 366 369 367 370 /** ··· 370 401 void *data, 371 402 const struct file_operations *fop) 372 403 { 373 - struct tracefs_inode *ti; 374 404 struct inode *inode; 375 405 376 406 if (!(mode & S_IFMT)) ··· 378 410 if (WARN_ON_ONCE(!S_ISREG(mode))) 379 411 return ERR_PTR(-EIO); 380 412 381 - inode = tracefs_get_inode(dentry->d_sb); 413 + /* Only directories have ti->private set to an ei, not files */ 414 + inode = eventfs_get_inode(dentry, attr, mode, NULL); 382 415 if (unlikely(!inode)) 383 416 return ERR_PTR(-ENOMEM); 384 - 385 - /* If the user updated the directory's attributes, use them */ 386 - update_inode_attr(dentry, inode, attr, mode); 387 417 388 418 inode->i_op = &eventfs_file_inode_operations; 389 419 inode->i_fop = fop; ··· 389 423 390 424 /* All files will have the same inode number */ 391 425 inode->i_ino = EVENTFS_FILE_INODE_INO; 392 - 393 - ti = get_tracefs(inode); 394 - ti->flags |= TRACEFS_EVENT_INODE; 395 426 396 427 // Files have their parent's ei as their fsdata 397 428 dentry->d_fsdata = get_ei(parent_ei); ··· 409 446 static struct dentry *lookup_dir_entry(struct dentry *dentry, 410 447 struct eventfs_inode *pei, struct eventfs_inode *ei) 411 448 { 412 - struct tracefs_inode *ti; 413 449 struct inode *inode; 450 + umode_t mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO; 414 451 415 - inode = tracefs_get_inode(dentry->d_sb); 452 + inode = eventfs_get_inode(dentry, &ei->attr, mode, ei); 416 453 if (unlikely(!inode)) 417 454 return ERR_PTR(-ENOMEM); 418 - 419 - /* If the user updated the directory's attributes, use them */ 420 - update_inode_attr(dentry, inode, &ei->attr, 421 - S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO); 422 455 423 456 inode->i_op = &eventfs_dir_inode_operations; 424 457 inode->i_fop = &eventfs_file_operations; 425 458 426 459 /* All directories will have the same inode number */ 427 460 inode->i_ino = eventfs_dir_ino(ei); 428 - 429 - ti = get_tracefs(inode); 430 - ti->flags |= TRACEFS_EVENT_INODE; 431 - /* Only directories have ti->private set to an ei, not files */ 432 - ti->private = ei; 433 461 434 462 dentry->d_fsdata = get_ei(ei); 435 463 ··· 782 828 // Note: we have a ref to the dentry from tracefs_start_creating() 783 829 rei = get_root_inode(ei); 784 830 rei->events_dir = dentry; 785 - rei->parent_inode = d_inode(dentry->d_sb->s_root); 786 831 787 832 ei->entries = entries; 788 833 ei->nr_entries = size; ··· 791 838 uid = d_inode(dentry->d_parent)->i_uid; 792 839 gid = d_inode(dentry->d_parent)->i_gid; 793 840 841 + /* 842 + * The ei->attr will be used as the default values for the 843 + * files beneath this directory. 844 + */ 794 845 ei->attr.uid = uid; 795 846 ei->attr.gid = gid; 796 - 797 - /* 798 - * When the "events" directory is created, it takes on the 799 - * permissions of its parent. But can be reset on remount. 800 - */ 801 - ei->attr.mode |= EVENTFS_SAVE_UID | EVENTFS_SAVE_GID; 802 847 803 848 INIT_LIST_HEAD(&ei->children); 804 849 INIT_LIST_HEAD(&ei->list);

+28 -18

fs/tracefs/inode.c

··· 373 373 374 374 rcu_read_lock(); 375 375 list_for_each_entry_rcu(ti, &tracefs_inodes, list) { 376 - if (update_uid) 376 + if (update_uid) { 377 377 ti->flags &= ~TRACEFS_UID_PERM_SET; 378 + ti->vfs_inode.i_uid = fsi->uid; 379 + } 378 380 379 - if (update_gid) 381 + if (update_gid) { 380 382 ti->flags &= ~TRACEFS_GID_PERM_SET; 383 + ti->vfs_inode.i_gid = fsi->gid; 384 + } 381 385 386 + /* 387 + * Note, the above ti->vfs_inode updates are 388 + * used in eventfs_remount() so they must come 389 + * before calling it. 390 + */ 382 391 if (ti->flags & TRACEFS_EVENT_INODE) 383 392 eventfs_remount(ti, update_uid, update_gid); 384 393 } ··· 426 417 return 0; 427 418 } 428 419 420 + static int tracefs_drop_inode(struct inode *inode) 421 + { 422 + struct tracefs_inode *ti = get_tracefs(inode); 423 + 424 + /* 425 + * This inode is being freed and cannot be used for 426 + * eventfs. Clear the flag so that it doesn't call into 427 + * eventfs during the remount flag updates. The eventfs_inode 428 + * gets freed after an RCU cycle, so the content will still 429 + * be safe if the iteration is going on now. 430 + */ 431 + ti->flags &= ~TRACEFS_EVENT_INODE; 432 + 433 + return 1; 434 + } 435 + 429 436 static const struct super_operations tracefs_super_operations = { 430 437 .alloc_inode = tracefs_alloc_inode, 431 438 .free_inode = tracefs_free_inode, 432 - .drop_inode = generic_delete_inode, 439 + .drop_inode = tracefs_drop_inode, 433 440 .statfs = simple_statfs, 434 441 .show_options = tracefs_show_options, 435 442 }; ··· 471 446 return !(ei && ei->is_freed); 472 447 } 473 448 474 - static void tracefs_d_iput(struct dentry *dentry, struct inode *inode) 475 - { 476 - struct tracefs_inode *ti = get_tracefs(inode); 477 - 478 - /* 479 - * This inode is being freed and cannot be used for 480 - * eventfs. Clear the flag so that it doesn't call into 481 - * eventfs during the remount flag updates. The eventfs_inode 482 - * gets freed after an RCU cycle, so the content will still 483 - * be safe if the iteration is going on now. 484 - */ 485 - ti->flags &= ~TRACEFS_EVENT_INODE; 486 - } 487 - 488 449 static const struct dentry_operations tracefs_dentry_operations = { 489 - .d_iput = tracefs_d_iput, 490 450 .d_revalidate = tracefs_d_revalidate, 491 451 .d_release = tracefs_d_release, 492 452 };

Configure Feed

Configure Feed