netdev - Re: [PATCH bpf-next v4 21/21] tools/bpf: selftests: add bpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 10 May 2020 09:14:59 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Network Development <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next v4 21/21] tools/bpf: selftests: add bpf_iter selftests

On Sat, May 9, 2020 at 10:01 PM Yonghong Song <yhs@...com> wrote:
>
>
>
> On 5/9/20 5:34 PM, Alexei Starovoitov wrote:
> > On Sat, May 09, 2020 at 10:59:23AM -0700, Yonghong Song wrote:
> >> +static volatile const __u32 ret1;
> >> +
> >> +SEC("iter/bpf_map")
> >> +int dump_bpf_map(struct bpf_iter__bpf_map *ctx)
> >> +{
> >> +    struct seq_file *seq = ctx->meta->seq;
> >> +    struct bpf_map *map = ctx->map;
> >> +    __u64 seq_num;
> >> +    int i, ret = 0;
> >> +
> >> +    if (map == (void *)0)
> >> +            return 0;
> >> +
> >> +    /* only dump map1_id and map2_id */
> >> +    if (map->id != map1_id && map->id != map2_id)
> >> +            return 0;
> >> +
> >> +    seq_num = ctx->meta->seq_num;
> >> +    if (map->id == map1_id) {
> >> +            map1_seqnum = seq_num;
> >> +            map1_accessed++;
> >> +    }
> >> +
> >> +    if (map->id == map2_id) {
> >> +            if (map2_accessed == 0) {
> >> +                    map2_seqnum1 = seq_num;
> >> +                    if (ret1)
> >> +                            ret = 1;
> >> +            } else {
> >> +                    map2_seqnum2 = seq_num;
> >> +            }
> >> +            map2_accessed++;
> >> +    }
> >> +
> >> +    /* fill seq_file buffer */
> >> +    for (i = 0; i < print_len; i++)
> >> +            bpf_seq_write(seq, &seq_num, sizeof(seq_num));
> >> +
> >> +    return ret;
> >> +}
> >
> > I couldn't find where 'return 1' behavior is documented clearly.
>
> It is in the commit comments:
>
> commit 15d83c4d7cef5c067a8b075ce59e97df4f60706e
> Author: Yonghong Song <yhs@...com>
> Date:   Sat May 9 10:59:00 2020 -0700
>
>      bpf: Allow loading of a bpf_iter program
> ...
>      The program return value must be 0 or 1 for now.
>        0 : successful, except potential seq_file buffer overflow
>            which is handled by seq_file reader.
>        1 : request to restart the same object
>
> Internally, bpf program returning 1 will translate
> show() return -EAGAIN and this error code will
> return to user space.
>
> I will add some comments in the code to
> document this behavior.
>
> > I think it's a workaround for overflow.
>
> This can be used for overflow but overflow already been taken
> care of by bpf_seq_read(). This is mostly used for other use
> cases:
>     - currently under RT-linux, bpf_seq_printf() may return
>       -EBUSY. In this case, bpf program itself can request
>       retrying the same object.
>     - for other conditions where bpf program itself wants
>       to retry the same object. For example, hash table full,
>       the bpf progam can return 1, in which case, user space
>       read() will receive -EAGAIN and may check and make room
>       for hash table and then read() again.
>
> > When bpf prog detects overflow it can request replay of the element?
>
> It can. But it can return 0 too since bpf_seq_read() handles
> this transparently.
>
> > What if it keeps returning 1 ? read() will never finish?
>
> The read() will finish and return -EAGAIN to user space.
> It is up to user space to decide whether to call read()
> again or not.

Ahh. Got it. So that EAGAIN returned by bpf_iter_run_prog()
propagates by bpf_seq_read() all the way to read() syscall.
Now I see it. Thanks for explaining.