[PATCH] D61809: [BPF] Preserve original struct/union type name/access index and array subscripts

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@rsmith @eli.friedman Thanks for your comments. I fully agree that it seems awkward that we have both GEP and intrinsic generation. I will try to do some experiments here to only have intrinsic generation. My only concern is possible performance degradation. I will post here once I got concrete implementation and numbers.

@rsmith regarding to your concerns about struct size change. Yes, the structure size may indeed change. Currently, we plan only to support field fields (non-struct/union type with 1/2/4/8 bytes, and strings). These are most common use cases. Let me explain a little bit more.

Here, we are targeting the bpf_probe_read (https://github.com/torvalds/linux/blob/master/include/uapi/linux/bpf.h#L532-L537)

  int bpf_probe_read(void *dst, u32 size, const void *src)

which tries to copy some data from kernel (src pointer) to the stack of the bpf program.

Here, if "src" points to a structure, and user intends to copy the whole structure out of kernel, then "size" may need to be different for different kernels if different kernels indeed have different structure size. 
But first, in all our use cases, users typically do not read the whole structure. Second, if "size" does change, we request users to use multiple versioning (using patchable global variables) with possibly different "size" values for different kernel versions. 
Note that "size" must be constant for bpf verifier to succeed.
(https://github.com/torvalds/linux/blob/master/kernel/trace/bpf_trace.c#L138-L156)

Just gave another two examples here about reading kernel memories in iovisor/bcc. Note that
bcc will transform certain pointer access "b->c" to "bpf_probe_read(&dest, size, &b->c)" internally.

The bcc tool biosnoop.py contain several bpf probe read:

  https://github.com/iovisor/bcc/blob/master/tools/biosnoop.py#L110
        data.len = req->__data_len;
  https://github.com/iovisor/bcc/blob/master/tools/biosnoop.py#L117
        data.len = req->__data_len;
  https://github.com/iovisor/bcc/blob/master/tools/biosnoop.py#L118
        data.sector = req->__sector;
  https://github.com/iovisor/bcc/blob/master/tools/biosnoop.py#L120
        struct gendisk *rq_disk = req->rq_disk;
  https://github.com/iovisor/bcc/blob/master/tools/biosnoop.py#L121-L122
       bpf_probe_read(&data.disk_name, sizeof(data.disk_name),
                       rq_disk->disk_name);

They are all primitive types and strings.

Another example for bcc tool tcpconnect.py

  https://github.com/iovisor/bcc/blob/master/tools/tcpconnect.py#L128
    u16 dport = skp->__sk_common.skc_dport;
  https://github.com/iovisor/bcc/blob/master/tools/tcpconnect.py#L136-L137
        data4.saddr = skp->__sk_common.skc_rcv_saddr;
        data4.daddr = skp->__sk_common.skc_daddr;    

.....

In summary, right now, the to-be-read kernel memory size (through a specific bpf_probe_read() call)
won't change between different kernel versions. So we do not need to handle structure allocation size change here.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61809/new/

https://reviews.llvm.org/D61809