| Message ID | 396f8edcbf8e4f65a1d76c228dc3d12aa4dd2f11.1650430389.git.geliang.tang@suse.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mat Martineau |
| Headers | show |
| Series | BPF packet scheduler | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 35 lines checked |
| matttbe/build | fail | Build error with: -Werror |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | warning | Unstable: 1 failed test(s): selftest_diag - Critical: KMemLeak ❌ |
Hi Geliang, On 20/04/2022 06:57, Geliang Tang wrote: > This patch exports the member last_snd of struct mptcp_sock in > bpf_mptcp_helpers.h, and adds BPF write access to it. Do we really need the have the different schedulers modifying it? Could we not let the caller of "sched->get_subflow()" setting it? I mean: 'last_snd' should always be the same as what get_subflow() will return, no? If yes, why not setting it in mptcp_sched_get_subflow()? Cheers, Matt
Hi Geliang, On 20/04/2022 06:57, Geliang Tang wrote: > This patch exports the member last_snd of struct mptcp_sock in > bpf_mptcp_helpers.h, and adds BPF write access to it. > > Signed-off-by: Geliang Tang <geliang.tang@suse.com> > --- > net/mptcp/bpf.c | 16 ++++++++++++++++ > tools/testing/selftests/bpf/bpf_mptcp_helpers.h | 1 + > 2 files changed, 17 insertions(+) > > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c > index e849fc3fb6c5..bd3c50b07ab2 100644 > --- a/net/mptcp/bpf.c > +++ b/net/mptcp/bpf.c > @@ -40,6 +40,7 @@ static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log, > { > const struct btf_type *state; > u32 type_id; > + size_t end; > > if (atype == BPF_READ) > return btf_struct_access(log, btf, t, off, size, atype, > @@ -55,6 +56,21 @@ static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log, > return -EACCES; > } > > + switch (off) { > + case offsetof(struct mptcp_sock, last_snd): > + end = offsetofend(struct mptcp_sock, last_snd); > + break; > + default: > + bpf_log(log, "no write support to mptcp_sock at off %d\n", off); > + return -EACCES; > + } > + > + if (off + size > end) { > + bpf_log(log, "access beyond mptcp_sock at off %u size %u ended at %lu", > + off, size, end); A small note about this line: the CI[1] seems to complain about it when compiling in i386: > net/mptcp/bpf.c: In function 'bpf_mptcp_sched_btf_struct_access': > net/mptcp/bpf.c:69:71: error: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'size_t' {aka 'unsigned int'} [-Werror=format=] > 69 | bpf_log(log, "access beyond mptcp_sock at off %u size %u ended at %lu", > | ~~^ > | | > | long unsigned int > | %u > 70 | off, size, end); > | ~~~ > | | > | size_t {aka unsigned int} > cc1: all warnings being treated as errors I think you need to use %zu. https://github.com/multipath-tcp/mptcp_net-next/runs/6089328349?check_suite_focus=true Cheers, Matt
Hi Geliang,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mptcp/export]
[cannot apply to bpf-next/master bpf/master linus/master v5.18-rc3 next-20220420]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Geliang-Tang/BPF-packet-scheduler/20220420-130015
base: https://github.com/multipath-tcp/mptcp_net-next.git export
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20220421/202204210456.03kTcuXu-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/c711e7d26efa699f1cfe5e2c9619bbf5cc98a7a7
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Geliang-Tang/BPF-packet-scheduler/20220420-130015
git checkout c711e7d26efa699f1cfe5e2c9619bbf5cc98a7a7
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/mptcp/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
net/mptcp/bpf.c: In function 'bpf_mptcp_sched_btf_struct_access':
>> net/mptcp/bpf.c:69:85: warning: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
69 | bpf_log(log, "access beyond mptcp_sock at off %u size %u ended at %lu",
| ~~^
| |
| long unsigned int
| %u
70 | off, size, end);
| ~~~
| |
| size_t {aka unsigned int}
vim +69 net/mptcp/bpf.c
33
34 static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log,
35 const struct btf *btf,
36 const struct btf_type *t, int off,
37 int size, enum bpf_access_type atype,
38 u32 *next_btf_id,
39 enum bpf_type_flag *flag)
40 {
41 const struct btf_type *state;
42 u32 type_id;
43 size_t end;
44
45 if (atype == BPF_READ)
46 return btf_struct_access(log, btf, t, off, size, atype,
47 next_btf_id, flag);
48
49 type_id = btf_find_by_name_kind(btf, "mptcp_sock", BTF_KIND_STRUCT);
50 if (type_id < 0)
51 return -EINVAL;
52
53 state = btf_type_by_id(btf, type_id);
54 if (t != state) {
55 bpf_log(log, "only read is supported\n");
56 return -EACCES;
57 }
58
59 switch (off) {
60 case offsetof(struct mptcp_sock, last_snd):
61 end = offsetofend(struct mptcp_sock, last_snd);
62 break;
63 default:
64 bpf_log(log, "no write support to mptcp_sock at off %d\n", off);
65 return -EACCES;
66 }
67
68 if (off + size > end) {
> 69 bpf_log(log, "access beyond mptcp_sock at off %u size %u ended at %lu",
70 off, size, end);
71 return -EACCES;
72 }
73
74 return NOT_INIT;
75 }
76
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c index e849fc3fb6c5..bd3c50b07ab2 100644 --- a/net/mptcp/bpf.c +++ b/net/mptcp/bpf.c @@ -40,6 +40,7 @@ static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log, { const struct btf_type *state; u32 type_id; + size_t end; if (atype == BPF_READ) return btf_struct_access(log, btf, t, off, size, atype, @@ -55,6 +56,21 @@ static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log, return -EACCES; } + switch (off) { + case offsetof(struct mptcp_sock, last_snd): + end = offsetofend(struct mptcp_sock, last_snd); + break; + default: + bpf_log(log, "no write support to mptcp_sock at off %d\n", off); + return -EACCES; + } + + if (off + size > end) { + bpf_log(log, "access beyond mptcp_sock at off %u size %u ended at %lu", + off, size, end); + return -EACCES; + } + return NOT_INIT; } diff --git a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h index eb51fc1f54d5..90accd94e947 100644 --- a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h +++ b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h @@ -21,6 +21,7 @@ struct mptcp_sched_ops { struct mptcp_sock { struct inet_connection_sock sk; + struct sock *last_snd; __u32 token; struct sock *first; struct mptcp_sched_ops *sched;
This patch exports the member last_snd of struct mptcp_sock in bpf_mptcp_helpers.h, and adds BPF write access to it. Signed-off-by: Geliang Tang <geliang.tang@suse.com> --- net/mptcp/bpf.c | 16 ++++++++++++++++ tools/testing/selftests/bpf/bpf_mptcp_helpers.h | 1 + 2 files changed, 17 insertions(+)