Message-ID: <cc514705-9974-9328-b66e-b57cfa61d417@fb.com>
Date:   Tue, 10 Sep 2019 07:43:04 +0000
From:   Yonghong Song <yhs@...com>
To:     Alan Maguire <alan.maguire@...cle.com>
CC:     "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        Martin Lau <kafai@...com>, Song Liu <songliubraving@...com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
        "hawk@...nel.org" <hawk@...nel.org>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "quentin.monnet@...ronome.com" <quentin.monnet@...ronome.com>,
        Andrey Ignatov <rdna@...com>,
        "joe@...d.net.nz" <joe@...d.net.nz>,
        "acme@...hat.com" <acme@...hat.com>,
        "jolsa@...nel.org" <jolsa@...nel.org>,
        "alexey.budankov@...ux.intel.com" <alexey.budankov@...ux.intel.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "namhyung@...nel.org" <namhyung@...nel.org>,
        "sdf@...gle.com" <sdf@...gle.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "shuah@...nel.org" <shuah@...nel.org>,
        "peter@...ensteyn.nl" <peter@...ensteyn.nl>,
        "ivan@...udflare.com" <ivan@...udflare.com>,
        Andrii Nakryiko <andriin@...com>,
        "bhole_prashant_q7@....ntt.co.jp" <bhole_prashant_q7@....ntt.co.jp>,
        "david.calavera@...il.com" <david.calavera@...il.com>,
        "danieltimlee@...il.com" <danieltimlee@...il.com>,
        Takshak Chahande <ctakshak@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "toke@...hat.com" <toke@...hat.com>,
        "jbenc@...hat.com" <jbenc@...hat.com>
Subject: Re: [RFC bpf-next 2/7] bpf: extend bpf_pcap support to tracing
 programs



On 9/9/19 11:25 PM, Alan Maguire wrote:
> On Sun, 8 Sep 2019, Yonghong Song wrote:
>   
>> For net side bpf_perf_event_output, we have
>> static unsigned long bpf_skb_copy(void *dst_buff, const void *skb,
>>                                     unsigned long off, unsigned long len)
>> {
>>           void *ptr = skb_header_pointer(skb, off, len, dst_buff);
>>
>>           if (unlikely(!ptr))
>>                   return len;
>>           if (ptr != dst_buff)
>>                   memcpy(dst_buff, ptr, len);
>>
>>           return 0;
>> }
>>
>> BPF_CALL_5(bpf_skb_event_output, struct sk_buff *, skb, struct bpf_map
>> *, map,
>>              u64, flags, void *, meta, u64, meta_size)
>> {
>>           u64 skb_size = (flags & BPF_F_CTXLEN_MASK) >> 32;
>>
>>           if (unlikely(flags & ~(BPF_F_CTXLEN_MASK | BPF_F_INDEX_MASK)))
>>                   return -EINVAL;
>>           if (unlikely(skb_size > skb->len))
>>                   return -EFAULT;
>>
>>           return bpf_event_output(map, flags, meta, meta_size, skb, skb_size,
>>                                   bpf_skb_copy);
>> }
>>
>> It does not really consider output all the frags.
>> I understand that to get truly all packet data, frags should be
>> considered, but seems we did not do it before? I am wondering
>> whether we need to do here.
> 
> Thanks for the feedback! In experimenting with packet capture,
> my original hope was to keep things simple and avoid fragment parsing
> if possible. However if scatter-gather is enabled for the networking
> device, or indeed if it's running in a VM it turns out a lot of the
> interesting packet data ends up in the fragments on transmit (ssh
> headers, http headers etc).  So I think it would be worth considering
> adding support for fragment traversal.  It's not needed as much
> in the skb program case - we can always pullup the skb - but in
> the tracing situation we probably wouldn't want to do something
> that invasive in tracing context.

Agree that in tracing context, we should avoid push/pull skb. It is
indeed invasive.

> 
> Fragment traversal might be worth breaking out as a separate patchset,
> perhaps triggered by a specific flag to bpf_skb_event_output?

This can be done for bpf_skb_event_output as the context is a sk_buff.
And you can just follow the frags to copy the whole thing without
bpf_probe_read().

