* [PATCH bpf-next] remove BPF_OBJ_NAME_LEN restriction when looking up bpf program by name
@ 2022-07-29 6:18 Manu Bretelle
2022-07-29 9:53 ` Quentin Monnet
0 siblings, 1 reply; 3+ messages in thread
From: Manu Bretelle @ 2022-07-29 6:18 UTC (permalink / raw)
To: bpf, andrii, quentin
From: chantra <chantr4@gmail.com>
bpftool was limiting the length of names to
[BPF_OBJ_NAME_LEN](https://github.com/libbpf/bpftool/blob/2d7bba1e8c17dd0422879c856cda66723b209952/src/common.c#L823-L826).
Since
https://github.com/libbpf/bpftool/commit/61833a284f48b90f6802c141c8356de64bb41e10
we can get the full program name from BTF.
This diffs remove the restriction of name length when running `bpftool
prog show name ${name}`.
Test:
Tested against some internal program names that were longer than
`BPF_OBJ_NAME_LEN`, here a redacted example of what was ran to test.
```
$ sudo bpftool prog show name some_long_program_name
Error: can't parse name
$ sudo ./bpftool prog show name some_long_program_name
123456789: tracing name some_long_program_name tag taghexa gpl ....
...
...
...
```
---
tools/bpf/bpftool/common.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index 067e9ea59e3b..bc9017877296 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -722,6 +722,7 @@ print_all_levels(__maybe_unused enum libbpf_print_level level,
static int prog_fd_by_nametag(void *nametag, int **fds, bool tag)
{
+ char prog_name[MAX_PROG_FULL_NAME];
unsigned int id = 0;
int fd, nb_fds = 0;
void *tmp;
@@ -754,12 +755,21 @@ static int prog_fd_by_nametag(void *nametag, int **fds, bool tag)
goto err_close_fd;
}
- if ((tag && memcmp(nametag, info.tag, BPF_TAG_SIZE)) ||
- (!tag && strncmp(nametag, info.name, BPF_OBJ_NAME_LEN))) {
+ if (tag && memcmp(nametag, info.tag, BPF_TAG_SIZE)) {
close(fd);
continue;
}
+
+
+ if (!tag) {
+ get_prog_full_name(&info, fd, prog_name, sizeof(prog_name));
+ if (strcmp(nametag, prog_name)) {
+ close(fd);
+ continue;
+ }
+ }
+
if (nb_fds > 0) {
tmp = realloc(*fds, (nb_fds + 1) * sizeof(int));
if (!tmp) {
@@ -820,10 +830,6 @@ int prog_parse_fds(int *argc, char ***argv, int **fds)
NEXT_ARGP();
name = **argv;
- if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
- p_err("can't parse name");
- return -1;
- }
NEXT_ARGP();
return prog_fd_by_nametag(name, fds, false);
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] remove BPF_OBJ_NAME_LEN restriction when looking up bpf program by name
2022-07-29 6:18 [PATCH bpf-next] remove BPF_OBJ_NAME_LEN restriction when looking up bpf program by name Manu Bretelle
@ 2022-07-29 9:53 ` Quentin Monnet
2022-07-29 18:54 ` Manu Bretelle
0 siblings, 1 reply; 3+ messages in thread
From: Quentin Monnet @ 2022-07-29 9:53 UTC (permalink / raw)
To: Manu Bretelle, bpf, andrii
On 29/07/2022 07:18, Manu Bretelle wrote:
> From: chantra <chantr4@gmail.com>
>
> bpftool was limiting the length of names to
> [BPF_OBJ_NAME_LEN](https://github.com/libbpf/bpftool/blob/2d7bba1e8c17dd0422879c856cda66723b209952/src/common.c#L823-L826).
>
> Since
> https://github.com/libbpf/bpftool/commit/61833a284f48b90f6802c141c8356de64bb41e10
> we can get the full program name from BTF.
>
> This diffs remove the restriction of name length when running `bpftool
-> "This patch removes"?
> prog show name ${name}`.
>
> Test:
> Tested against some internal program names that were longer than
> `BPF_OBJ_NAME_LEN`, here a redacted example of what was ran to test.
>
> ```
> $ sudo bpftool prog show name some_long_program_name
> Error: can't parse name
> $ sudo ./bpftool prog show name some_long_program_name
> 123456789: tracing name some_long_program_name tag taghexa gpl ....
> ...
> ...
> ...
> ```
Thanks a lot for the patch! The suggested change looks good, but the
code and the patch themselves need some adjustments.
Regarding your commit object (and email subject): Please prefix with the
component that you update. For your next version, this should be:
[PATCH bpf-next v2] bpftool: Remove BPF_OBJ_NAME_LEN...
For the commit description, please avoid external links (GitHub). Prefer
function names (we can grep for them) or commit references [0]. I would
also recommend against too much Markdown mark-up, the triple quotes
could be removed and the snippet indented instead.
Your commit is also missing your Signed-off-by tag in its description,
you will need to add it [1].
[0]
https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed+off#describe-your-changes
[1]
https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed+off#sign-your-work-the-developer-s-certificate-of-origin
> ---
> tools/bpf/bpftool/common.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 067e9ea59e3b..bc9017877296 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -722,6 +722,7 @@ print_all_levels(__maybe_unused enum libbpf_print_level level,
>
> static int prog_fd_by_nametag(void *nametag, int **fds, bool tag)
> {
> + char prog_name[MAX_PROG_FULL_NAME];
> unsigned int id = 0;
> int fd, nb_fds = 0;
> void *tmp;
> @@ -754,12 +755,21 @@ static int prog_fd_by_nametag(void *nametag, int **fds, bool tag)
> goto err_close_fd;
> }
>
> - if ((tag && memcmp(nametag, info.tag, BPF_TAG_SIZE)) ||
> - (!tag && strncmp(nametag, info.name, BPF_OBJ_NAME_LEN))) {
> + if (tag && memcmp(nametag, info.tag, BPF_TAG_SIZE)) {
> close(fd);
> continue;
> }
>
> +
> +
Too many blank lines, please use just one.
> + if (!tag) {
> + get_prog_full_name(&info, fd, prog_name, sizeof(prog_name));
> + if (strcmp(nametag, prog_name)) {
strncmp(), please
> + close(fd);
> + continue;
> + }
> + }
> +
> if (nb_fds > 0) {
> tmp = realloc(*fds, (nb_fds + 1) * sizeof(int));
> if (!tmp) {
> @@ -820,10 +830,6 @@ int prog_parse_fds(int *argc, char ***argv, int **fds)
> NEXT_ARGP();
>
> name = **argv;
> - if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
> - p_err("can't parse name");
> - return -1;
> - }
Why removing the check? Just update the bound to MAX_PROG_FULL_NAME - 1?
> NEXT_ARGP();
>
> return prog_fd_by_nametag(name, fds, false);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] remove BPF_OBJ_NAME_LEN restriction when looking up bpf program by name
2022-07-29 9:53 ` Quentin Monnet
@ 2022-07-29 18:54 ` Manu Bretelle
0 siblings, 0 replies; 3+ messages in thread
From: Manu Bretelle @ 2022-07-29 18:54 UTC (permalink / raw)
To: Quentin Monnet; +Cc: bpf, andrii
On Fri, Jul 29, 2022 at 2:53 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> On 29/07/2022 07:18, Manu Bretelle wrote:
> > From: chantra <chantr4@gmail.com>
> >
> > bpftool was limiting the length of names to
> > [BPF_OBJ_NAME_LEN](https://github.com/libbpf/bpftool/blob/2d7bba1e8c17dd0422879c856cda66723b209952/src/common.c#L823-L826).
> >
> > Since
> > https://github.com/libbpf/bpftool/commit/61833a284f48b90f6802c141c8356de64bb41e10
> > we can get the full program name from BTF.
> >
> > This diffs remove the restriction of name length when running `bpftool
>
> -> "This patch removes"?
>
> > prog show name ${name}`.
> >
> > Test:
> > Tested against some internal program names that were longer than
> > `BPF_OBJ_NAME_LEN`, here a redacted example of what was ran to test.
> >
> > ```
> > $ sudo bpftool prog show name some_long_program_name
> > Error: can't parse name
> > $ sudo ./bpftool prog show name some_long_program_name
> > 123456789: tracing name some_long_program_name tag taghexa gpl ....
> > ...
> > ...
> > ...
> > ```
>
> Thanks a lot for the patch! The suggested change looks good, but the
> code and the patch themselves need some adjustments.
>
> Regarding your commit object (and email subject): Please prefix with the
> component that you update. For your next version, this should be:
>
> [PATCH bpf-next v2] bpftool: Remove BPF_OBJ_NAME_LEN...
>
> For the commit description, please avoid external links (GitHub). Prefer
> function names (we can grep for them) or commit references [0]. I would
> also recommend against too much Markdown mark-up, the triple quotes
> could be removed and the snippet indented instead.
>
> Your commit is also missing your Signed-off-by tag in its description,
> you will need to add it [1].
>
> [0]
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed+off#describe-your-changes
> [1]
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed+off#sign-your-work-the-developer-s-certificate-of-origin
>
Thanks for the feedback and providing relevant links. I will amend the
commit to reflect your feedback. Markdown was an artifact of this
being a GH PR originally. I will go for plaintext.
> > ---
> > tools/bpf/bpftool/common.c | 18 ++++++++++++------
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 067e9ea59e3b..bc9017877296 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> > @@ -722,6 +722,7 @@ print_all_levels(__maybe_unused enum libbpf_print_level level,
> >
> > static int prog_fd_by_nametag(void *nametag, int **fds, bool tag)
> > {
> > + char prog_name[MAX_PROG_FULL_NAME];
> > unsigned int id = 0;
> > int fd, nb_fds = 0;
> > void *tmp;
> > @@ -754,12 +755,21 @@ static int prog_fd_by_nametag(void *nametag, int **fds, bool tag)
> > goto err_close_fd;
> > }
> >
> > - if ((tag && memcmp(nametag, info.tag, BPF_TAG_SIZE)) ||
> > - (!tag && strncmp(nametag, info.name, BPF_OBJ_NAME_LEN))) {
> > + if (tag && memcmp(nametag, info.tag, BPF_TAG_SIZE)) {
> > close(fd);
> > continue;
> > }
> >
> > +
> > +
>
> Too many blank lines, please use just one.
>
> > + if (!tag) {
> > + get_prog_full_name(&info, fd, prog_name, sizeof(prog_name));
> > + if (strcmp(nametag, prog_name)) {
>
> strncmp(), please
>
Both are NULL terminated, but I am happy to add the extra safeguard.
> > + close(fd);
> > + continue;
> > + }
> > + }
> > +
> > if (nb_fds > 0) {
> > tmp = realloc(*fds, (nb_fds + 1) * sizeof(int));
> > if (!tmp) {
> > @@ -820,10 +830,6 @@ int prog_parse_fds(int *argc, char ***argv, int **fds)
> > NEXT_ARGP();
> >
> > name = **argv;
> > - if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
> > - p_err("can't parse name");
> > - return -1;
> > - }
>
> Why removing the check? Just update the bound to MAX_PROG_FULL_NAME - 1?
>
> > NEXT_ARGP();
> >
> > return prog_fd_by_nametag(name, fds, false);
>
Make sense. Will do.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-29 18:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-29 6:18 [PATCH bpf-next] remove BPF_OBJ_NAME_LEN restriction when looking up bpf program by name Manu Bretelle
2022-07-29 9:53 ` Quentin Monnet
2022-07-29 18:54 ` Manu Bretelle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).