[Bug translator/31018] New: Map operations might get no lock protections due to "pushdown lock" bugs

["agentzh at gmail dot com" <sourceware-bugzilla@sourceware.org>]

https://sourceware.org/bugzilla/show_bug.cgi?id=31018

            Bug ID: 31018
           Summary: Map operations might get no lock protections due to
                    "pushdown lock" bugs
           Product: systemtap
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: translator
          Assignee: systemtap at sourceware dot org
          Reporter: agentzh at gmail dot com
  Target Milestone: ---

The stap translator's "pushdown lock" optimization is buggy in that it may not
emit locking code for all the code branches in the first statement needing a
lock, making subsequent statements actually needing a lock go without a lock.
Consider the following minimal example:

```
global stats;

probe process.function("main") {
    my_pid = pid();
    v = @var("argc");
    if (v > 3) {
        delete stats;
    }
    stats[my_pid] <<< v;  // this line has no lock held
}
```

The translator emits locking code inside the `if` statement's "then" branch but
when the `if`'s condition is not met, the subsequent `stats[my_id] <<< v`
statement would not get any locks held at all.

Such bugs lead to corruption in the unprotected map data structure when the
probe handler runs in parallel on different CPUs. We saw panic messages like
these:

```
[ 1274.534162] list_del corruption. next->prev should be ffffc90002180b80, but
was ffffc90002180be0
[ 1274.534850] kernel BUG at lib/list_debug.c:54!
[ 1274.506139] list_add double add: new=ffffc90002180fd0,
prev=ffffc90002180fd0, next=ffffc90000eb9020.
```

The backtrace looks like this:

```
[ 1274.513560] Call Trace:
[ 1274.513700]  _new_map_create+0x158/0x540
[stap_ac8469bf9b1b0dd44e159b94c8bba7f8_16730]
[ 1274.514133]  _stp_map_set_ii+0x122/0x1e0
[stap_ac8469bf9b1b0dd44e159b94c8bba7f8_16730]
[ 1274.514574]  probe_6385+0xe20/0x1bd0
[stap_ac8469bf9b1b0dd44e159b94c8bba7f8_16730]
[ 1274.514990]  ? probe_6394+0x1560/0x1560
[stap_ac8469bf9b1b0dd44e159b94c8bba7f8_16730]
[ 1274.515424]  ? _stp_ctl_write_cmd+0x16f0/0x16f0
[stap_ac8469bf9b1b0dd44e159b94c8bba7f8_16730]
[ 1274.515891]  enter_kprobe_probe+0x529/0xcc0
[stap_ac8469bf9b1b0dd44e159b94c8bba7f8_16730]
```

The guilty code snippet in the translate is here, in C++ function
c_unparser::visit_block():

```
      // if needed, find the lock insertion site; instruct it to lock
      if (pushdown_lock_p(s))
        for (unsigned i=0; i<s->statements.size(); i++)
          if (locks_needed_p (s->statements[i]))
            { pushdown_lock.insert(s->statements[i]); pushed_lock_down = true;
break; }
```

Here the first "if" statement in the sample stap script above would would hit
the "if (locks_needed_p (s->statements[i]))" condition in this C++ code
snippet, shortcuiting the surrounding C++ "for" loop, leaving the subsequent
C++ statements like "stats[my_pid] <<< v;" go without "pushdown_lock" marking.
So the subsequent statements would not emit any locking code at all.

The correct fix is to always emit locking code for statements that might "push
down" the locks, until seeing a simple statements what don't (like an
assignment statement using "<<<").

I'm preparing a patch to fix this problem.

-- 
You are receiving this mail because:
You are the assignee for the bug.