Re: [PATCH bpf-next 09/15] bpf: Mark OBJ_RELEASE argument as MEM_RCU when possible

[Yonghong Song <yonghong.song@linux.dev>]

On 8/18/23 6:44 PM, Kumar Kartikeya Dwivedi wrote:
> On Mon, 14 Aug 2023 at 23:00, Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> In previous selftests/bpf patch, we have
>>    p = bpf_percpu_obj_new(struct val_t);
>>    if (!p)
>>            goto out;
>>
>>    p1 = bpf_kptr_xchg(&e->pc, p);
>>    if (p1) {
>>            /* race condition */
>>            bpf_percpu_obj_drop(p1);
>>    }
>>
>>    p = e->pc;
>>    if (!p)
>>            goto out;
>>
>> After bpf_kptr_xchg(), we need to re-read e->pc into 'p'.
>> This is due to that the second argument of bpf_kptr_xchg() is marked
>> OBJ_RELEASE and it will be marked as invalid after the call.
>> So after bpf_kptr_xchg(), 'p' is an unknown scalar,
>> and the bpf program needs to reread from the map value.
>>
>> This patch checks if the 'p' has type MEM_ALLOC and MEM_PERCPU,
>> and if 'p' is RCU protected. If this is the case, 'p' can be marked
>> as MEM_RCU. MEM_ALLOC needs to be removed since 'p' is not
>> an owning reference any more. Such a change makes re-read
>> from the map value unnecessary.
>>
>> Note that re-reading 'e->pc' after bpf_kptr_xchg() might get
>> a different value from 'p' if immediately before 'p = e->pc',
>> another cpu may do another bpf_kptr_xchg() and swap in another value
>> into 'e->pc'. If this is the case, then 'p = e->pc' may
>> get either 'p' or another value, and race condition already exists.
>> So removing direct re-reading seems fine too.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   kernel/bpf/verifier.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 6fc200cb68b6..6fa458e13bfc 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -8854,8 +8854,15 @@ static int release_reference(struct bpf_verifier_env *env,
>>                  return err;
>>
>>          bpf_for_each_reg_in_vstate(env->cur_state, state, reg, ({
>> -               if (reg->ref_obj_id == ref_obj_id)
>> -                       mark_reg_invalid(env, reg);
>> +               if (reg->ref_obj_id == ref_obj_id) {
>> +                       if (in_rcu_cs(env) && (reg->type & MEM_ALLOC) && (reg->type & MEM_PERCPU)) {
> 
> Wouldn't this check also be true in case of bpf_percpu_obj_drop(p)
> inside RCU CS/non-sleepable prog?
> Do we want to permit access to p after drop in that case? I think it
> will be a bit unintuitive.
> I think we should preserve normal behavior for everything except for
> kptr_xchg of a percpu_kptr.

You are correct. Above condition also applies to bpf_percpu_obj_drop()
and we should should change MEM_ALLOC to MEM_RCU only for
bpf_percpu_obj_new(). Will fix.

> 
>> +                               reg->ref_obj_id = 0;
>> +                               reg->type &= ~MEM_ALLOC;
>> +                               reg->type |= MEM_RCU;
>> +                       } else {
>> +                               mark_reg_invalid(env, reg);
>> +                       }
>> +               }
>>          }));
>>
>>          return 0;
>> --
>> 2.34.1
>>
>>