netdev - Re: [PATCH v2 bpf-next 2/5] bpf: Add support to attach bpf program to a devmap entry

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]

Message-ID: <191ba79f-3aeb-718c-644e-bd2cc539dc60@gmail.com>
Date:   Thu, 28 May 2020 16:40:06 -0600
From:   David Ahern <dsahern@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        David Ahern <dsahern@...nel.org>
Cc:     Networking <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        john fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <ast@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>
Subject: Re: [PATCH v2 bpf-next 2/5] bpf: Add support to attach bpf program to
 a devmap entry

On 5/28/20 1:01 AM, Andrii Nakryiko wrote:
> 
> Please cc bpf@...r.kernel.org in the future for patches related to BPF
> in general.

added to my script

> 
>>  include/linux/bpf.h            |  5 +++
>>  include/uapi/linux/bpf.h       |  5 +++
>>  kernel/bpf/devmap.c            | 79 +++++++++++++++++++++++++++++++++-
>>  net/core/dev.c                 | 18 ++++++++
>>  tools/include/uapi/linux/bpf.h |  5 +++
>>  5 files changed, 110 insertions(+), 2 deletions(-)
>>
> 
> [...]
> 
>>
>> +static struct xdp_buff *dev_map_run_prog(struct net_device *dev,
>> +                                        struct xdp_buff *xdp,
>> +                                        struct bpf_prog *xdp_prog)
>> +{
>> +       u32 act;
>> +
>> +       act = bpf_prog_run_xdp(xdp_prog, xdp);
>> +       switch (act) {
>> +       case XDP_DROP:
>> +               fallthrough;
> 
> nit: I don't think fallthrough is necessary for cases like:
> 
> case XDP_DROP:
> case XDP_PASS:
>     /* do something */
> 
>> +       case XDP_PASS:
>> +               break;
>> +       default:
>> +               bpf_warn_invalid_xdp_action(act);
>> +               fallthrough;
>> +       case XDP_ABORTED:
>> +               trace_xdp_exception(dev, xdp_prog, act);
>> +               act = XDP_DROP;
>> +               break;
>> +       }
>> +
>> +       if (act == XDP_DROP) {
>> +               xdp_return_buff(xdp);
>> +               xdp = NULL;
> 
> hm.. if you move XDP_DROP case to after XDP_ABORTED and do fallthrough
> from XDP_ABORTED, you won't even need to override act and it will just
> handle all the cases, no?
> 
> switch (act) {
> case XDP_PASS:
>     return xdp;
> default:
>     bpf_warn_invalid_xdp_action(act);
>     fallthrough;
> case XDP_ABORTED:
>     trace_xdp_exception(dev, xdp_prog, act);
>     fallthrough;
> case XDP_DROP:
>     xdp_return_buff(xdp);
>     return NULL;
> }
> 
> Wouldn't this be simpler?
> 

Switched it to this which captures your intent with a more traditional
return location.

        act = bpf_prog_run_xdp(xdp_prog, xdp);
        switch (act) {
        case XDP_PASS:
                return xdp;
        case XDP_DROP:
                break;
        default:
                bpf_warn_invalid_xdp_action(act);
                fallthrough;
        case XDP_ABORTED:
                trace_xdp_exception(dev, xdp_prog, act);
                break;
        }

        xdp_return_buff(xdp);
        return NULL;