ebpf-kernel/length_ebpf.p4 test fails to pass eBPF verifier

[thomascalvert-xlnx]

With p4c 555029d03, running Ubuntu 20.04 with kernel 5.4.0-144, I see the above test fail:

0: (61) r2 = *(u32 *)(r1 +80)
1: (61) r1 = *(u32 *)(r1 +76)
2: (bc) w3 = w2
3: (1c) w3 -= w1
last_idx 3 first_idx 0
regs=8 stack=0 before 2: (bc) w3 = w2
regs=4 stack=0 before 1: (61) r1 = *(u32 *)(r1 +76)
regs=4 stack=0 before 0: (61) r2 = *(u32 *)(r1 +80)
R3 32-bit pointer arithmetic prohibited

Note that kernel tests need to be run by root, and are skipped otherwise. The simplest way to reproduce is, from your build directory:

sudo ctest --output-on-failure -R 'ebpf-kernel/testdata/p4_16_samples/length_ebpf.p4'

The actual C instruction which causes the issue is:

u32 ebpf_pkt_len = ebpf_packetEnd - ebpf_packetStart;

which I believe is used to support the packet.length() call used in the program's parser. I suspect that, since no other test program uses this method, the variable is usually optimised out. (The only other uses are for trace messages which again are generally disabled.)

[fruffy]

I believe the eBPF back end has no dedicated maintainer. It might take some time until this bug is fixed.

[mihaibudiu]

The ebpf backend contributors have been usually very responsive.

[fruffy]

The ebpf backend contributors have been usually very responsive.

This bug is in the classic eBPF kernel tests, not sure whether it extends to the PSA eBPF portion.

@tatry @osinstom

Is this a bug in parts of the code that you depend on?

[tatry]

I believe the problem is related to classic eBPF only (filter model). Packet length is calculated here:

p4c/backends/ebpf/ebpfProgram.cpp

Lines 245 to 246 in 27b7171

	builder->appendFormat("u32 %s = %s - %s", lengthVar.c_str(), packetEndVar.c_str(),
	packetStartVar.c_str());

which generates aforementioned problematic code. This is not used for PSA architecture.

As a side note, PSA architecture has its own method to calculate packet length here:

p4c/backends/ebpf/psa/ebpfPipeline.cpp

Lines 164 to 171 in 27b7171

	void EBPFPipeline::emitPacketLength(CodeBuilder *builder) {
	if (this->is<XDPIngressPipeline>() || this->is<XDPEgressPipeline>()) {
	builder->appendFormat("%s->data_end - %s->data", this->contextVar.c_str(),
	this->contextVar.c_str());
	} else {
	builder->appendFormat("%s->len", this->contextVar.c_str());
	}
	}

[thomascalvert-xlnx]

It's not immediately apparent to me why the PSA target backend seems to duplicate so much logic. Probably because I'm not very familiar with the compiler.

In any case the fix is fairly straightforward by just copying the strategy which @tatry helpfully points out. With the patch in PR #4159 the test is now passing.

[thomascalvert-xlnx]

Fix pushed. Copying this from the PR for reference:

After some more digging I found this patch, which seems to suggest that the kernel verifier accepts this construct as of kernel 5.9. This is consistent with the observed behaviour.

In summary: this patch only applies to kernels versions prior to 5.9. At a glance, the CI runs all seem to be running more recent kernels (Ubuntu20: 5.15.0-1046-azure; all others: 6.2.0-1011-azure)