netdev - Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,

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:   Mon, 03 Jun 2019 12:56:49 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Björn Töpel <bjorn.topel@...il.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Netdev <netdev@...r.kernel.org>,
        Björn Töpel 
        <bjorn.topel@...el.com>,
        "Karlsson\, Magnus" <magnus.karlsson@...el.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        bpf <bpf@...r.kernel.org>, Saeed Mahameed <saeedm@...lanox.com>
Subject: Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev

Björn Töpel <bjorn.topel@...il.com> writes:

> On Sat, 1 Jun 2019 at 22:02, Jakub Kicinski
> <jakub.kicinski@...ronome.com> wrote:
>>
>> On Fri, 31 May 2019 11:42:14 +0200, Björn Töpel wrote:
>> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> > index 44b47e9df94a..f3a875a52c6c 100644
>> > --- a/include/linux/netdevice.h
>> > +++ b/include/linux/netdevice.h
>> > @@ -1940,6 +1940,9 @@ struct net_device {
>> >  #endif
>> >       struct hlist_node       index_hlist;
>> >
>> > +     struct bpf_prog         *xdp_prog_hw;
>>
>> IDK if we should pay the cost of this pointer for every netdev on the
>> system just for the single production driver out there that implements
>> HW offload :(  I'm on the fence about this..
>>
>
> Hmm. Adding a config option? Keep the QUERY_PROG_HW?
>
>> > +     u32                     xdp_flags;
>> > +
>> >  /*
>> >   * Cache lines mostly used on transmit path
>> >   */
>>
>> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> > index adcc045952c2..5e396fd01d8b 100644
>> > --- a/net/core/rtnetlink.c
>> > +++ b/net/core/rtnetlink.c
>> > @@ -1360,42 +1360,44 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
>> >       return 0;
>> >  }
>> >
>> > -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
>> > +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
>> >  {
>> > -     const struct bpf_prog *generic_xdp_prog;
>> > -
>> > -     ASSERT_RTNL();
>> > -
>> > -     generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
>> > -     if (!generic_xdp_prog)
>> > -             return 0;
>> > -     return generic_xdp_prog->aux->id;
>> > -}
>> > -
>> > -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
>> > -{
>> > -     return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf, XDP_QUERY_PROG);
>> > +     switch (tgt_mode) {
>> > +     case XDP_ATTACHED_DRV:
>> > +             return XDP_FLAGS_DRV_MODE;
>> > +     case XDP_ATTACHED_SKB:
>> > +             return XDP_FLAGS_SKB_MODE;
>> > +     case XDP_ATTACHED_HW:
>> > +             return XDP_FLAGS_HW_MODE;
>> > +     }
>> > +     return 0;
>> >  }
>> >
>> > -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
>> > +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
>> >  {
>> > -     return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
>> > -                            XDP_QUERY_PROG_HW);
>> > +     switch (tgt_mode) {
>> > +     case XDP_ATTACHED_DRV:
>> > +             return IFLA_XDP_DRV_PROG_ID;
>> > +     case XDP_ATTACHED_SKB:
>> > +             return IFLA_XDP_SKB_PROG_ID;
>> > +     case XDP_ATTACHED_HW:
>> > +             return IFLA_XDP_HW_PROG_ID;
>> > +     }
>> > +     return 0;
>> >  }
>> >
>> >  static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
>> > -                            u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
>> > -                            u32 (*get_prog_id)(struct net_device *dev))
>> > +                            u32 *prog_id, u8 *mode, u8 tgt_mode)
>> >  {
>> >       u32 curr_id;
>> >       int err;
>> >
>> > -     curr_id = get_prog_id(dev);
>> > +     curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
>> >       if (!curr_id)
>> >               return 0;
>> >
>> >       *prog_id = curr_id;
>> > -     err = nla_put_u32(skb, attr, curr_id);
>> > +     err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
>> >       if (err)
>> >               return err;
>> >
>> > @@ -1420,16 +1422,13 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
>> >
>> >       prog_id = 0;
>> >       mode = XDP_ATTACHED_NONE;
>> > -     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB,
>> > -                               IFLA_XDP_SKB_PROG_ID, rtnl_xdp_prog_skb);
>> > +     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB);
>> >       if (err)
>> >               goto err_cancel;
>> > -     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV,
>> > -                               IFLA_XDP_DRV_PROG_ID, rtnl_xdp_prog_drv);
>> > +     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV);
>> >       if (err)
>> >               goto err_cancel;
>> > -     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW,
>> > -                               IFLA_XDP_HW_PROG_ID, rtnl_xdp_prog_hw);
>> > +     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW);
>> >       if (err)
>> >               goto err_cancel;
>> >
>>
>> So you remove all the attr and flag params just to add a conversion
>> helpers to get them based on mode?  Why?  Seems like unnecessary churn,
>> and questionable change :S
>>
>
> Fair enough. I'll address this!

I think this was actually my idea, wasn't it? :)

My thought being that if you just do the minimal change here, we'll end
up with three empty wrapper functions, which we might as well just fold
into the caller...

-Toke