diff mbox series

[bpf-next,v3,2/2] bpf: Warn on non-preallocated case for missed trace types

Message ID 20220709154457.57379-3-laoar.shao@gmail.com (mailing list archive)
State New
Headers show
Series bpf: Minor fixes for non-preallocated memory | expand

Commit Message

Yafang Shao July 9, 2022, 3:44 p.m. UTC 
The raw tracepoint may cause unexpected memory allocation if we set
BPF_F_NO_PREALLOC. So let's warn on it.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/bpf/verifier.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Yonghong Song July 10, 2022, 5:51 p.m. UTC | #1 
On 7/9/22 8:44 AM, Yafang Shao wrote:
> The raw tracepoint may cause unexpected memory allocation if we set
> BPF_F_NO_PREALLOC. So let's warn on it.

Please extend raw_tracepoint to other attach types which
may cause runtime map allocations.

> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>   kernel/bpf/verifier.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e3cf6194c24f..3cd8260827e0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -12574,14 +12574,20 @@ static int check_map_prealloc(struct bpf_map *map)
>   		!(map->map_flags & BPF_F_NO_PREALLOC);
>   }
>   
> -static bool is_tracing_prog_type(enum bpf_prog_type type)
> +static bool is_tracing_prog_type(enum bpf_prog_type prog_type,
> +				 enum bpf_attach_type attach_type)
>   {
> -	switch (type) {
> +	switch (prog_type) {
>   	case BPF_PROG_TYPE_KPROBE:
>   	case BPF_PROG_TYPE_TRACEPOINT:
>   	case BPF_PROG_TYPE_PERF_EVENT:
>   	case BPF_PROG_TYPE_RAW_TRACEPOINT:
> +	case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
>   		return true;
> +	case BPF_PROG_TYPE_TRACING:
> +		if (attach_type == BPF_TRACE_RAW_TP)
> +			return true;

As Alexei mentioned earlier, here we should have
		if (attach_type != BPF_TRACE_ITER)
			return true;

For attach types with BPF_PROG_TYPE_TRACING programs,
BPF_TRACE_ITER attach type can only appear in process context.
All other attach types may appear in non-process context.

> +		return false;
>   	default:
>   		return false;
>   	}
> @@ -12601,7 +12607,9 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
>   					struct bpf_prog *prog)
>   
>   {
> +	enum bpf_attach_type attach_type = prog->expected_attach_type;
>   	enum bpf_prog_type prog_type = resolve_prog_type(prog);
> +
[...]
Yafang Shao July 11, 2022, 6:48 a.m. UTC | #2 
On Mon, Jul 11, 2022 at 1:51 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/9/22 8:44 AM, Yafang Shao wrote:
> > The raw tracepoint may cause unexpected memory allocation if we set
> > BPF_F_NO_PREALLOC. So let's warn on it.
>
> Please extend raw_tracepoint to other attach types which
> may cause runtime map allocations.
>

Per my understanding, it is safe to allocate memory in a non-process
context as long as we don't allow blocking it.
So this issue doesn't matter with whether it causes runtime map
allocations or not, while it really matters with the tracepoint or
kprobe.
Regarding the tracepoint or kprobe, if we don't use non-preallocated
maps, it may allocate other extra memory besides the map element
itself.
I have verified that it is safe to use non-preallocated maps in
BPF_TRACE_ITER or BPF_TRACE_FENTRY.
So filtering out BPF_TRACE_RAW_TP only is enough.

> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >   kernel/bpf/verifier.c | 18 +++++++++++++-----
> >   1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index e3cf6194c24f..3cd8260827e0 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -12574,14 +12574,20 @@ static int check_map_prealloc(struct bpf_map *map)
> >               !(map->map_flags & BPF_F_NO_PREALLOC);
> >   }
> >
> > -static bool is_tracing_prog_type(enum bpf_prog_type type)
> > +static bool is_tracing_prog_type(enum bpf_prog_type prog_type,
> > +                              enum bpf_attach_type attach_type)
> >   {
> > -     switch (type) {
> > +     switch (prog_type) {
> >       case BPF_PROG_TYPE_KPROBE:
> >       case BPF_PROG_TYPE_TRACEPOINT:
> >       case BPF_PROG_TYPE_PERF_EVENT:
> >       case BPF_PROG_TYPE_RAW_TRACEPOINT:
> > +     case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
> >               return true;
> > +     case BPF_PROG_TYPE_TRACING:
> > +             if (attach_type == BPF_TRACE_RAW_TP)
> > +                     return true;
>
> As Alexei mentioned earlier, here we should have
>                 if (attach_type != BPF_TRACE_ITER)
>                         return true;

That will break selftests/bpf/progs/timer.c, because it uses timer in fentry.

> For attach types with BPF_PROG_TYPE_TRACING programs,
> BPF_TRACE_ITER attach type can only appear in process context.
> All other attach types may appear in non-process context.
>

Thanks for the explanation.

> > +             return false;
> >       default:
> >               return false;
> >       }
> > @@ -12601,7 +12607,9 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
> >                                       struct bpf_prog *prog)
> >
> >   {
> > +     enum bpf_attach_type attach_type = prog->expected_attach_type;
> >       enum bpf_prog_type prog_type = resolve_prog_type(prog);
> > +
> [...]
Yonghong Song July 11, 2022, 7:04 p.m. UTC | #3 
On 7/10/22 11:48 PM, Yafang Shao wrote:
> On Mon, Jul 11, 2022 at 1:51 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 7/9/22 8:44 AM, Yafang Shao wrote:
>>> The raw tracepoint may cause unexpected memory allocation if we set
>>> BPF_F_NO_PREALLOC. So let's warn on it.
>>
>> Please extend raw_tracepoint to other attach types which
>> may cause runtime map allocations.
>>
> 
> Per my understanding, it is safe to allocate memory in a non-process
> context as long as we don't allow blocking it.
> So this issue doesn't matter with whether it causes runtime map
> allocations or not, while it really matters with the tracepoint or
> kprobe.
> Regarding the tracepoint or kprobe, if we don't use non-preallocated
> maps, it may allocate other extra memory besides the map element
> itself.
> I have verified that it is safe to use non-preallocated maps in
> BPF_TRACE_ITER or BPF_TRACE_FENTRY.
> So filtering out BPF_TRACE_RAW_TP only is enough. >
>>>
>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>> ---
>>>    kernel/bpf/verifier.c | 18 +++++++++++++-----
>>>    1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index e3cf6194c24f..3cd8260827e0 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -12574,14 +12574,20 @@ static int check_map_prealloc(struct bpf_map *map)
>>>                !(map->map_flags & BPF_F_NO_PREALLOC);
>>>    }
>>>
>>> -static bool is_tracing_prog_type(enum bpf_prog_type type)
>>> +static bool is_tracing_prog_type(enum bpf_prog_type prog_type,
>>> +                              enum bpf_attach_type attach_type)
>>>    {
>>> -     switch (type) {
>>> +     switch (prog_type) {
>>>        case BPF_PROG_TYPE_KPROBE:
>>>        case BPF_PROG_TYPE_TRACEPOINT:
>>>        case BPF_PROG_TYPE_PERF_EVENT:
>>>        case BPF_PROG_TYPE_RAW_TRACEPOINT:
>>> +     case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
>>>                return true;
>>> +     case BPF_PROG_TYPE_TRACING:
>>> +             if (attach_type == BPF_TRACE_RAW_TP)
>>> +                     return true;
>>
>> As Alexei mentioned earlier, here we should have
>>                  if (attach_type != BPF_TRACE_ITER)
>>                          return true;
> 
> That will break selftests/bpf/progs/timer.c, because it uses timer in fentry.

Okay. Let us remove BPF_PROG_TYPE_TRACING from this patch for now.
fentry/fexit/fmod may attach any kallsyms functions and many of them
are called in process context and non-preallocated hashmap totally fine.
It is not good to prohibit non-preallocated hashmap for any 
fentry/fexit/fmod bpf programs.

> 
>> For attach types with BPF_PROG_TYPE_TRACING programs,
>> BPF_TRACE_ITER attach type can only appear in process context.
>> All other attach types may appear in non-process context.
>>
> 
> Thanks for the explanation.
> 
>>> +             return false;
>>>        default:
>>>                return false;
>>>        }
>>> @@ -12601,7 +12607,9 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
>>>                                        struct bpf_prog *prog)
>>>
>>>    {
>>> +     enum bpf_attach_type attach_type = prog->expected_attach_type;
>>>        enum bpf_prog_type prog_type = resolve_prog_type(prog);
>>> +
>> [...]
> 
> 
>
Yafang Shao July 12, 2022, 8:26 a.m. UTC | #4 
On Tue, Jul 12, 2022 at 3:04 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/10/22 11:48 PM, Yafang Shao wrote:
> > On Mon, Jul 11, 2022 at 1:51 AM Yonghong Song <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> On 7/9/22 8:44 AM, Yafang Shao wrote:
> >>> The raw tracepoint may cause unexpected memory allocation if we set
> >>> BPF_F_NO_PREALLOC. So let's warn on it.
> >>
> >> Please extend raw_tracepoint to other attach types which
> >> may cause runtime map allocations.
> >>
> >
> > Per my understanding, it is safe to allocate memory in a non-process
> > context as long as we don't allow blocking it.
> > So this issue doesn't matter with whether it causes runtime map
> > allocations or not, while it really matters with the tracepoint or
> > kprobe.
> > Regarding the tracepoint or kprobe, if we don't use non-preallocated
> > maps, it may allocate other extra memory besides the map element
> > itself.
> > I have verified that it is safe to use non-preallocated maps in
> > BPF_TRACE_ITER or BPF_TRACE_FENTRY.
> > So filtering out BPF_TRACE_RAW_TP only is enough. >
> >>>
> >>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> >>> ---
> >>>    kernel/bpf/verifier.c | 18 +++++++++++++-----
> >>>    1 file changed, 13 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >>> index e3cf6194c24f..3cd8260827e0 100644
> >>> --- a/kernel/bpf/verifier.c
> >>> +++ b/kernel/bpf/verifier.c
> >>> @@ -12574,14 +12574,20 @@ static int check_map_prealloc(struct bpf_map *map)
> >>>                !(map->map_flags & BPF_F_NO_PREALLOC);
> >>>    }
> >>>
> >>> -static bool is_tracing_prog_type(enum bpf_prog_type type)
> >>> +static bool is_tracing_prog_type(enum bpf_prog_type prog_type,
> >>> +                              enum bpf_attach_type attach_type)
> >>>    {
> >>> -     switch (type) {
> >>> +     switch (prog_type) {
> >>>        case BPF_PROG_TYPE_KPROBE:
> >>>        case BPF_PROG_TYPE_TRACEPOINT:
> >>>        case BPF_PROG_TYPE_PERF_EVENT:
> >>>        case BPF_PROG_TYPE_RAW_TRACEPOINT:
> >>> +     case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
> >>>                return true;
> >>> +     case BPF_PROG_TYPE_TRACING:
> >>> +             if (attach_type == BPF_TRACE_RAW_TP)
> >>> +                     return true;
> >>
> >> As Alexei mentioned earlier, here we should have
> >>                  if (attach_type != BPF_TRACE_ITER)
> >>                          return true;
> >
> > That will break selftests/bpf/progs/timer.c, because it uses timer in fentry.
>
> Okay. Let us remove BPF_PROG_TYPE_TRACING from this patch for now.
> fentry/fexit/fmod may attach any kallsyms functions and many of them
> are called in process context and non-preallocated hashmap totally fine.
> It is not good to prohibit non-preallocated hashmap for any
> fentry/fexit/fmod bpf programs.
>

Got it. I will do it.
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e3cf6194c24f..3cd8260827e0 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12574,14 +12574,20 @@  static int check_map_prealloc(struct bpf_map *map)
 		!(map->map_flags & BPF_F_NO_PREALLOC);
 }
 
-static bool is_tracing_prog_type(enum bpf_prog_type type)
+static bool is_tracing_prog_type(enum bpf_prog_type prog_type,
+				 enum bpf_attach_type attach_type)
 {
-	switch (type) {
+	switch (prog_type) {
 	case BPF_PROG_TYPE_KPROBE:
 	case BPF_PROG_TYPE_TRACEPOINT:
 	case BPF_PROG_TYPE_PERF_EVENT:
 	case BPF_PROG_TYPE_RAW_TRACEPOINT:
+	case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
 		return true;
+	case BPF_PROG_TYPE_TRACING:
+		if (attach_type == BPF_TRACE_RAW_TP)
+			return true;
+		return false;
 	default:
 		return false;
 	}
@@ -12601,7 +12607,9 @@  static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 					struct bpf_prog *prog)
 
 {
+	enum bpf_attach_type attach_type = prog->expected_attach_type;
 	enum bpf_prog_type prog_type = resolve_prog_type(prog);
+
 	/*
 	 * Validate that trace type programs use preallocated hash maps.
 	 *
@@ -12619,7 +12627,7 @@  static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 	 * now, but warnings are emitted so developers are made aware of
 	 * the unsafety and can fix their programs before this is enforced.
 	 */
-	if (is_tracing_prog_type(prog_type) && !is_preallocated_map(map)) {
+	if (is_tracing_prog_type(prog_type, attach_type) && !is_preallocated_map(map)) {
 		if (prog_type == BPF_PROG_TYPE_PERF_EVENT) {
 			verbose(env, "perf_event programs can only use preallocated hash map\n");
 			return -EINVAL;
@@ -12638,7 +12646,7 @@  static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 			return -EINVAL;
 		}
 
-		if (is_tracing_prog_type(prog_type)) {
+		if (is_tracing_prog_type(prog_type, attach_type)) {
 			verbose(env, "tracing progs cannot use bpf_spin_lock yet\n");
 			return -EINVAL;
 		}
@@ -12650,7 +12658,7 @@  static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 	}
 
 	if (map_value_has_timer(map)) {
-		if (is_tracing_prog_type(prog_type)) {
+		if (is_tracing_prog_type(prog_type, attach_type)) {
 			verbose(env, "tracing progs cannot use bpf_timer yet\n");
 			return -EINVAL;
 		}