Re: [PATCH] powerpc/bpf: populate extable entries only during the last pass

[Hari Bathini <hbathini@linux.ibm.com>]

Hi Naveen,

On 24/04/23 5:25 pm, Naveen N. Rao wrote:
> Hari Bathini wrote:
>> Hello Christophe,
>>
>> Thanks for the review.
>>
>> On 07/04/23 11:31 am, Christophe Leroy wrote:
>>>
>>>
>>> Le 06/04/2023 à 09:35, Hari Bathini a écrit :
>>>> Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes
>>>> to update addresses"), two additional passes are performed to avoid
>>>> space and CPU time wastage on powerpc. But these extra passes led to
>>>> WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding
>>>> extable entries during the extra pass.
>>>
>>> Are you sure this change is correct ?
>>
>> Actually, I was in two minds about that owing to commit 04c04205bc35
>> ("bpf powerpc: Remove extra_pass from bpf_jit_build_body()").
> 
> Right, but Christophe's series adding complete passes during the 
> extra_pass phase added 'extra_pass' parameter back to bpf_jit_build_body().
> 
>>
>>> During the extra pass the code can get shrinked or expanded (within the
>>> limits of the size of the preliminary pass). Shouldn't extable entries
>>> be populated during the last pass ?
>>
>> Unlikely, but the intention there was to eliminate a regression in case
>> extra_pass ends up being 'false' always in any subsequent change.
> 
> But, the current approach risks generating incorrect offsets in the 
> extable. The main motivation for the extra pass is to generate more 
> compact code, so there is a good chance that offsets are going to change 
> (especially with bpf subprogs).
> 
>>
>> - Hari
>>
>>>>
>>>> Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to 
>>>> update addresses")
>>>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>>>> ---
>>>>    arch/powerpc/net/bpf_jit_comp32.c | 2 +-
>>>>    arch/powerpc/net/bpf_jit_comp64.c | 2 +-
>>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/net/bpf_jit_comp32.c 
>>>> b/arch/powerpc/net/bpf_jit_comp32.c
>>>> index 7f91ea064c08..e788b1fbeee6 100644
>>>> --- a/arch/powerpc/net/bpf_jit_comp32.c
>>>> +++ b/arch/powerpc/net/bpf_jit_comp32.c
>>>> @@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 
>>>> *image, struct codegen_context *
>>>>                if (size != BPF_DW && !fp->aux->verifier_zext)
>>>>                    EMIT(PPC_RAW_LI(dst_reg_h, 0));
>>>> -            if (BPF_MODE(code) == BPF_PROBE_MEM) {
>>>> +            if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) {
> 
> It is probably better to pass 'extra_pass' into bpf_add_extable_entry() 
> to keep all those checks together.
> 

Thanks for the review and also the suggestion (offline) to reset index
during extra pass, for my concern about possible regression. Posted v2.

- Hari