diff mbox series

[bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno

Message ID 20221203093740.218935-1-liuxin350@huawei.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_progs_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-37 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for test_verifier on x86_64 with llvm-16

Commit Message

Xin Liu Dec. 3, 2022, 9:37 a.m. UTC
This is a small improvement in libbpf_strerror. When libbpf_strerror
is used to obtain the system error description, if the length of the
buf is insufficient, libbpf_sterror returns ERANGE and sets errno to
ERANGE.

However, this processing is not performed when the error code
customized by libbpf is obtained. Make some minor improvements here,
return -ERANGE and set errno to ERANGE when buf is not enough for
custom description.

Signed-off-by: Xin Liu <liuxin350@huawei.com>
---
 tools/lib/bpf/libbpf_errno.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Daniel Borkmann Dec. 5, 2022, 9:10 p.m. UTC | #1
On 12/3/22 10:37 AM, Xin Liu wrote:
> This is a small improvement in libbpf_strerror. When libbpf_strerror
> is used to obtain the system error description, if the length of the
> buf is insufficient, libbpf_sterror returns ERANGE and sets errno to
> ERANGE.
> 
> However, this processing is not performed when the error code
> customized by libbpf is obtained. Make some minor improvements here,
> return -ERANGE and set errno to ERANGE when buf is not enough for
> custom description.
> 
> Signed-off-by: Xin Liu <liuxin350@huawei.com>
> ---
>   tools/lib/bpf/libbpf_errno.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c
> index 96f67a772a1b..48ce7d5b5bf9 100644
> --- a/tools/lib/bpf/libbpf_errno.c
> +++ b/tools/lib/bpf/libbpf_errno.c
> @@ -54,10 +54,16 @@ int libbpf_strerror(int err, char *buf, size_t size)
>   
>   	if (err < __LIBBPF_ERRNO__END) {
>   		const char *msg;
> +		size_t msg_size;
>   
>   		msg = libbpf_strerror_table[ERRNO_OFFSET(err)];
>   		snprintf(buf, size, "%s", msg);
>   		buf[size - 1] = '\0';
> +
> +		msg_size = strlen(msg);
> +		if (msg_size >= size)
> +			return libbpf_err(-ERANGE);

Given this is related to libbpf_strerror_table[] where the error strings are known
lets do compile-time error instead. All callers should pass in a buffer of STRERR_BUFSIZE
size in libbpf.

>   		return 0;
>   	}
>   
>
Andrii Nakryiko Dec. 7, 2022, midnight UTC | #2
On Mon, Dec 5, 2022 at 1:11 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 12/3/22 10:37 AM, Xin Liu wrote:
> > This is a small improvement in libbpf_strerror. When libbpf_strerror
> > is used to obtain the system error description, if the length of the
> > buf is insufficient, libbpf_sterror returns ERANGE and sets errno to
> > ERANGE.
> >
> > However, this processing is not performed when the error code
> > customized by libbpf is obtained. Make some minor improvements here,
> > return -ERANGE and set errno to ERANGE when buf is not enough for
> > custom description.
> >
> > Signed-off-by: Xin Liu <liuxin350@huawei.com>
> > ---
> >   tools/lib/bpf/libbpf_errno.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c
> > index 96f67a772a1b..48ce7d5b5bf9 100644
> > --- a/tools/lib/bpf/libbpf_errno.c
> > +++ b/tools/lib/bpf/libbpf_errno.c
> > @@ -54,10 +54,16 @@ int libbpf_strerror(int err, char *buf, size_t size)
> >
> >       if (err < __LIBBPF_ERRNO__END) {
> >               const char *msg;
> > +             size_t msg_size;
> >
> >               msg = libbpf_strerror_table[ERRNO_OFFSET(err)];
> >               snprintf(buf, size, "%s", msg);
> >               buf[size - 1] = '\0';
> > +
> > +             msg_size = strlen(msg);
> > +             if (msg_size >= size)
> > +                     return libbpf_err(-ERANGE);
>
> Given this is related to libbpf_strerror_table[] where the error strings are known
> lets do compile-time error instead. All callers should pass in a buffer of STRERR_BUFSIZE
> size in libbpf.

That sounds a bit too pessimistic?.. If the actual error message fits
in the buffer, why return -ERANGE just because theoretically some
error descriptions might fit?

But I don't think we need to calculate strlen(). snprintf above
returns the number of bytes required to print a full string, even if
it was truncated. So just comparing snprintf's result to size should
be enough.

>
> >               return 0;
> >       }
> >
> >
>
Daniel Borkmann Dec. 7, 2022, 9:09 a.m. UTC | #3
On 12/7/22 1:00 AM, Andrii Nakryiko wrote:
> On Mon, Dec 5, 2022 at 1:11 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> On 12/3/22 10:37 AM, Xin Liu wrote:
>>> This is a small improvement in libbpf_strerror. When libbpf_strerror
>>> is used to obtain the system error description, if the length of the
>>> buf is insufficient, libbpf_sterror returns ERANGE and sets errno to
>>> ERANGE.
>>>
>>> However, this processing is not performed when the error code
>>> customized by libbpf is obtained. Make some minor improvements here,
>>> return -ERANGE and set errno to ERANGE when buf is not enough for
>>> custom description.
>>>
>>> Signed-off-by: Xin Liu <liuxin350@huawei.com>
>>> ---
>>>    tools/lib/bpf/libbpf_errno.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c
>>> index 96f67a772a1b..48ce7d5b5bf9 100644
>>> --- a/tools/lib/bpf/libbpf_errno.c
>>> +++ b/tools/lib/bpf/libbpf_errno.c
>>> @@ -54,10 +54,16 @@ int libbpf_strerror(int err, char *buf, size_t size)
>>>
>>>        if (err < __LIBBPF_ERRNO__END) {
>>>                const char *msg;
>>> +             size_t msg_size;
>>>
>>>                msg = libbpf_strerror_table[ERRNO_OFFSET(err)];
>>>                snprintf(buf, size, "%s", msg);
>>>                buf[size - 1] = '\0';
>>> +
>>> +             msg_size = strlen(msg);
>>> +             if (msg_size >= size)
>>> +                     return libbpf_err(-ERANGE);
>>
>> Given this is related to libbpf_strerror_table[] where the error strings are known
>> lets do compile-time error instead. All callers should pass in a buffer of STRERR_BUFSIZE
>> size in libbpf.
> 
> That sounds a bit too pessimistic?.. If the actual error message fits
> in the buffer, why return -ERANGE just because theoretically some
> error descriptions might fit?
> 
> But I don't think we need to calculate strlen(). snprintf above
> returns the number of bytes required to print a full string, even if
> it was truncated. So just comparing snprintf's result to size should
> be enough.

I meant sth like below. For example if we were to shrink STRERR_BUFSIZE down
to 32 for testing, you'd then get:

# make libbpf_errno.o
gcc -g -O2 -std=gnu89 -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls -Wstrict-prototypes -Wswitch-default -Wswitch-enum -Wundef -Wwrite-strings -Wformat -Wno-type-limits -Wstrict-aliasing=3 -Wshadow -Wno-switch-enum -Werror -Wall -I. -I/home/darkstar/trees/bpf-next/tools/include -I/home/darkstar/trees/bpf-next/tools/include/uapi -fvisibility=hidden -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64    -c -o libbpf_errno.o libbpf_errno.c
libbpf_errno.c:27:31: error: initializer-string for array of chars is too long [-Werror]
    27 |  [ERRCODE_OFFSET(KVERSION)] = "'version' section incorrect or lost",
       |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libbpf_errno.c:27:31: note: (near initialization for ‘libbpf_strerror_table[2]’)
libbpf_errno.c:31:29: error: initializer-string for array of chars is too long [-Werror]
    31 |  [ERRCODE_OFFSET(VERIFY)] = "Kernel verifier blocks program loading",
       |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libbpf_errno.c:31:29: note: (near initialization for ‘libbpf_strerror_table[7]’)
libbpf_errno.c:34:31: error: initializer-string for array of chars is too long [-Werror]
    34 |  [ERRCODE_OFFSET(PROGTYPE)] = "Kernel doesn't support this program type",
       |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libbpf_errno.c:34:31: note: (near initialization for ‘libbpf_strerror_table[10]’)
libbpf_errno.c:37:30: error: initializer-string for array of chars is too long [-Werror]
    37 |  [ERRCODE_OFFSET(NLPARSE)] = "Incorrect netlink message parsing",
       |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libbpf_errno.c:37:30: note: (near initialization for ‘libbpf_strerror_table[13]’)
cc1: all warnings being treated as errors
make: *** [<builtin>: libbpf_errno.o] Error 1



diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 2a82f49ce16f..2e5df1624f79 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -265,8 +265,6 @@ static void pr_perm_msg(int err)
  		buf);
  }

