diff mbox series

[bpf-next,v5,3/6] bpf: Remove is_valid_bpf_tramp_flags()

Message ID 20220518131638.3401509-4-xukuohai@huawei.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series bpf trampoline for arm64 | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
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: 3 this patch: 3
netdev/cc_maintainers success CCed 19 of 19 maintainers
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 3 this patch: 3
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 50 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-PR fail merge-conflict
bpf/vmtest-bpf-next-VM_Test-1 success Logs for Kernel LATEST on ubuntu-latest with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Kernel LATEST on ubuntu-latest with llvm-15
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Kernel LATEST on z15 with gcc

Commit Message

Xu Kuohai May 18, 2022, 1:16 p.m. UTC
BPF_TRAM_F_XXX flags are not used by user code and are almost constant
at compile time, so run time validation is a bit overkill. Remove
is_valid_bpf_tramp_flags() and add some usage comments.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
Acked-by: Song Liu <songliubraving@fb.com>
---
 arch/x86/net/bpf_jit_comp.c | 20 --------------------
 kernel/bpf/bpf_struct_ops.c |  3 +++
 kernel/bpf/trampoline.c     |  3 +++
 3 files changed, 6 insertions(+), 20 deletions(-)

Comments

Mark Rutland May 25, 2022, 1:45 p.m. UTC | #1
On Wed, May 18, 2022 at 09:16:35AM -0400, Xu Kuohai wrote:
> BPF_TRAM_F_XXX flags are not used by user code and are almost constant
> at compile time, so run time validation is a bit overkill. Remove
> is_valid_bpf_tramp_flags() and add some usage comments.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> Acked-by: Song Liu <songliubraving@fb.com>

Am I right in thinking this is independent of the arm64-specific bits, and
could be taken on its own now?

Mark.

> ---
>  arch/x86/net/bpf_jit_comp.c | 20 --------------------
>  kernel/bpf/bpf_struct_ops.c |  3 +++
>  kernel/bpf/trampoline.c     |  3 +++
>  3 files changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index a2b6d197c226..7698ef3b4821 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1922,23 +1922,6 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
>  	return 0;
>  }
>  
> -static bool is_valid_bpf_tramp_flags(unsigned int flags)
> -{
> -	if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
> -	    (flags & BPF_TRAMP_F_SKIP_FRAME))
> -		return false;
> -
> -	/*
> -	 * BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
> -	 * and it must be used alone.
> -	 */
> -	if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
> -	    (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
> -		return false;
> -
> -	return true;
> -}
> -
>  /* Example:
>   * __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
>   * its 'struct btf_func_model' will be nr_args=2
> @@ -2017,9 +2000,6 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>  	if (nr_args > 6)
>  		return -ENOTSUPP;
>  
> -	if (!is_valid_bpf_tramp_flags(flags))
> -		return -EINVAL;
> -
>  	/* Generated trampoline stack layout:
>  	 *
>  	 * RBP + 8         [ return address  ]
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index d9a3c9207240..0572cc5aeb28 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -341,6 +341,9 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
>  
>  	tlinks[BPF_TRAMP_FENTRY].links[0] = link;
>  	tlinks[BPF_TRAMP_FENTRY].nr_links = 1;
> +	/* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
> +	 * and it must be used alone.
> +	 */
>  	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
>  	return arch_prepare_bpf_trampoline(NULL, image, image_end,
>  					   model, flags, tlinks, NULL);
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 93c7675f0c9e..bd3f2e673874 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -358,6 +358,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
>  
>  	if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
>  	    tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links)
> +		/* NOTE: BPF_TRAMP_F_RESTORE_REGS and BPF_TRAMP_F_SKIP_FRAME
> +		 * should not be set together.
> +		 */
>  		flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
>  
>  	if (ip_arg)
> -- 
> 2.30.2
>
Xu Kuohai May 26, 2022, 9:45 a.m. UTC | #2
On 5/25/2022 9:45 PM, Mark Rutland wrote:
> On Wed, May 18, 2022 at 09:16:35AM -0400, Xu Kuohai wrote:
>> BPF_TRAM_F_XXX flags are not used by user code and are almost constant
>> at compile time, so run time validation is a bit overkill. Remove
>> is_valid_bpf_tramp_flags() and add some usage comments.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> Acked-by: Song Liu <songliubraving@fb.com>
> 
> Am I right in thinking this is independent of the arm64-specific bits, and
> could be taken on its own now?
> 

