Re: [bpf-next PATCH 05/16] bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data

On 03/07/2018 04:25 AM, John Fastabend wrote:
[...]

Thought about this a bit more and chatted with Daniel a bit. I think
a better solution is to set data_start = data_end = 0 by default in the
sendpage case. This will disallow any read/writes into the sendpage
data. Then if the user needs to read/write data we can use a helper
bpf_sk_msg_pull_data(start_byte, end_byte) which can pull the data into a
linear buffer as needed. This will ensure any user writes will not
change data after the BPF verdict (your concern). Also it will minimize
the amount of data that needs to be copied (my concern). In some of my
use cases where no data is needed we can simple not use the helper. Then
on the sendmsg side we can continue to set the (data_start, data_end)
pointers to the first scatterlist element. But, also use this helper to
set the data pointers past the first scatterlist element if needed. So
if someone wants to read past the first 4k bytes on a large send for
example this can be done with the above helper. BPF programs just
need to check (start,end) data pointers and can be oblivious to
if the program is being invoked by a call from sendpage or sendmsg.

I think this is a fairly elegant solution. Finally we can further
optimize later with a flag if needed to cover the case where we
want to read lots of bytes but _not_ do the copy. We can debate
the usefulness of this later with actual perf data.

All this has the added bonus that all I need is another patch on
top to add the helper. Pseudo code might look like this,

my_bpf_prog(struct sk_msg_md *msg) {
	void *data_end = msg->data_end;
	void *data_start = msg->data_start;

	need = PARSE_BYTES;

	// ensure user actually sent full header
	if (msg->size < PARSE_BYTES) {
		bpf_msg_cork(PARSE_BYTES);
		return SK_DROP;
	}

	/* ensure we can read full header, if this is a
	 * sendmsg system call AND PARSE_BYTES are all in
	 * the first scatterlist elem this is a no-op.
	 * If this is a sendpage call will put PARSE_BYTES
	 * in a psock buffer to avoid user modifications.
	 */
	if (data_end - data_start < PARSE_BYTES) {

I think it might need to look like 'data_start + PARSE_BYTES > data_end'
for verifier to recognize (unless LLVM generates code that way).

		err = bpf_sk_msg_pull_data(0, PARSE_BYTES, flags);
		if (err)
			return SK_DROP;

Above should be:

		if (unlikely(err || data_start + PARSE_BYTES > data_end))
			return SK_DROP;

Here for the successful case, you need to recheck since data pointers
were invalidated due to the helper call. bpf_sk_msg_pull_data() would
for the very first case potentially be called unconditionally at prog
start though when you start out with 0 len anyway, basically right after
msg->size test.

	}

	// we have the full header parse it now
	verdict = my_bpf_header_parser(msg);
	return verdict;
}

Future optimization can work with prologue to pull in bytes
more efficiently. And for what its worth I found a couple bugs
in the error path of the sendpage hook I can fix in the v2 as well.

What do you think? 

@Daniel, sound more or less like what you were thinking?

Yes, absolutely what I was thinking.

We have exactly the same logic in tc/BPF today for the case when the direct
packet access test fails and we want to pull in skb data from non-linear
area, so we can in such case just call bpf_skb_pull_data(skb, len) and redo
the test to access it privately after that.

Thanks,
Daniel