> 
> Feedback from folks at Linux Plumbers (I hope I'm summarizing correctly)
> seemed to agree with what you mentioned WRT the first patch in this
> series.  The gist was we probably don't want to force the metadata to be a
> specific packet capture type; we'd rather use the existing perf event
> mechanisms and if we are indeed doing packet capture, simply specify that
> data in the program as metadata.

Agree, you can have whatever metadata you have for bpf_perf_event_output.

> 
> I'd be happy with that approach myself if I could capture skb
> fragments in tracing programs - being able to do that would give
> equivalent functionality to what I proposed but without having a packet
> capture-specific helper.

That won't work for tracing program. Full of bpf_probe_read()
in tracing version of packet copying is not nice either.

We may still need a different helper for tracing programs.

I think we need something like below:
   - vmlinux BTF at /sys/kernel/btf/kernel, is loaded into kernel.
     (/sys/kernel/btf/kernel is the source of truth)
   - For a tracing bpf program, if that function eventually
     copy  helper
         bpf_skb_event_output(..., skb, ...)
     the verifier needs to verify skb is indeed a valid skb
     by tracing back to one of parameters.

     Here, I use skb as an example, maybe it can be extended
     to other data structures as well.

With this approach, you can reuse some of functions from
tracing side to deal with frag copying and no bpf_probe_read()
is needed.

Here, I use skb as an example, maybe it can be extended
to other data structures as well if needed.

>>
>> If we indeed do not need to handle frags here, I think maybe
>> bpf_probe_read() in existing bpf kprobe function should be
>> enough, we do not need this helper?
>>
> 
> Certainly for many use cases, that will get you most of what you need -
> particularly if you're just looking at L2 to L4 data. For full packet
> capture however I think we may need to think about fragment traversal.
> 
>>> +
>>> +/* Derive protocol for some of the easier cases.  For tracing, a probe point
>>> + * may be dealing with packets in various states. Common cases are IP
>>> + * packets prior to adding MAC header (_PCAP_TYPE_IP) and a full packet
>>> + * (_PCAP_TYPE_ETH).  For other cases the caller must specify the
>>> + * protocol they expect.  Other heuristics for packet identification
>>> + * should be added here as needed, since determining the packet type
>>> + * ensures we do not capture packets that fail to match the desired
>>> + * pcap type in BPF_F_PCAP_STRICT_TYPE mode.
>>> + */
>>> +static inline int bpf_skb_protocol_get(struct sk_buff *skb)
>>> +{
>>> +	switch (htons(skb->protocol)) {
>>> +	case ETH_P_IP:
>>> +	case ETH_P_IPV6:
>>> +		if (skb_network_header(skb) == skb->data)
>>> +			return BPF_PCAP_TYPE_IP;
>>> +		else
>>> +			return BPF_PCAP_TYPE_ETH;
>>> +	default:
>>> +		return BPF_PCAP_TYPE_UNSET;
>>> +	}
>>> +}
>>> +
>>> +BPF_CALL_5(bpf_trace_pcap, void *, data, u32, size, struct bpf_map *, map,
>>> +	   int, protocol_wanted, u64, flags)
>>
>> Up to now, for helpers, verifier has a way to verifier it is used
>> properly regarding to the context. For example, for xdp version
>> perf_event_output, the help prototype,
>>     BPF_CALL_5(bpf_xdp_event_output, struct xdp_buff *, xdp, struct
>> bpf_map *, map,
>>              u64, flags, void *, meta, u64, meta_size)
>> the verifier is able to guarantee that the first parameter
>> has correct type xdp_buff, not something from type cast.
>>     .arg1_type      = ARG_PTR_TO_CTX,
>>
>> This helper, in the below we have
>>     .arg1_type	= ARG_ANYTHING,
>>
>> So it is not really enforced. Bringing BTF can help, but type
>> name matching typically bad.
>>
>>
> One thing we were discussing - and I think this is similar to what
> you're suggesting - is to investigate if there might be a way to
> leverage BTF to provide additional guarantees that the tracing
> data we are handling is indeed an skb.  Specifically if we
> trace a kprobe function argument or a tracepoint function, and
> if we had that guarantee, we could perhaps invoke the skb-style
> perf event output function (trace both the skb data and the metadata).
> The challenge would be how to do that type-based matching; we'd
> need the function argument information from BTF _and_ need to
> somehow associate it at probe attach time.
> 
> Thanks again for looking at the code!
> 
> Alan
> 

Powered by blists - more mailing lists