| Message ID | 20190227224642.1069138-2-andriin@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | btf_dedup algorithm and test fixes | expand |
On Wed, Feb 27, 2019 at 2:47 PM Andrii Nakryiko <andriin@fb.com> wrote: > > btf_dedup testing code doesn't account for length of struct btf_header > when calculating the start of a string section. This patch fixes this > problem. > > Fixes: 49b57e0d01db ("tools/bpf: remove btf__get_strings() superseded by raw data API") > Signed-off-by: Andrii Nakryiko <andriin@fb.com> Acked-by: Song Liu <songliubraving@fb.com> > --- > tools/testing/selftests/bpf/.gitignore | 1 + > tools/testing/selftests/bpf/test_btf.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore > index e47168d1257d..3b74d23fffab 100644 > --- a/tools/testing/selftests/bpf/.gitignore > +++ b/tools/testing/selftests/bpf/.gitignore > @@ -14,6 +14,7 @@ feature > test_libbpf_open > test_sock > test_sock_addr > +test_sock_fields > urandom_read > test_btf > test_sockmap > diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c > index 02d314383a9c..1426c0a905c8 100644 > --- a/tools/testing/selftests/bpf/test_btf.c > +++ b/tools/testing/selftests/bpf/test_btf.c > @@ -5936,9 +5936,9 @@ static int do_test_dedup(unsigned int test_num) > } > > test_hdr = test_btf_data; > - test_strs = test_btf_data + test_hdr->str_off; > + test_strs = test_btf_data + sizeof(*test_hdr) + test_hdr->str_off; > expect_hdr = expect_btf_data; > - expect_strs = expect_btf_data + expect_hdr->str_off; > + expect_strs = expect_btf_data + sizeof(*test_hdr) + expect_hdr->str_off; > if (CHECK(test_hdr->str_len != expect_hdr->str_len, > "test_hdr->str_len:%u != expect_hdr->str_len:%u", > test_hdr->str_len, expect_hdr->str_len)) { > -- > 2.17.1 >
On 2/27/19 2:46 PM, Andrii Nakryiko wrote: > btf_dedup testing code doesn't account for length of struct btf_header > when calculating the start of a string section. This patch fixes this > problem. > > Fixes: 49b57e0d01db ("tools/bpf: remove btf__get_strings() superseded by raw data API") > Signed-off-by: Andrii Nakryiko <andriin@fb.com> Acked-by: Yonghong Song <yhs@fb.com> for Patch #1 - #3. > --- > tools/testing/selftests/bpf/.gitignore | 1 + > tools/testing/selftests/bpf/test_btf.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore > index e47168d1257d..3b74d23fffab 100644 > --- a/tools/testing/selftests/bpf/.gitignore > +++ b/tools/testing/selftests/bpf/.gitignore > @@ -14,6 +14,7 @@ feature > test_libbpf_open > test_sock > test_sock_addr > +test_sock_fields > urandom_read > test_btf > test_sockmap > diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c > index 02d314383a9c..1426c0a905c8 100644 > --- a/tools/testing/selftests/bpf/test_btf.c > +++ b/tools/testing/selftests/bpf/test_btf.c > @@ -5936,9 +5936,9 @@ static int do_test_dedup(unsigned int test_num) > } > > test_hdr = test_btf_data; > - test_strs = test_btf_data + test_hdr->str_off; > + test_strs = test_btf_data + sizeof(*test_hdr) + test_hdr->str_off; > expect_hdr = expect_btf_data; > - expect_strs = expect_btf_data + expect_hdr->str_off; > + expect_strs = expect_btf_data + sizeof(*test_hdr) + expect_hdr->str_off; > if (CHECK(test_hdr->str_len != expect_hdr->str_len, > "test_hdr->str_len:%u != expect_hdr->str_len:%u", > test_hdr->str_len, expect_hdr->str_len)) { >
Em Wed, Feb 27, 2019 at 02:46:37PM -0800, Andrii Nakryiko escreveu: > btf_dedup testing code doesn't account for length of struct btf_header > when calculating the start of a string section. This patch fixes this > problem. > > Fixes: 49b57e0d01db ("tools/bpf: remove btf__get_strings() superseded by raw data API") I think this clarifies things, but a Fixes seems excessive, right? I.e. if you missed it in both sides of the (a != b) expression, the test will be just as valid. I say this because Fixes tags are now tracked and generates backporting efforts that sometimes end up causing unnecessary human exchanges when the patches don't apply because of some other patch.o Or is there some further use of 'test_strs' and 'expect_strs' further down that do_test_dedup() function? /me scratches head, probably missing something... - Arnaldo > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > tools/testing/selftests/bpf/.gitignore | 1 + > tools/testing/selftests/bpf/test_btf.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore > index e47168d1257d..3b74d23fffab 100644 > --- a/tools/testing/selftests/bpf/.gitignore > +++ b/tools/testing/selftests/bpf/.gitignore > @@ -14,6 +14,7 @@ feature > test_libbpf_open > test_sock > test_sock_addr > +test_sock_fields > urandom_read > test_btf > test_sockmap > diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c > index 02d314383a9c..1426c0a905c8 100644 > --- a/tools/testing/selftests/bpf/test_btf.c > +++ b/tools/testing/selftests/bpf/test_btf.c > @@ -5936,9 +5936,9 @@ static int do_test_dedup(unsigned int test_num) > } > > test_hdr = test_btf_data; > - test_strs = test_btf_data + test_hdr->str_off; > + test_strs = test_btf_data + sizeof(*test_hdr) + test_hdr->str_off; > expect_hdr = expect_btf_data; > - expect_strs = expect_btf_data + expect_hdr->str_off; > + expect_strs = expect_btf_data + sizeof(*test_hdr) + expect_hdr->str_off; > if (CHECK(test_hdr->str_len != expect_hdr->str_len, > "test_hdr->str_len:%u != expect_hdr->str_len:%u", > test_hdr->str_len, expect_hdr->str_len)) { > -- > 2.17.1
On Thu, Feb 28, 2019 at 10:52 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Wed, Feb 27, 2019 at 02:46:37PM -0800, Andrii Nakryiko escreveu: > > btf_dedup testing code doesn't account for length of struct btf_header > > when calculating the start of a string section. This patch fixes this > > problem. > > > > Fixes: 49b57e0d01db ("tools/bpf: remove btf__get_strings() superseded by raw data API") > > I think this clarifies things, but a Fixes seems excessive, right? I.e. > if you missed it in both sides of the (a != b) expression, the test will > be just as valid. > > I say this because Fixes tags are now tracked and generates backporting > efforts that sometimes end up causing unnecessary human exchanges when > the patches don't apply because of some other patch.o I think this is a legitimate fix. The bug wasn't exposed until I added a new test from this patchset, but still. See below. > > Or is there some further use of 'test_strs' and 'expect_strs' further > down that do_test_dedup() function? Yes, few lines further down we use those pointers to iterate over all strings in BTF string section and compare them: test_str_cur = test_strs; test_str_end = test_strs + test_hdr->str_len; expect_str_cur = expect_strs; expect_str_end = expect_strs + expect_hdr->str_len; while (test_str_cur < test_str_end && expect_str_cur < expect_str_end) { ... compare strings ... } With this bug, what ended up happening was that we never tested last sizeof(struct btf_header) bytes of string section. It worked fine for existing tests by luck, but in general it would fail, because after that loop we check that we finished iteration exactly where we should: if (CHECK(test_str_cur != test_str_end, "test_str_cur:%p != test_str_end:%p", test_str_cur, test_str_end)) { err = -1; goto done; } New test I added didn't have lucky byte arrangement that made test_str_cur == test_str_end true for previous tests. > > /me scratches head, probably missing something... > > - Arnaldo > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- > > tools/testing/selftests/bpf/.gitignore | 1 + > > tools/testing/selftests/bpf/test_btf.c | 4 ++-- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore > > index e47168d1257d..3b74d23fffab 100644 > > --- a/tools/testing/selftests/bpf/.gitignore > > +++ b/tools/testing/selftests/bpf/.gitignore > > @@ -14,6 +14,7 @@ feature > > test_libbpf_open > > test_sock > > test_sock_addr > > +test_sock_fields > > urandom_read > > test_btf > > test_sockmap > > diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c > > index 02d314383a9c..1426c0a905c8 100644 > > --- a/tools/testing/selftests/bpf/test_btf.c > > +++ b/tools/testing/selftests/bpf/test_btf.c > > @@ -5936,9 +5936,9 @@ static int do_test_dedup(unsigned int test_num) > > } > > > > test_hdr = test_btf_data; > > - test_strs = test_btf_data + test_hdr->str_off; > > + test_strs = test_btf_data + sizeof(*test_hdr) + test_hdr->str_off; > > expect_hdr = expect_btf_data; > > - expect_strs = expect_btf_data + expect_hdr->str_off; > > + expect_strs = expect_btf_data + sizeof(*test_hdr) + expect_hdr->str_off; > > if (CHECK(test_hdr->str_len != expect_hdr->str_len, > > "test_hdr->str_len:%u != expect_hdr->str_len:%u", > > test_hdr->str_len, expect_hdr->str_len)) { > > -- > > 2.17.1 > > -- > > - Arnaldo
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore index e47168d1257d..3b74d23fffab 100644 --- a/tools/testing/selftests/bpf/.gitignore +++ b/tools/testing/selftests/bpf/.gitignore @@ -14,6 +14,7 @@ feature test_libbpf_open test_sock test_sock_addr +test_sock_fields urandom_read test_btf test_sockmap diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c index 02d314383a9c..1426c0a905c8 100644 --- a/tools/testing/selftests/bpf/test_btf.c +++ b/tools/testing/selftests/bpf/test_btf.c @@ -5936,9 +5936,9 @@ static int do_test_dedup(unsigned int test_num) } test_hdr = test_btf_data; - test_strs = test_btf_data + test_hdr->str_off; + test_strs = test_btf_data + sizeof(*test_hdr) + test_hdr->str_off; expect_hdr = expect_btf_data; - expect_strs = expect_btf_data + expect_hdr->str_off; + expect_strs = expect_btf_data + sizeof(*test_hdr) + expect_hdr->str_off; if (CHECK(test_hdr->str_len != expect_hdr->str_len, "test_hdr->str_len:%u != expect_hdr->str_len:%u", test_hdr->str_len, expect_hdr->str_len)) {
btf_dedup testing code doesn't account for length of struct btf_header when calculating the start of a string section. This patch fixes this problem. Fixes: 49b57e0d01db ("tools/bpf: remove btf__get_strings() superseded by raw data API") Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/testing/selftests/bpf/.gitignore | 1 + tools/testing/selftests/bpf/test_btf.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)