Currenly is_valid_bpf_tramp_flags() is defined in x86 and called before
bpf trampoline is constructed. The check logic is irrelevant to the
architecture code. So we also need to call this function on arm64. But
as Alexei pointed out, the check is not requried, so it's better to
remove it before adding bpf trampoline to arm64.

> Mark.
> 
>> ---
>>  arch/x86/net/bpf_jit_comp.c | 20 --------------------
>>  kernel/bpf/bpf_struct_ops.c |  3 +++
>>  kernel/bpf/trampoline.c     |  3 +++
>>  3 files changed, 6 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index a2b6d197c226..7698ef3b4821 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -1922,23 +1922,6 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
>>  	return 0;
>>  }
>>  
>> -static bool is_valid_bpf_tramp_flags(unsigned int flags)
>> -{
>> -	if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
>> -	    (flags & BPF_TRAMP_F_SKIP_FRAME))
>> -		return false;
>> -
>> -	/*
>> -	 * BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
>> -	 * and it must be used alone.
>> -	 */
>> -	if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
>> -	    (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
>> -		return false;
>> -
>> -	return true;
>> -}
>> -
>>  /* Example:
>>   * __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
>>   * its 'struct btf_func_model' will be nr_args=2
>> @@ -2017,9 +2000,6 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>>  	if (nr_args > 6)
>>  		return -ENOTSUPP;
>>  
>> -	if (!is_valid_bpf_tramp_flags(flags))
>> -		return -EINVAL;
>> -
>>  	/* Generated trampoline stack layout:
>>  	 *
>>  	 * RBP + 8         [ return address  ]
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index d9a3c9207240..0572cc5aeb28 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -341,6 +341,9 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
>>  
>>  	tlinks[BPF_TRAMP_FENTRY].links[0] = link;
>>  	tlinks[BPF_TRAMP_FENTRY].nr_links = 1;
>> +	/* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
>> +	 * and it must be used alone.
>> +	 */
>>  	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
>>  	return arch_prepare_bpf_trampoline(NULL, image, image_end,
>>  					   model, flags, tlinks, NULL);
>> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
>> index 93c7675f0c9e..bd3f2e673874 100644
>> --- a/kernel/bpf/trampoline.c
>> +++ b/kernel/bpf/trampoline.c
>> @@ -358,6 +358,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
>>  
>>  	if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
>>  	    tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links)
>> +		/* NOTE: BPF_TRAMP_F_RESTORE_REGS and BPF_TRAMP_F_SKIP_FRAME
>> +		 * should not be set together.
>> +		 */
>>  		flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
>>  
>>  	if (ip_arg)
>> -- 
>> 2.30.2
>>
> .
Mark Rutland May 26, 2022, 10:12 a.m. UTC | #3
On Thu, May 26, 2022 at 05:45:25PM +0800, Xu Kuohai wrote:
> On 5/25/2022 9:45 PM, Mark Rutland wrote:
> > On Wed, May 18, 2022 at 09:16:35AM -0400, Xu Kuohai wrote:
> >> BPF_TRAM_F_XXX flags are not used by user code and are almost constant
> >> at compile time, so run time validation is a bit overkill. Remove
> >> is_valid_bpf_tramp_flags() and add some usage comments.
> >>
> >> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> >> Acked-by: Song Liu <songliubraving@fb.com>
> > 
> > Am I right in thinking this is independent of the arm64-specific bits, and
> > could be taken on its own now?
> > 
> 
> Currenly is_valid_bpf_tramp_flags() is defined in x86 and called before
> bpf trampoline is constructed. The check logic is irrelevant to the
> architecture code. So we also need to call this function on arm64. But
> as Alexei pointed out, the check is not requried, so it's better to
> remove it before adding bpf trampoline to arm64.

Cool. So this patch could be merged now, even if the rest of the series needs
more work?

Thanks,
Mark.

