Date:   Wed, 29 Apr 2020 11:14:43 -0700
From:   Yonghong Song <yhs@...com>
To:     Alexei Starovoitov <ast@...com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     Martin KaFai Lau <kafai@...com>, Andrii Nakryiko <andriin@...com>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next v1 03/19] bpf: add bpf_map iterator



On 4/29/20 8:34 AM, Alexei Starovoitov wrote:
> On 4/28/20 11:44 PM, Yonghong Song wrote:
>>
>>
>> On 4/28/20 11:40 PM, Andrii Nakryiko wrote:
>>> On Tue, Apr 28, 2020 at 11:30 PM Alexei Starovoitov <ast@...com> wrote:
>>>>
>>>> On 4/28/20 11:20 PM, Yonghong Song wrote:
>>>>>
>>>>>
>>>>> On 4/28/20 11:08 PM, Andrii Nakryiko wrote:
>>>>>> On Tue, Apr 28, 2020 at 10:10 PM Yonghong Song <yhs@...com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 4/28/20 7:44 PM, Alexei Starovoitov wrote:
>>>>>>>> On 4/28/20 6:15 PM, Yonghong Song wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 4/28/20 5:48 PM, Alexei Starovoitov wrote:
>>>>>>>>>> On 4/28/20 5:37 PM, Martin KaFai Lau wrote:
>>>>>>>>>>>> +    prog = bpf_iter_get_prog(seq, sizeof(struct
>>>>>>>>>>>> bpf_iter_seq_map_info),
>>>>>>>>>>>> +                 &meta.session_id, &meta.seq_num,
>>>>>>>>>>>> +                 v == (void *)0);
>>>>>>>>>>>    From looking at seq_file.c, when will show() be called 
>>>>>>>>>>> with "v ==
>>>>>>>>>>> NULL"?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> that v == NULL here and the whole verifier change just to allow
>>>>>>>>>> NULL...
>>>>>>>>>> may be use seq_num as an indicator of the last elem instead?
>>>>>>>>>> Like seq_num with upper bit set to indicate that it's last?
>>>>>>>>>
>>>>>>>>> We could. But then verifier won't have an easy way to verify that.
>>>>>>>>> For example, the above is expected:
>>>>>>>>>
>>>>>>>>>         int prog(struct bpf_map *map, u64 seq_num) {
>>>>>>>>>            if (seq_num >> 63)
>>>>>>>>>              return 0;
>>>>>>>>>            ... map->id ...
>>>>>>>>>            ... map->user_cnt ...
>>>>>>>>>         }
>>>>>>>>>
>>>>>>>>> But if user writes
>>>>>>>>>
>>>>>>>>>         int prog(struct bpf_map *map, u64 seq_num) {
>>>>>>>>>             ... map->id ...
>>>>>>>>>             ... map->user_cnt ...
>>>>>>>>>         }
>>>>>>>>>
>>>>>>>>> verifier won't be easy to conclude inproper map pointer tracing
>>>>>>>>> here and in the above map->id, map->user_cnt will cause
>>>>>>>>> exceptions and they will silently get value 0.
>>>>>>>>
>>>>>>>> I mean always pass valid object pointer into the prog.
>>>>>>>> In above case 'map' will always be valid.
>>>>>>>> Consider prog that iterating all map elements.
>>>>>>>> It's weird that the prog would always need to do
>>>>>>>> if (map == 0)
>>>>>>>>      goto out;
>>>>>>>> even if it doesn't care about finding last.
>>>>>>>> All progs would have to have such extra 'if'.
>>>>>>>> If we always pass valid object than there is no need
>>>>>>>> for such extra checks inside the prog.
>>>>>>>> First and last element can be indicated via seq_num
>>>>>>>> or via another flag or via helper call like is_this_last_elem()
>>>>>>>> or something.
>>>>>>>
>>>>>>> Okay, I see what you mean now. Basically this means
>>>>>>> seq_ops->next() should try to get/maintain next two elements,
>>>>>>
>>>>>> What about the case when there are no elements to iterate to begin
>>>>>> with? In that case, we still need to call bpf_prog for (empty)
>>>>>> post-aggregation, but we have no valid element... For bpf_map
>>>>>> iteration we could have fake empty bpf_map that would be passed, but
>>>>>> I'm not sure it's applicable for any time of object (e.g., having a
>>>>>> fake task_struct is probably quite a bit more problematic?)...
>>>>>
>>>>> Oh, yes, thanks for reminding me of this. I put a call to
>>>>> bpf_prog in seq_ops->stop() especially to handle no object
>>>>> case. In that case, seq_ops->start() will return NULL,
>>>>> seq_ops->next() won't be called, and then seq_ops->stop()
>>>>> is called. My earlier attempt tries to hook with next()
>>>>> and then find it not working in all cases.
>>>>
>>>> wait a sec. seq_ops->stop() is not the end.
>>>> With lseek of seq_file it can be called multiple times.
>>
>> Yes, I have taken care of this. when the object is NULL,
>> bpf program will be called. When the object is NULL again,
>> it won't be called. The private data remembers it has
>> been called with NULL.
> 
> Even without lseek stop() will be called multiple times.
> If I read seq_file.c correctly it will be called before
> every copy_to_user(). Which means that for a lot of text
> (or if read() is done with small buffer) there will be
> plenty of start,show,show,stop sequences.

That is true, this may cause revisit the same object if the object
still exists return start() called again. I followed similar
practice with ipv6_route(), trying to looking up the same
object at start() and only advanced right before next().

Powered by blists - more mailing lists