-#define STRERR_BUFSIZE  128
-
  /* Copied from tools/perf/util/util.h */
  #ifndef zfree
  # define zfree(ptr) ({ free(*ptr); *ptr = NULL; })
diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c
index 96f67a772a1b..2f03f861b8b6 100644
--- a/tools/lib/bpf/libbpf_errno.c
+++ b/tools/lib/bpf/libbpf_errno.c
@@ -21,7 +21,7 @@
  #define ERRCODE_OFFSET(c)	ERRNO_OFFSET(LIBBPF_ERRNO__##c)
  #define NR_ERRNO	(__LIBBPF_ERRNO__END - __LIBBPF_ERRNO__START)

-static const char *libbpf_strerror_table[NR_ERRNO] = {
+static const char libbpf_strerror_table[NR_ERRNO][STRERR_BUFSIZE] = {
  	[ERRCODE_OFFSET(LIBELF)]	= "Something wrong in libelf",
  	[ERRCODE_OFFSET(FORMAT)]	= "BPF object format invalid",
  	[ERRCODE_OFFSET(KVERSION)]	= "'version' section incorrect or lost",
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 377642ff51fc..d4dc4fe945a6 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -57,6 +57,8 @@
  #define ELF64_ST_VISIBILITY(o) ((o) & 0x03)
  #endif

+#define STRERR_BUFSIZE	128
+
  #define BTF_INFO_ENC(kind, kind_flag, vlen) \
  	((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN))
  #define BTF_TYPE_ENC(name, info, size_or_type) (name), (info), (size_or_type)
Andrii Nakryiko Dec. 7, 2022, 6:31 p.m. UTC | #4
On Wed, Dec 7, 2022 at 1:09 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 12/7/22 1:00 AM, Andrii Nakryiko wrote:
> > On Mon, Dec 5, 2022 at 1:11 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>
> >> On 12/3/22 10:37 AM, Xin Liu wrote:
> >>> This is a small improvement in libbpf_strerror. When libbpf_strerror
> >>> is used to obtain the system error description, if the length of the
> >>> buf is insufficient, libbpf_sterror returns ERANGE and sets errno to
> >>> ERANGE.
> >>>
> >>> However, this processing is not performed when the error code
> >>> customized by libbpf is obtained. Make some minor improvements here,
> >>> return -ERANGE and set errno to ERANGE when buf is not enough for
> >>> custom description.
> >>>
> >>> Signed-off-by: Xin Liu <liuxin350@huawei.com>
> >>> ---
> >>>    tools/lib/bpf/libbpf_errno.c | 6 ++++++
> >>>    1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c
> >>> index 96f67a772a1b..48ce7d5b5bf9 100644
> >>> --- a/tools/lib/bpf/libbpf_errno.c
> >>> +++ b/tools/lib/bpf/libbpf_errno.c
> >>> @@ -54,10 +54,16 @@ int libbpf_strerror(int err, char *buf, size_t size)
> >>>
> >>>        if (err < __LIBBPF_ERRNO__END) {
> >>>                const char *msg;
> >>> +             size_t msg_size;
> >>>
> >>>                msg = libbpf_strerror_table[ERRNO_OFFSET(err)];
> >>>                snprintf(buf, size, "%s", msg);
> >>>                buf[size - 1] = '\0';
> >>> +
> >>> +             msg_size = strlen(msg);
> >>> +             if (msg_size >= size)
> >>> +                     return libbpf_err(-ERANGE);
> >>
> >> Given this is related to libbpf_strerror_table[] where the error strings are known
> >> lets do compile-time error instead. All callers should pass in a buffer of STRERR_BUFSIZE
> >> size in libbpf.
> >
> > That sounds a bit too pessimistic?.. If the actual error message fits
> > in the buffer, why return -ERANGE just because theoretically some
> > error descriptions might fit?
> >
> > But I don't think we need to calculate strlen(). snprintf above
> > returns the number of bytes required to print a full string, even if
> > it was truncated. So just comparing snprintf's result to size should
> > be enough.
>
> I meant sth like below. For example if we were to shrink STRERR_BUFSIZE down
> to 32 for testing, you'd then get:

Sure, but I'm not sure why do we need to do this? Array of pointers to
string will overall use less memory, as each string will take as much
space as needed and no more.

I guess I'm missing which problem you are trying to solve. I believe
Xin was addressing a concern of extern (to libbpf) callers not getting
-ERANGE in cases when they provide too small a buffer, which is just a
simple snprintf() use adjustment to be a proper fix.

>
> # make libbpf_errno.o
> gcc -g -O2 -std=gnu89 -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls -Wstrict-prototypes -Wswitch-default -Wswitch-enum -Wundef -Wwrite-strings -Wformat -Wno-type-limits -Wstrict-aliasing=3 -Wshadow -Wno-switch-enum -Werror -Wall -I. -I/home/darkstar/trees/bpf-next/tools/include -I/home/darkstar/trees/bpf-next/tools/include/uapi -fvisibility=hidden -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64    -c -o libbpf_errno.o libbpf_errno.c
> libbpf_errno.c:27:31: error: initializer-string for array of chars is too long [-Werror]
>     27 |  [ERRCODE_OFFSET(KVERSION)] = "'version' section incorrect or lost",
>        |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> libbpf_errno.c:27:31: note: (near initialization for ‘libbpf_strerror_table[2]’)
> libbpf_errno.c:31:29: error: initializer-string for array of chars is too long [-Werror]
>     31 |  [ERRCODE_OFFSET(VERIFY)] = "Kernel verifier blocks program loading",
>        |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> libbpf_errno.c:31:29: note: (near initialization for ‘libbpf_strerror_table[7]’)
> libbpf_errno.c:34:31: error: initializer-string for array of chars is too long [-Werror]
>     34 |  [ERRCODE_OFFSET(PROGTYPE)] = "Kernel doesn't support this program type",
>        |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> libbpf_errno.c:34:31: note: (near initialization for ‘libbpf_strerror_table[10]’)
> libbpf_errno.c:37:30: error: initializer-string for array of chars is too long [-Werror]
>     37 |  [ERRCODE_OFFSET(NLPARSE)] = "Incorrect netlink message parsing",
>        |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> libbpf_errno.c:37:30: note: (near initialization for ‘libbpf_strerror_table[13]’)
> cc1: all warnings being treated as errors
> make: *** [<builtin>: libbpf_errno.o] Error 1
>
>
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 2a82f49ce16f..2e5df1624f79 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -265,8 +265,6 @@ static void pr_perm_msg(int err)
>                 buf);
>   }
>
> -#define STRERR_BUFSIZE  128
> -
>   /* Copied from tools/perf/util/util.h */
>   #ifndef zfree
>   # define zfree(ptr) ({ free(*ptr); *ptr = NULL; })
> diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c
> index 96f67a772a1b..2f03f861b8b6 100644
> --- a/tools/lib/bpf/libbpf_errno.c
> +++ b/tools/lib/bpf/libbpf_errno.c
> @@ -21,7 +21,7 @@
>   #define ERRCODE_OFFSET(c)     ERRNO_OFFSET(LIBBPF_ERRNO__##c)
>   #define NR_ERRNO      (__LIBBPF_ERRNO__END - __LIBBPF_ERRNO__START)
>
> -static const char *libbpf_strerror_table[NR_ERRNO] = {
> +static const char libbpf_strerror_table[NR_ERRNO][STRERR_BUFSIZE] = {
>         [ERRCODE_OFFSET(LIBELF)]        = "Something wrong in libelf",
>         [ERRCODE_OFFSET(FORMAT)]        = "BPF object format invalid",
>         [ERRCODE_OFFSET(KVERSION)]      = "'version' section incorrect or lost",
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 377642ff51fc..d4dc4fe945a6 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -57,6 +57,8 @@
>   #define ELF64_ST_VISIBILITY(o) ((o) & 0x03)
>   #endif
>
> +#define STRERR_BUFSIZE 128
> +
>   #define BTF_INFO_ENC(kind, kind_flag, vlen) \
>         ((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN))
>   #define BTF_TYPE_ENC(name, info, size_or_type) (name), (info), (size_or_type)
Xin Liu Dec. 9, 2022, 8:40 a.m. UTC | #5
On Wed,  7 Dec 2022 6:31:34 PM Andrii Nakryiko wrote:
> On Wed, Dec 7, 2022 at 1:09 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 12/7/22 1:00 AM, Andrii Nakryiko wrote:
> > > On Mon, Dec 5, 2022 at 1:11 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >>
> > >> On 12/3/22 10:37 AM, Xin Liu wrote:
> > >>> This is a small improvement in libbpf_strerror. When libbpf_strerror
> > >>> is used to obtain the system error description, if the length of the
> > >>> buf is insufficient, libbpf_sterror returns ERANGE and sets errno to
> > >>> ERANGE.
> > >>>
> > >>> However, this processing is not performed when the error code
> > >>> customized by libbpf is obtained. Make some minor improvements here,
> > >>> return -ERANGE and set errno to ERANGE when buf is not enough for
> > >>> custom description.
> > >>>
> > >>> Signed-off-by: Xin Liu <liuxin350@huawei.com>
> > >>> ---
> > >>>    tools/lib/bpf/libbpf_errno.c | 6 ++++++
> > >>>    1 file changed, 6 insertions(+)
> > >>>
> > >>> diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c
> > >>> index 96f67a772a1b..48ce7d5b5bf9 100644
> > >>> --- a/tools/lib/bpf/libbpf_errno.c
> > >>> +++ b/tools/lib/bpf/libbpf_errno.c
> > >>> @@ -54,10 +54,16 @@ int libbpf_strerror(int err, char *buf, size_t size)
> > >>>
> > >>>        if (err < __LIBBPF_ERRNO__END) {
> > >>>                const char *msg;
> > >>> +             size_t msg_size;
> > >>>
> > >>>                msg = libbpf_strerror_table[ERRNO_OFFSET(err)];
> > >>>                snprintf(buf, size, "%s", msg);
> > >>>                buf[size - 1] = '\0';
> > >>> +
> > >>> +             msg_size = strlen(msg);
> > >>> +             if (msg_size >= size)
> > >>> +                     return libbpf_err(-ERANGE);
> > >>
> > >> Given this is related to libbpf_strerror_table[] where the error strings are known
> > >> lets do compile-time error instead. All callers should pass in a buffer of STRERR_BUFSIZE
> > >> size in libbpf.
> > >
> > > That sounds a bit too pessimistic?.. If the actual error message fits
> > > in the buffer, why return -ERANGE just because theoretically some
> > > error descriptions might fit?
> > >
> > > But I don't think we need to calculate strlen(). snprintf above
> > > returns the number of bytes required to print a full string, even if
> > > it was truncated. So just comparing snprintf's result to size should
> > > be enough.
> >
> > I meant sth like below. For example if we were to shrink STRERR_BUFSIZE down
> > to 32 for testing, you'd then get:
> 
> Sure, but I'm not sure why do we need to do this? Array of pointers to
> string will overall use less memory, as each string will take as much
> space as needed and no more.
> 
> I guess I'm missing which problem you are trying to solve. I believe
> Xin was addressing a concern of extern (to libbpf) callers not getting
> -ERANGE in cases when they provide too small a buffer, which is just a
> simple snprintf() use adjustment to be a proper fix.
> 
> >
> > # make libbpf_errno.o
> > gcc -g -O2 -std=gnu89 -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls -Wstrict-prototypes -Wswitch-default -Wswitch-enum -Wundef -Wwrite-strings -Wformat -Wno-type-limits -Wstrict-aliasing=3 -Wshadow -Wno-switch-enum -Werror -Wall -I. -I/home/darkstar/trees/bpf-next/tools/include -I/home/darkstar/trees/bpf-next/tools/include/uapi -fvisibility=hidden -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64    -c -o libbpf_errno.o libbpf_errno.c
> > libbpf_errno.c:27:31: error: initializer-string for array of chars is too long [-Werror]
> >     27 |  [ERRCODE_OFFSET(KVERSION)] = "'version' section incorrect or lost",
> >        |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > libbpf_errno.c:27:31: note: (near initialization for ‘libbpf_strerror_table[2]’)
> > libbpf_errno.c:31:29: error: initializer-string for array of chars is too long [-Werror]
> >     31 |  [ERRCODE_OFFSET(VERIFY)] = "Kernel verifier blocks program loading",
> >        |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > libbpf_errno.c:31:29: note: (near initialization for ‘libbpf_strerror_table[7]’)
> > libbpf_errno.c:34:31: error: initializer-string for array of chars is too long [-Werror]
> >     34 |  [ERRCODE_OFFSET(PROGTYPE)] = "Kernel doesn't support this program type",
> >        |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > libbpf_errno.c:34:31: note: (near initialization for ‘libbpf_strerror_table[10]’)
> > libbpf_errno.c:37:30: error: initializer-string for array of chars is too long [-Werror]
> >     37 |  [ERRCODE_OFFSET(NLPARSE)] = "Incorrect netlink message parsing",
> >        |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > libbpf_errno.c:37:30: note: (near initialization for ‘libbpf_strerror_table[13]’)
> > cc1: all warnings being treated as errors
> > make: *** [<builtin>: libbpf_errno.o] Error 1
> >
> >
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 2a82f49ce16f..2e5df1624f79 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -265,8 +265,6 @@ static void pr_perm_msg(int err)
> >                 buf);
> >   }
> >
> > -#define STRERR_BUFSIZE  128
> > -
> >   /* Copied from tools/perf/util/util.h */
> >   #ifndef zfree
> >   # define zfree(ptr) ({ free(*ptr); *ptr = NULL; })
> > diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c
> > index 96f67a772a1b..2f03f861b8b6 100644
> > --- a/tools/lib/bpf/libbpf_errno.c
> > +++ b/tools/lib/bpf/libbpf_errno.c
> > @@ -21,7 +21,7 @@
> >   #define ERRCODE_OFFSET(c)     ERRNO_OFFSET(LIBBPF_ERRNO__##c)
> >   #define NR_ERRNO      (__LIBBPF_ERRNO__END - __LIBBPF_ERRNO__START)
> >
> > -static const char *libbpf_strerror_table[NR_ERRNO] = {
> > +static const char libbpf_strerror_table[NR_ERRNO][STRERR_BUFSIZE] = {
> >         [ERRCODE_OFFSET(LIBELF)]        = "Something wrong in libelf",
> >         [ERRCODE_OFFSET(FORMAT)]        = "BPF object format invalid",
> >         [ERRCODE_OFFSET(KVERSION)]      = "'version' section incorrect or lost",
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index 377642ff51fc..d4dc4fe945a6 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -57,6 +57,8 @@
> >   #define ELF64_ST_VISIBILITY(o) ((o) & 0x03)
> >   #endif
> >
> > +#define STRERR_BUFSIZE 128
> > +
> >   #define BTF_INFO_ENC(kind, kind_flag, vlen) \
> >         ((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN))
> >   #define BTF_TYPE_ENC(name, info, size_or_type) (name), (info), (size_or_type)

I also think it would be more appropriate to modify snprintf slightly
here to keep memory usage as low as possible, and I will submit an
optimized version of v2, thanks to Daniel and Andrii for their
suggestions.
diff mbox series

Patch

diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c
index 96f67a772a1b..48ce7d5b5bf9 100644
--- a/tools/lib/bpf/libbpf_errno.c
+++ b/tools/lib/bpf/libbpf_errno.c
@@ -54,10 +54,16 @@  int libbpf_strerror(int err, char *buf, size_t size)
 
 	if (err < __LIBBPF_ERRNO__END) {
 		const char *msg;
+		size_t msg_size;
 
 		msg = libbpf_strerror_table[ERRNO_OFFSET(err)];
 		snprintf(buf, size, "%s", msg);
 		buf[size - 1] = '\0';
+
+		msg_size = strlen(msg);
+		if (msg_size >= size)
+			return libbpf_err(-ERANGE);
+
 		return 0;
 	}