[PATCH v2 bpf-next 0/9] bpf_refcount followups (part 1)

* [PATCH v2 bpf-next 0/9] bpf_refcount followups (part 1)
@ 2023-06-02  2:26 Dave Marchevsky
  2023-06-02  2:26 ` [PATCH v2 bpf-next 1/9] [DONOTAPPLY] Revert "bpf: Disable bpf_refcount_acquire kfunc calls until race conditions are fixed" Dave Marchevsky
                   ` (9 more replies)
  0 siblings, 10 replies; 11+ messages in thread
From: Dave Marchevsky @ 2023-06-02  2:26 UTC (permalink / raw)
  To: bpf
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Kernel Team, Dave Marchevsky

This series is the first of two (or more) followups to address issues in the
bpf_refcount shared ownership implementation discovered by Kumar.
Specifically, this series addresses the "bpf_refcount_acquire on non-owning ref
in another tree" scenario described in [0], and does _not_ address issues
raised in [1]. Further followups will address the other issues.

The series can be applied without re-enabling bpf_refcount_acquire calls, which
were disabled in commit 7deca5eae833 ("bpf: Disable bpf_refcount_acquire kfunc
calls until race conditions are fixed") until all issues are addressed. Some
extra patches are included so that BPF CI tests will exercise test changes in
the series.

Patch contents:
  * Patch 1 reverts earlier disabling of bpf_refcount_acquire calls
    * Selftest added later in the series need to call bpf_refcount_acquire
    * This patch should not be applied and is included to allow CI to run the
      newly-added test and exercise test changes in patch 6
  * Patches 2 and 3 fix other bugs introduced in bpf_refcount series which were
    discovered while reproducing the main issue this series addresses
  * Patch 4 fixes the bpf_refcount_acquire issue by making it fallible for
    non-owning references
  * Patch 5 allows KF_DESTRUCTIVE kfuncs to be called when spinlock is held
    * This patch, and all patches further in the series, should not be
    applied
  * Patch 6 introduces some destructive bpf_testmod kfuncs which the selftest
    added later in the series needs
  * Patch 7 adds a selftest which uses the kfuncs introduced in patch 5 to
    replicate the exact scenario raised by Kumar
  * Patch 8 disables the test added in patch 7
    * This is so the series (aside from DONOTAPPLY patches) can be applied
      without re-enabling bpf_refcount_acquire yet.
  * Patch 9 reverts patch 8 so that CI can run the newly-added test

The first and last patches in the series are included to allow the CI to run
newly-added tests and should not be applied. First patch reverts earlier
disabling of bpf_refcount_acquire calls as the test reproducing
"bpf_refcount_acquire on non-owning ref in another tree" scenario obviously
needs to be able to call bpf_refcount_acquire.

While reproducing the scenario Kumar described in [0], which should cause a
refcount use-after-free, two unrelated bugs were found and are fixed by this
series.

  [0]: https://lore.kernel.org/bpf/atfviesiidev4hu53hzravmtlau3wdodm2vqs7rd7tnwft34e3@xktodqeqevir/
  [1]: https://lore.kernel.org/bpf/d7hyspcow5wtjcmw4fugdgyp3fwhljwuscp3xyut5qnwivyeru@ysdq543otzv2/

Changelog:

v1 -> v2: lore.kernel.org/bpf/20230504053338.1778690-1-davemarchevsky@fb.com

Patch #s used below refer to the patch's position in v1 unless otherwise
specified.

  * General
    * Rebase onto latest bpf-next

    * All bpf_testmod-related changes (destructive kfuncs, etc) have been
      marked [DONOTAPPLY] in response to Alexei's comments in v1 and followup
      conversations offline. They're still included as part of the series so
      that CI can exercise the fixed functionality.
      * v1's Patch 5 - "selftests/bpf: Add unsafe lock/unlock and refcount_read
        kfuncs to bpf_testmod" - was moved after the patches which are meant to
        be applied to make it more obvious that it shouldn't be applied.

    * 4d585f48ee6b ("bpf: Remove anonymous union in bpf_kfunc_call_arg_meta")
      was shipped separately from this series in response to Alexei's comments
      about the anonymous union in kfunc_call_arg_meta. That patch removes the
      anonymous union and its members (arg_obj_drop, etc) in favor of the
      simpler approach suggested by Alexei in v1. This series'
      kfunc_call_arg_meta changes are modified to follow the new pattern.

Dave Marchevsky (9):
  [DONOTAPPLY] Revert "bpf: Disable bpf_refcount_acquire kfunc calls
    until race conditions are fixed"
  bpf: Set kptr_struct_meta for node param to list and rbtree insert
    funcs
  bpf: Fix __bpf_{list,rbtree}_add's beginning-of-node calculation
  bpf: Make bpf_refcount_acquire fallible for non-owning refs
  [DONOTAPPLY] bpf: Allow KF_DESTRUCTIVE-flagged kfuncs to be called
    under spinlock
  [DONOTAPPLY] selftests/bpf: Add unsafe lock/unlock and refcount_read
    kfuncs to bpf_testmod
  [DONOTAPPLY] selftests/bpf: Add test exercising bpf_refcount_acquire
    race condition
  [DONOTAPPLY] selftests/bpf: Disable newly-added refcounted_kptr_races
    test
  [DONOTAPPLY] Revert "selftests/bpf: Disable newly-added
    refcounted_kptr_races test"

 kernel/bpf/helpers.c                          |  12 +-
 kernel/bpf/verifier.c                         |  55 ++++--
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  61 +++++++
 .../bpf/prog_tests/refcounted_kptr.c          | 106 +++++++++++-
 .../selftests/bpf/progs/refcounted_kptr.c     | 160 ++++++++++++++++++
 .../bpf/progs/refcounted_kptr_fail.c          |   4 +-
 6 files changed, 379 insertions(+), 19 deletions(-)

-- 
2.34.1

^ permalink raw reply	[flat|nested] 11+ messages in thread