| Message ID | 20211011155636.2666408-2-sdf@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,1/3] libbpf: use func name when pinning programs with LIBBPF_STRICT_SEC_NAME | expand |
Stanislav Fomichev wrote: > Otherwise, attaching with bpftool doesn't work with strict section names. > > Also, switch to libbpf strict mode to use the latest conventions > (note, I don't think we have any cli api guarantees?). > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- > tools/bpf/bpftool/main.c | 4 ++++ > tools/bpf/bpftool/prog.c | 15 +-------------- > 2 files changed, 5 insertions(+), 14 deletions(-) > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c > index 02eaaf065f65..8223bac1e401 100644 > --- a/tools/bpf/bpftool/main.c > +++ b/tools/bpf/bpftool/main.c > @@ -409,6 +409,10 @@ int main(int argc, char **argv) > block_mount = false; > bin_name = argv[0]; > > + ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL); > + if (ret) > + p_err("failed to enable libbpf strict mode: %d", ret); > + Would it better to just warn? Seems like this shouldn't be fatal from bpftool side? Also this is a potentially breaking change correct? Programs that _did_ work in the unstrict might suddently fail in the strict mode? If this is the case whats the versioning plan? We don't want to leak these type of changes across multiple versions, idealy we have a hard break and bump the version. I didn't catch a cover letter on the series. A small note about versioning and upgrading bpftool would be helpful. > hash_init(prog_table.table); > hash_init(map_table.table); > hash_init(link_table.table); > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index 277d51c4c5d9..17505dc1243e 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -1396,8 +1396,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > while (argc) { > if (is_prefix(*argv, "type")) { > - char *type; > - > NEXT_ARG(); > > if (common_prog_type != BPF_PROG_TYPE_UNSPEC) { > @@ -1407,19 +1405,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > if (!REQ_ARGS(1)) > goto err_free_reuse_maps; > > - /* Put a '/' at the end of type to appease libbpf */ > - type = malloc(strlen(*argv) + 2); > - if (!type) { > - p_err("mem alloc failed"); > - goto err_free_reuse_maps; > - } > - *type = 0; > - strcat(type, *argv); > - strcat(type, "/"); > - > - err = get_prog_type_by_name(type, &common_prog_type, > + err = get_prog_type_by_name(*argv, &common_prog_type, > &expected_attach_type); > - free(type); > if (err < 0) > goto err_free_reuse_maps; This wont potentially break existing programs correct? It looks like just adding a '/' should be fine. Thanks, John
John Fastabend wrote: > Stanislav Fomichev wrote: > > Otherwise, attaching with bpftool doesn't work with strict section names. > > > > Also, switch to libbpf strict mode to use the latest conventions > > (note, I don't think we have any cli api guarantees?). > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > tools/bpf/bpftool/main.c | 4 ++++ > > tools/bpf/bpftool/prog.c | 15 +-------------- > > 2 files changed, 5 insertions(+), 14 deletions(-) > > > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c > > index 02eaaf065f65..8223bac1e401 100644 > > --- a/tools/bpf/bpftool/main.c > > +++ b/tools/bpf/bpftool/main.c > > @@ -409,6 +409,10 @@ int main(int argc, char **argv) > > block_mount = false; > > bin_name = argv[0]; > > > > + ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL); > > + if (ret) > > + p_err("failed to enable libbpf strict mode: %d", ret); > > + > > Would it better to just warn? Seems like this shouldn't be fatal from > bpftool side? > > Also this is a potentially breaking change correct? Programs that _did_ > work in the unstrict might suddently fail in the strict mode? If this > is the case whats the versioning plan? We don't want to leak these > type of changes across multiple versions, idealy we have a hard > break and bump the version. > > I didn't catch a cover letter on the series. A small > note about versioning and upgrading bpftool would be helpful. > > > > hash_init(prog_table.table); > > hash_init(map_table.table); > > hash_init(link_table.table); > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > > index 277d51c4c5d9..17505dc1243e 100644 > > --- a/tools/bpf/bpftool/prog.c > > +++ b/tools/bpf/bpftool/prog.c > > @@ -1396,8 +1396,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > > > while (argc) { > > if (is_prefix(*argv, "type")) { > > - char *type; > > - > > NEXT_ARG(); > > > > if (common_prog_type != BPF_PROG_TYPE_UNSPEC) { > > @@ -1407,19 +1405,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > if (!REQ_ARGS(1)) > > goto err_free_reuse_maps; > > > > - /* Put a '/' at the end of type to appease libbpf */ > > - type = malloc(strlen(*argv) + 2); > > - if (!type) { > > - p_err("mem alloc failed"); > > - goto err_free_reuse_maps; > > - } > > - *type = 0; > > - strcat(type, *argv); > > - strcat(type, "/"); > > - > > - err = get_prog_type_by_name(type, &common_prog_type, > > + err = get_prog_type_by_name(*argv, &common_prog_type, > > &expected_attach_type); > > - free(type); > > if (err < 0) > > goto err_free_reuse_maps; > > This wont potentially break existing programs correct? It looks like > just adding a '/' should be fine. > > Thanks, > John Oops wrong version of the patch. I'll reply in the more recent one.
On Fri, Oct 22, 2021 at 10:05 AM John Fastabend <john.fastabend@gmail.com> wrote: > > Stanislav Fomichev wrote: > > Otherwise, attaching with bpftool doesn't work with strict section names. > > > > Also, switch to libbpf strict mode to use the latest conventions > > (note, I don't think we have any cli api guarantees?). > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > tools/bpf/bpftool/main.c | 4 ++++ > > tools/bpf/bpftool/prog.c | 15 +-------------- > > 2 files changed, 5 insertions(+), 14 deletions(-) > > > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c > > index 02eaaf065f65..8223bac1e401 100644 > > --- a/tools/bpf/bpftool/main.c > > +++ b/tools/bpf/bpftool/main.c > > @@ -409,6 +409,10 @@ int main(int argc, char **argv) > > block_mount = false; > > bin_name = argv[0]; > > > > + ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL); > > + if (ret) > > + p_err("failed to enable libbpf strict mode: %d", ret); > > + > > Would it better to just warn? Seems like this shouldn't be fatal from > bpftool side? > > Also this is a potentially breaking change correct? Programs that _did_ > work in the unstrict might suddently fail in the strict mode? If this > is the case whats the versioning plan? We don't want to leak these > type of changes across multiple versions, idealy we have a hard > break and bump the version. > > I didn't catch a cover letter on the series. A small > note about versioning and upgrading bpftool would be helpful. Yeah, it is a breaking change, every program that has non-strict section names will be rejected. I mentioned that in the bpftool's commit description: Also, switch to libbpf strict mode to use the latest conventions (note, I don't think we have any cli api guarantees?). So I'm actually not sure what's the best way to handle this migration and whether we really provide any cli guarantees to the users. I was always assuming that bpftool is mostly for debugging/introspection, but not sure. As Andrii suggested in another email, I can add a flag to disable this strict mode. Any better ideas? > > hash_init(prog_table.table); > > hash_init(map_table.table); > > hash_init(link_table.table); > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > > index 277d51c4c5d9..17505dc1243e 100644 > > --- a/tools/bpf/bpftool/prog.c > > +++ b/tools/bpf/bpftool/prog.c > > @@ -1396,8 +1396,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > > > while (argc) { > > if (is_prefix(*argv, "type")) { > > - char *type; > > - > > NEXT_ARG(); > > > > if (common_prog_type != BPF_PROG_TYPE_UNSPEC) { > > @@ -1407,19 +1405,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > if (!REQ_ARGS(1)) > > goto err_free_reuse_maps; > > > > - /* Put a '/' at the end of type to appease libbpf */ > > - type = malloc(strlen(*argv) + 2); > > - if (!type) { > > - p_err("mem alloc failed"); > > - goto err_free_reuse_maps; > > - } > > - *type = 0; > > - strcat(type, *argv); > > - strcat(type, "/"); > > - > > - err = get_prog_type_by_name(type, &common_prog_type, > > + err = get_prog_type_by_name(*argv, &common_prog_type, > > &expected_attach_type); > > - free(type); > > if (err < 0) > > goto err_free_reuse_maps; > > This wont potentially break existing programs correct? It looks like > just adding a '/' should be fine. > > Thanks, > John
On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev <sdf@google.com> wrote: > > On Fri, Oct 22, 2021 at 10:05 AM John Fastabend > <john.fastabend@gmail.com> wrote: > > > > Stanislav Fomichev wrote: > > > Otherwise, attaching with bpftool doesn't work with strict section names. > > > > > > Also, switch to libbpf strict mode to use the latest conventions > > > (note, I don't think we have any cli api guarantees?). > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > --- > > > tools/bpf/bpftool/main.c | 4 ++++ > > > tools/bpf/bpftool/prog.c | 15 +-------------- > > > 2 files changed, 5 insertions(+), 14 deletions(-) > > > > > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c > > > index 02eaaf065f65..8223bac1e401 100644 > > > --- a/tools/bpf/bpftool/main.c > > > +++ b/tools/bpf/bpftool/main.c > > > @@ -409,6 +409,10 @@ int main(int argc, char **argv) > > > block_mount = false; > > > bin_name = argv[0]; > > > > > > + ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL); > > > + if (ret) > > > + p_err("failed to enable libbpf strict mode: %d", ret); > > > + > > > > Would it better to just warn? Seems like this shouldn't be fatal from > > bpftool side? > > > > Also this is a potentially breaking change correct? Programs that _did_ > > work in the unstrict might suddently fail in the strict mode? If this > > is the case whats the versioning plan? We don't want to leak these > > type of changes across multiple versions, idealy we have a hard > > break and bump the version. > > > > I didn't catch a cover letter on the series. A small > > note about versioning and upgrading bpftool would be helpful. > > Yeah, it is a breaking change, every program that has non-strict > section names will be rejected. > > I mentioned that in the bpftool's commit description: > Also, switch to libbpf strict mode to use the latest conventions > (note, I don't think we have any cli api guarantees?). > > So I'm actually not sure what's the best way to handle this migration > and whether we really provide any cli guarantees to the users. I was > always assuming that bpftool is mostly for debugging/introspection, > but not sure. > > As Andrii suggested in another email, I can add a flag to disable this > strict mode. Any better ideas? Maybe the other way around for the transition period. Add a --strict flag to turn on libbpf strict mode? This follows libbpf's opt-in approach to breaking change. We can also emit warnings when people are trying to pin programs and mention that they should switch to --strict as in some future version this will be the default. Would that be better for users? > > > > > > > hash_init(prog_table.table); > > > hash_init(map_table.table); > > > hash_init(link_table.table); > > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > > > index 277d51c4c5d9..17505dc1243e 100644 > > > --- a/tools/bpf/bpftool/prog.c > > > +++ b/tools/bpf/bpftool/prog.c > > > @@ -1396,8 +1396,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > > > > > while (argc) { > > > if (is_prefix(*argv, "type")) { > > > - char *type; > > > - > > > NEXT_ARG(); > > > > > > if (common_prog_type != BPF_PROG_TYPE_UNSPEC) { > > > @@ -1407,19 +1405,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > > if (!REQ_ARGS(1)) > > > goto err_free_reuse_maps; > > > > > > - /* Put a '/' at the end of type to appease libbpf */ > > > - type = malloc(strlen(*argv) + 2); > > > - if (!type) { > > > - p_err("mem alloc failed"); > > > - goto err_free_reuse_maps; > > > - } > > > - *type = 0; > > > - strcat(type, *argv); > > > - strcat(type, "/"); > > > - > > > - err = get_prog_type_by_name(type, &common_prog_type, > > > + err = get_prog_type_by_name(*argv, &common_prog_type, > > > &expected_attach_type); > > > - free(type); > > > if (err < 0) > > > goto err_free_reuse_maps; > > > > This wont potentially break existing programs correct? It looks like > > just adding a '/' should be fine. > > > > Thanks, > > John
On Mon, Oct 25, 2021 at 9:27 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > On Fri, Oct 22, 2021 at 10:05 AM John Fastabend > > <john.fastabend@gmail.com> wrote: > > > > > > Stanislav Fomichev wrote: > > > > Otherwise, attaching with bpftool doesn't work with strict section names. > > > > > > > > Also, switch to libbpf strict mode to use the latest conventions > > > > (note, I don't think we have any cli api guarantees?). > > > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > --- > > > > tools/bpf/bpftool/main.c | 4 ++++ > > > > tools/bpf/bpftool/prog.c | 15 +-------------- > > > > 2 files changed, 5 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c > > > > index 02eaaf065f65..8223bac1e401 100644 > > > > --- a/tools/bpf/bpftool/main.c > > > > +++ b/tools/bpf/bpftool/main.c > > > > @@ -409,6 +409,10 @@ int main(int argc, char **argv) > > > > block_mount = false; > > > > bin_name = argv[0]; > > > > > > > > + ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL); > > > > + if (ret) > > > > + p_err("failed to enable libbpf strict mode: %d", ret); > > > > + > > > > > > Would it better to just warn? Seems like this shouldn't be fatal from > > > bpftool side? > > > > > > Also this is a potentially breaking change correct? Programs that _did_ > > > work in the unstrict might suddently fail in the strict mode? If this > > > is the case whats the versioning plan? We don't want to leak these > > > type of changes across multiple versions, idealy we have a hard > > > break and bump the version. > > > > > > I didn't catch a cover letter on the series. A small > > > note about versioning and upgrading bpftool would be helpful. > > > > Yeah, it is a breaking change, every program that has non-strict > > section names will be rejected. > > > > I mentioned that in the bpftool's commit description: > > Also, switch to libbpf strict mode to use the latest conventions > > (note, I don't think we have any cli api guarantees?). > > > > So I'm actually not sure what's the best way to handle this migration > > and whether we really provide any cli guarantees to the users. I was > > always assuming that bpftool is mostly for debugging/introspection, > > but not sure. > > > > As Andrii suggested in another email, I can add a flag to disable this > > strict mode. Any better ideas? > > Maybe the other way around for the transition period. Add a --strict > flag to turn on libbpf strict mode? This follows libbpf's opt-in > approach to breaking change. We can also emit warnings when people are > trying to pin programs and mention that they should switch to --strict > as in some future version this will be the default. Would that be > better for users? Agreed, that sounds better for backwards compatibility. However, I'm not sure when we set that --strict to 'true' by default. The same moment libbpf loses non-strict behavior?
On Tue, Oct 26, 2021 at 8:46 AM Stanislav Fomichev <sdf@google.com> wrote: > > On Mon, Oct 25, 2021 at 9:27 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Mon, Oct 25, 2021 at 8:59 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > On Fri, Oct 22, 2021 at 10:05 AM John Fastabend > > > <john.fastabend@gmail.com> wrote: > > > > > > > > Stanislav Fomichev wrote: > > > > > Otherwise, attaching with bpftool doesn't work with strict section names. > > > > > > > > > > Also, switch to libbpf strict mode to use the latest conventions > > > > > (note, I don't think we have any cli api guarantees?). > > > > > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > > --- > > > > > tools/bpf/bpftool/main.c | 4 ++++ > > > > > tools/bpf/bpftool/prog.c | 15 +-------------- > > > > > 2 files changed, 5 insertions(+), 14 deletions(-) > > > > > > > > > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c > > > > > index 02eaaf065f65..8223bac1e401 100644 > > > > > --- a/tools/bpf/bpftool/main.c > > > > > +++ b/tools/bpf/bpftool/main.c > > > > > @@ -409,6 +409,10 @@ int main(int argc, char **argv) > > > > > block_mount = false; > > > > > bin_name = argv[0]; > > > > > > > > > > + ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL); > > > > > + if (ret) > > > > > + p_err("failed to enable libbpf strict mode: %d", ret); > > > > > + > > > > > > > > Would it better to just warn? Seems like this shouldn't be fatal from > > > > bpftool side? > > > > > > > > Also this is a potentially breaking change correct? Programs that _did_ > > > > work in the unstrict might suddently fail in the strict mode? If this > > > > is the case whats the versioning plan? We don't want to leak these > > > > type of changes across multiple versions, idealy we have a hard > > > > break and bump the version. > > > > > > > > I didn't catch a cover letter on the series. A small > > > > note about versioning and upgrading bpftool would be helpful. > > > > > > Yeah, it is a breaking change, every program that has non-strict > > > section names will be rejected. > > > > > > I mentioned that in the bpftool's commit description: > > > Also, switch to libbpf strict mode to use the latest conventions > > > (note, I don't think we have any cli api guarantees?). > > > > > > So I'm actually not sure what's the best way to handle this migration > > > and whether we really provide any cli guarantees to the users. I was > > > always assuming that bpftool is mostly for debugging/introspection, > > > but not sure. > > > > > > As Andrii suggested in another email, I can add a flag to disable this > > > strict mode. Any better ideas? > > > > Maybe the other way around for the transition period. Add a --strict > > flag to turn on libbpf strict mode? This follows libbpf's opt-in > > approach to breaking change. We can also emit warnings when people are > > trying to pin programs and mention that they should switch to --strict > > as in some future version this will be the default. Would that be > > better for users? > > Agreed, that sounds better for backwards compatibility. However, I'm > not sure when we set that --strict to 'true' by default. The same > moment libbpf loses non-strict behavior? Yep, probably it will have to coincide with libbpf 1.0 release. And it's not setting it to true, it's more like enforcing it to true (or just dropping the --strict flag altogether).
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c index 02eaaf065f65..8223bac1e401 100644 --- a/tools/bpf/bpftool/main.c +++ b/tools/bpf/bpftool/main.c @@ -409,6 +409,10 @@ int main(int argc, char **argv) block_mount = false; bin_name = argv[0]; + ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL); + if (ret) + p_err("failed to enable libbpf strict mode: %d", ret); + hash_init(prog_table.table); hash_init(map_table.table); hash_init(link_table.table); diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index 277d51c4c5d9..17505dc1243e 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -1396,8 +1396,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) while (argc) { if (is_prefix(*argv, "type")) { - char *type; - NEXT_ARG(); if (common_prog_type != BPF_PROG_TYPE_UNSPEC) { @@ -1407,19 +1405,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) if (!REQ_ARGS(1)) goto err_free_reuse_maps; - /* Put a '/' at the end of type to appease libbpf */ - type = malloc(strlen(*argv) + 2); - if (!type) { - p_err("mem alloc failed"); - goto err_free_reuse_maps; - } - *type = 0; - strcat(type, *argv); - strcat(type, "/"); - - err = get_prog_type_by_name(type, &common_prog_type, + err = get_prog_type_by_name(*argv, &common_prog_type, &expected_attach_type); - free(type); if (err < 0) goto err_free_reuse_maps;
Otherwise, attaching with bpftool doesn't work with strict section names. Also, switch to libbpf strict mode to use the latest conventions (note, I don't think we have any cli api guarantees?). Signed-off-by: Stanislav Fomichev <sdf@google.com> --- tools/bpf/bpftool/main.c | 4 ++++ tools/bpf/bpftool/prog.c | 15 +-------------- 2 files changed, 5 insertions(+), 14 deletions(-)