> >> ---
> >>  arch/x86/net/bpf_jit_comp.c | 20 --------------------
> >>  kernel/bpf/bpf_struct_ops.c |  3 +++
> >>  kernel/bpf/trampoline.c     |  3 +++
> >>  3 files changed, 6 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> >> index a2b6d197c226..7698ef3b4821 100644
> >> --- a/arch/x86/net/bpf_jit_comp.c
> >> +++ b/arch/x86/net/bpf_jit_comp.c
> >> @@ -1922,23 +1922,6 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
> >>  	return 0;
> >>  }
> >>  
> >> -static bool is_valid_bpf_tramp_flags(unsigned int flags)
> >> -{
> >> -	if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
> >> -	    (flags & BPF_TRAMP_F_SKIP_FRAME))
> >> -		return false;
> >> -
> >> -	/*
> >> -	 * BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
> >> -	 * and it must be used alone.
> >> -	 */
> >> -	if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
> >> -	    (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
> >> -		return false;
> >> -
> >> -	return true;
> >> -}
> >> -
> >>  /* Example:
> >>   * __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
> >>   * its 'struct btf_func_model' will be nr_args=2
> >> @@ -2017,9 +2000,6 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> >>  	if (nr_args > 6)
> >>  		return -ENOTSUPP;
> >>  
> >> -	if (!is_valid_bpf_tramp_flags(flags))
> >> -		return -EINVAL;
> >> -
> >>  	/* Generated trampoline stack layout:
> >>  	 *
> >>  	 * RBP + 8         [ return address  ]
> >> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> >> index d9a3c9207240..0572cc5aeb28 100644
> >> --- a/kernel/bpf/bpf_struct_ops.c
> >> +++ b/kernel/bpf/bpf_struct_ops.c
> >> @@ -341,6 +341,9 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
> >>  
> >>  	tlinks[BPF_TRAMP_FENTRY].links[0] = link;
> >>  	tlinks[BPF_TRAMP_FENTRY].nr_links = 1;
> >> +	/* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
> >> +	 * and it must be used alone.
> >> +	 */
> >>  	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
> >>  	return arch_prepare_bpf_trampoline(NULL, image, image_end,
> >>  					   model, flags, tlinks, NULL);
> >> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> >> index 93c7675f0c9e..bd3f2e673874 100644
> >> --- a/kernel/bpf/trampoline.c
> >> +++ b/kernel/bpf/trampoline.c
> >> @@ -358,6 +358,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
> >>  
> >>  	if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
> >>  	    tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links)
> >> +		/* NOTE: BPF_TRAMP_F_RESTORE_REGS and BPF_TRAMP_F_SKIP_FRAME
> >> +		 * should not be set together.
> >> +		 */
> >>  		flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
> >>  
> >>  	if (ip_arg)
> >> -- 
> >> 2.30.2
> >>
> > .
>
Xu Kuohai May 26, 2022, 2:46 p.m. UTC | #4
On 5/26/2022 6:12 PM, Mark Rutland wrote:
> On Thu, May 26, 2022 at 05:45:25PM +0800, Xu Kuohai wrote:
>> On 5/25/2022 9:45 PM, Mark Rutland wrote:
>>> On Wed, May 18, 2022 at 09:16:35AM -0400, Xu Kuohai wrote:
>>>> BPF_TRAM_F_XXX flags are not used by user code and are almost constant
>>>> at compile time, so run time validation is a bit overkill. Remove
>>>> is_valid_bpf_tramp_flags() and add some usage comments.
>>>>
>>>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>>>> Acked-by: Song Liu <songliubraving@fb.com>
>>>
>>> Am I right in thinking this is independent of the arm64-specific bits, and
>>> could be taken on its own now?
>>>
>>
>> Currenly is_valid_bpf_tramp_flags() is defined in x86 and called before
>> bpf trampoline is constructed. The check logic is irrelevant to the
>> architecture code. So we also need to call this function on arm64. But
>> as Alexei pointed out, the check is not requried, so it's better to
>> remove it before adding bpf trampoline to arm64.
> 
> Cool. So this patch could be merged now, even if the rest of the series needs
> more work?
> 

Agree with you, thanks.

