| Message ID | 05989b20a8793d1ee1fa70a8a7a4328a768263d0.1644314545.git.fmaurer@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: Do not try bpf_msg_push_data with len 0 | expand |
On 2/8/22 2:45 AM, Felix Maurer wrote: > If bpf_msg_push_data is called with len 0 (as it happens during > selftests/bpf/test_sockmap), we do not need to do anything and can > return early. > > Signed-off-by: Felix Maurer <fmaurer@redhat.com> > --- > net/core/filter.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 4603b7cd3cd1..9eb785842258 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -2710,6 +2710,9 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start, > if (unlikely(flags)) > return -EINVAL; > > + if (unlikely(len == 0)) > + return 0; If len == 0 is really unlikely in production environment, we probably can keep it as is. There are some helpers like this with a 'len' parameter, e.g., bpf_probe_read_kernel, bpf_probe_read_user, etc. which don't have 'size == 0' check. John, could you also take a look? > + > /* First find the starting scatterlist element */ > i = msg->sg.start; > do {
On 08.02.22 17:23, Yonghong Song wrote: > On 2/8/22 2:45 AM, Felix Maurer wrote: >> If bpf_msg_push_data is called with len 0 (as it happens during >> selftests/bpf/test_sockmap), we do not need to do anything and can >> return early. >> >> Signed-off-by: Felix Maurer <fmaurer@redhat.com> >> --- >> net/core/filter.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/net/core/filter.c b/net/core/filter.c >> index 4603b7cd3cd1..9eb785842258 100644 >> --- a/net/core/filter.c >> +++ b/net/core/filter.c >> @@ -2710,6 +2710,9 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, >> msg, u32, start, >> if (unlikely(flags)) >> return -EINVAL; >> + if (unlikely(len == 0)) >> + return 0; > > If len == 0 is really unlikely in production environment, we > probably can keep it as is. There are some helpers like this > with a 'len' parameter, e.g., bpf_probe_read_kernel, > bpf_probe_read_user, etc. which don't have 'size == 0' check. My point with this is that the rest of the code does not expect len to be 0. E.g., we later call get_order(copy + len); if len is 0, copy + len is also often 0 and get_order returns some undefined value (at the moment 52). alloc_pages catches that and fails, but then bpf_msg_push_data returns ENOMEM. This seems wrong because we are not out of memory and actually do not need any additional memory. > John, could you also take a look? > >> + >> /* First find the starting scatterlist element */ >> i = msg->sg.start; >> do { >
On 2/8/22 8:45 AM, Felix Maurer wrote: > On 08.02.22 17:23, Yonghong Song wrote: >> On 2/8/22 2:45 AM, Felix Maurer wrote: >>> If bpf_msg_push_data is called with len 0 (as it happens during >>> selftests/bpf/test_sockmap), we do not need to do anything and can >>> return early. >>> >>> Signed-off-by: Felix Maurer <fmaurer@redhat.com> >>> --- >>> net/core/filter.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/net/core/filter.c b/net/core/filter.c >>> index 4603b7cd3cd1..9eb785842258 100644 >>> --- a/net/core/filter.c >>> +++ b/net/core/filter.c >>> @@ -2710,6 +2710,9 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, >>> msg, u32, start, >>> if (unlikely(flags)) >>> return -EINVAL; >>> + if (unlikely(len == 0)) >>> + return 0; >> >> If len == 0 is really unlikely in production environment, we >> probably can keep it as is. There are some helpers like this >> with a 'len' parameter, e.g., bpf_probe_read_kernel, >> bpf_probe_read_user, etc. which don't have 'size == 0' check. > > My point with this is that the rest of the code does not expect len to > be 0. E.g., we later call get_order(copy + len); if len is 0, copy + len > is also often 0 and get_order returns some undefined value (at the > moment 52). alloc_pages catches that and fails, but then > bpf_msg_push_data returns ENOMEM. This seems wrong because we are not > out of memory and actually do not need any additional memory. So this actually a bug fix. Then please add the above to commit messages and also add a Fixes tag and resubmit. Thanks! > >> John, could you also take a look? >> >>> + >>> /* First find the starting scatterlist element */ >>> i = msg->sg.start; >>> do { >> >
diff --git a/net/core/filter.c b/net/core/filter.c index 4603b7cd3cd1..9eb785842258 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -2710,6 +2710,9 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start, if (unlikely(flags)) return -EINVAL; + if (unlikely(len == 0)) + return 0; + /* First find the starting scatterlist element */ i = msg->sg.start; do {
If bpf_msg_push_data is called with len 0 (as it happens during selftests/bpf/test_sockmap), we do not need to do anything and can return early. Signed-off-by: Felix Maurer <fmaurer@redhat.com> --- net/core/filter.c | 3 +++ 1 file changed, 3 insertions(+)