Commit 71434f2fcba5c22d6e0d51878ba8e241a5dea5fb

Authored by Oleg Nesterov
1 parent 4710f05fd1

uprobes: Fix the racy uprobe->flags manipulation

Multiple threads can manipulate uprobe->flags, this is obviously
unsafe. For example mmap can set UPROBE_COPY_INSN while register
tries to set UPROBE_RUN_HANDLER, the latter can also race with
can_skip_sstep() which clears UPROBE_SKIP_SSTEP.

Change this code to use bitops.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Showing 1 changed file with 14 additions and 14 deletions Side-by-side Diff

kernel/events/uprobes.c
... ... @@ -79,11 +79,11 @@
79 79 static atomic_t uprobe_events = ATOMIC_INIT(0);
80 80  
81 81 /* Have a copy of original instruction */
82   -#define UPROBE_COPY_INSN 0x1
  82 +#define UPROBE_COPY_INSN 0
83 83 /* Dont run handlers when first register/ last unregister in progress*/
84   -#define UPROBE_RUN_HANDLER 0x2
  84 +#define UPROBE_RUN_HANDLER 1
85 85 /* Can skip singlestep */
86   -#define UPROBE_SKIP_SSTEP 0x4
  86 +#define UPROBE_SKIP_SSTEP 2
87 87  
88 88 struct uprobe {
89 89 struct rb_node rb_node; /* node in the rb tree */
... ... @@ -94,7 +94,7 @@
94 94 struct uprobe_consumer *consumers;
95 95 struct inode *inode; /* Also hold a ref to inode */
96 96 loff_t offset;
97   - int flags;
  97 + unsigned long flags;
98 98 struct arch_uprobe arch;
99 99 };
100 100  
... ... @@ -423,7 +423,7 @@
423 423 spin_unlock(&uprobes_treelock);
424 424  
425 425 /* For now assume that the instruction need not be single-stepped */
426   - uprobe->flags |= UPROBE_SKIP_SSTEP;
  426 + __set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
427 427  
428 428 return u;
429 429 }
... ... @@ -466,7 +466,7 @@
466 466 {
467 467 struct uprobe_consumer *uc;
468 468  
469   - if (!(uprobe->flags & UPROBE_RUN_HANDLER))
  469 + if (!test_bit(UPROBE_RUN_HANDLER, &uprobe->flags))
470 470 return;
471 471  
472 472 down_read(&uprobe->consumer_rwsem);
473 473  
... ... @@ -577,11 +577,11 @@
577 577 {
578 578 int ret = 0;
579 579  
580   - if (uprobe->flags & UPROBE_COPY_INSN)
  580 + if (test_bit(UPROBE_COPY_INSN, &uprobe->flags))
581 581 return ret;
582 582  
583 583 mutex_lock(&uprobe->copy_mutex);
584   - if (uprobe->flags & UPROBE_COPY_INSN)
  584 + if (test_bit(UPROBE_COPY_INSN, &uprobe->flags))
585 585 goto out;
586 586  
587 587 ret = copy_insn(uprobe, file);
... ... @@ -601,7 +601,7 @@
601 601 UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
602 602  
603 603 smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
604   - uprobe->flags |= UPROBE_COPY_INSN;
  604 + set_bit(UPROBE_COPY_INSN, &uprobe->flags);
605 605  
606 606 out:
607 607 mutex_unlock(&uprobe->copy_mutex);
... ... @@ -852,7 +852,7 @@
852 852 uprobe->consumers = NULL;
853 853 __uprobe_unregister(uprobe);
854 854 } else {
855   - uprobe->flags |= UPROBE_RUN_HANDLER;
  855 + set_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
856 856 }
857 857 }
858 858  
... ... @@ -885,7 +885,7 @@
885 885 if (consumer_del(uprobe, uc)) {
886 886 if (!uprobe->consumers) {
887 887 __uprobe_unregister(uprobe);
888   - uprobe->flags &= ~UPROBE_RUN_HANDLER;
  888 + clear_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
889 889 }
890 890 }
891 891  
892 892  
... ... @@ -1346,10 +1346,10 @@
1346 1346 */
1347 1347 static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
1348 1348 {
1349   - if (uprobe->flags & UPROBE_SKIP_SSTEP) {
  1349 + if (test_bit(UPROBE_SKIP_SSTEP, &uprobe->flags)) {
1350 1350 if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
1351 1351 return true;
1352   - uprobe->flags &= ~UPROBE_SKIP_SSTEP;
  1352 + clear_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
1353 1353 }
1354 1354 return false;
1355 1355 }
... ... @@ -1473,7 +1473,7 @@
1473 1473 * new and not-yet-analyzed uprobe at the same address, restart.
1474 1474 */
1475 1475 smp_rmb(); /* pairs with wmb() in install_breakpoint() */
1476   - if (unlikely(!(uprobe->flags & UPROBE_COPY_INSN)))
  1476 + if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
1477 1477 goto restart;
1478 1478  
1479 1479 utask = current->utask;