Re: len = bpf_probe_read_str(); bpf_perf_event_output(... len) == FAIL

On Tue, Nov 21, 2017 at 11:29:05AM -0300, Arnaldo Carvalho de Melo wrote:

Em Tue, Nov 14, 2017 at 02:58:24PM -0800, Yonghong Song escreveu:

quoted

On 11/14/17 12:25 PM, Daniel Borkmann wrote:

quoted

Yeah, I know, that's what I mentioned earlier in this thread to resolve it,
but do we really want to add this hack everywhere? :( Potentially any function
having ARG_CONST_SIZE would need to handle size 0 and bail out again in their
helper implementation and it ends up that progs start relying on this runtime
check where we won't be able to get rid of it later on anymore.

quoted

The compiler actually does the right thing for the below code:
         int ret = bpf_probe_read_str(filename, sizeof(filename),
                                      filename_ptr);
         if (ret > 0)
           bpf_perf_event_output(ctx, &__bpf_stdout__,BPF_F_CURRENT_CPU,
                           filename, ret & (sizeof(filename) - 1));

quoted

Just from the above code without consulting bpf_probe_read_str internals, it
is totally possible that ret = 128, then
ret & (sizeof(filename) - 1) = 0.

quoted

The issue is that the verifier did not set the "ret" initial range as (-inf,
sizeof(filename) - 1). We could have this information associated with helper
and feed back to verifier.

quoted

If we have this range, later for ret & (sizeof(filename) - 1) with ret >= 1,
the verifier should be able to conclude
 ret & (sizeof(filename) - 1) >= 1.

quoted

To workaround the immediate problem, I tested the following hack
with bcc and it works fine.

quoted

BPF_PERF_OUTPUT(events);
int trace(struct pt_regs *ctx) {
  char filename[128];
  int ret = bpf_probe_read_str(filename, sizeof(filename), 0);
  if (ret > 0) {
    if (ret == 1)
          events.perf_submit(ctx, filename, ret);
    else if (ret < 128)
          events.perf_submit(ctx, filename, ret);

...

SEC("prog=do_sys_open filename")
int prog(void *ctx, int err, char *filename_ptr)
{
	char filename[128];
	int len = bpf_probe_read_str(filename, sizeof(filename), filename_ptr); 
	if (len > 0) {
		if (len == 1)
       			perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename, len);
		else if (len < 128)
       			perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename, len);

yeah sorry about this hack. Gianluca reported this issue as well.
Yonghong fixed it for bpf_probe_read only. We will extend
the fix to bpf_probe_read_str() and bpf_perf_event_output() asap.
The above workaround gets too much into llvm and verifier details
we should strive to make bpf program writing as easy as possible.