Message ID | 20180628164014.4925-3-jack@suse.cz (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@suse.cz> wrote: > When an inode is tagged with a tree, tag_chunk() checks whether there is > audit_tree_group mark attached to the inode and adds one if not. However > nothing protects another tag_chunk() to add the mark between we've > checked and try to add the fsnotify mark thus resulting in an error from > fsnotify_add_mark() and consequently an ENOSPC error from tag_chunk(). > > Fix the problem by holding mark_mutex over the whole check-insert code > sequence. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- Never dived into the audit_tree code, but this looks sane. Thanks, Amir.
Hi Jan, url: https://github.com/0day-ci/linux/commits/Jan-Kara/audit-Fix-various-races-when-tagging-and-untagging-mounts/20180629-043337 smatch warnings: kernel/audit_tree.c:484 tag_chunk() warn: inconsistent returns 'mutex:&audit_tree_group->mark_mutex'. Locked on: line 400 Unlocked on: line 411 # https://github.com/0day-ci/linux/commit/86c9c9a738e409c85891519c17d94043b7f434d5 git remote add linux-review https://github.com/0day-ci/linux git remote update linux-review git checkout 86c9c9a738e409c85891519c17d94043b7f434d5 vim +484 kernel/audit_tree.c 74c3cbe33 Al Viro 2007-07-22 386 74c3cbe33 Al Viro 2007-07-22 387 /* the first tagged inode becomes root of tree */ 74c3cbe33 Al Viro 2007-07-22 388 static int tag_chunk(struct inode *inode, struct audit_tree *tree) 74c3cbe33 Al Viro 2007-07-22 389 { e61ce8673 Eric Paris 2009-12-17 390 struct fsnotify_mark *old_entry, *chunk_entry; 74c3cbe33 Al Viro 2007-07-22 391 struct audit_tree *owner; 74c3cbe33 Al Viro 2007-07-22 392 struct audit_chunk *chunk, *old; 74c3cbe33 Al Viro 2007-07-22 393 struct node *p; 74c3cbe33 Al Viro 2007-07-22 394 int n; 74c3cbe33 Al Viro 2007-07-22 395 86c9c9a73 Jan Kara 2018-06-28 396 mutex_lock(&audit_tree_group->mark_mutex); b1362edfe Jan Kara 2016-12-21 397 old_entry = fsnotify_find_mark(&inode->i_fsnotify_marks, b1362edfe Jan Kara 2016-12-21 398 audit_tree_group); 28a3a7eb3 Eric Paris 2009-12-17 399 if (!old_entry) 74c3cbe33 Al Viro 2007-07-22 400 return create_chunk(inode, tree); ^^^^^^^^^^^^^^^^^^^^^^^^^ Should we drop the lock before this return? 74c3cbe33 Al Viro 2007-07-22 401 28a3a7eb3 Eric Paris 2009-12-17 402 old = container_of(old_entry, struct audit_chunk, mark); 74c3cbe33 Al Viro 2007-07-22 403 74c3cbe33 Al Viro 2007-07-22 404 /* are we already there? */ 74c3cbe33 Al Viro 2007-07-22 405 spin_lock(&hash_lock); 74c3cbe33 Al Viro 2007-07-22 406 for (n = 0; n < old->count; n++) { 74c3cbe33 Al Viro 2007-07-22 407 if (old->owners[n].owner == tree) { 74c3cbe33 Al Viro 2007-07-22 408 spin_unlock(&hash_lock); 86c9c9a73 Jan Kara 2018-06-28 409 mutex_unlock(&audit_tree_group->mark_mutex); 28a3a7eb3 Eric Paris 2009-12-17 410 fsnotify_put_mark(old_entry); 74c3cbe33 Al Viro 2007-07-22 411 return 0; 74c3cbe33 Al Viro 2007-07-22 412 } 74c3cbe33 Al Viro 2007-07-22 413 } 74c3cbe33 Al Viro 2007-07-22 414 spin_unlock(&hash_lock); 74c3cbe33 Al Viro 2007-07-22 415 74c3cbe33 Al Viro 2007-07-22 416 chunk = alloc_chunk(old->count + 1); b4c30aad3 Al Viro 2009-12-19 417 if (!chunk) { 86c9c9a73 Jan Kara 2018-06-28 418 mutex_unlock(&audit_tree_group->mark_mutex); 28a3a7eb3 Eric Paris 2009-12-17 419 fsnotify_put_mark(old_entry); 74c3cbe33 Al Viro 2007-07-22 420 return -ENOMEM; b4c30aad3 Al Viro 2009-12-19 421 } 74c3cbe33 Al Viro 2007-07-22 422 28a3a7eb3 Eric Paris 2009-12-17 423 chunk_entry = &chunk->mark; 28a3a7eb3 Eric Paris 2009-12-17 424 6b3f05d24 Jan Kara 2016-12-21 425 /* 6b3f05d24 Jan Kara 2016-12-21 426 * mark_mutex protects mark from getting detached and thus also from 36f10f55f Amir Goldstein 2018-06-23 427 * mark->connector->obj getting NULL. 6b3f05d24 Jan Kara 2016-12-21 428 */ 43471d15d Jan Kara 2017-04-03 429 if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { 28a3a7eb3 Eric Paris 2009-12-17 430 /* old_entry is being shot, lets just lie */ 86c9c9a73 Jan Kara 2018-06-28 431 mutex_unlock(&audit_tree_group->mark_mutex); 28a3a7eb3 Eric Paris 2009-12-17 432 fsnotify_put_mark(old_entry); 7b1293234 Jan Kara 2016-12-21 433 fsnotify_put_mark(&chunk->mark); 28a3a7eb3 Eric Paris 2009-12-17 434 return -ENOENT; 28a3a7eb3 Eric Paris 2009-12-17 435 } 28a3a7eb3 Eric Paris 2009-12-17 436 36f10f55f Amir Goldstein 2018-06-23 437 if (fsnotify_add_mark_locked(chunk_entry, old_entry->connector->obj, 36f10f55f Amir Goldstein 2018-06-23 438 FSNOTIFY_OBJ_TYPE_INODE, 1)) { 86c9c9a73 Jan Kara 2018-06-28 439 mutex_unlock(&audit_tree_group->mark_mutex); 0fe33aae0 Miklos Szeredi 2012-08-15 440 fsnotify_put_mark(chunk_entry); 28a3a7eb3 Eric Paris 2009-12-17 441 fsnotify_put_mark(old_entry); 74c3cbe33 Al Viro 2007-07-22 442 return -ENOSPC; 74c3cbe33 Al Viro 2007-07-22 443 } 28a3a7eb3 Eric Paris 2009-12-17 444 74c3cbe33 Al Viro 2007-07-22 445 spin_lock(&hash_lock); 74c3cbe33 Al Viro 2007-07-22 446 if (tree->goner) { 74c3cbe33 Al Viro 2007-07-22 447 spin_unlock(&hash_lock); 74c3cbe33 Al Viro 2007-07-22 448 chunk->dead = 1; 86c9c9a73 Jan Kara 2018-06-28 449 mutex_unlock(&audit_tree_group->mark_mutex); 28a3a7eb3 Eric Paris 2009-12-17 450 e2a29943e Lino Sanfilippo 2011-06-14 451 fsnotify_destroy_mark(chunk_entry, audit_tree_group); 28a3a7eb3 Eric Paris 2009-12-17 452 28a3a7eb3 Eric Paris 2009-12-17 453 fsnotify_put_mark(chunk_entry); 28a3a7eb3 Eric Paris 2009-12-17 454 fsnotify_put_mark(old_entry); 74c3cbe33 Al Viro 2007-07-22 455 return 0; 74c3cbe33 Al Viro 2007-07-22 456 } 74c3cbe33 Al Viro 2007-07-22 457 list_replace_init(&old->trees, &chunk->trees); 74c3cbe33 Al Viro 2007-07-22 458 for (n = 0, p = chunk->owners; n < old->count; n++, p++) { 74c3cbe33 Al Viro 2007-07-22 459 struct audit_tree *s = old->owners[n].owner; 74c3cbe33 Al Viro 2007-07-22 460 p->owner = s; 74c3cbe33 Al Viro 2007-07-22 461 p->index = old->owners[n].index; 74c3cbe33 Al Viro 2007-07-22 462 if (!s) /* result of fallback in untag */ 74c3cbe33 Al Viro 2007-07-22 463 continue; 74c3cbe33 Al Viro 2007-07-22 464 get_tree(s); 74c3cbe33 Al Viro 2007-07-22 465 list_replace_init(&old->owners[n].list, &p->list); 74c3cbe33 Al Viro 2007-07-22 466 } 74c3cbe33 Al Viro 2007-07-22 467 p->index = (chunk->count - 1) | (1U<<31); 74c3cbe33 Al Viro 2007-07-22 468 p->owner = tree; 74c3cbe33 Al Viro 2007-07-22 469 get_tree(tree); 74c3cbe33 Al Viro 2007-07-22 470 list_add(&p->list, &tree->chunks); 74c3cbe33 Al Viro 2007-07-22 471 list_replace_rcu(&old->hash, &chunk->hash); 74c3cbe33 Al Viro 2007-07-22 472 list_for_each_entry(owner, &chunk->trees, same_root) 74c3cbe33 Al Viro 2007-07-22 473 owner->root = chunk; 74c3cbe33 Al Viro 2007-07-22 474 old->dead = 1; 74c3cbe33 Al Viro 2007-07-22 475 if (!tree->root) { 74c3cbe33 Al Viro 2007-07-22 476 tree->root = chunk; 74c3cbe33 Al Viro 2007-07-22 477 list_add(&tree->same_root, &chunk->trees); 74c3cbe33 Al Viro 2007-07-22 478 } 74c3cbe33 Al Viro 2007-07-22 479 spin_unlock(&hash_lock); 86c9c9a73 Jan Kara 2018-06-28 480 mutex_unlock(&audit_tree_group->mark_mutex); e2a29943e Lino Sanfilippo 2011-06-14 481 fsnotify_destroy_mark(old_entry, audit_tree_group); b3e8692b4 Miklos Szeredi 2012-08-15 482 fsnotify_put_mark(chunk_entry); /* drop initial reference */ 28a3a7eb3 Eric Paris 2009-12-17 483 fsnotify_put_mark(old_entry); /* pair to fsnotify_find mark_entry */ 74c3cbe33 Al Viro 2007-07-22 @484 return 0; 74c3cbe33 Al Viro 2007-07-22 485 } 74c3cbe33 Al Viro 2007-07-22 486 :::::: The code at line 484 was first introduced by commit :::::: 74c3cbe33bc077ac1159cadfea608b501e100344 [PATCH] audit: watching subtrees :::::: TO: Al Viro <viro@zeniv.linux.org.uk> :::::: CC: Al Viro <viro@zeniv.linux.org.uk> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Dan! On Mon 02-07-18 09:05:49, Dan Carpenter wrote: > url: https://github.com/0day-ci/linux/commits/Jan-Kara/audit-Fix-various-races-when-tagging-and-untagging-mounts/20180629-043337 > > smatch warnings: > kernel/audit_tree.c:484 tag_chunk() warn: inconsistent returns 'mutex:&audit_tree_group->mark_mutex'. > Locked on: line 400 > Unlocked on: line 411 > > # https://github.com/0day-ci/linux/commit/86c9c9a738e409c85891519c17d94043b7f434d5 > git remote add linux-review https://github.com/0day-ci/linux > git remote update linux-review > git checkout 86c9c9a738e409c85891519c17d94043b7f434d5 > vim +484 kernel/audit_tree.c > > 74c3cbe33 Al Viro 2007-07-22 386 > 74c3cbe33 Al Viro 2007-07-22 387 /* the first tagged inode becomes root of tree */ > 74c3cbe33 Al Viro 2007-07-22 388 static int tag_chunk(struct inode *inode, struct audit_tree *tree) > 74c3cbe33 Al Viro 2007-07-22 389 { > e61ce8673 Eric Paris 2009-12-17 390 struct fsnotify_mark *old_entry, *chunk_entry; > 74c3cbe33 Al Viro 2007-07-22 391 struct audit_tree *owner; > 74c3cbe33 Al Viro 2007-07-22 392 struct audit_chunk *chunk, *old; > 74c3cbe33 Al Viro 2007-07-22 393 struct node *p; > 74c3cbe33 Al Viro 2007-07-22 394 int n; > 74c3cbe33 Al Viro 2007-07-22 395 > 86c9c9a73 Jan Kara 2018-06-28 396 mutex_lock(&audit_tree_group->mark_mutex); > b1362edfe Jan Kara 2016-12-21 397 old_entry = fsnotify_find_mark(&inode->i_fsnotify_marks, > b1362edfe Jan Kara 2016-12-21 398 audit_tree_group); > 28a3a7eb3 Eric Paris 2009-12-17 399 if (!old_entry) > 74c3cbe33 Al Viro 2007-07-22 400 return create_chunk(inode, tree); > ^^^^^^^^^^^^^^^^^^^^^^^^^ > Should we drop the lock before this return? No, because create_chunk() drops &audit_tree_group->mark_mutex in all the cases. It's a bit ugly to have a function entered with mutex held and release it but in this case it's somewhat difficult to avoid... Honza
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 1c82eb6674c4..de8d344d91b1 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -342,25 +342,29 @@ static void untag_chunk(struct node *p) spin_lock(&hash_lock); } +/* Call with group->mark_mutex held, releases it */ static int create_chunk(struct inode *inode, struct audit_tree *tree) { struct fsnotify_mark *entry; struct audit_chunk *chunk = alloc_chunk(1); - if (!chunk) + + if (!chunk) { + mutex_unlock(&audit_tree_group->mark_mutex); return -ENOMEM; + } entry = &chunk->mark; - if (fsnotify_add_inode_mark(entry, inode, 0)) { + if (fsnotify_add_inode_mark_locked(entry, inode, 0)) { + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(entry); return -ENOSPC; } - mutex_lock(&entry->group->mark_mutex); spin_lock(&hash_lock); if (tree->goner) { spin_unlock(&hash_lock); chunk->dead = 1; - mutex_unlock(&entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_destroy_mark(entry, audit_tree_group); fsnotify_put_mark(entry); return 0; @@ -375,7 +379,7 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) } insert_hash(chunk); spin_unlock(&hash_lock); - mutex_unlock(&entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(entry); /* drop initial reference */ return 0; } @@ -389,6 +393,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) struct node *p; int n; + mutex_lock(&audit_tree_group->mark_mutex); old_entry = fsnotify_find_mark(&inode->i_fsnotify_marks, audit_tree_group); if (!old_entry) @@ -401,6 +406,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) for (n = 0; n < old->count; n++) { if (old->owners[n].owner == tree) { spin_unlock(&hash_lock); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(old_entry); return 0; } @@ -409,20 +415,20 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) chunk = alloc_chunk(old->count + 1); if (!chunk) { + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(old_entry); return -ENOMEM; } chunk_entry = &chunk->mark; - mutex_lock(&old_entry->group->mark_mutex); /* * mark_mutex protects mark from getting detached and thus also from * mark->connector->obj getting NULL. */ if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { /* old_entry is being shot, lets just lie */ - mutex_unlock(&old_entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(old_entry); fsnotify_put_mark(&chunk->mark); return -ENOENT; @@ -430,7 +436,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) if (fsnotify_add_mark_locked(chunk_entry, old_entry->connector->obj, FSNOTIFY_OBJ_TYPE_INODE, 1)) { - mutex_unlock(&old_entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(chunk_entry); fsnotify_put_mark(old_entry); return -ENOSPC; @@ -440,7 +446,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) if (tree->goner) { spin_unlock(&hash_lock); chunk->dead = 1; - mutex_unlock(&old_entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_destroy_mark(chunk_entry, audit_tree_group); @@ -471,7 +477,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) list_add(&tree->same_root, &chunk->trees); } spin_unlock(&hash_lock); - mutex_unlock(&old_entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_destroy_mark(old_entry, audit_tree_group); fsnotify_put_mark(chunk_entry); /* drop initial reference */ fsnotify_put_mark(old_entry); /* pair to fsnotify_find mark_entry */
When an inode is tagged with a tree, tag_chunk() checks whether there is audit_tree_group mark attached to the inode and adds one if not. However nothing protects another tag_chunk() to add the mark between we've checked and try to add the fsnotify mark thus resulting in an error from fsnotify_add_mark() and consequently an ENOSPC error from tag_chunk(). Fix the problem by holding mark_mutex over the whole check-insert code sequence. Signed-off-by: Jan Kara <jack@suse.cz> --- kernel/audit_tree.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)