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:
> > On 11/14/17 12:25 PM, Daniel Borkmann wrote:
> > > 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.
>
> > 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));
>
> > 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.
>
> > 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.
>
> > 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.
>
> > To workaround the immediate problem, I tested the following hack
> > with bcc and it works fine.
>
> > 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.
