Skip to content
  1. Dec 07, 2022
  3. Dec 06, 2022
  5. Dec 05, 2022
  7. Dec 03, 2022
  9. Dec 02, 2022
    • Dave Marchevsky's avatar
      selftests/bpf: Validate multiple ref release_on_unlock logic · 78b037bd
      Dave Marchevsky authored 
      

Modify list_push_pop_multiple to alloc and insert nodes 2-at-a-time.
Without the previous patch's fix, this block of code:

  bpf_spin_lock(lock);
  bpf_list_push_front(head, &f[i]->node);
  bpf_list_push_front(head, &f[i + 1]->node);
  bpf_spin_unlock(lock);

would fail check_reference_leak check as release_on_unlock logic would miss
a ref that should've been released.

Signed-off-by: default avatarDave Marchevsky <davemarchevsky@fb.com>
cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Acked-by: default avatarYonghong Song <yhs@fb.com>
Link: https://lore.kernel.org/r/20221201183406.1203621-2-davemarchevsky@fb.com


Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      78b037bd
    • Dave Marchevsky's avatar
      bpf: Fix release_on_unlock release logic for multiple refs · 1f82dffc
      Dave Marchevsky authored 
      

Consider a verifier state with three acquired references, all with
release_on_unlock = true:

            idx  0 1 2
  state->refs = [2 4 6]

(with 2, 4, and 6 being the ref ids).

When bpf_spin_unlock is called, process_spin_lock will loop through all
acquired_refs and, for each ref, if it's release_on_unlock, calls
release_reference on it. That function in turn calls
release_reference_state, which removes the reference from state->refs by
swapping the reference state with the last reference state in
refs array and decrements acquired_refs count.

process_spin_lock's loop logic, which is essentially:

  for (i = 0; i < state->acquired_refs; i++) {
    if (!state->refs[i].release_on_unlock)
      continue;
    release_reference(state->refs[i].id);
  }

will fail to release release_on_unlock references which are swapped from
the end. Running this logic on our example demonstrates:

  state->refs = [2 4 6] (start of idx=0 iter)
    release state->refs[0] by swapping w/ state->refs[2]

  state->refs = [6 4]   (start of idx=1)
    release state->refs[1], no need to swap as it's the last idx

  state->refs = [6]     (start of idx=2, loop terminates)

ref_id 6 should have been removed but was skipped.

Fix this by looping from back-to-front, which results in refs that are
candidates for removal being swapped with refs which have already been
examined and kept.

If we modify our initial example such that ref 6 is replaced with ref 7,
which is _not_ release_on_unlock, and loop from the back, we'd see:

  state->refs = [2 4 7] (start of idx=2)

  state->refs = [2 4 7] (start of idx=1)

  state->refs = [2 7]   (start of idx=0, refs 7 and 4 swapped)

  state->refs = [7]     (after idx=0, 7 and 2 swapped, loop terminates)

Signed-off-by: default avatarDave Marchevsky <davemarchevsky@fb.com>
Acked-by: default avatarYonghong Song <yhs@fb.com>
cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Fixes: 534e86bc ("bpf: Add 'release on unlock' logic for bpf_list_push_{front,back}")
Link: https://lore.kernel.org/r/20221201183406.1203621-1-davemarchevsky@fb.com


Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
      1f82dffc
  11. Dec 01, 2022