* [PATCH bpf-next 0/2] Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release
@ 2019-03-01 7:03 Martin KaFai Lau
2019-03-01 7:03 ` [PATCH bpf-next 1/2] bpf: " Martin KaFai Lau
2019-03-01 7:03 ` [PATCH bpf-next 2/2] bpf: Test ref release issue in bpf_tcp_sock and bpf_sk_fullsock Martin KaFai Lau
0 siblings, 2 replies; 6+ messages in thread
From: Martin KaFai Lau @ 2019-03-01 7:03 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
This set addresses issue about accessing invalid
ptr returned from bpf_tcp_sock() and bpf_sk_fullsock()
after bpf_sk_release().
Martin KaFai Lau (2):
bpf: Fix bpf_tcp_sock and bpf_sk_fullsock issue related to
bpf_sk_release
bpf: Test ref release issue in bpf_tcp_sock and bpf_sk_fullsock.
include/linux/bpf_verifier.h | 9 ++
kernel/bpf/verifier.c | 89 ++++++++++++-------
.../selftests/bpf/verifier/ref_tracking.c | 73 +++++++++++++++
3 files changed, 137 insertions(+), 34 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 1/2] bpf: Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release
2019-03-01 7:03 [PATCH bpf-next 0/2] Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release Martin KaFai Lau
@ 2019-03-01 7:03 ` Martin KaFai Lau
2019-03-01 15:58 ` Lorenz Bauer
2019-03-01 7:03 ` [PATCH bpf-next 2/2] bpf: Test ref release issue in bpf_tcp_sock and bpf_sk_fullsock Martin KaFai Lau
1 sibling, 1 reply; 6+ messages in thread
From: Martin KaFai Lau @ 2019-03-01 7:03 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Lorenz Bauer
Lorenz Bauer [thanks!] reported that a ptr returned by bpf_tcp_sock(sk)
can still be accessed after bpf_sk_release(sk).
Both bpf_tcp_sock() and bpf_sk_fullsock() have the same issue.
This patch addresses them together.
A simple reproducer looks like this:
sk = bpf_sk_lookup_tcp();
/* if (!sk) ... */
tp = bpf_tcp_sock(sk);
/* if (!tp) ... */
bpf_sk_release(sk);
snd_cwnd = tp->snd_cwnd; /* oops! The verifier does not complain. */
The problem is the verifier did not scrub the register's states of
the tcp_sock ptr (tp) after bpf_sk_release(sk).
[ Note that when calling bpf_tcp_sock(sk), the sk is not always
refcount-acquired. e.g. bpf_tcp_sock(skb->sk). The verifier works
fine for this case. ]
Currently, the verifier does not track if a helper's return ptr (in REG_0)
is "carry"-ing one of its argument's refcount status. To carry this info,
the reg1->id needs to be stored in reg0. The reg0->id has already
been used for NULL checking purpose. Hence, a new "refcount_id"
is needed in "struct bpf_reg_state".
With refcount_id, when bpf_sk_release(sk) is called, the verifier can scrub
all reg states which has a refcount_id match. It is done with the changes
in release_reg_references().
When acquiring and releasing a refcount, the reg->id is still used.
Hence, we cannot do "bpf_sk_release(tp)" in the above reproducer
example.
Misc change notes:
- With the new refcount_id, reg_is_refcounted() test can now be
done with "reg->refcount_id && reg->id == reg->refcount_id" instead of
testing the ptr type.
The type_is_refcounted() and type_is_refcounted_or_null()
are no longer needed, so removed.
- An anonymous struct is added to bpf_call_arg_meta to store
the reg->id and reg->refcount_id of the arg. Otherwise,
they will be unavailable after check_helper_call()
has cleared all CALLER_SAVED_REGS.
- The check_func_arg() can only allow one refcount-ed arg. It is
guaranteed by check_refcount_ok() which ensures at most one arg can be
refcount-ed. Hence, it is a verifier internal error if >1 refcount arg
found in check_func_arg().
- The check_func_arg() also complains if a "is_acquire_function(func_id)"
helper is having a refcount-ed arg. No func_id is doing this now
and should have been rejected earlier, so it is treated as
verifier internal error also.
- In check_func_arg(), the "!reg->id" check is removed under
the ARG_PTR_TO_SOCKET case. It is because a PTR_TO_SOCKET
can be obtained from bpf_sk_fullsock() and it does not
take a refcount. The verifier will still complain during
release_reference() but it does not treat it as a
verifier internal error anymore.
- In release_reference(), release_reference_state() is called
first to ensure a match on "reg->id" can be found before
scrubbing the reg states with release_reg_references().
Fixes: 655a51e536c0 ("bpf: Add struct bpf_tcp_sock and BPF_FUNC_tcp_sock")
Cc: Lorenz Bauer <lmb@cloudflare.com>
Reported-by: Lorenz Bauer <lmb@cloudflare.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
include/linux/bpf_verifier.h | 9 ++++
kernel/bpf/verifier.c | 89 ++++++++++++++++++++++--------------
2 files changed, 64 insertions(+), 34 deletions(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 69f7a3449eda..b7698d0534cb 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -66,6 +66,15 @@ struct bpf_reg_state {
* same reference to the socket, to determine proper reference freeing.
*/
u32 id;
+ /* For PTR_TO_SOCKET and PTR_TO_TCP_SOCK, this ptr may not actually
+ * hold a refcount of a socket but instead it is a ptr
+ * returned from a helper which is based on its refcount-ed
+ * ptr argument (e.g. bpf_tcp_sock()).
+ * "refcount_id" stores which refcount-ed argument it
+ * originally derived from. When this original argument's
+ * refcount is released, this ptr will also be invalidated.
+ */
+ u32 refcount_id;
/* For scalar types (SCALAR_VALUE), this represents our knowledge of
* the actual value.
* For pointer types, this represents the variable part of the offset
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1b9496c41383..682b7a6f0d2a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -212,7 +212,10 @@ struct bpf_call_arg_meta {
int access_size;
s64 msize_smax_value;
u64 msize_umax_value;
- int ptr_id;
+ struct {
+ u32 id;
+ u32 refcount_id;
+ } refcount_reg;
int func_id;
};
@@ -346,19 +349,9 @@ static bool reg_type_may_be_null(enum bpf_reg_type type)
type == PTR_TO_TCP_SOCK_OR_NULL;
}
-static bool type_is_refcounted(enum bpf_reg_type type)
-{
- return type == PTR_TO_SOCKET;
-}
-
-static bool type_is_refcounted_or_null(enum bpf_reg_type type)
-{
- return type == PTR_TO_SOCKET || type == PTR_TO_SOCKET_OR_NULL;
-}
-
static bool reg_is_refcounted(const struct bpf_reg_state *reg)
{
- return type_is_refcounted(reg->type);
+ return reg->refcount_id && reg->id == reg->refcount_id;
}
static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg)
@@ -367,14 +360,10 @@ static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg)
map_value_has_spin_lock(reg->map_ptr);
}
-static bool reg_is_refcounted_or_null(const struct bpf_reg_state *reg)
-{
- return type_is_refcounted_or_null(reg->type);
-}
-
static bool arg_type_is_refcounted(enum bpf_arg_type type)
{
- return type == ARG_PTR_TO_SOCKET;
+ return type == ARG_PTR_TO_SOCKET ||
+ type == ARG_PTR_TO_SOCK_COMMON;
}
/* Determine whether the function releases some resources allocated by another
@@ -392,6 +381,12 @@ static bool is_acquire_function(enum bpf_func_id func_id)
func_id == BPF_FUNC_sk_lookup_udp;
}
+static bool is_refcount_carrying_function(enum bpf_func_id func_id)
+{
+ return func_id == BPF_FUNC_tcp_sock ||
+ func_id == BPF_FUNC_sk_fullsock;
+}
+
/* string representation of 'enum bpf_reg_type' */
static const char * const reg_type_str[] = {
[NOT_INIT] = "?",
@@ -465,7 +460,8 @@ static void print_verifier_state(struct bpf_verifier_env *env,
if (t == PTR_TO_STACK)
verbose(env, ",call_%d", func(env, reg)->callsite);
} else {
- verbose(env, "(id=%d", reg->id);
+ verbose(env, "(id=%d refcount_id=%d", reg->id,
+ reg->refcount_id);
if (t != SCALAR_VALUE)
verbose(env, ",off=%d", reg->off);
if (type_is_pkt_pointer(t))
@@ -2418,12 +2414,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
expected_type = PTR_TO_SOCKET;
if (type != expected_type)
goto err_type;
- if (meta->ptr_id || !reg->id) {
- verbose(env, "verifier internal error: mismatched references meta=%d, reg=%d\n",
- meta->ptr_id, reg->id);
- return -EFAULT;
- }
- meta->ptr_id = reg->id;
} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
if (meta->func_id == BPF_FUNC_spin_lock) {
if (process_spin_lock(env, regno, true))
@@ -2532,6 +2522,26 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
zero_size_allowed, meta);
}
+ if (reg->refcount_id) {
+ if (meta->refcount_reg.refcount_id) {
+ verbose(env, "verifier internal error: more than one arg with refcount_id R%d %u %u\n",
+ regno, reg->refcount_id,
+ meta->refcount_reg.refcount_id);
+ return -EFAULT;
+ }
+
+ if (is_acquire_function(meta->func_id)) {
+ verbose(env,
+ "verifier internal error: func %s#%d taking an already refcount-ed arg R%d\n",
+ func_id_name(meta->func_id), meta->func_id,
+ regno);
+ return -EFAULT;
+ }
+
+ meta->refcount_reg.id = reg->id;
+ meta->refcount_reg.refcount_id = reg->refcount_id;
+ }
+
return err;
err_type:
verbose(env, "R%d type=%s expected=%s\n", regno,
@@ -2799,19 +2809,20 @@ static void clear_all_pkt_pointers(struct bpf_verifier_env *env)
}
static void release_reg_references(struct bpf_verifier_env *env,
- struct bpf_func_state *state, int id)
+ struct bpf_func_state *state,
+ int refcount_id)
{
struct bpf_reg_state *regs = state->regs, *reg;
int i;
for (i = 0; i < MAX_BPF_REG; i++)
- if (regs[i].id == id)
+ if (regs[i].refcount_id == refcount_id)
mark_reg_unknown(env, regs, i);
bpf_for_each_spilled_reg(i, state, reg) {
if (!reg)
continue;
- if (reg_is_refcounted(reg) && reg->id == id)
+ if (reg->refcount_id == refcount_id)
__mark_reg_unknown(reg);
}
}
@@ -2819,16 +2830,21 @@ static void release_reg_references(struct bpf_verifier_env *env,
/* The pointer with the specified id has released its reference to kernel
* resources. Identify all copies of the same pointer and clear the reference.
*/
-static int release_reference(struct bpf_verifier_env *env,
- struct bpf_call_arg_meta *meta)
+static int release_reference(struct bpf_verifier_env *env, u32 id,
+ u32 refcount_id)
{
struct bpf_verifier_state *vstate = env->cur_state;
+ int err;
int i;
+ err = release_reference_state(cur_func(env), id);
+ if (err)
+ return err;
+
for (i = 0; i <= vstate->curframe; i++)
- release_reg_references(env, vstate->frame[i], meta->ptr_id);
+ release_reg_references(env, vstate->frame[i], refcount_id);
- return release_reference_state(cur_func(env), meta->ptr_id);
+ return 0;
}
static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
@@ -3093,7 +3109,8 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
return err;
}
} else if (is_release_function(func_id)) {
- err = release_reference(env, &meta);
+ err = release_reference(env, meta.refcount_reg.id,
+ meta.refcount_reg.refcount_id);
if (err) {
verbose(env, "func %s#%d reference has not been acquired before\n",
func_id_name(func_id), func_id);
@@ -3156,6 +3173,7 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
return id;
/* For release_reference() */
regs[BPF_REG_0].id = id;
+ regs[BPF_REG_0].refcount_id = id;
} else {
/* For mark_ptr_or_null_reg() */
regs[BPF_REG_0].id = ++env->id_gen;
@@ -3170,6 +3188,9 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
return -EINVAL;
}
+ if (is_refcount_carrying_function(func_id))
+ regs[BPF_REG_0].refcount_id = meta.refcount_reg.refcount_id;
+
do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
err = check_map_func_compatibility(env, meta.map_ptr, func_id);
@@ -4687,7 +4708,7 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno,
u32 id = regs[regno].id;
int i, j;
- if (reg_is_refcounted_or_null(®s[regno]) && is_null)
+ if (reg_is_refcounted(®s[regno]) && is_null)
release_reference_state(state, id);
for (i = 0; i < MAX_BPF_REG; i++)
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] bpf: Test ref release issue in bpf_tcp_sock and bpf_sk_fullsock
2019-03-01 7:03 [PATCH bpf-next 0/2] Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release Martin KaFai Lau
2019-03-01 7:03 ` [PATCH bpf-next 1/2] bpf: " Martin KaFai Lau
@ 2019-03-01 7:03 ` Martin KaFai Lau
1 sibling, 0 replies; 6+ messages in thread
From: Martin KaFai Lau @ 2019-03-01 7:03 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Lorenz Bauer
Adding verifier tests to ensure the ptr returned from
bpf_tcp_sock() and bpf_sk_fullsock() cannot be accessed
after bpf_sk_release() is called. It is derived from a
reproducer test from Lorenz Bauer.
Cc: Lorenz Bauer <lmb@cloudflare.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
.../selftests/bpf/verifier/ref_tracking.c | 73 +++++++++++++++++++
1 file changed, 73 insertions(+)
diff --git a/tools/testing/selftests/bpf/verifier/ref_tracking.c b/tools/testing/selftests/bpf/verifier/ref_tracking.c
index 3ed3593bd8b6..9695f8e9b58b 100644
--- a/tools/testing/selftests/bpf/verifier/ref_tracking.c
+++ b/tools/testing/selftests/bpf/verifier/ref_tracking.c
@@ -605,3 +605,76 @@
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
.result = ACCEPT,
},
+{
+ "reference tracking: use ptr from bpf_tcp_sock() after release",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_tcp_sock),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 3),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct bpf_tcp_sock, snd_cwnd)),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = REJECT,
+ .errstr = "invalid mem access",
+},
+{
+ "reference tracking: use ptr from bpf_sk_fullsock() after release",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_fullsock),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 3),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct bpf_sock, type)),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = REJECT,
+ .errstr = "invalid mem access",
+},
+{
+ "reference tracking: use ptr from bpf_sk_fullsock(tp) after release",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_tcp_sock),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 3),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_fullsock),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_6, 0, 1),
+ BPF_EXIT_INSN(),
+ BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_6, offsetof(struct bpf_sock, type)),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = REJECT,
+ .errstr = "invalid mem access",
+},
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release
2019-03-01 7:03 ` [PATCH bpf-next 1/2] bpf: " Martin KaFai Lau
@ 2019-03-01 15:58 ` Lorenz Bauer
2019-03-01 17:07 ` Martin Lau
0 siblings, 1 reply; 6+ messages in thread
From: Lorenz Bauer @ 2019-03-01 15:58 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team
On Fri, 1 Mar 2019 at 07:03, Martin KaFai Lau <kafai@fb.com> wrote:
>
> Lorenz Bauer [thanks!] reported that a ptr returned by bpf_tcp_sock(sk)
> can still be accessed after bpf_sk_release(sk).
> Both bpf_tcp_sock() and bpf_sk_fullsock() have the same issue.
> This patch addresses them together.
>
> A simple reproducer looks like this:
>
> sk = bpf_sk_lookup_tcp();
> /* if (!sk) ... */
> tp = bpf_tcp_sock(sk);
> /* if (!tp) ... */
> bpf_sk_release(sk);
> snd_cwnd = tp->snd_cwnd; /* oops! The verifier does not complain. */
>
> The problem is the verifier did not scrub the register's states of
> the tcp_sock ptr (tp) after bpf_sk_release(sk).
>
> [ Note that when calling bpf_tcp_sock(sk), the sk is not always
> refcount-acquired. e.g. bpf_tcp_sock(skb->sk). The verifier works
> fine for this case. ]
>
> Currently, the verifier does not track if a helper's return ptr (in REG_0)
> is "carry"-ing one of its argument's refcount status. To carry this info,
> the reg1->id needs to be stored in reg0. The reg0->id has already
> been used for NULL checking purpose. Hence, a new "refcount_id"
> is needed in "struct bpf_reg_state".
>
> With refcount_id, when bpf_sk_release(sk) is called, the verifier can scrub
> all reg states which has a refcount_id match. It is done with the changes
> in release_reg_references().
>
> When acquiring and releasing a refcount, the reg->id is still used.
> Hence, we cannot do "bpf_sk_release(tp)" in the above reproducer
> example.
>
> Misc change notes:
> - With the new refcount_id, reg_is_refcounted() test can now be
> done with "reg->refcount_id && reg->id == reg->refcount_id" instead of
> testing the ptr type.
>
> The type_is_refcounted() and type_is_refcounted_or_null()
> are no longer needed, so removed.
>
> - An anonymous struct is added to bpf_call_arg_meta to store
> the reg->id and reg->refcount_id of the arg. Otherwise,
> they will be unavailable after check_helper_call()
> has cleared all CALLER_SAVED_REGS.
>
> - The check_func_arg() can only allow one refcount-ed arg. It is
> guaranteed by check_refcount_ok() which ensures at most one arg can be
> refcount-ed. Hence, it is a verifier internal error if >1 refcount arg
> found in check_func_arg().
>
> - The check_func_arg() also complains if a "is_acquire_function(func_id)"
> helper is having a refcount-ed arg. No func_id is doing this now
> and should have been rejected earlier, so it is treated as
> verifier internal error also.
>
> - In check_func_arg(), the "!reg->id" check is removed under
> the ARG_PTR_TO_SOCKET case. It is because a PTR_TO_SOCKET
> can be obtained from bpf_sk_fullsock() and it does not
> take a refcount. The verifier will still complain during
> release_reference() but it does not treat it as a
> verifier internal error anymore.
>
> - In release_reference(), release_reference_state() is called
> first to ensure a match on "reg->id" can be found before
> scrubbing the reg states with release_reg_references().
>
> Fixes: 655a51e536c0 ("bpf: Add struct bpf_tcp_sock and BPF_FUNC_tcp_sock")
> Cc: Lorenz Bauer <lmb@cloudflare.com>
> Reported-by: Lorenz Bauer <lmb@cloudflare.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
> include/linux/bpf_verifier.h | 9 ++++
> kernel/bpf/verifier.c | 89 ++++++++++++++++++++++--------------
> 2 files changed, 64 insertions(+), 34 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 69f7a3449eda..b7698d0534cb 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -66,6 +66,15 @@ struct bpf_reg_state {
> * same reference to the socket, to determine proper reference freeing.
> */
> u32 id;
> + /* For PTR_TO_SOCKET and PTR_TO_TCP_SOCK, this ptr may not actually
> + * hold a refcount of a socket but instead it is a ptr
> + * returned from a helper which is based on its refcount-ed
> + * ptr argument (e.g. bpf_tcp_sock()).
> + * "refcount_id" stores which refcount-ed argument it
> + * originally derived from. When this original argument's
> + * refcount is released, this ptr will also be invalidated.
> + */
> + u32 refcount_id;
> /* For scalar types (SCALAR_VALUE), this represents our knowledge of
> * the actual value.
> * For pointer types, this represents the variable part of the offset
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1b9496c41383..682b7a6f0d2a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -212,7 +212,10 @@ struct bpf_call_arg_meta {
> int access_size;
> s64 msize_smax_value;
> u64 msize_umax_value;
> - int ptr_id;
> + struct {
> + u32 id;
> + u32 refcount_id;
> + } refcount_reg;
> int func_id;
> };
>
> @@ -346,19 +349,9 @@ static bool reg_type_may_be_null(enum bpf_reg_type type)
> type == PTR_TO_TCP_SOCK_OR_NULL;
> }
>
> -static bool type_is_refcounted(enum bpf_reg_type type)
> -{
> - return type == PTR_TO_SOCKET;
> -}
> -
> -static bool type_is_refcounted_or_null(enum bpf_reg_type type)
> -{
> - return type == PTR_TO_SOCKET || type == PTR_TO_SOCKET_OR_NULL;
> -}
> -
> static bool reg_is_refcounted(const struct bpf_reg_state *reg)
> {
> - return type_is_refcounted(reg->type);
> + return reg->refcount_id && reg->id == reg->refcount_id;
> }
>
> static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg)
> @@ -367,14 +360,10 @@ static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg)
> map_value_has_spin_lock(reg->map_ptr);
> }
>
> -static bool reg_is_refcounted_or_null(const struct bpf_reg_state *reg)
> -{
> - return type_is_refcounted_or_null(reg->type);
> -}
> -
> static bool arg_type_is_refcounted(enum bpf_arg_type type)
> {
> - return type == ARG_PTR_TO_SOCKET;
> + return type == ARG_PTR_TO_SOCKET ||
> + type == ARG_PTR_TO_SOCK_COMMON;
> }
>
> /* Determine whether the function releases some resources allocated by another
> @@ -392,6 +381,12 @@ static bool is_acquire_function(enum bpf_func_id func_id)
> func_id == BPF_FUNC_sk_lookup_udp;
> }
>
> +static bool is_refcount_carrying_function(enum bpf_func_id func_id)
> +{
> + return func_id == BPF_FUNC_tcp_sock ||
> + func_id == BPF_FUNC_sk_fullsock;
> +}
> +
> /* string representation of 'enum bpf_reg_type' */
> static const char * const reg_type_str[] = {
> [NOT_INIT] = "?",
> @@ -465,7 +460,8 @@ static void print_verifier_state(struct bpf_verifier_env *env,
> if (t == PTR_TO_STACK)
> verbose(env, ",call_%d", func(env, reg)->callsite);
> } else {
> - verbose(env, "(id=%d", reg->id);
> + verbose(env, "(id=%d refcount_id=%d", reg->id,
> + reg->refcount_id);
> if (t != SCALAR_VALUE)
> verbose(env, ",off=%d", reg->off);
> if (type_is_pkt_pointer(t))
> @@ -2418,12 +2414,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
> expected_type = PTR_TO_SOCKET;
> if (type != expected_type)
> goto err_type;
> - if (meta->ptr_id || !reg->id) {
> - verbose(env, "verifier internal error: mismatched references meta=%d, reg=%d\n",
> - meta->ptr_id, reg->id);
> - return -EFAULT;
> - }
> - meta->ptr_id = reg->id;
> } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
> if (meta->func_id == BPF_FUNC_spin_lock) {
> if (process_spin_lock(env, regno, true))
> @@ -2532,6 +2522,26 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
> zero_size_allowed, meta);
> }
>
> + if (reg->refcount_id) {
> + if (meta->refcount_reg.refcount_id) {
> + verbose(env, "verifier internal error: more than one arg with refcount_id R%d %u %u\n",
> + regno, reg->refcount_id,
> + meta->refcount_reg.refcount_id);
> + return -EFAULT;
> + }
> +
> + if (is_acquire_function(meta->func_id)) {
> + verbose(env,
> + "verifier internal error: func %s#%d taking an already refcount-ed arg R%d\n",
> + func_id_name(meta->func_id), meta->func_id,
> + regno);
> + return -EFAULT;
> + }
> +
> + meta->refcount_reg.id = reg->id;
> + meta->refcount_reg.refcount_id = reg->refcount_id;
> + }
> +
> return err;
> err_type:
> verbose(env, "R%d type=%s expected=%s\n", regno,
> @@ -2799,19 +2809,20 @@ static void clear_all_pkt_pointers(struct bpf_verifier_env *env)
> }
>
> static void release_reg_references(struct bpf_verifier_env *env,
> - struct bpf_func_state *state, int id)
> + struct bpf_func_state *state,
> + int refcount_id)
Could this just take id? release_reference could then also only take
id. The way I see it, release_reg_references should clear all
registers and stack spills where either id == id, or reference_id ==
id.
In the current code that is given because id == reference_id if
release_reference_state succeeds, but I find that harder to
understand.
> {
> struct bpf_reg_state *regs = state->regs, *reg;
> int i;
>
> for (i = 0; i < MAX_BPF_REG; i++)
> - if (regs[i].id == id)
> + if (regs[i].refcount_id == refcount_id)
> mark_reg_unknown(env, regs, i);
>
> bpf_for_each_spilled_reg(i, state, reg) {
> if (!reg)
> continue;
> - if (reg_is_refcounted(reg) && reg->id == id)
> + if (reg->refcount_id == refcount_id)
> __mark_reg_unknown(reg);
> }
> }
> @@ -2819,16 +2830,21 @@ static void release_reg_references(struct bpf_verifier_env *env,
> /* The pointer with the specified id has released its reference to kernel
> * resources. Identify all copies of the same pointer and clear the reference.
> */
> -static int release_reference(struct bpf_verifier_env *env,
> - struct bpf_call_arg_meta *meta)
> +static int release_reference(struct bpf_verifier_env *env, u32 id,
> + u32 refcount_id)
> {
> struct bpf_verifier_state *vstate = env->cur_state;
> + int err;
> int i;
>
> + err = release_reference_state(cur_func(env), id);
> + if (err)
> + return err;
> +
> for (i = 0; i <= vstate->curframe; i++)
> - release_reg_references(env, vstate->frame[i], meta->ptr_id);
> + release_reg_references(env, vstate->frame[i], refcount_id);
>
> - return release_reference_state(cur_func(env), meta->ptr_id);
> + return 0;
> }
>
> static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> @@ -3093,7 +3109,8 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> return err;
> }
> } else if (is_release_function(func_id)) {
> - err = release_reference(env, &meta);
> + err = release_reference(env, meta.refcount_reg.id,
> + meta.refcount_reg.refcount_id);
> if (err) {
> verbose(env, "func %s#%d reference has not been acquired before\n",
> func_id_name(func_id), func_id);
> @@ -3156,6 +3173,7 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> return id;
> /* For release_reference() */
> regs[BPF_REG_0].id = id;
> + regs[BPF_REG_0].refcount_id = id;
> } else {
> /* For mark_ptr_or_null_reg() */
> regs[BPF_REG_0].id = ++env->id_gen;
> @@ -3170,6 +3188,9 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> return -EINVAL;
> }
>
> + if (is_refcount_carrying_function(func_id))
> + regs[BPF_REG_0].refcount_id = meta.refcount_reg.refcount_id;
> +
> do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
>
> err = check_map_func_compatibility(env, meta.map_ptr, func_id);
> @@ -4687,7 +4708,7 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno,
> u32 id = regs[regno].id;
> int i, j;
>
> - if (reg_is_refcounted_or_null(®s[regno]) && is_null)
> + if (reg_is_refcounted(®s[regno]) && is_null)
> release_reference_state(state, id);
Since reg_is_refcounted() is true, release_reference_state should
always succeed. Check the error code?
>
> for (i = 0; i < MAX_BPF_REG; i++)
> --
> 2.17.1
>
How about resetting reg->refcount_id in mark_ptr_or_null_reg as well?
(Aside: where does the
state pruning referenced by the comment in that function take place?)
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release
2019-03-01 15:58 ` Lorenz Bauer
@ 2019-03-01 17:07 ` Martin Lau
2019-03-02 15:35 ` Martin Lau
0 siblings, 1 reply; 6+ messages in thread
From: Martin Lau @ 2019-03-01 17:07 UTC (permalink / raw)
To: Lorenz Bauer; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Fri, Mar 01, 2019 at 03:58:52PM +0000, Lorenz Bauer wrote:
> On Fri, 1 Mar 2019 at 07:03, Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > Lorenz Bauer [thanks!] reported that a ptr returned by bpf_tcp_sock(sk)
> > can still be accessed after bpf_sk_release(sk).
> > Both bpf_tcp_sock() and bpf_sk_fullsock() have the same issue.
> > This patch addresses them together.
> >
> > A simple reproducer looks like this:
> >
> > sk = bpf_sk_lookup_tcp();
> > /* if (!sk) ... */
> > tp = bpf_tcp_sock(sk);
> > /* if (!tp) ... */
> > bpf_sk_release(sk);
> > snd_cwnd = tp->snd_cwnd; /* oops! The verifier does not complain. */
> >
> > The problem is the verifier did not scrub the register's states of
> > the tcp_sock ptr (tp) after bpf_sk_release(sk).
> >
> > [ Note that when calling bpf_tcp_sock(sk), the sk is not always
> > refcount-acquired. e.g. bpf_tcp_sock(skb->sk). The verifier works
> > fine for this case. ]
> >
> > Currently, the verifier does not track if a helper's return ptr (in REG_0)
> > is "carry"-ing one of its argument's refcount status. To carry this info,
> > the reg1->id needs to be stored in reg0. The reg0->id has already
> > been used for NULL checking purpose. Hence, a new "refcount_id"
> > is needed in "struct bpf_reg_state".
> >
> > With refcount_id, when bpf_sk_release(sk) is called, the verifier can scrub
> > all reg states which has a refcount_id match. It is done with the changes
> > in release_reg_references().
> >
> > When acquiring and releasing a refcount, the reg->id is still used.
> > Hence, we cannot do "bpf_sk_release(tp)" in the above reproducer
> > example.
> >
> > Misc change notes:
> > - With the new refcount_id, reg_is_refcounted() test can now be
> > done with "reg->refcount_id && reg->id == reg->refcount_id" instead of
> > testing the ptr type.
> >
> > The type_is_refcounted() and type_is_refcounted_or_null()
> > are no longer needed, so removed.
> >
> > - An anonymous struct is added to bpf_call_arg_meta to store
> > the reg->id and reg->refcount_id of the arg. Otherwise,
> > they will be unavailable after check_helper_call()
> > has cleared all CALLER_SAVED_REGS.
> >
> > - The check_func_arg() can only allow one refcount-ed arg. It is
> > guaranteed by check_refcount_ok() which ensures at most one arg can be
> > refcount-ed. Hence, it is a verifier internal error if >1 refcount arg
> > found in check_func_arg().
> >
> > - The check_func_arg() also complains if a "is_acquire_function(func_id)"
> > helper is having a refcount-ed arg. No func_id is doing this now
> > and should have been rejected earlier, so it is treated as
> > verifier internal error also.
> >
> > - In check_func_arg(), the "!reg->id" check is removed under
> > the ARG_PTR_TO_SOCKET case. It is because a PTR_TO_SOCKET
> > can be obtained from bpf_sk_fullsock() and it does not
> > take a refcount. The verifier will still complain during
> > release_reference() but it does not treat it as a
> > verifier internal error anymore.
> >
> > - In release_reference(), release_reference_state() is called
> > first to ensure a match on "reg->id" can be found before
> > scrubbing the reg states with release_reg_references().
> >
> > Fixes: 655a51e536c0 ("bpf: Add struct bpf_tcp_sock and BPF_FUNC_tcp_sock")
> > Cc: Lorenz Bauer <lmb@cloudflare.com>
> > Reported-by: Lorenz Bauer <lmb@cloudflare.com>
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
> > include/linux/bpf_verifier.h | 9 ++++
> > kernel/bpf/verifier.c | 89 ++++++++++++++++++++++--------------
> > 2 files changed, 64 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 69f7a3449eda..b7698d0534cb 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -66,6 +66,15 @@ struct bpf_reg_state {
> > * same reference to the socket, to determine proper reference freeing.
> > */
> > u32 id;
> > + /* For PTR_TO_SOCKET and PTR_TO_TCP_SOCK, this ptr may not actually
> > + * hold a refcount of a socket but instead it is a ptr
> > + * returned from a helper which is based on its refcount-ed
> > + * ptr argument (e.g. bpf_tcp_sock()).
> > + * "refcount_id" stores which refcount-ed argument it
> > + * originally derived from. When this original argument's
> > + * refcount is released, this ptr will also be invalidated.
> > + */
> > + u32 refcount_id;
> > /* For scalar types (SCALAR_VALUE), this represents our knowledge of
> > * the actual value.
> > * For pointer types, this represents the variable part of the offset
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 1b9496c41383..682b7a6f0d2a 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -212,7 +212,10 @@ struct bpf_call_arg_meta {
> > int access_size;
> > s64 msize_smax_value;
> > u64 msize_umax_value;
> > - int ptr_id;
> > + struct {
> > + u32 id;
> > + u32 refcount_id;
> > + } refcount_reg;
> > int func_id;
> > };
> >
> > @@ -346,19 +349,9 @@ static bool reg_type_may_be_null(enum bpf_reg_type type)
> > type == PTR_TO_TCP_SOCK_OR_NULL;
> > }
> >
> > -static bool type_is_refcounted(enum bpf_reg_type type)
> > -{
> > - return type == PTR_TO_SOCKET;
> > -}
> > -
> > -static bool type_is_refcounted_or_null(enum bpf_reg_type type)
> > -{
> > - return type == PTR_TO_SOCKET || type == PTR_TO_SOCKET_OR_NULL;
> > -}
> > -
> > static bool reg_is_refcounted(const struct bpf_reg_state *reg)
> > {
> > - return type_is_refcounted(reg->type);
> > + return reg->refcount_id && reg->id == reg->refcount_id;
> > }
> >
> > static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg)
> > @@ -367,14 +360,10 @@ static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg)
> > map_value_has_spin_lock(reg->map_ptr);
> > }
> >
> > -static bool reg_is_refcounted_or_null(const struct bpf_reg_state *reg)
> > -{
> > - return type_is_refcounted_or_null(reg->type);
> > -}
> > -
> > static bool arg_type_is_refcounted(enum bpf_arg_type type)
> > {
> > - return type == ARG_PTR_TO_SOCKET;
> > + return type == ARG_PTR_TO_SOCKET ||
> > + type == ARG_PTR_TO_SOCK_COMMON;
> > }
> >
> > /* Determine whether the function releases some resources allocated by another
> > @@ -392,6 +381,12 @@ static bool is_acquire_function(enum bpf_func_id func_id)
> > func_id == BPF_FUNC_sk_lookup_udp;
> > }
> >
> > +static bool is_refcount_carrying_function(enum bpf_func_id func_id)
> > +{
> > + return func_id == BPF_FUNC_tcp_sock ||
> > + func_id == BPF_FUNC_sk_fullsock;
> > +}
> > +
> > /* string representation of 'enum bpf_reg_type' */
> > static const char * const reg_type_str[] = {
> > [NOT_INIT] = "?",
> > @@ -465,7 +460,8 @@ static void print_verifier_state(struct bpf_verifier_env *env,
> > if (t == PTR_TO_STACK)
> > verbose(env, ",call_%d", func(env, reg)->callsite);
> > } else {
> > - verbose(env, "(id=%d", reg->id);
> > + verbose(env, "(id=%d refcount_id=%d", reg->id,
> > + reg->refcount_id);
> > if (t != SCALAR_VALUE)
> > verbose(env, ",off=%d", reg->off);
> > if (type_is_pkt_pointer(t))
> > @@ -2418,12 +2414,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
> > expected_type = PTR_TO_SOCKET;
> > if (type != expected_type)
> > goto err_type;
> > - if (meta->ptr_id || !reg->id) {
> > - verbose(env, "verifier internal error: mismatched references meta=%d, reg=%d\n",
> > - meta->ptr_id, reg->id);
> > - return -EFAULT;
> > - }
> > - meta->ptr_id = reg->id;
> > } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
> > if (meta->func_id == BPF_FUNC_spin_lock) {
> > if (process_spin_lock(env, regno, true))
> > @@ -2532,6 +2522,26 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
> > zero_size_allowed, meta);
> > }
> >
> > + if (reg->refcount_id) {
> > + if (meta->refcount_reg.refcount_id) {
> > + verbose(env, "verifier internal error: more than one arg with refcount_id R%d %u %u\n",
> > + regno, reg->refcount_id,
> > + meta->refcount_reg.refcount_id);
> > + return -EFAULT;
> > + }
> > +
> > + if (is_acquire_function(meta->func_id)) {
> > + verbose(env,
> > + "verifier internal error: func %s#%d taking an already refcount-ed arg R%d\n",
> > + func_id_name(meta->func_id), meta->func_id,
> > + regno);
> > + return -EFAULT;
> > + }
> > +
> > + meta->refcount_reg.id = reg->id;
> > + meta->refcount_reg.refcount_id = reg->refcount_id;
> > + }
> > +
> > return err;
> > err_type:
> > verbose(env, "R%d type=%s expected=%s\n", regno,
> > @@ -2799,19 +2809,20 @@ static void clear_all_pkt_pointers(struct bpf_verifier_env *env)
> > }
> >
> > static void release_reg_references(struct bpf_verifier_env *env,
> > - struct bpf_func_state *state, int id)
> > + struct bpf_func_state *state,
> > + int refcount_id)
>
> Could this just take id? release_reference could then also only take
> id. The way I see it, release_reg_references should clear all
> registers and stack spills where either id == id, or reference_id ==
> id.
> In the current code that is given because id == reference_id if
> release_reference_state succeeds, but I find that harder to
> understand.
Agree. only id is needed.
>
> > {
> > struct bpf_reg_state *regs = state->regs, *reg;
> > int i;
> >
> > for (i = 0; i < MAX_BPF_REG; i++)
> > - if (regs[i].id == id)
> > + if (regs[i].refcount_id == refcount_id)
> > mark_reg_unknown(env, regs, i);
> >
> > bpf_for_each_spilled_reg(i, state, reg) {
> > if (!reg)
> > continue;
> > - if (reg_is_refcounted(reg) && reg->id == id)
> > + if (reg->refcount_id == refcount_id)
> > __mark_reg_unknown(reg);
> > }
> > }
> > @@ -2819,16 +2830,21 @@ static void release_reg_references(struct bpf_verifier_env *env,
> > /* The pointer with the specified id has released its reference to kernel
> > * resources. Identify all copies of the same pointer and clear the reference.
> > */
> > -static int release_reference(struct bpf_verifier_env *env,
> > - struct bpf_call_arg_meta *meta)
> > +static int release_reference(struct bpf_verifier_env *env, u32 id,
> > + u32 refcount_id)
> > {
> > struct bpf_verifier_state *vstate = env->cur_state;
> > + int err;
> > int i;
> >
> > + err = release_reference_state(cur_func(env), id);
> > + if (err)
> > + return err;
> > +
> > for (i = 0; i <= vstate->curframe; i++)
> > - release_reg_references(env, vstate->frame[i], meta->ptr_id);
> > + release_reg_references(env, vstate->frame[i], refcount_id);
> >
> > - return release_reference_state(cur_func(env), meta->ptr_id);
> > + return 0;
> > }
> >
> > static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> > @@ -3093,7 +3109,8 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> > return err;
> > }
> > } else if (is_release_function(func_id)) {
> > - err = release_reference(env, &meta);
> > + err = release_reference(env, meta.refcount_reg.id,
> > + meta.refcount_reg.refcount_id);
> > if (err) {
> > verbose(env, "func %s#%d reference has not been acquired before\n",
> > func_id_name(func_id), func_id);
> > @@ -3156,6 +3173,7 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> > return id;
> > /* For release_reference() */
> > regs[BPF_REG_0].id = id;
> > + regs[BPF_REG_0].refcount_id = id;
> > } else {
> > /* For mark_ptr_or_null_reg() */
> > regs[BPF_REG_0].id = ++env->id_gen;
> > @@ -3170,6 +3188,9 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> > return -EINVAL;
> > }
> >
> > + if (is_refcount_carrying_function(func_id))
> > + regs[BPF_REG_0].refcount_id = meta.refcount_reg.refcount_id;
> > +
> > do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
> >
> > err = check_map_func_compatibility(env, meta.map_ptr, func_id);
> > @@ -4687,7 +4708,7 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno,
> > u32 id = regs[regno].id;
> > int i, j;
> >
> > - if (reg_is_refcounted_or_null(®s[regno]) && is_null)
> > + if (reg_is_refcounted(®s[regno]) && is_null)
> > release_reference_state(state, id);
>
> Since reg_is_refcounted() is true, release_reference_state should
> always succeed. Check the error code?
Check and log an internal verifier bug?
Sure. It will need another change to pass down the "env" to here though.
May be a WARN_ON_ONCE() instead.
>
> >
> > for (i = 0; i < MAX_BPF_REG; i++)
> > --
> > 2.17.1
> >
>
> How about resetting reg->refcount_id in mark_ptr_or_null_reg as well?
I don't think so. release_reg_references() would not work then.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release
2019-03-01 17:07 ` Martin Lau
@ 2019-03-02 15:35 ` Martin Lau
0 siblings, 0 replies; 6+ messages in thread
From: Martin Lau @ 2019-03-02 15:35 UTC (permalink / raw)
To: Lorenz Bauer; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Fri, Mar 01, 2019 at 09:07:48AM -0800, Martin Lau wrote:
> > How about resetting reg->refcount_id in mark_ptr_or_null_reg as well?
> I don't think so. release_reg_references() would not work then.
After a second thought, 'reg->refcount_id = 0' should be done
for the is_null case. I will respin.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-02 15:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-01 7:03 [PATCH bpf-next 0/2] Fix bpf_tcp_sock and bpf_sk_fullsock issue related to bpf_sk_release Martin KaFai Lau
2019-03-01 7:03 ` [PATCH bpf-next 1/2] bpf: " Martin KaFai Lau
2019-03-01 15:58 ` Lorenz Bauer
2019-03-01 17:07 ` Martin Lau
2019-03-02 15:35 ` Martin Lau
2019-03-01 7:03 ` [PATCH bpf-next 2/2] bpf: Test ref release issue in bpf_tcp_sock and bpf_sk_fullsock Martin KaFai Lau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.