| Message ID | YtZ+/dAA195d99ak@kili (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 14229b8153a3ca51d97a22a18c68deeae64afce0 |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: fix str_has_sfx() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
On Tue, Jul 19, 2022 at 12:53:01PM +0300, Dan Carpenter wrote: > The return from strcmp() is inverted so the it returns true instead > of false and vise versa. > > Fixes: a1c9d61b19cb ("libbpf: Improve library identification for uprobe binary path resolution") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > Spotted during review. *cmp() functions should always have a comparison > to zero. > if (strcmp(a, b) < 0) { <-- means a < b > if (strcmp(a, b) >= 0) { <-- means a >= b > if (strcmp(a, b) != 0) { <-- means a != b > etc. > > tools/lib/bpf/libbpf_internal.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h > index 9cd7829cbe41..008485296a29 100644 > --- a/tools/lib/bpf/libbpf_internal.h > +++ b/tools/lib/bpf/libbpf_internal.h > @@ -108,9 +108,9 @@ static inline bool str_has_sfx(const char *str, const char *sfx) > size_t str_len = strlen(str); > size_t sfx_len = strlen(sfx); > > - if (sfx_len <= str_len) > - return strcmp(str + str_len - sfx_len, sfx); > - return false; > + if (sfx_len > str_len) > + return false; > + return strcmp(str + str_len - sfx_len, sfx) == 0; Please tag the subject with "bpf" next time. Acked-by: Martin KaFai Lau <kafai@fb.com>
On Tue, Jul 19, 2022 at 10:19:02AM -0700, Martin KaFai Lau wrote: > > @@ -108,9 +108,9 @@ static inline bool str_has_sfx(const char *str, const char *sfx) > > size_t str_len = strlen(str); > > size_t sfx_len = strlen(sfx); > > > > - if (sfx_len <= str_len) > > - return strcmp(str + str_len - sfx_len, sfx); > > - return false; > > + if (sfx_len > str_len) > > + return false; > > + return strcmp(str + str_len - sfx_len, sfx) == 0; > Please tag the subject with "bpf" next time. I always work against linux-next. Would it help if I put that in the subject? Otherwise I don't have a way to figure this stuff out. I kind of know networking tree but not 100% and that is a massive pain in the butt. Until there is an automated way that then those kind of requests are not reasonable. regards, dan carpenter
On Tue, Jul 19, 2022 at 10:19 AM Martin KaFai Lau <kafai@fb.com> wrote: > > On Tue, Jul 19, 2022 at 12:53:01PM +0300, Dan Carpenter wrote: > > The return from strcmp() is inverted so the it returns true instead > > of false and vise versa. > > > > Fixes: a1c9d61b19cb ("libbpf: Improve library identification for uprobe binary path resolution") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > Spotted during review. *cmp() functions should always have a comparison > > to zero. > > if (strcmp(a, b) < 0) { <-- means a < b > > if (strcmp(a, b) >= 0) { <-- means a >= b > > if (strcmp(a, b) != 0) { <-- means a != b > > etc. > > > > tools/lib/bpf/libbpf_internal.h | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h > > index 9cd7829cbe41..008485296a29 100644 > > --- a/tools/lib/bpf/libbpf_internal.h > > +++ b/tools/lib/bpf/libbpf_internal.h > > @@ -108,9 +108,9 @@ static inline bool str_has_sfx(const char *str, const char *sfx) > > size_t str_len = strlen(str); > > size_t sfx_len = strlen(sfx); > > > > - if (sfx_len <= str_len) > > - return strcmp(str + str_len - sfx_len, sfx); > > - return false; > > + if (sfx_len > str_len) > > + return false; > > + return strcmp(str + str_len - sfx_len, sfx) == 0; > Please tag the subject with "bpf" next time. > > Acked-by: Martin KaFai Lau <kafai@fb.com> Alan, If it was so broken how did it work earlier? Do we have a test for this?
On Tue, Jul 19, 2022 at 10:51 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Tue, Jul 19, 2022 at 10:19:02AM -0700, Martin KaFai Lau wrote: > > > @@ -108,9 +108,9 @@ static inline bool str_has_sfx(const char *str, const char *sfx) > > > size_t str_len = strlen(str); > > > size_t sfx_len = strlen(sfx); > > > > > > - if (sfx_len <= str_len) > > > - return strcmp(str + str_len - sfx_len, sfx); > > > - return false; > > > + if (sfx_len > str_len) > > > + return false; > > > + return strcmp(str + str_len - sfx_len, sfx) == 0; > > Please tag the subject with "bpf" next time. > > I always work against linux-next. Would it help if I put that in the > subject? > > Otherwise I don't have a way to figure this stuff out. I kind of know > networking tree but not 100% and that is a massive pain in the butt. > Until there is an automated way that then those kind of requests are > not reasonable. Dan, you were told multiple times to follow the rules. bpf patches should target bpf of bpf-next trees only. If you send against linux-next you're taking a random chance that they will pass CI. In turn making everyone waste time. Please follow the simple rules.
On 19/07/2022 18:51, Alexei Starovoitov wrote: > On Tue, Jul 19, 2022 at 10:19 AM Martin KaFai Lau <kafai@fb.com> wrote: >> >> On Tue, Jul 19, 2022 at 12:53:01PM +0300, Dan Carpenter wrote: >>> The return from strcmp() is inverted so the it returns true instead >>> of false and vise versa. >>> >>> Fixes: a1c9d61b19cb ("libbpf: Improve library identification for uprobe binary path resolution") >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>> --- >>> Spotted during review. *cmp() functions should always have a comparison >>> to zero. >>> if (strcmp(a, b) < 0) { <-- means a < b >>> if (strcmp(a, b) >= 0) { <-- means a >= b >>> if (strcmp(a, b) != 0) { <-- means a != b >>> etc. >>> >>> tools/lib/bpf/libbpf_internal.h | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h >>> index 9cd7829cbe41..008485296a29 100644 >>> --- a/tools/lib/bpf/libbpf_internal.h >>> +++ b/tools/lib/bpf/libbpf_internal.h >>> @@ -108,9 +108,9 @@ static inline bool str_has_sfx(const char *str, const char *sfx) >>> size_t str_len = strlen(str); >>> size_t sfx_len = strlen(sfx); >>> >>> - if (sfx_len <= str_len) >>> - return strcmp(str + str_len - sfx_len, sfx); >>> - return false; >>> + if (sfx_len > str_len) >>> + return false; >>> + return strcmp(str + str_len - sfx_len, sfx) == 0; >> Please tag the subject with "bpf" next time. >> >> Acked-by: Martin KaFai Lau <kafai@fb.com> > > Alan, > > If it was so broken how did it work earlier? > Do we have a test for this? > We have tests for automatic path determination, yes, but those tests specify libc.so.6, so are testing the strstr(file, ".so.") predicate below: if (str_has_sfx(file, ".so") || strstr(file, ".so.")) { Problem is, on many systems, libc.so is a GNU ld script rather than an actual library: cat /usr/lib64/libc.so /* GNU ld script Use the shared library, but some functions are only in the static library, so try that secondarily. */ OUTPUT_FORMAT(elf64-x86-64) GROUP ( /lib64/libc.so.6 /usr/lib64/libc_nonshared.a AS_NEEDED ( /lib64/ld-linux-x86-64.so.2 ) ) ...so we can't rely on system library .so files actually containing the library to instrument. Maybe we can do something with liburandom_read.so now we have it there; I was looking at extending the auto-path determination to usdt too, so we could add a new test to cover this then I think. Alan
On Tue, Jul 19, 2022 at 10:54:13AM -0700, Alexei Starovoitov wrote: > On Tue, Jul 19, 2022 at 10:51 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > On Tue, Jul 19, 2022 at 10:19:02AM -0700, Martin KaFai Lau wrote: > > > > @@ -108,9 +108,9 @@ static inline bool str_has_sfx(const char *str, const char *sfx) > > > > size_t str_len = strlen(str); > > > > size_t sfx_len = strlen(sfx); > > > > > > > > - if (sfx_len <= str_len) > > > > - return strcmp(str + str_len - sfx_len, sfx); > > > > - return false; > > > > + if (sfx_len > str_len) > > > > + return false; > > > > + return strcmp(str + str_len - sfx_len, sfx) == 0; > > > Please tag the subject with "bpf" next time. > > > > I always work against linux-next. Would it help if I put that in the > > subject? > > > > Otherwise I don't have a way to figure this stuff out. I kind of know > > networking tree but not 100% and that is a massive pain in the butt. > > Until there is an automated way that then those kind of requests are > > not reasonable. > > Dan, > > you were told multiple times to follow the rules. > bpf patches should target bpf of bpf-next trees only. > If you send against linux-next you're taking a random chance > that they will pass CI. > In turn making everyone waste time. > Please follow the simple rules. That's true. We have this discussion every time and I always tell you that the rules are untenable. I'm just going to send bug reports to whoever introduces bug and they can deal with it. regards, dan carpenter
Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Tue, 19 Jul 2022 12:53:01 +0300 you wrote: > The return from strcmp() is inverted so the it returns true instead > of false and vise versa. > > Fixes: a1c9d61b19cb ("libbpf: Improve library identification for uprobe binary path resolution") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > Spotted during review. *cmp() functions should always have a comparison > to zero. > if (strcmp(a, b) < 0) { <-- means a < b > if (strcmp(a, b) >= 0) { <-- means a >= b > if (strcmp(a, b) != 0) { <-- means a != b > etc. > > [...] Here is the summary with links: - libbpf: fix str_has_sfx() https://git.kernel.org/bpf/bpf-next/c/14229b8153a3 You are awesome, thank you!
On Wed, Jul 20, 2022 at 12:48 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > On 19/07/2022 18:51, Alexei Starovoitov wrote: > > On Tue, Jul 19, 2022 at 10:19 AM Martin KaFai Lau <kafai@fb.com> wrote: > >> > >> On Tue, Jul 19, 2022 at 12:53:01PM +0300, Dan Carpenter wrote: > >>> The return from strcmp() is inverted so the it returns true instead > >>> of false and vise versa. > >>> > >>> Fixes: a1c9d61b19cb ("libbpf: Improve library identification for uprobe binary path resolution") > >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > >>> --- > >>> Spotted during review. *cmp() functions should always have a comparison > >>> to zero. > >>> if (strcmp(a, b) < 0) { <-- means a < b > >>> if (strcmp(a, b) >= 0) { <-- means a >= b > >>> if (strcmp(a, b) != 0) { <-- means a != b > >>> etc. > >>> > >>> tools/lib/bpf/libbpf_internal.h | 6 +++--- > >>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h > >>> index 9cd7829cbe41..008485296a29 100644 > >>> --- a/tools/lib/bpf/libbpf_internal.h > >>> +++ b/tools/lib/bpf/libbpf_internal.h > >>> @@ -108,9 +108,9 @@ static inline bool str_has_sfx(const char *str, const char *sfx) > >>> size_t str_len = strlen(str); > >>> size_t sfx_len = strlen(sfx); > >>> > >>> - if (sfx_len <= str_len) > >>> - return strcmp(str + str_len - sfx_len, sfx); > >>> - return false; > >>> + if (sfx_len > str_len) > >>> + return false; > >>> + return strcmp(str + str_len - sfx_len, sfx) == 0; > >> Please tag the subject with "bpf" next time. > >> > >> Acked-by: Martin KaFai Lau <kafai@fb.com> > > > > Alan, > > > > If it was so broken how did it work earlier? > > Do we have a test for this? > > > > We have tests for automatic path determination, yes, but those > tests specify libc.so.6, so are testing the strstr(file, ".so.") > predicate below: > > if (str_has_sfx(file, ".so") || strstr(file, ".so.")) { > > Problem is, on many systems, libc.so is a GNU ld script rather > than an actual library: > > cat /usr/lib64/libc.so > /* GNU ld script > Use the shared library, but some functions are only in > the static library, so try that secondarily. */ > OUTPUT_FORMAT(elf64-x86-64) > GROUP ( /lib64/libc.so.6 /usr/lib64/libc_nonshared.a AS_NEEDED ( /lib64/ld-linux-x86-64.so.2 ) ) > > ...so we can't rely on system library .so files actually containing > the library to instrument. > > Maybe we can do something with liburandom_read.so now we have it > there; I was looking at extending the auto-path determination > to usdt too, so we could add a new test to cover this then I think. Library path resolution should already work for USDTs (note that I reuse resolve_full_path() in bpf_program__attach_usdt()), but having explicit tests would be great. It might be simplest to temporarily override LD_LIBRARY_PATH with a path to liburandom_read.so? So please consider sending a patch with tests, thanks! > > Alan
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h index 9cd7829cbe41..008485296a29 100644 --- a/tools/lib/bpf/libbpf_internal.h +++ b/tools/lib/bpf/libbpf_internal.h @@ -108,9 +108,9 @@ static inline bool str_has_sfx(const char *str, const char *sfx) size_t str_len = strlen(str); size_t sfx_len = strlen(sfx); - if (sfx_len <= str_len) - return strcmp(str + str_len - sfx_len, sfx); - return false; + if (sfx_len > str_len) + return false; + return strcmp(str + str_len - sfx_len, sfx) == 0; } /* Symbol versioning is different between static and shared library.
The return from strcmp() is inverted so the it returns true instead of false and vise versa. Fixes: a1c9d61b19cb ("libbpf: Improve library identification for uprobe binary path resolution") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- Spotted during review. *cmp() functions should always have a comparison to zero. if (strcmp(a, b) < 0) { <-- means a < b if (strcmp(a, b) >= 0) { <-- means a >= b if (strcmp(a, b) != 0) { <-- means a != b etc. tools/lib/bpf/libbpf_internal.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)