[PATCH iproute2] bpf: initialise map symbol before retrieving and comparing its type

[PATCH iproute2] bpf: initialise map symbol before retrieving and comparing its type

[Quentin Monnet]

In order to compare BPF map symbol type correctly in regard to the
latest LLVM, commit 7a04dd84a7f9 ("bpf: check map symbol type properly
with newer llvm compiler") compares map symbol type to both NOTYPE and
OBJECT. To do so, it first retrieves the type from "sym.st_info" and
stores it into a temporary variable.

However, the type is collected from the symbol "sym" before this latter
symbol is actually updated. gelf_getsym() is called after that and
updates "sym", and when comparison with OBJECT or NOTYPE happens it is
done on the type of the symbol collected in the previous passage of the
loop (or on an uninitialised symbol on the first passage). This may
eventually break map collection from the ELF file.

Fix this by assigning the type to the temporary variable only after the
call to gelf_getsym().

Fixes: 7a04dd84a7f9 ("bpf: check map symbol type properly with newer llvm compiler")
Reported-by: Ron Philip <ron.philip@netronome.com>
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jiong Wang <jiong.wang@netronome.com>
---
 lib/bpf.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/bpf.c b/lib/bpf.c
index 45f279fa4a41..6aff8f7bad7f 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -1758,11 +1758,12 @@ static const char *bpf_map_fetch_name(struct bpf_elf_ctx *ctx, int which)
 	int i;
 
 	for (i = 0; i < ctx->sym_num; i++) {
-		int type = GELF_ST_TYPE(sym.st_info);
+		int type;
 
 		if (gelf_getsym(ctx->sym_tab, i, &sym) != &sym)
 			continue;
 
+		type = GELF_ST_TYPE(sym.st_info);
 		if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL ||
 		    (type != STT_NOTYPE && type != STT_OBJECT) ||
 		    sym.st_shndx != ctx->sec_maps ||
@@ -1851,11 +1852,12 @@ static int bpf_map_num_sym(struct bpf_elf_ctx *ctx)
 	GElf_Sym sym;
 
 	for (i = 0; i < ctx->sym_num; i++) {
-		int type = GELF_ST_TYPE(sym.st_info);
+		int type;
 
 		if (gelf_getsym(ctx->sym_tab, i, &sym) != &sym)
 			continue;
 
+		type = GELF_ST_TYPE(sym.st_info);
 		if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL ||
 		    (type != STT_NOTYPE && type != STT_OBJECT) ||
 		    sym.st_shndx != ctx->sec_maps)
@@ -1931,10 +1933,12 @@ static int bpf_map_verify_all_offs(struct bpf_elf_ctx *ctx, int end)
 		 * the table again.
 		 */
 		for (i = 0; i < ctx->sym_num; i++) {
-			int type = GELF_ST_TYPE(sym.st_info);
+			int type;
 
 			if (gelf_getsym(ctx->sym_tab, i, &sym) != &sym)
 				continue;
+
+			type = GELF_ST_TYPE(sym.st_info);
 			if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL ||
 			    (type != STT_NOTYPE && type != STT_OBJECT) ||
 			    sym.st_shndx != ctx->sec_maps)
-- 
2.7.4

Re: [PATCH iproute2] bpf: initialise map symbol before retrieving and comparing its type

[Y Song]

On Mon, Nov 19, 2018 at 5:28 PM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> In order to compare BPF map symbol type correctly in regard to the
> latest LLVM, commit 7a04dd84a7f9 ("bpf: check map symbol type properly
> with newer llvm compiler") compares map symbol type to both NOTYPE and
> OBJECT. To do so, it first retrieves the type from "sym.st_info" and
> stores it into a temporary variable.
>
> However, the type is collected from the symbol "sym" before this latter
> symbol is actually updated. gelf_getsym() is called after that and
> updates "sym", and when comparison with OBJECT or NOTYPE happens it is
> done on the type of the symbol collected in the previous passage of the
> loop (or on an uninitialised symbol on the first passage). This may
> eventually break map collection from the ELF file.
>
> Fix this by assigning the type to the temporary variable only after the
> call to gelf_getsym().
>
> Fixes: 7a04dd84a7f9 ("bpf: check map symbol type properly with newer llvm compiler")
> Reported-by: Ron Philip <ron.philip@netronome.com>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jiong Wang <jiong.wang@netronome.com>

Thanks for the fix!
Acked-by: Yonghong Song <yhs@fb.com>

> ---
>  lib/bpf.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 45f279fa4a41..6aff8f7bad7f 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -1758,11 +1758,12 @@ static const char *bpf_map_fetch_name(struct bpf_elf_ctx *ctx, int which)
>         int i;
>
>         for (i = 0; i < ctx->sym_num; i++) {
> -               int type = GELF_ST_TYPE(sym.st_info);
> +               int type;
>
>                 if (gelf_getsym(ctx->sym_tab, i, &sym) != &sym)
>                         continue;
>
> +               type = GELF_ST_TYPE(sym.st_info);
>                 if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL ||
>                     (type != STT_NOTYPE && type != STT_OBJECT) ||
>                     sym.st_shndx != ctx->sec_maps ||
> @@ -1851,11 +1852,12 @@ static int bpf_map_num_sym(struct bpf_elf_ctx *ctx)
>         GElf_Sym sym;
>
>         for (i = 0; i < ctx->sym_num; i++) {
> -               int type = GELF_ST_TYPE(sym.st_info);
> +               int type;
>
>                 if (gelf_getsym(ctx->sym_tab, i, &sym) != &sym)
>                         continue;
>
> +               type = GELF_ST_TYPE(sym.st_info);
>                 if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL ||
>                     (type != STT_NOTYPE && type != STT_OBJECT) ||
>                     sym.st_shndx != ctx->sec_maps)
> @@ -1931,10 +1933,12 @@ static int bpf_map_verify_all_offs(struct bpf_elf_ctx *ctx, int end)
>                  * the table again.
>                  */
>                 for (i = 0; i < ctx->sym_num; i++) {
> -                       int type = GELF_ST_TYPE(sym.st_info);
> +                       int type;
>
>                         if (gelf_getsym(ctx->sym_tab, i, &sym) != &sym)
>                                 continue;
> +
> +                       type = GELF_ST_TYPE(sym.st_info);
>                         if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL ||
>                             (type != STT_NOTYPE && type != STT_OBJECT) ||
>                             sym.st_shndx != ctx->sec_maps)
> --
> 2.7.4
>

Re: [PATCH iproute2] bpf: initialise map symbol before retrieving and comparing its type

[Stephen Hemminger]

On Tue, 20 Nov 2018 01:26:27 +0000
Quentin Monnet <quentin.monnet@netronome.com> wrote:

> In order to compare BPF map symbol type correctly in regard to the
> latest LLVM, commit 7a04dd84a7f9 ("bpf: check map symbol type properly
> with newer llvm compiler") compares map symbol type to both NOTYPE and
> OBJECT. To do so, it first retrieves the type from "sym.st_info" and
> stores it into a temporary variable.
> 
> However, the type is collected from the symbol "sym" before this latter
> symbol is actually updated. gelf_getsym() is called after that and
> updates "sym", and when comparison with OBJECT or NOTYPE happens it is
> done on the type of the symbol collected in the previous passage of the
> loop (or on an uninitialised symbol on the first passage). This may
> eventually break map collection from the ELF file.
> 
> Fix this by assigning the type to the temporary variable only after the
> call to gelf_getsym().
> 
> Fixes: 7a04dd84a7f9 ("bpf: check map symbol type properly with newer llvm compiler")
> Reported-by: Ron Philip <ron.philip@netronome.com>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jiong Wang <jiong.wang@netronome.com>
> ---
>  lib/bpf.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 


Applied.