| Message ID | 20240503-bpf_fix_access-v1-1-5a714318ea64@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 17783ae3885104bf37d057c4f32562e69a42f672 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-next] Squash to "mptcp: add bpf_mptcp_sched_ops" -- fix bpf access | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 49 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | warning | Unstable: 1 failed test(s): selftest_simult_flows |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
Hi Gregory, Thank you for your modifications, that's great! Our CI did some validations and here is its report: - KVM Validation: normal: Unstable: 1 failed test(s): selftest_simult_flows
Hi Gregory, On Fri, May 03, 2024 at 07:33:25PM +0000, Gregory Detal wrote: > The current behavior allows to write to mptcp_sock at offset that is > defined in mptcp_subflow_context and vice versa. > > This fixes this by splitting the checks for each struct type. > > Signed-off-by: Gregory Detal <gregory.detal@gmail.com> Thanks for this fix. Looks good to me. Reviewed-by: Geliang Tang <geliang@kernel.org> -Geliang > --- > net/mptcp/bpf.c | 42 +++++++++++++++++++++++++----------------- > 1 file changed, 25 insertions(+), 17 deletions(-) > > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c > index 208e5d3f066f..57c47bb430b1 100644 > --- a/net/mptcp/bpf.c > +++ b/net/mptcp/bpf.c > @@ -47,24 +47,32 @@ static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log, > size_t end; > > t = btf_type_by_id(reg->btf, reg->btf_id); > - if (t != mptcp_sock_type && t != mptcp_subflow_type) { > - bpf_log(log, "only access to mptcp sock or subflow is supported\n"); > - return -EACCES; > - } > > - switch (off) { > - case offsetof(struct mptcp_sock, snd_burst): > - end = offsetofend(struct mptcp_sock, snd_burst); > - break; > - case offsetof(struct mptcp_subflow_context, scheduled): > - end = offsetofend(struct mptcp_subflow_context, scheduled); > - break; > - case offsetof(struct mptcp_subflow_context, avg_pacing_rate): > - end = offsetofend(struct mptcp_subflow_context, avg_pacing_rate); > - break; > - default: > - bpf_log(log, "no write support to %s at off %d\n", > - t == mptcp_sock_type ? "mptcp_sock" : "mptcp_subflow_context", off); > + if (t == mptcp_sock_type) { > + switch (off) { > + case offsetof(struct mptcp_sock, snd_burst): > + end = offsetofend(struct mptcp_sock, snd_burst); > + break; > + default: > + bpf_log(log, "no write support to mptcp_sock at off %d\n", > + off); > + return -EACCES; > + } > + } else if (t == mptcp_subflow_type) { > + switch (off) { > + case offsetof(struct mptcp_subflow_context, scheduled): > + end = offsetofend(struct mptcp_subflow_context, scheduled); > + break; > + case offsetof(struct mptcp_subflow_context, avg_pacing_rate): > + end = offsetofend(struct mptcp_subflow_context, avg_pacing_rate); > + break; > + default: > + bpf_log(log, "no write support to mptcp_subflow_context at off %d\n", > + off); > + return -EACCES; > + } > + } else { > + bpf_log(log, "only access to mptcp sock or subflow is supported\n"); > return -EACCES; > } > > > --- > base-commit: 56030f9d3812071365435354c0eb5ffb3504e58a > change-id: 20240503-bpf_fix_access-a360b88c1534 > > Best regards, > -- > Gregory Detal <gregory.detal@gmail.com> >
Hi Gregory, Geliang, On 03/05/2024 21:33, Gregory Detal wrote: > The current behavior allows to write to mptcp_sock at offset that is > defined in mptcp_subflow_context and vice versa. > > This fixes this by splitting the checks for each struct type. Thank you for the fix and the review! Now in our tree: New patches for t/upstream: - 17783ae38851: "squashed" in "bpf: Add bpf_mptcp_sched_ops" - c0f6d508db13: "Signed-off-by" + "Co-developed-by" - Results: 77024827f43c..f16f6f211e69 (export) Tests are now in progress: - export: https://github.com/multipath-tcp/mptcp_net-next/commit/26924223a2c1354a5444e3e70b286fee1a3f1c67/checks Cheers, Matt
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c index 208e5d3f066f..57c47bb430b1 100644 --- a/net/mptcp/bpf.c +++ b/net/mptcp/bpf.c @@ -47,24 +47,32 @@ static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log, size_t end; t = btf_type_by_id(reg->btf, reg->btf_id); - if (t != mptcp_sock_type && t != mptcp_subflow_type) { - bpf_log(log, "only access to mptcp sock or subflow is supported\n"); - return -EACCES; - } - switch (off) { - case offsetof(struct mptcp_sock, snd_burst): - end = offsetofend(struct mptcp_sock, snd_burst); - break; - case offsetof(struct mptcp_subflow_context, scheduled): - end = offsetofend(struct mptcp_subflow_context, scheduled); - break; - case offsetof(struct mptcp_subflow_context, avg_pacing_rate): - end = offsetofend(struct mptcp_subflow_context, avg_pacing_rate); - break; - default: - bpf_log(log, "no write support to %s at off %d\n", - t == mptcp_sock_type ? "mptcp_sock" : "mptcp_subflow_context", off); + if (t == mptcp_sock_type) { + switch (off) { + case offsetof(struct mptcp_sock, snd_burst): + end = offsetofend(struct mptcp_sock, snd_burst); + break; + default: + bpf_log(log, "no write support to mptcp_sock at off %d\n", + off); + return -EACCES; + } + } else if (t == mptcp_subflow_type) { + switch (off) { + case offsetof(struct mptcp_subflow_context, scheduled): + end = offsetofend(struct mptcp_subflow_context, scheduled); + break; + case offsetof(struct mptcp_subflow_context, avg_pacing_rate): + end = offsetofend(struct mptcp_subflow_context, avg_pacing_rate); + break; + default: + bpf_log(log, "no write support to mptcp_subflow_context at off %d\n", + off); + return -EACCES; + } + } else { + bpf_log(log, "only access to mptcp sock or subflow is supported\n"); return -EACCES; }
The current behavior allows to write to mptcp_sock at offset that is defined in mptcp_subflow_context and vice versa. This fixes this by splitting the checks for each struct type. Signed-off-by: Gregory Detal <gregory.detal@gmail.com> --- net/mptcp/bpf.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) --- base-commit: 56030f9d3812071365435354c0eb5ffb3504e58a change-id: 20240503-bpf_fix_access-a360b88c1534 Best regards,