[DTrace-devel] [PATCH 2/2] Implement the pcap action

[Kris Van Hees]

On Wed, Oct 11, 2023 at 12:33:15PM +0100, Alan Maguire via DTrace-devel wrote:
> On 10/10/2023 23:31, Eugene Loh via DTrace-devel wrote:
> > On 10/10/23 17:33, Kris Van Hees wrote:
> > 
> >> On Tue, Oct 10, 2023 at 01:39:04PM -0400, Eugene Loh via DTrace-devel
> >> wrote:
> >>> On 10/10/23 00:35, Kris Van Hees wrote:
> >>>> On Mon, Oct 09, 2023 at 11:54:32PM -0400, Eugene Loh via
> >>>> DTrace-devel wrote:
> >>>>> I am not familiar with pcap...
> >>> In legacy DTrace, in dtrace/dtrace_probe.c, dtrace_probe_pcap() deals
> >>> with
> >>> some complications around fragments.  Why are there no such
> >>> complications in
> >>> this patch?
> >> Like your io provider patch, there are still some (uncommon) cases
> >> that are not
> >> fully covered.  In discussing this with Alan Maguire, we decided that
> >> the case
> >> of fragment reconstruction is sufficiently rare that this version is
> >> definitely
> >> the most important support that is needed.  Support for fragmented
> >> packets is
> >> a follow-up development.
> > 
> > Ah.  Worth mentioning in the commit message or even in dt_cg_act_pcap()?
> >
> 
> I think so. The key point for pcap is having non-linear skbs isn't rare;
> rather wanting to capture anything from the non-linear skb portion is.
> It's very unusual in my experience for protocol headers to be in the
> non-linear portion of the skb, and that's generally what we're
> interested in seeing with pcap. So dealing with the non-linear portion
> later seems like a totally reasonable approach which gives 99% of
> users what they want.

Yes, I didn't express that very well - thanks for the clarification.  I indeed
meant that the case of *needing* the non-linear portion is quite rare.  Not
that it is rare for there to be a non-linear portion.

Again, thanks for clarifying that.

> I always struggle with the skb format but one thing we need to be
> careful about I think is correctly collecting the skb size _and_ the
> captured size - this corresponds to skb_headlen() for the reasons
> described above (we're only capturing the linear portion, the "head
> length").
> 
> But skb_headlen() != skb->len, it's actually skb->len - skb->data_len.
> For a fully linear packet, skb_headlen == skb->len but for a
> non-linear one it does not.
> 
> So I _think_ what we want to do in BPF context is
> 
> 1. compute skb_headlen(skb) as above
> 2. record skb_headlen
> 3. record skb_len
> 4. bpf_skb_probe_read skb_headlen bytes from skb->data to get the
> linear portion of the skb data.

Yes, I always get this wrong.  I'll correct the calculation.

> Then when pcap'ing we can correctly record caplen as skb_headlen
> while also recording actual packet length; this will allow pcap to
> report that we've not captured everything in those cases where portion
> of the skb is nonlinear. This is important as the consumer then knows
> that data is missing.

I believe that the consumer is already doing this part.  I'll double-check.
If anything, it is *supposed* to already be doing that because we always pass
caplen bytes of data in the trace buffer from the producer to the consumer.
So the consumer is responsible for handling the case where the recorded packet
data length is less than caplen.

So as long as the data to be copied is corrected as you outline above, we will
be storing that (corrected) value is size in the buffer, and all ought to be
well.

And I am looking at the case of dealing with non-linear data.  But that won't
be in the first version for sure.

> Apologies if I've got any of the above wrong, I usually find myself
> rereading "How SKBs work" [1] a few times a year ;-)
> 
> [1] http://vger.kernel.org/~davem/skb_data.html

Thanks for the reminder!