Message-ID: <20170425113453.5c72080f@redhat.com>
Date:   Tue, 25 Apr 2017 11:34:53 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Andy Gospodarek <andy@...yhouse.net>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <ast@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "xdp-newbies@...r.kernel.org" <xdp-newbies@...r.kernel.org>,
        John Fastabend <john.fastabend@...il.com>, brouer@...hat.com
Subject: Re: xdp_redirect ifindex vs port. Was: best API for
 returning/setting egress port?

On Thu, 20 Apr 2017 10:10:08 -0700
Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:

> On Thu, Apr 20, 2017 at 08:10:51AM +0200, Jesper Dangaard Brouer wrote:
> > On Wed, 19 Apr 2017 19:56:13 -0700
> > Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> >   
> > > On Thu, Apr 20, 2017 at 12:51:31AM +0200, Daniel Borkmann wrote:  
> > > > 
> > > > Is there a concrete reason that all the proposed future cases like sockets
> > > > have to be handled within the very same XDP_REDIRECT return code? F.e. why
> > > > not XDP_TX_NIC that only assumes ifindex as proposed in the patch, and future
> > > > ones would get a different return code f.e. XDP_TX_SK only handling sockets
> > > > when we get there implementation-wise?    
> > > 
> > > yeah. let's keep redirect to sockets, tunnels, crypto and exotic things
> > > out of this discussion.
> > > XDP_REDIRECT should assume L2 raw packet is being redirected to another L2 netdev.
> > > If we make it too generic it will lose performance.
> > > 
> > > For cls_bpf the ifindex concept is symmetric. The program can access it as
> > > skb->ifindex on receive and can redirect to another ifindex via bpf_redirect() helper.
> > > Since netdev is not locked, it's actually big plus, since container management
> > > control plane can simply delete netns+veth and it goes away. The program can
> > > have dangling ifindex (if control plane is buggy and didn't update the bpf side),
> > > but it's harmless. Packets that redirect to non-existing ifindex are dropped.
> > > This approach already understood and works well, so for XDP I suggest to use
> > > the same approach initially before starting to reinvent the wheel.
> > > struct xdp_md needs ifindex field and xdp_redirect() helper that redirects
> > > to L2 netdev only. That's it. Simple and easy.
> > > I think the main use cases in John's and Jesper's minds is something like
> > > xdpswitch where packets are coming from VMs and from physical eths and
> > > being redirected to either physical eth or to VM via upcoming vhost+xdp support.
> > > This xdp_md->ifindex + xdp_redirect(to_ifindex) will solve it just fine.
> > > 
> > > Once we have vhost+xdp and all other bits implemented, we must come back
> > > to this discussion about having port mapping table. As I mentioned
> > > during netconf I think it's very useful, but I don't think we should
> > > gate vhost+xdp and xdp_redirect work on this discussion.
> > > As far as this port mapping table we would need 'port' field in xdp_md as well
> > > and xdp_redirect_port() done via helper or via extra 'out_port' field in xdp_md
> > > plus another XDP_REDIRECT_PORT action code.  
> > 
> > Guess it would be easier to talk about if we name it "ingress_port" and
> > "egress_port".
> >   
> > > The actual port table (array) should be populated by user space with netdevs
> > > and these netdev will have their refcnt incremented. Then we'll have discussion
> > > what to do with netdev_unregister notifiers, whether they should be auto-removed
> > > from port table or bpf should have a chance to be notified and act on it.
> > > Such port mapping will allow us to optimize inevitable call
> > > dev_get_by_index_rcu(dev_net(skb->dev), ri->ifindex);
> > > away, since netdevs will be stored in the port table and direct deref
> > > port_map_array[xdp_md->out_port] will give us target netdev quickly.  
> > 
> > I agree with above paragraph, and is happy that you can see that this
> > will actually be faster than using ifindex'es.
> >   
> > > It's nice optimization and there are other more powerful optimizations we
> > > can do with such port table (since we will know in advance which netdevs
> > > the program will be redirecting too), but I still think we should do
> > > ifindex based xdp_redirect first and only add this port table later.  
> > 
> > No, we cannot first do an ifindex based xdp_redirect. The point of the
> > port table is to sandbox which ports XDP can use.  
> 
> hmm. port table cannot sandbox the ports. The only thing it does
> from 'safety' point of view is moving the checks from run-time into
> static insertion time.
> So the checks that we would do on netdev after looking it up
> based on ifindex are the same checks we will do at insertion time
> into port table. The user space will insert/delete them live
> from that port table, so from program point of view it's exactly
> the same as ifindex. The ports can disappear and can be added
> while the program is running.

I agree, that from the eBPF programs point of view using an ifindex or a
port number is the same.  And I do like this model, that this is just a
number seem from bpf.  It provides a clean separation between the
kernel and ebpf program world.


> Note the very first bpf patchset years ago contained the port table
> abstraction. ovs has concept of vports as well. These two very
> different projects needed port table to provide a layer of
> indirection between ifindex==netdev and virtual port number.
> This is still the case and I'd like to see this port table to be
> implemented for both cls_bpf and xdp. In that sense xdp is not
> special.

Glad to hear you want to see this implemented, I will start coding on
this then.  Good point with cls_bpf, I was planning to make this port
table strongly connected to XDP, guess I should also think of cls_bpf.


> > XDP is different than TC/cls_bpf, as it does "bypass", there is no
> > other layer that can stop or inspect these packets. The TC hooks
> > redirect into the network stack, which have all the usual facilities
> > available for filtering, inspection and debugging what is going on
> > (e.g. tcpdump works for TC redirect).  
> 
> not true. when bpf_redirect() drops the packet due to incorrect ifindex
> that packet disappears without a trace. No tcpdump and no counter.
> And this is fine. We can add tracepoint there for debugging,
> but it wasn't a problem for anyone who's using it today, so it's
> 'nice to have', but certainly not mandatory.
 
I'm not worried about the DROP case, I agree that is fine (as you also
say).  The problem is unintentionally sending a packet to a wrong
ifindex.  This is clearly an eBPF program error, BUT with XDP this
becomes a very hard to debug program error.  With TC-redirect/cls_bpf
we can tcpdump the packets, with XDP there is no visibility into this
happening (the NSA is going to love this "feature").  Maybe we could add
yet-another tracepoint to allow debugging this.  My proposal that we
simply remove the possibility for such program errors, by as you say
move the validation from run-time into static insertion-time, via a
port table.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Powered by blists - more mailing lists