Re: [PATCH bpf-next v2 1/5] bpf: Support injecting chain calls into BPF programs on load

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Mon, Oct 07, 2019 at 12:11:31PM +0200, Toke Høiland-Jørgensen wrote:
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> 
> > On Fri, Oct 04, 2019 at 07:22:41PM +0200, Toke Høiland-Jørgensen wrote:
> >> From: Toke Høiland-Jørgensen <toke@redhat.com>
> >> 
> >> This adds support for injecting chain call logic into eBPF programs before
> >> they return. The code injection is controlled by a flag at program load
> >> time; if the flag is set, the verifier will add code to every BPF_EXIT
> >> instruction that first does a lookup into a chain call structure to see if
> >> it should call into another program before returning. The actual calls
> >> reuse the tail call infrastructure.
> >> 
> >> Ideally, it shouldn't be necessary to set the flag on program load time,
> >> but rather inject the calls when a chain call program is first loaded.
> >> However, rewriting the program reallocates the bpf_prog struct, which is
> >> obviously not possible after the program has been attached to something.
> >> 
> >> One way around this could be a sysctl to force the flag one (for enforcing
> >> system-wide support). Another could be to have the chain call support
> >> itself built into the interpreter and JIT, which could conceivably be
> >> re-run each time we attach a new chain call program. This would also allow
> >> the JIT to inject direct calls to the next program instead of using the
> >> tail call infrastructure, which presumably would be a performance win. The
> >> drawback is, of course, that it would require modifying all the JITs.
> >> 
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > ...
> >>  
> >> +static int bpf_inject_chain_calls(struct bpf_verifier_env *env)
> >> +{
> >> +	struct bpf_prog *prog = env->prog;
> >> +	struct bpf_insn *insn = prog->insnsi;
> >> +	int i, cnt, delta = 0, ret = -ENOMEM;
> >> +	const int insn_cnt = prog->len;
> >> +	struct bpf_array *prog_array;
> >> +	struct bpf_prog *new_prog;
> >> +	size_t array_size;
> >> +
> >> +	struct bpf_insn call_next[] = {
> >> +		BPF_LD_IMM64(BPF_REG_2, 0),
> >> +		/* Save real return value for later */
> >> +		BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
> >> +		/* First try tail call with index ret+1 */
> >> +		BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),
> >> +		BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, 1),
> >> +		BPF_RAW_INSN(BPF_JMP | BPF_TAIL_CALL, 0, 0, 0, 0),
> >> +		/* If that doesn't work, try with index 0 (wildcard) */
> >> +		BPF_MOV64_IMM(BPF_REG_3, 0),
> >> +		BPF_RAW_INSN(BPF_JMP | BPF_TAIL_CALL, 0, 0, 0, 0),
> >> +		/* Restore saved return value and exit */
> >> +		BPF_MOV64_REG(BPF_REG_0, BPF_REG_6),
> >> +		BPF_EXIT_INSN()
> >> +	};
> >
> > How did you test it?
> > With the only test from patch 5?
> > +int xdp_drop_prog(struct xdp_md *ctx)
> > +{
> > +       return XDP_DROP;
> > +}
> >
> > Please try different program with more than one instruction.
> > And then look at above asm and think how it can be changed to
> > get valid R1 all the way to each bpf_exit insn.
> > Do you see amount of headaches this approach has?
> 
> Ah yes, that's a good point. It seems that I totally overlooked that
> issue, somehow...
> 
> > The way you explained the use case of XDP-based firewall plus XDP-based
> > IPS/IDS it's about "knows nothing" admin that has to deal with more than
> > one XDP application on an unfamiliar server.
> > This is the case of debugging.
> 
> This is not about debugging. The primary use case is about deploying
> multiple, independently developed, XDP-enabled applications on the same
> server.
> 
> Basically, we want the admin to be able to do:
> 
> # yum install MyIDS
> # yum install MyXDPFirewall
> 
> and then have both of those *just work* in XDP mode, on the same
> interface.
> 
> I originally started solving this in an XDP-specific way (v1 of this
> patch set), but the reactions to that was pretty unanimous that this
> could be useful as a general eBPF feature. Do you agree with this?

Chaining in general is useful, but
yum install ids
yum install firewall
is not.

Say, xdp doesn't exist. Such ids and firewall will be using iptables.
And they will collide and conflict all over it.
The problem of mixing unrelated ids and firewall is not new.