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

[David Ahern]

On 5/28/20 1:01 AM, Andrii Nakryiko wrote:
> 
> Please cc b...@vger.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;