> Thanks,
> Mark.
> 
>>>> ---
>>>>  arch/x86/net/bpf_jit_comp.c | 20 --------------------
>>>>  kernel/bpf/bpf_struct_ops.c |  3 +++
>>>>  kernel/bpf/trampoline.c     |  3 +++
>>>>  3 files changed, 6 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>>>> index a2b6d197c226..7698ef3b4821 100644
>>>> --- a/arch/x86/net/bpf_jit_comp.c
>>>> +++ b/arch/x86/net/bpf_jit_comp.c
>>>> @@ -1922,23 +1922,6 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static bool is_valid_bpf_tramp_flags(unsigned int flags)
>>>> -{
>>>> -	if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
>>>> -	    (flags & BPF_TRAMP_F_SKIP_FRAME))
>>>> -		return false;
>>>> -
>>>> -	/*
>>>> -	 * BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
>>>> -	 * and it must be used alone.
>>>> -	 */
>>>> -	if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
>>>> -	    (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
>>>> -		return false;
>>>> -
>>>> -	return true;
>>>> -}
>>>> -
>>>>  /* Example:
>>>>   * __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
>>>>   * its 'struct btf_func_model' will be nr_args=2
>>>> @@ -2017,9 +2000,6 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>>>>  	if (nr_args > 6)
>>>>  		return -ENOTSUPP;
>>>>  
>>>> -	if (!is_valid_bpf_tramp_flags(flags))
>>>> -		return -EINVAL;
>>>> -
>>>>  	/* Generated trampoline stack layout:
>>>>  	 *
>>>>  	 * RBP + 8         [ return address  ]
>>>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>>>> index d9a3c9207240..0572cc5aeb28 100644
>>>> --- a/kernel/bpf/bpf_struct_ops.c
>>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>>> @@ -341,6 +341,9 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
>>>>  
>>>>  	tlinks[BPF_TRAMP_FENTRY].links[0] = link;
>>>>  	tlinks[BPF_TRAMP_FENTRY].nr_links = 1;
>>>> +	/* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
>>>> +	 * and it must be used alone.
>>>> +	 */
>>>>  	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
>>>>  	return arch_prepare_bpf_trampoline(NULL, image, image_end,
>>>>  					   model, flags, tlinks, NULL);
>>>> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
>>>> index 93c7675f0c9e..bd3f2e673874 100644
>>>> --- a/kernel/bpf/trampoline.c
>>>> +++ b/kernel/bpf/trampoline.c
>>>> @@ -358,6 +358,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
>>>>  
>>>>  	if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
>>>>  	    tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links)
>>>> +		/* NOTE: BPF_TRAMP_F_RESTORE_REGS and BPF_TRAMP_F_SKIP_FRAME
>>>> +		 * should not be set together.
>>>> +		 */
>>>>  		flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
>>>>  
>>>>  	if (ip_arg)
>>>> -- 
>>>> 2.30.2
>>>>
>>> .
>>
> .
diff mbox series

Patch

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index a2b6d197c226..7698ef3b4821 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1922,23 +1922,6 @@  static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
 	return 0;
 }
 
-static bool is_valid_bpf_tramp_flags(unsigned int flags)
-{
-	if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
-	    (flags & BPF_TRAMP_F_SKIP_FRAME))
-		return false;
-
-	/*
-	 * BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
-	 * and it must be used alone.
-	 */
-	if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
-	    (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
-		return false;
-
-	return true;
-}
-
 /* Example:
  * __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
  * its 'struct btf_func_model' will be nr_args=2
@@ -2017,9 +2000,6 @@  int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	if (nr_args > 6)
 		return -ENOTSUPP;
 
-	if (!is_valid_bpf_tramp_flags(flags))
-		return -EINVAL;
-
 	/* Generated trampoline stack layout:
 	 *
 	 * RBP + 8         [ return address  ]
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index d9a3c9207240..0572cc5aeb28 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -341,6 +341,9 @@  int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
 
 	tlinks[BPF_TRAMP_FENTRY].links[0] = link;
 	tlinks[BPF_TRAMP_FENTRY].nr_links = 1;
+	/* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
+	 * and it must be used alone.
+	 */
 	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
 	return arch_prepare_bpf_trampoline(NULL, image, image_end,
 					   model, flags, tlinks, NULL);
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 93c7675f0c9e..bd3f2e673874 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -358,6 +358,9 @@  static int bpf_trampoline_update(struct bpf_trampoline *tr)
 
 	if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
 	    tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links)
+		/* NOTE: BPF_TRAMP_F_RESTORE_REGS and BPF_TRAMP_F_SKIP_FRAME
+		 * should not be set together.
+		 */
 		flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
 
 	if (ip_arg)