[bpf-next,v3,14/15] bpf: allow '?' at the beginning of DATASEC names

Commit Message

Eduard Zingerman March 4, 2024, 10:51 p.m. UTC

Currently kernel does not allow question marks in BTF names.
This commit makes an exception, allowing first character of the
DATASEC name to be a question mark.

The intent is to allow libbpf to use SEC("?.struct_ops") to identify
struct_ops maps that are optional, e.g. like in the following BPF code:

    SEC("?.struct_ops")
    struct test_ops optional_map = { ... };

Which yields the following BTF:

    ...
    [13] DATASEC '?.struct_ops' size=0 vlen=...
    ...

To load such BTF libbpf rewrites DATASEC name before load.
After this patch the rewrite won't be necessary.

Signed-off-by: Eduard Zingerman <[e-mail]>
---
 kernel/bpf/btf.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Andrii Nakryiko March 5, 2024, 9:43 p.m. UTC | #1

On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <[e-mail]> wrote:
>
> Currently kernel does not allow question marks in BTF names.
> This commit makes an exception, allowing first character of the
> DATASEC name to be a question mark.
>
> The intent is to allow libbpf to use SEC("?.struct_ops") to identify
> struct_ops maps that are optional, e.g. like in the following BPF code:
>
>     SEC("?.struct_ops")
>     struct test_ops optional_map = { ... };
>
> Which yields the following BTF:
>
>     ...
>     [13] DATASEC '?.struct_ops' size=0 vlen=...
>     ...
>
> To load such BTF libbpf rewrites DATASEC name before load.
> After this patch the rewrite won't be necessary.
>
> Signed-off-by: Eduard Zingerman <[e-mail]>
> ---
>  kernel/bpf/btf.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 6ff0bd1a91d5..a25fb6bce808 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -761,12 +761,13 @@ static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
>         return offset < btf->hdr.str_len;
>  }
>
> -static bool __btf_name_char_ok(char c, bool first)
> +static bool __btf_name_char_ok(char c, bool first, bool allow_qmark)
>  {
>         if ((first ? !isalpha(c) :
>                      !isalnum(c)) &&
>             c != '_' &&
> -           c != '.')
> +           c != '.' &&
> +           (allow_qmark && first ? c != '?' : true))

ELF data section can have pretty much arbitrary characters in the
name. I don't think we should allow question mark only at the
beginning. Let's just allow any printable character, anywhere (for
DATASEC only, of course)? There is no danger in doing this, data
section name is not a variable name or some C identifier, and so won't
be used like that.

>                 return false;
>         return true;
>  }
> @@ -783,20 +784,20 @@ static const char *btf_str_by_offset(const struct btf *btf, u32 offset)
>         return NULL;
>  }
>
> -static bool __btf_name_valid(const struct btf *btf, u32 offset)
> +static bool __btf_name_valid(const struct btf *btf, u32 offset, bool allow_qmark)
>  {
>         /* offset must be valid */
>         const char *src = btf_str_by_offset(btf, offset);
>         const char *src_limit;
>
> -       if (!__btf_name_char_ok(*src, true))
> +       if (!__btf_name_char_ok(*src, true, allow_qmark))
>                 return false;
>
>         /* set a limit on identifier length */
>         src_limit = src + KSYM_NAME_LEN;
>         src++;
>         while (*src && src < src_limit) {
> -               if (!__btf_name_char_ok(*src, false))
> +               if (!__btf_name_char_ok(*src, false, false))
>                         return false;
>                 src++;
>         }
> @@ -806,12 +807,12 @@ static bool __btf_name_valid(const struct btf *btf, u32 offset)
>
>  static bool btf_name_valid_identifier(const struct btf *btf, u32 offset)
>  {
> -       return __btf_name_valid(btf, offset);
> +       return __btf_name_valid(btf, offset, false);
>  }
>
>  static bool btf_name_valid_section(const struct btf *btf, u32 offset)
>  {
> -       return __btf_name_valid(btf, offset);
> +       return __btf_name_valid(btf, offset, true);
>  }
>
>  static const char *__btf_name_by_offset(const struct btf *btf, u32 offset)
> @@ -4481,7 +4482,7 @@ static s32 btf_var_check_meta(struct btf_verifier_env *env,
>         }
>
>         if (!t->name_off ||
> -           !__btf_name_valid(env->btf, t->name_off)) {
> +           !btf_name_valid_identifier(env->btf, t->name_off)) {
>                 btf_verifier_log_type(env, t, "Invalid name");
>                 return -EINVAL;
>         }
> --
> 2.43.0
>

Eduard Zingerman March 6, 2024, 2:04 a.m. UTC | #2

On Tue, 2024-03-05 at 13:43 -0800, Andrii Nakryiko wrote:
[...]

> > @@ -761,12 +761,13 @@ static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
> >         return offset < btf->hdr.str_len;
> >  }
> > 
> > -static bool __btf_name_char_ok(char c, bool first)
> > +static bool __btf_name_char_ok(char c, bool first, bool allow_qmark)
> >  {
> >         if ((first ? !isalpha(c) :
> >                      !isalnum(c)) &&
> >             c != '_' &&
> > -           c != '.')
> > +           c != '.' &&
> > +           (allow_qmark && first ? c != '?' : true))
> 
> ELF data section can have pretty much arbitrary characters in the
> name. I don't think we should allow question mark only at the
> beginning. Let's just allow any printable character, anywhere (for
> DATASEC only, of course)? There is no danger in doing this, data
> section name is not a variable name or some C identifier, and so won't
> be used like that.

