| Message ID | 20200807223846.4190917-1-sdf@google.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] selftests/bpf: fix v4_to_v6 in sk_lookup | expand |
On Sat, Aug 08, 2020 at 12:38 AM CEST, Stanislav Fomichev wrote: > I'm getting some garbage in bytes 8 and 9 when doing conversion > from sockaddr_in to sockaddr_in6 (leftover from AF_INET?). > Let's explicitly clear the higher bytes. > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- > tools/testing/selftests/bpf/prog_tests/sk_lookup.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > index c571584c00f5..9ff0412e1fd3 100644 > --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > @@ -309,6 +309,7 @@ static void v4_to_v6(struct sockaddr_storage *ss) > v6->sin6_addr.s6_addr[10] = 0xff; > v6->sin6_addr.s6_addr[11] = 0xff; > memcpy(&v6->sin6_addr.s6_addr[12], &v4.sin_addr.s_addr, 4); > + memset(&v6->sin6_addr.s6_addr[0], 0, 10); > } > > static int udp_recv_send(int server_fd) That was badly written. Sorry about that. And thanks for the fix. I'd even zero out the whole thing: memset(v6, 0, sizeof(*v6)); ... because right now IPv4 address is left as sin6_flowinfo. I can follow up with that change, unless you'd like to roll a v2. Fixes: 0ab5539f8584 ("selftests/bpf: Tests for BPF_SK_LOOKUP attach point") Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
On Sat, Aug 8, 2020 at 11:46 AM Jakub Sitnicki <jakub@cloudflare.com> wrote: > > On Sat, Aug 08, 2020 at 12:38 AM CEST, Stanislav Fomichev wrote: > > I'm getting some garbage in bytes 8 and 9 when doing conversion > > from sockaddr_in to sockaddr_in6 (leftover from AF_INET?). > > Let's explicitly clear the higher bytes. > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > tools/testing/selftests/bpf/prog_tests/sk_lookup.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > > index c571584c00f5..9ff0412e1fd3 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > > +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c > > @@ -309,6 +309,7 @@ static void v4_to_v6(struct sockaddr_storage *ss) > > v6->sin6_addr.s6_addr[10] = 0xff; > > v6->sin6_addr.s6_addr[11] = 0xff; > > memcpy(&v6->sin6_addr.s6_addr[12], &v4.sin_addr.s_addr, 4); > > + memset(&v6->sin6_addr.s6_addr[0], 0, 10); > > } > > > > static int udp_recv_send(int server_fd) > > That was badly written. Sorry about that. And thanks for the fix. > > I'd even zero out the whole thing: > > memset(v6, 0, sizeof(*v6)); > > ... because right now IPv4 address is left as sin6_flowinfo. I can > follow up with that change, unless you'd like to roll a v2. Up to you, but I'm not sure zeroing out the whole v6 portion is the best way forward. IMO, it's a bit confusing when reading the code. It will work, but only because v4 and v6 address portions don't really overlap :-/ I was thinking about adding new, on the stack sin6, fully initializing it and then doing memcpy into ss. But I decided that adding memset is probably good enough :-)
On Mon, Aug 10, 2020 at 06:14 PM CEST, Stanislav Fomichev wrote: > On Sat, Aug 8, 2020 at 11:46 AM Jakub Sitnicki <jakub@cloudflare.com> wrote: >> >> On Sat, Aug 08, 2020 at 12:38 AM CEST, Stanislav Fomichev wrote: >> > I'm getting some garbage in bytes 8 and 9 when doing conversion >> > from sockaddr_in to sockaddr_in6 (leftover from AF_INET?). >> > Let's explicitly clear the higher bytes. >> > >> > Signed-off-by: Stanislav Fomichev <sdf@google.com> >> > --- >> > tools/testing/selftests/bpf/prog_tests/sk_lookup.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c >> > index c571584c00f5..9ff0412e1fd3 100644 >> > --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c >> > +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c >> > @@ -309,6 +309,7 @@ static void v4_to_v6(struct sockaddr_storage *ss) >> > v6->sin6_addr.s6_addr[10] = 0xff; >> > v6->sin6_addr.s6_addr[11] = 0xff; >> > memcpy(&v6->sin6_addr.s6_addr[12], &v4.sin_addr.s_addr, 4); >> > + memset(&v6->sin6_addr.s6_addr[0], 0, 10); >> > } >> > >> > static int udp_recv_send(int server_fd) >> >> That was badly written. Sorry about that. And thanks for the fix. >> >> I'd even zero out the whole thing: >> >> memset(v6, 0, sizeof(*v6)); >> >> ... because right now IPv4 address is left as sin6_flowinfo. I can >> follow up with that change, unless you'd like to roll a v2. > Up to you, but I'm not sure zeroing out the whole v6 portion is the > best way forward. > IMO, it's a bit confusing when reading the code. > It will work, but only because v4 and v6 address portions don't really > overlap :-/ It's not that hacky :-) We copy sockaddr_in bits before overwriting ss: struct sockaddr_in v4 = *(struct sockaddr_in *)ss; It could be easier to read, perhaps by copying just the fields we need: struct sockaddr_in *v4 = (struct sockaddr_in *)ss; uint32_t addr = v4->sin_addr.saddr; in_port_t port = v4->sin_port; > I was thinking about adding new, on the stack sin6, fully initializing > it and then doing memcpy into ss. > But I decided that adding memset is probably good enough :-) Makes sense. Either way sounds good to me.
On 8/8/20 12:38 AM, Stanislav Fomichev wrote: > I'm getting some garbage in bytes 8 and 9 when doing conversion > from sockaddr_in to sockaddr_in6 (leftover from AF_INET?). > Let's explicitly clear the higher bytes. > > Signed-off-by: Stanislav Fomichev <sdf@google.com> Applied, thanks!
diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c index c571584c00f5..9ff0412e1fd3 100644 --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c @@ -309,6 +309,7 @@ static void v4_to_v6(struct sockaddr_storage *ss) v6->sin6_addr.s6_addr[10] = 0xff; v6->sin6_addr.s6_addr[11] = 0xff; memcpy(&v6->sin6_addr.s6_addr[12], &v4.sin_addr.s_addr, 4); + memset(&v6->sin6_addr.s6_addr[0], 0, 10); } static int udp_recv_send(int server_fd)
I'm getting some garbage in bytes 8 and 9 when doing conversion from sockaddr_in to sockaddr_in6 (leftover from AF_INET?). Let's explicitly clear the higher bytes. Signed-off-by: Stanislav Fomichev <sdf@google.com> --- tools/testing/selftests/bpf/prog_tests/sk_lookup.c | 1 + 1 file changed, 1 insertion(+)