Re: [PATCH 0/5] Return fds from privileged sockhash/sockmap lookup

[John Fastabend <john.fastabend@gmail.com>]

Lorenz Bauer wrote:
> On Sat, 14 Mar 2020 at 02:58, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > I'm not following. There is skb->sk. Why do you need to lookup sk ? Because
> > your hook is before demux and skb->sk is not set? Then move your hook to after?
> >
> > I think we're arguing in circles because in this thread I haven't seen the
> > explanation of the problem you're trying to solve. We argued about your
> > proposed solution and got stuck. Can we restart from the beginning with all
> > details?
> 
> Yes, that's a good idea. I mentioned this in passing in my cover
> letter, but should
> have provided more context.
> 
> Jakub is working on a patch series to add a BPF hook to socket dispatch [1] aka
> the inet_lookup function. The core idea is to control skb->sk via a BPF program.
> Hence, we can't use skb->sk.
> 
> Introducing this hook poses another problem: we need to get the struct sk from
> somewhere. The canonical way in BPF is to use the lookup_sk helpers. Of course
> that doesn't work, since our hook would invoke itself. So we need a
> data structure
> that can hold sockets, to be used by programs attached on the new hook.
> 
> Jakub's RFC patch set used REUSEPORT_SOCKARRAY for this. During LPC '19
> we got feedback that sockmap is probably the better choice. As a
> result, Jakub started
> working on extending sockmap TCP support and after a while I joined to add UDP.
> 
> Now, we are looking at what our control plane could look like. Based
> on the inet-tool
> work that Marek Majkowski has done [2], we currently have the following set up:
> 
> * An LPM map that goes from IP prefix and port to an index in a sockmap

As an aside we could do a LPM version of sockmap to avoid the extra lookup,
but thats just an optimization for later.

> * A sockmap that holds sockets
> * A BPF program that performs the business logic
> 
> inet-tool is used to update the two maps to add and remove mappings on the fly.
> Essentially, services donate their sockets either via fork+exec or SCM_RIGHTS on
> a Unix socket.

This looks a lot like one of the LBs we prototyped early on.

> 
> Once we have inserted a socket in the sockmap, it's not possible to
> retrieve it again.
> This makes it impossible to change the position of a socket in the
> map, to resize the
> map, etc. with our current design.

Is it fair to say then that you don't actually need/care about the fd it
just happens to be something stable you could grab relatively easy from
the sockmap side and push back at a sockmap?

> 
> One way to work around this is to add a persistent component to our
> control plane:
> a process can hold on to the sockets and re-build the map when necessary. The
> downsides are that upgrading the service is non-trivial (since we need
> to pass the
> socket fds) and that a failure of this service is catastrophic. Once
> it happens, we
> probably have to reboot the machine to get it into a workable state again.

Agreed this is not a good place to be in. We use the kernel maps for
persistence in many cases today, such as updates or when the application
crashes we have the nice property that the datapath keeps working without
interruption. 

> 
> We'd like to avoid a persistent service if we can. By allowing to look
> up fds from the
> sockmap, we could make this part of our control plane more robust.
> 
> 1: https://www.youtube.com/watch?v=qRDoUpqvYjY
> 2: https://github.com/majek/inet-tool
> 
> I hope this explanation helps, sorry for not being more thorough in the original
> cover letter!

Helps a lot for me at least.

So instead of fd how about,

  sock_map_lookup returns bpf_sock
  sock_map_update can consume an fd or a bpf_sock

Userland can do a dump of the sock_map then get a set of bpf_socks and
push them into another map via updates. Nothing too special compared
to other maps. In cilium for example I could plug this into our normal
flows and we would get rid of the current corner case where upgrades
and crashes lose sockmap state.

The update hooks in sock_map already know how to deal with socks so
the trick would be to do the lookup from bpf_sock to a real sock. For
that I think we can just use sk_lookup(). Maybe bpf_sock needs to
additionally include the cookie? Including the cookie in bpf_sock
seems generally useful as well. I would probably use it outside
of sock_map for example.

Thoughts? I think it helps with Alexei's concern around passing fds. 

> 
> Lorenz
> 
> -- 
> Lorenz Bauer  |  Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
> 
> www.cloudflare.com