Makes sense to me.

Thank you for reviewing this patch-set, I've applied the changes,
will share v4 tomorrow.

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 6ff0bd1a91d5..a25fb6bce808 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -761,12 +761,13 @@  static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
 	return offset < btf->hdr.str_len;
 }
 
-static bool __btf_name_char_ok(char c, bool first)
+static bool __btf_name_char_ok(char c, bool first, bool allow_qmark)
 {
 	if ((first ? !isalpha(c) :
 		     !isalnum(c)) &&
 	    c != '_' &&
-	    c != '.')
+	    c != '.' &&
+	    (allow_qmark && first ? c != '?' : true))
 		return false;
 	return true;
 }
@@ -783,20 +784,20 @@  static const char *btf_str_by_offset(const struct btf *btf, u32 offset)
 	return NULL;
 }
 
-static bool __btf_name_valid(const struct btf *btf, u32 offset)
+static bool __btf_name_valid(const struct btf *btf, u32 offset, bool allow_qmark)
 {
 	/* offset must be valid */
 	const char *src = btf_str_by_offset(btf, offset);
 	const char *src_limit;
 
-	if (!__btf_name_char_ok(*src, true))
+	if (!__btf_name_char_ok(*src, true, allow_qmark))
 		return false;
 
 	/* set a limit on identifier length */
 	src_limit = src + KSYM_NAME_LEN;
 	src++;
 	while (*src && src < src_limit) {
-		if (!__btf_name_char_ok(*src, false))
+		if (!__btf_name_char_ok(*src, false, false))
 			return false;
 		src++;
 	}
@@ -806,12 +807,12 @@  static bool __btf_name_valid(const struct btf *btf, u32 offset)
 
 static bool btf_name_valid_identifier(const struct btf *btf, u32 offset)
 {
-	return __btf_name_valid(btf, offset);
+	return __btf_name_valid(btf, offset, false);
 }
 
 static bool btf_name_valid_section(const struct btf *btf, u32 offset)
 {
-	return __btf_name_valid(btf, offset);
+	return __btf_name_valid(btf, offset, true);
 }
 
 static const char *__btf_name_by_offset(const struct btf *btf, u32 offset)
@@ -4481,7 +4482,7 @@  static s32 btf_var_check_meta(struct btf_verifier_env *env,
 	}
 
 	if (!t->name_off ||
-	    !__btf_name_valid(env->btf, t->name_off)) {
+	    !btf_name_valid_identifier(env->btf, t->name_off)) {
 		btf_verifier_log_type(env, t, "Invalid name");
 		return -EINVAL;
 	}