From patchwork Sat Nov 9 22:52:42 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13869723 X-Patchwork-Delegate: bpf@iogearbox.net Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1A1721A0AF7 for ; Sat, 9 Nov 2024 22:52:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.65 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731192770; cv=none; b=JXIEuVeP4iCUyrH7EmykzVl8KbfrZzZNRb6Eh1dR/a+os3XUSEZuZPcaFyitigZcYXJKYEg/APsB4uRkJeAIJwoPDmlkiWxVv4MyBYDZ3FmbyWK9AkSaVcOlRgJhaGnwZXUQswufXon/mYAPzusWtTqWKEZJZCydVom/YT4AfSg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731192770; c=relaxed/simple; bh=1L0j81axCpw/t/meBKHEpSaZvIALDOdm/DMdwxxSwJI=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=VU7mJaD+qUJYrjwRMFULKtdwkR1mcavGjNsN61pVbKGMbw/mHmJLUTVOtV+q2YhUd+gEHfIcWVmcM53vGctnPfy6mmpX80dQu0DDFcFCMGXbobuFSogDgcCCvXm4S7tcr4AqU4H1Rrxv3vRdxwujUYgP50dqn2OAHXn2TZPGPpI= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=PVqKBujb; arc=none smtp.client-ip=209.85.221.65 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="PVqKBujb" Received: by mail-wr1-f65.google.com with SMTP id ffacd0b85a97d-37d4c1b1455so2264156f8f.3 for ; Sat, 09 Nov 2024 14:52:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1731192766; x=1731797566; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=YPEhNKfVPNlOODIl/9V5tdXI+E5KE1pFRtY1J/An5tM=; b=PVqKBujb79rr5sCBAA7RS6TRfWgJP7QUiAP8CBVE9jd8q+8infPCIKHFS675BsGzOQ MnfDrALUA4ZSXjAeOYbezxTqgO45LIJG/CKrmSdvSZWyDHTAPLYS94FvPYw/12ICp3Bo qpARb0dUhlxjLwtKMvO0CZ+tl6aLHKa6gKGocFzglZ7yJTLmN/Hn9NGtIlzWyZn7tpjL WcXMvmt4l/bdU/C77D/RM9CTpjlido1VPeRDZ00uEj0T4CMXifDvl4NRGPGbps+0vPZw G2oDjK35mRTlHsu9BlxARyQc6PwdCza+ZTh03VgzVaQ2mu32FETo63+5gUSRT53g0pAJ obMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731192766; x=1731797566; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=YPEhNKfVPNlOODIl/9V5tdXI+E5KE1pFRtY1J/An5tM=; b=vyYFfnW/N/TJt5wxjau6jj6rIl9s+wKY+2del1vP7NAHyAVzmSy4w+edFERR7oIYTD Ce+mxM08qtA2i+yZ4CFFE11+D2ck6l93adNV2jv+KSOOcw5LvfVNG9tUzCsggny9YxW2 8aiD/Z7uRTC5BJhnktwWluw5HpHcR+dVKhtY9F+gXF/li4opL/CSftUrFCOWuSV4npHl iv1RD6po0QtxCoKYje1Zu14W50HxiCg/8IVK46AewZCxJRR3HpUWumeSMsBC5BP95gbF r/fHzs8uf3jzrh304FmEvRUIBJ0oNWfLg/oTs1xojKILMb88P5DjK+ShMoBuyKgVKKHH FNRQ== X-Gm-Message-State: AOJu0YzAsAF2QC4gOP31ys65YE5YCGvnlHGakWhNB0afZaMfo1K/zWdI oXq635AnVKPWpOC0zQqo3l/0COkOYynT0qpPQa6T8fQmDGHb0+7tWf/6zPPm4Zo= X-Google-Smtp-Source: AGHT+IGzxS6Mg5s4bKRca8XmZXvSW0603ZSsUkq8dxOml8jeuOtM7t7Gqxlyy3SxphkM9gJDkEnJjQ== X-Received: by 2002:a05:6000:2a82:b0:381:f595:fd0a with SMTP id ffacd0b85a97d-381f595fd36mr3839770f8f.16.1731192765759; Sat, 09 Nov 2024 14:52:45 -0800 (PST) Received: from localhost (fwdproxy-cln-035.fbsv.net. [2a03:2880:31ff:23::face:b00c]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-381ed970e23sm8780802f8f.18.2024.11.09.14.52.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 09 Nov 2024 14:52:45 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: kkd@meta.com, Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau , Eduard Zingerman , kernel-team@fb.com Subject: [PATCH bpf-next v5 1/2] bpf: Refactor active lock management Date: Sat, 9 Nov 2024 14:52:42 -0800 Message-ID: <20241109225243.2306756-2-memxor@gmail.com> X-Mailer: git-send-email 2.43.5 In-Reply-To: <20241109225243.2306756-1-memxor@gmail.com> References: <20241109225243.2306756-1-memxor@gmail.com> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=15061; h=from:subject; bh=1L0j81axCpw/t/meBKHEpSaZvIALDOdm/DMdwxxSwJI=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBnL+eapvfyDN/tPk8DnkRWJxpZ7cIunPxIZT7HCsSJ n0W57PmJAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCZy/nmgAKCRBM4MiGSL8RypT5D/ 98xqPJYwz9iNWH4o6bYPVnpP16JaOovWfRVgXn14TLrnSNmXUYflLypWgL5Y2AjW3FF9P+qwjzMqCk sEkS9qaNh2RXoq1U4dZ343mLuHnMQbzPC3bnQHkEk31+ccRHrNkBage4nWzY3jq81DX1sKuM9z8rSL fKM9dYcO6tFvalhvrGCbIsXKO8kDWENSvPr0LsfZutZm1S5DwHYbfVuBaRcNPNZBQ+GYPbn6wDYfpS pEpKawTv2A2R0BoTSPUNKgACV2Ac9JGkSgkZF/M/CA/w3lhl3e5m53wN3GZ1ZORWvPAcNZ90D95jjU LPhMNdESrX0dwi2M75NFFQToPdwwEUHaxwO9rFKCag77UIm/ovE3hduoM+unthY3aCW/RlaNHnoguu ucmpq3o+exAd6VHz6jrJyvCaaREP/ZlpQ+gkQnuAAaw6KCl5nTgxQSWRtiFJdlPWnF2QoUtJfVjo33 tYwBtjJlXwLa6IbWz/At4ojevGkrCbdSlk4CPf9/3roLs3CojwyRLRgO+EZ0xDZ/SwGl0skZFIT5hj mUpyBrU3qK9XE8J1evTwkaDVFuhWCEIGGJ1KW9taEXkd83dt+B76hGyKD/m1gi9uoO/txtfssPCqSm Q/5T8gNxrDYXVauip93z+YtRYQ3F9fGuLAZ7JgYFvKI6uczoyeaT1RityMyw== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA X-Patchwork-Delegate: bpf@iogearbox.net When bpf_spin_lock was introduced originally, there was deliberation on whether to use an array of lock IDs, but since bpf_spin_lock is limited to holding a single lock at any given time, we've been using a single ID to identify the held lock. In preparation for introducing spin locks that can be taken multiple times, introduce support for acquiring multiple lock IDs. For this purpose, reuse the acquired_refs array and store both lock and pointer references. We tag the entry with REF_TYPE_PTR or REF_TYPE_LOCK to disambiguate and find the relevant entry. The ptr field is used to track the map_ptr or btf (for bpf_obj_new allocations) to ensure locks can be matched with protected fields within the same "allocation", i.e. bpf_obj_new object or map value. The struct active_lock is changed to an int as the state is part of the acquired_refs array, and we only need active_lock as a cheap way of detecting lock presence. Signed-off-by: Kumar Kartikeya Dwivedi --- include/linux/bpf_verifier.h | 53 ++++++------- kernel/bpf/verifier.c | 149 ++++++++++++++++++++++++++--------- 2 files changed, 135 insertions(+), 67 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 4513372c5bc8..d84beed92ae4 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -48,22 +48,6 @@ enum bpf_reg_liveness { REG_LIVE_DONE = 0x8, /* liveness won't be updating this register anymore */ }; -/* For every reg representing a map value or allocated object pointer, - * we consider the tuple of (ptr, id) for them to be unique in verifier - * context and conside them to not alias each other for the purposes of - * tracking lock state. - */ -struct bpf_active_lock { - /* This can either be reg->map_ptr or reg->btf. If ptr is NULL, - * there's no active lock held, and other fields have no - * meaning. If non-NULL, it indicates that a lock is held and - * id member has the reg->id of the register which can be >= 0. - */ - void *ptr; - /* This will be reg->id */ - u32 id; -}; - #define ITER_PREFIX "bpf_iter_" enum bpf_iter_state { @@ -266,6 +250,13 @@ struct bpf_stack_state { }; struct bpf_reference_state { + /* Each reference object has a type. Ensure REF_TYPE_PTR is zero to + * default to pointer reference on zero initialization of a state. + */ + enum ref_state_type { + REF_TYPE_PTR = 0, + REF_TYPE_LOCK, + } type; /* Track each reference created with a unique id, even if the same * instruction creates the reference multiple times (eg, via CALL). */ @@ -274,17 +265,23 @@ struct bpf_reference_state { * is used purely to inform the user of a reference leak. */ int insn_idx; - /* There can be a case like: - * main (frame 0) - * cb (frame 1) - * func (frame 3) - * cb (frame 4) - * Hence for frame 4, if callback_ref just stored boolean, it would be - * impossible to distinguish nested callback refs. Hence store the - * frameno and compare that to callback_ref in check_reference_leak when - * exiting a callback function. - */ - int callback_ref; + union { + /* There can be a case like: + * main (frame 0) + * cb (frame 1) + * func (frame 3) + * cb (frame 4) + * Hence for frame 4, if callback_ref just stored boolean, it would be + * impossible to distinguish nested callback refs. Hence store the + * frameno and compare that to callback_ref in check_reference_leak when + * exiting a callback function. + */ + int callback_ref; + /* Use to keep track of the source object of a lock, to ensure + * it matches on unlock. + */ + void *ptr; + }; }; struct bpf_retval_range { @@ -332,6 +329,7 @@ struct bpf_func_state { /* The following fields should be last. See copy_func_state() */ int acquired_refs; + int active_locks; struct bpf_reference_state *refs; /* The state of the stack. Each element of the array describes BPF_REG_SIZE * (i.e. 8) bytes worth of stack memory. @@ -434,7 +432,6 @@ struct bpf_verifier_state { u32 insn_idx; u32 curframe; - struct bpf_active_lock active_lock; bool speculative; bool active_rcu_lock; u32 active_preempt_lock; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 132fc172961f..e3f06a4410d8 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1284,6 +1284,7 @@ static int copy_reference_state(struct bpf_func_state *dst, const struct bpf_fun if (!dst->refs) return -ENOMEM; + dst->active_locks = src->active_locks; dst->acquired_refs = src->acquired_refs; return 0; } @@ -1354,6 +1355,7 @@ static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx) if (err) return err; id = ++env->id_gen; + state->refs[new_ofs].type = REF_TYPE_PTR; state->refs[new_ofs].id = id; state->refs[new_ofs].insn_idx = insn_idx; state->refs[new_ofs].callback_ref = state->in_callback_fn ? state->frameno : 0; @@ -1361,6 +1363,24 @@ static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx) return id; } +static int acquire_lock_state(struct bpf_verifier_env *env, int insn_idx, enum ref_state_type type, + int id, void *ptr) +{ + struct bpf_func_state *state = cur_func(env); + int new_ofs = state->acquired_refs; + int err; + + err = resize_reference_state(state, state->acquired_refs + 1); + if (err) + return err; + state->refs[new_ofs].type = type; + state->refs[new_ofs].id = id; + state->refs[new_ofs].insn_idx = insn_idx; + state->refs[new_ofs].ptr = ptr; + + return 0; +} + /* release function corresponding to acquire_reference_state(). Idempotent. */ static int release_reference_state(struct bpf_func_state *state, int ptr_id) { @@ -1368,6 +1388,8 @@ static int release_reference_state(struct bpf_func_state *state, int ptr_id) last_idx = state->acquired_refs - 1; for (i = 0; i < state->acquired_refs; i++) { + if (state->refs[i].type != REF_TYPE_PTR) + continue; if (state->refs[i].id == ptr_id) { /* Cannot release caller references in callbacks */ if (state->in_callback_fn && state->refs[i].callback_ref != state->frameno) @@ -1383,6 +1405,44 @@ static int release_reference_state(struct bpf_func_state *state, int ptr_id) return -EINVAL; } +static int release_lock_state(struct bpf_func_state *state, int type, int id, void *ptr) +{ + int i, last_idx; + + last_idx = state->acquired_refs - 1; + for (i = 0; i < state->acquired_refs; i++) { + if (state->refs[i].type != type) + continue; + if (state->refs[i].id == id && state->refs[i].ptr == ptr) { + if (last_idx && i != last_idx) + memcpy(&state->refs[i], &state->refs[last_idx], + sizeof(*state->refs)); + memset(&state->refs[last_idx], 0, sizeof(*state->refs)); + state->acquired_refs--; + return 0; + } + } + return -EINVAL; +} + +static struct bpf_reference_state *find_lock_state(struct bpf_verifier_env *env, enum ref_state_type type, + int id, void *ptr) +{ + struct bpf_func_state *state = cur_func(env); + int i; + + for (i = 0; i < state->acquired_refs; i++) { + struct bpf_reference_state *s = &state->refs[i]; + + if (s->type == REF_TYPE_PTR || s->type != type) + continue; + + if (s->id == id && s->ptr == ptr) + return s; + } + return NULL; +} + static void free_func_state(struct bpf_func_state *state) { if (!state) @@ -1453,8 +1513,6 @@ static int copy_verifier_state(struct bpf_verifier_state *dst_state, dst_state->active_preempt_lock = src->active_preempt_lock; dst_state->in_sleepable = src->in_sleepable; dst_state->curframe = src->curframe; - dst_state->active_lock.ptr = src->active_lock.ptr; - dst_state->active_lock.id = src->active_lock.id; dst_state->branches = src->branches; dst_state->parent = src->parent; dst_state->first_insn_idx = src->first_insn_idx; @@ -5442,7 +5500,7 @@ static bool in_sleepable(struct bpf_verifier_env *env) static bool in_rcu_cs(struct bpf_verifier_env *env) { return env->cur_state->active_rcu_lock || - env->cur_state->active_lock.ptr || + cur_func(env)->active_locks || !in_sleepable(env); } @@ -7724,19 +7782,20 @@ static int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg * Since only one bpf_spin_lock is allowed the checks are simpler than * reg_is_refcounted() logic. The verifier needs to remember only * one spin_lock instead of array of acquired_refs. - * cur_state->active_lock remembers which map value element or allocated + * cur_func(env)->active_locks remembers which map value element or allocated * object got locked and clears it after bpf_spin_unlock. */ static int process_spin_lock(struct bpf_verifier_env *env, int regno, bool is_lock) { struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; - struct bpf_verifier_state *cur = env->cur_state; bool is_const = tnum_is_const(reg->var_off); + struct bpf_func_state *cur = cur_func(env); u64 val = reg->var_off.value; struct bpf_map *map = NULL; struct btf *btf = NULL; struct btf_record *rec; + int err; if (!is_const) { verbose(env, @@ -7768,16 +7827,27 @@ static int process_spin_lock(struct bpf_verifier_env *env, int regno, return -EINVAL; } if (is_lock) { - if (cur->active_lock.ptr) { + void *ptr; + + if (map) + ptr = map; + else + ptr = btf; + + if (cur->active_locks) { verbose(env, "Locking two bpf_spin_locks are not allowed\n"); return -EINVAL; } - if (map) - cur->active_lock.ptr = map; - else - cur->active_lock.ptr = btf; - cur->active_lock.id = reg->id; + err = acquire_lock_state(env, env->insn_idx, REF_TYPE_LOCK, reg->id, ptr); + if (err < 0) { + verbose(env, "Failed to acquire lock state\n"); + return err; + } + /* It is not safe to allow multiple bpf_spin_lock calls, so + * disallow them until this lock has been unlocked. + */ + cur->active_locks++; } else { void *ptr; @@ -7786,20 +7856,18 @@ static int process_spin_lock(struct bpf_verifier_env *env, int regno, else ptr = btf; - if (!cur->active_lock.ptr) { + if (!cur->active_locks) { verbose(env, "bpf_spin_unlock without taking a lock\n"); return -EINVAL; } - if (cur->active_lock.ptr != ptr || - cur->active_lock.id != reg->id) { + + if (release_lock_state(cur_func(env), REF_TYPE_LOCK, reg->id, ptr)) { verbose(env, "bpf_spin_unlock of different lock\n"); return -EINVAL; } invalidate_non_owning_refs(env); - - cur->active_lock.ptr = NULL; - cur->active_lock.id = 0; + cur->active_locks--; } return 0; } @@ -9861,7 +9929,7 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn, const char *sub_name = subprog_name(env, subprog); /* Only global subprogs cannot be called with a lock held. */ - if (env->cur_state->active_lock.ptr) { + if (cur_func(env)->active_locks) { verbose(env, "global function calls are not allowed while holding a lock,\n" "use static function instead\n"); return -EINVAL; @@ -10386,6 +10454,8 @@ static int check_reference_leak(struct bpf_verifier_env *env, bool exception_exi return 0; for (i = 0; i < state->acquired_refs; i++) { + if (state->refs[i].type != REF_TYPE_PTR) + continue; if (!exception_exit && state->in_callback_fn && state->refs[i].callback_ref != state->frameno) continue; verbose(env, "Unreleased reference id=%d alloc_insn=%d\n", @@ -10399,7 +10469,7 @@ static int check_resource_leak(struct bpf_verifier_env *env, bool exception_exit { int err; - if (check_lock && env->cur_state->active_lock.ptr) { + if (check_lock && cur_func(env)->active_locks) { verbose(env, "%s cannot be used inside bpf_spin_lock-ed region\n", prefix); return -EINVAL; } @@ -11620,10 +11690,9 @@ static int process_kf_arg_ptr_to_btf_id(struct bpf_verifier_env *env, static int ref_set_non_owning(struct bpf_verifier_env *env, struct bpf_reg_state *reg) { - struct bpf_verifier_state *state = env->cur_state; struct btf_record *rec = reg_btf_record(reg); - if (!state->active_lock.ptr) { + if (!cur_func(env)->active_locks) { verbose(env, "verifier internal error: ref_set_non_owning w/o active lock\n"); return -EFAULT; } @@ -11720,6 +11789,7 @@ static int ref_convert_owning_non_owning(struct bpf_verifier_env *env, u32 ref_o */ static int check_reg_allocation_locked(struct bpf_verifier_env *env, struct bpf_reg_state *reg) { + struct bpf_reference_state *s; void *ptr; u32 id; @@ -11736,10 +11806,10 @@ static int check_reg_allocation_locked(struct bpf_verifier_env *env, struct bpf_ } id = reg->id; - if (!env->cur_state->active_lock.ptr) + if (!cur_func(env)->active_locks) return -EINVAL; - if (env->cur_state->active_lock.ptr != ptr || - env->cur_state->active_lock.id != id) { + s = find_lock_state(env, REF_TYPE_LOCK, id, ptr); + if (!s) { verbose(env, "held lock and object are not in the same allocation\n"); return -EINVAL; } @@ -17635,8 +17705,22 @@ static bool refsafe(struct bpf_func_state *old, struct bpf_func_state *cur, return false; for (i = 0; i < old->acquired_refs; i++) { - if (!check_ids(old->refs[i].id, cur->refs[i].id, idmap)) + if (!check_ids(old->refs[i].id, cur->refs[i].id, idmap) || + old->refs[i].type != cur->refs[i].type) return false; + switch (old->refs[i].type) { + case REF_TYPE_PTR: + if (old->refs[i].callback_ref != cur->refs[i].callback_ref) + return false; + break; + case REF_TYPE_LOCK: + if (old->refs[i].ptr != cur->refs[i].ptr) + return false; + break; + default: + WARN_ONCE(1, "Unhandled enum type for reference state: %d\n", old->refs[i].type); + return false; + } } return true; @@ -17714,19 +17798,6 @@ static bool states_equal(struct bpf_verifier_env *env, if (old->speculative && !cur->speculative) return false; - if (old->active_lock.ptr != cur->active_lock.ptr) - return false; - - /* Old and cur active_lock's have to be either both present - * or both absent. - */ - if (!!old->active_lock.id != !!cur->active_lock.id) - return false; - - if (old->active_lock.id && - !check_ids(old->active_lock.id, cur->active_lock.id, &env->idmap_scratch)) - return false; - if (old->active_rcu_lock != cur->active_rcu_lock) return false; @@ -18625,7 +18696,7 @@ static int do_check(struct bpf_verifier_env *env) return -EINVAL; } - if (env->cur_state->active_lock.ptr) { + if (cur_func(env)->active_locks) { if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) || (insn->src_reg == BPF_PSEUDO_KFUNC_CALL && (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) { From patchwork Sat Nov 9 22:52:43 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13869724 X-Patchwork-Delegate: bpf@iogearbox.net Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 47EA4143725 for ; Sat, 9 Nov 2024 22:52:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.66 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731192771; cv=none; b=NPph3JN36mx7hi9If/WFCS4MinAxQCZ6qprL6NwqsmNua8RVXx/ueboegRBHgyoCw5FldBeN/TShOrqITs3PTXuso3XZLYGA9P/FWiViqPEAMIRGP32ebKEq1PLlIJuB0sPZwTcfiB38LKuFAeLVp6yGzbCK54zdcrKyZjCVch0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731192771; c=relaxed/simple; bh=M0IsXjn6dSMydpILK8oj7gvtxwjdTNysOp41kJknPhQ=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=lkpgfTrt6JlZBQ78+SMj4QAwsOD0pfiiiwQHrbqZfwLF6wavfuMK1YmjIQTigEFS0GVRbw810vkCE59K2FmExops23WOIyHxnwoiu5BjILzPbZVxSbYpJy2Wp1HJJSyQ9DtKmhrbWURnM9mlZygpWHjD7pCt4Aic4KMOgFUAaXk= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=i4HQ8ar4; arc=none smtp.client-ip=209.85.221.66 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="i4HQ8ar4" Received: by mail-wr1-f66.google.com with SMTP id ffacd0b85a97d-37d6ff1cbe1so2512138f8f.3 for ; Sat, 09 Nov 2024 14:52:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1731192767; x=1731797567; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=oNpVZcX6g4mDzqjLsJIXfg8I2iznqpWFa62kxrZlXaE=; b=i4HQ8ar4ky5PP97zVqbHqwTIgq4CnvJ6xCDEdSReW/5E83/Fl7Uf8ms5jw8J/j+QFI yAQevZjawx0xQtMCVY0GQbDQnmAytk2vBMqFfbxvvQl3dIhEpyTo34nM/odkoFRxaG6I pGxxBZFbfJH0RaaKsgdVqF+Hb+m2xGtYUiH/lxh/fz7s6eIw8YAF70Zt8mLZNuyf04Q2 Hc2hX2R6dOEvfL/sDmpGnyldZx0147dLgjh3mOocUTPGtOS08ZSUrq67HXio4iYrWk7e hOKMKAHJI8F4sEONR/cqxDrnxqOgI1zqniSQBMNs65GY8CQOs9PGAO2RLigMM3yuU2l3 Thwg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731192767; x=1731797567; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=oNpVZcX6g4mDzqjLsJIXfg8I2iznqpWFa62kxrZlXaE=; b=ibAhSATPri7OrBXT0j/gBZXRMPZH0fubfWxn/SBYDqWlWHfslM9UOnbyNId1hwI2P9 OKaNJB9wUn9QnX8wiu2VoVuLOszO8ufdEoCmjvzRPlCf9AGisJsTx8Kp/QPJ5Iku9I9l Xs94aXW2HfpTgp56fRMxVoHwYEmzoeKy6JBG6/oNgkLgTaE79JmzmLVDAHlm1PLWJdGi it897ECr9SLj6aqhn2UUX3tBhjZdOoqJ7rMwaWNeHOKd9+/3LvAH85ijB26tm6dhIvTT 83dD/7EKT1aMA0bECSGEdY1S/NIo5o2mO0iRRNBZuCHc3TyXN8SxMTM/P2yDcOX4O4Rc VQYw== X-Gm-Message-State: AOJu0YzGLyDjNTyCQRlKS3AGDXYPdST9ehnMdhLhl0rDcB0xapXN8+4a et7tZRmVLTs1oAaMqzeJgHqZsO/MLXqtaZ50vILm/cwJYhaOEhCP/0WII4DCKWU= X-Google-Smtp-Source: AGHT+IH6CoV7At04fYH3tVG+jBE3t/kx+43RCsw7tbbP6fHydvckca2WMQa1T9EzMF0A0p0/ujHSbA== X-Received: by 2002:a5d:5f48:0:b0:37d:498a:a237 with SMTP id ffacd0b85a97d-381f171ccc1mr6738650f8f.8.1731192767082; Sat, 09 Nov 2024 14:52:47 -0800 (PST) Received: from localhost (fwdproxy-cln-002.fbsv.net. [2a03:2880:31ff:2::face:b00c]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-381ed9ea93esm8714156f8f.66.2024.11.09.14.52.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 09 Nov 2024 14:52:46 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: kkd@meta.com, Eduard Zingerman , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau , kernel-team@fb.com Subject: [PATCH bpf-next v5 2/2] bpf: Drop special callback reference handling Date: Sat, 9 Nov 2024 14:52:43 -0800 Message-ID: <20241109225243.2306756-3-memxor@gmail.com> X-Mailer: git-send-email 2.43.5 In-Reply-To: <20241109225243.2306756-1-memxor@gmail.com> References: <20241109225243.2306756-1-memxor@gmail.com> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=5826; h=from:subject; bh=M0IsXjn6dSMydpILK8oj7gvtxwjdTNysOp41kJknPhQ=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBnL+ea/mpTqaPJ7mIm+HR/B1h/necjQU9RnltGo5Wj W8oDgdqJAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCZy/nmgAKCRBM4MiGSL8Ryv8gD/ 91G9fgBhN3NwZ5Ex8RH/6KEjv4zdbKcfEvqNDM6n+8EAbTaPx6N4NQNZtud65L4Mz5Nvj3OqNOZkQn 9HC/yNCPI/vdyMXIR9Ys+FhBg00+je2lFbrQ3BhClGJCWC3oEExEqp9yU7W/no8TVb/69cumiSUQCF 0RET68/fjO/0nPvvJCo9u03VTNRqyF0PWKingEAePUVNqII0spHBSHFFBYnmcMOF6vfPY83zByV32w G7CTti6FQU7b2q5C54aJUSSTonVcyxGUIXrrzWolMj5aKQPdYhlmlR8OD3BeZhnkVcpWOTIccsOVNz lfs+LKzr/lXB/mb0Dp6HVZzX1brnUY2lLj5wMXbk4QkvIJFMCaICnI/aHGzH7/qOUZhFk+sGwl1p5n mmoXtDt43RtBgZ7KwVeO34bh/qHRVuhsiWR95hiYccificOIBRgizpkrGlEwFu6/PgtElh80mo2vEc kqFV6KDOanu0tpBe5RPRS5zWDPvQOh88hQx0tLbfibv+MsDBXKRnDjSD1cHvpAuXhi4S0UXvUymTAN JjQ5orCp0xaSnKgq/BsjGrj+jJtHJiOTVN7f0E2p/TqoTI+3g1dNDKd0cp6vQH0Dw59HrFGdRkRK09 5CR8+eGYgyY5BVcwTfXVCjjP+0zlSsD82nutOe4Rm58jXK/O/SenznUw61lA== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA X-Patchwork-Delegate: bpf@iogearbox.net Logic to prevent callbacks from acquiring new references for the program (i.e. leaving acquired references), and releasing caller references (i.e. those acquired in parent frames) was introduced in commit 9d9d00ac29d0 ("bpf: Fix reference state management for synchronous callbacks"). This was necessary because back then, the verifier simulated each callback once (that could potentially be executed N times, where N can be zero). This meant that callbacks that left lingering resources or cleared caller resources could do it more than once, operating on undefined state or leaking memory. With the fixes to callback verification in commit ab5cfac139ab ("bpf: verify callbacks as if they are called unknown number of times"), all of this extra logic is no longer necessary. Hence, drop it as part of this commit. Cc: Eduard Zingerman Signed-off-by: Kumar Kartikeya Dwivedi --- include/linux/bpf_verifier.h | 21 +++------------- kernel/bpf/verifier.c | 25 ++++--------------- .../selftests/bpf/prog_tests/cb_refs.c | 4 +-- 3 files changed, 11 insertions(+), 39 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index d84beed92ae4..3a74033d49c4 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -265,23 +265,10 @@ struct bpf_reference_state { * is used purely to inform the user of a reference leak. */ int insn_idx; - union { - /* There can be a case like: - * main (frame 0) - * cb (frame 1) - * func (frame 3) - * cb (frame 4) - * Hence for frame 4, if callback_ref just stored boolean, it would be - * impossible to distinguish nested callback refs. Hence store the - * frameno and compare that to callback_ref in check_reference_leak when - * exiting a callback function. - */ - int callback_ref; - /* Use to keep track of the source object of a lock, to ensure - * it matches on unlock. - */ - void *ptr; - }; + /* Use to keep track of the source object of a lock, to ensure + * it matches on unlock. + */ + void *ptr; }; struct bpf_retval_range { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e3f06a4410d8..45cd620a4032 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1358,7 +1358,6 @@ static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx) state->refs[new_ofs].type = REF_TYPE_PTR; state->refs[new_ofs].id = id; state->refs[new_ofs].insn_idx = insn_idx; - state->refs[new_ofs].callback_ref = state->in_callback_fn ? state->frameno : 0; return id; } @@ -1391,9 +1390,6 @@ static int release_reference_state(struct bpf_func_state *state, int ptr_id) if (state->refs[i].type != REF_TYPE_PTR) continue; if (state->refs[i].id == ptr_id) { - /* Cannot release caller references in callbacks */ - if (state->in_callback_fn && state->refs[i].callback_ref != state->frameno) - return -EINVAL; if (last_idx && i != last_idx) memcpy(&state->refs[i], &state->refs[last_idx], sizeof(*state->refs)); @@ -10270,17 +10266,10 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) caller->regs[BPF_REG_0] = *r0; } - /* callback_fn frame should have released its own additions to parent's - * reference state at this point, or check_reference_leak would - * complain, hence it must be the same as the caller. There is no need - * to copy it back. - */ - if (!callee->in_callback_fn) { - /* Transfer references to the caller */ - err = copy_reference_state(caller, callee); - if (err) - return err; - } + /* Transfer references to the caller */ + err = copy_reference_state(caller, callee); + if (err) + return err; /* for callbacks like bpf_loop or bpf_for_each_map_elem go back to callsite, * there function call logic would reschedule callback visit. If iteration @@ -10450,14 +10439,12 @@ static int check_reference_leak(struct bpf_verifier_env *env, bool exception_exi bool refs_lingering = false; int i; - if (!exception_exit && state->frameno && !state->in_callback_fn) + if (!exception_exit && state->frameno) return 0; for (i = 0; i < state->acquired_refs; i++) { if (state->refs[i].type != REF_TYPE_PTR) continue; - if (!exception_exit && state->in_callback_fn && state->refs[i].callback_ref != state->frameno) - continue; verbose(env, "Unreleased reference id=%d alloc_insn=%d\n", state->refs[i].id, state->refs[i].insn_idx); refs_lingering = true; @@ -17710,8 +17697,6 @@ static bool refsafe(struct bpf_func_state *old, struct bpf_func_state *cur, return false; switch (old->refs[i].type) { case REF_TYPE_PTR: - if (old->refs[i].callback_ref != cur->refs[i].callback_ref) - return false; break; case REF_TYPE_LOCK: if (old->refs[i].ptr != cur->refs[i].ptr) diff --git a/tools/testing/selftests/bpf/prog_tests/cb_refs.c b/tools/testing/selftests/bpf/prog_tests/cb_refs.c index 3bff680de16c..c40df623a8f7 100644 --- a/tools/testing/selftests/bpf/prog_tests/cb_refs.c +++ b/tools/testing/selftests/bpf/prog_tests/cb_refs.c @@ -11,8 +11,8 @@ struct { const char *prog_name; const char *err_msg; } cb_refs_tests[] = { - { "underflow_prog", "reference has not been acquired before" }, - { "leak_prog", "Unreleased reference" }, + { "underflow_prog", "must point to scalar, or struct with scalar" }, + { "leak_prog", "Possibly NULL pointer passed to helper arg2" }, { "nested_cb", "Unreleased reference id=4 alloc_insn=2" }, /* alloc_insn=2{4,5} */ { "non_cb_transfer_ref", "Unreleased reference id=4 alloc_insn=1" }, /* alloc_insn=1{1,2} */ };