On Mon, Jan 15, 2018 at 2:40 AM, Daniel Borkmann <dan...@iogearbox.net> wrote:
> On 01/15/2018 07:38 AM, Y Song wrote:
>> On Fri, Jan 12, 2018 at 11:23 AM, Daniel Borkmann <dan...@iogearbox.net>
>> wrote:
> [...]
>>>
>>> I've been thinking to additionally reject arithmetic on ctx
>>> pointer in adjust_ptr_min_max_vals() right upfront as well
>>> since we reject actual access in such case later on anyway,
>>> but there's a use case in tracing (in bcc) in combination
>>> with passing such ctx to bpf_probe_read(), so we cannot do
>>> that part.
>>
>> There is a reason why bcc does this. For example, suppose that we want to
>> trace kernel tracepoint, sched_process_exec,
>>
>> TRACE_EVENT(sched_process_exec,
>>
>> TP_PROTO(struct task_struct *p, pid_t old_pid,
>> struct linux_binprm *bprm),
>>
>> TP_ARGS(p, old_pid, bprm),
>>
>> TP_STRUCT__entry(
>> __string( filename, bprm->filename )
>> __field( pid_t, pid )
>> __field( pid_t, old_pid )
>> ),
>>
>> TP_fast_assign(
>> __assign_str(filename, bprm->filename);
>> __entry->pid = p->pid;
>> __entry->old_pid = old_pid;
>> ),
>>
>> TP_printk("filename=%s pid=%d old_pid=%d", __get_str(filename),
>> __entry->pid, __entry->old_pid)
>> );
>>
>> Eventually structure at
>> /sys/kernel/debug/tracing/event/sched/sched_process_exec/format:
>> ......
>> field:__data_loc char[] filename; offset:8;
>> size:4; signed:1;
>> field:pid_t pid; offset:12; size:4; signed:1;
>> field:pid_t old_pid; offset:16; size:4; signed:1;
>>
>> and "data_loc filename" is the offset in the structure where
>> "filename" is stored.
>>
>> Therefore, in bcc, the access sequence is:
>> offset = args->filename; /* field __data_loc filename */
>> bpf_probe_read(&dst, len, (char *)args + offset);
>>
>> For this kind of dynamic array in the tracepoint, the offset to access
>> certain field in ctx will be unknown at verification time.
>
> Right, that is exactly what I said in above paragraph.
>
>> So I suggest to remove the above paragraph regarding to potential ctx+offset
>> rejection.
>
> I'm confused, I mentioned we cannot reject exactly because of this
> use-case, I thought it's worth having it in the log for future
> reference so we don't forget about it since it's not too obvious.
I thought it was irrelevant to the main subject of the patch (e.g., dealing with
constant min/max, etc.) But I agree that having a log for a reference is
better since it is not too obvious.
Could you describe the "use case in bcc" part a little more in detail then?
Otherwise, it may not be really clear for some people.
Sorry for causing your confusion!
Thanks,
>
> Cheers,
> Daniel
