* [PATCH bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno
@ 2022-12-03 9:37 Xin Liu
2022-12-05 21:10 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Xin Liu @ 2022-12-03 9:37 UTC (permalink / raw)
To: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
Cc: bpf, linux-kernel, yanan, wuchangye, xiesongyang, kongweibin2,
liuxin350, zhangmingyi5
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);
+
return 0;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno
2022-12-03 9:37 [PATCH bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno Xin Liu
@ 2022-12-05 21:10 ` Daniel Borkmann
2022-12-07 0:00 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2022-12-05 21:10 UTC (permalink / raw)
To: Xin Liu, andrii, ast, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
Cc: bpf, linux-kernel, yanan, wuchangye, xiesongyang, kongweibin2,
zhangmingyi5
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;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno
2022-12-05 21:10 ` Daniel Borkmann
@ 2022-12-07 0:00 ` Andrii Nakryiko
2022-12-07 9:09 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2022-12-07 0:00 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Xin Liu, andrii, ast, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, yanan, wuchangye,
xiesongyang, kongweibin2, zhangmingyi5
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;
> > }
> >
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno
2022-12-07 0:00 ` Andrii Nakryiko
@ 2022-12-07 9:09 ` Daniel Borkmann
2022-12-07 18:31 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2022-12-07 9:09 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Xin Liu, andrii, ast, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, yanan, wuchangye,
xiesongyang, kongweibin2, zhangmingyi5
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)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno
2022-12-07 9:09 ` Daniel Borkmann
@ 2022-12-07 18:31 ` Andrii Nakryiko
2022-12-09 8:40 ` Xin Liu
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2022-12-07 18:31 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Xin Liu, andrii, ast, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, yanan, wuchangye,
xiesongyang, kongweibin2, zhangmingyi5
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno
2022-12-07 18:31 ` Andrii Nakryiko
@ 2022-12-09 8:40 ` Xin Liu
0 siblings, 0 replies; 6+ messages in thread
From: Xin Liu @ 2022-12-09 8:40 UTC (permalink / raw)
To: andrii.nakryiko
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa,
kongweibin2, kpsingh, linux-kernel, liuxin350, martin.lau, sdf,
song, wuchangye, xiesongyang, yanan, yhs, zhangmingyi5
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-09 8:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-03 9:37 [PATCH bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno Xin Liu
2022-12-05 21:10 ` Daniel Borkmann
2022-12-07 0:00 ` Andrii Nakryiko
2022-12-07 9:09 ` Daniel Borkmann
2022-12-07 18:31 ` Andrii Nakryiko
2022-12-09 8:40 ` Xin Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.