On Sat, 21 Nov 2020 20:30:44 +0100 Johannes Berg wrote:

On Sat, 2020-11-21 at 10:35 -0800, Jakub Kicinski wrote:

quoted

On Sat, 21 Nov 2020 19:12:21 +0100 Johannes Berg wrote:  

quoted

quoted

So I'm leaning towards reverting the whole thing. You can attach
kretprobes and record the information you need in BPF maps.    

I'm not going to object to reverting it (and perhaps redoing it better
later), but I will point out that kretprobe isn't going to work, you
eventually need kcov_remote_start() to be called in strategic points
before processing the skb after it bounced through the system.

IOW, it's not really about serving userland, it's about enabling (and
later disabling) coverage collection for the bits of code it cares
about, mostly because collecting it for _everything_ is going to be too
slow and will mess up the data since for coverage guided fuzzing you
really need the reported coverage data to be only about the injected
fuzz data...  

All you need is make kcov_remote_start_common() be BPF-able, like 
the LSM hooks are now, right? And then BPF can return whatever handle 
it pleases.  

Not sure I understand. Are you saying something should call
"kcov_remote_start_common()" with, say, the SKB, and leave it to a mass
of bpf hooks to figure out where the SKB got cloned or copied or
whatnot, track that in a map, and then ... no, wait, I don't really see
what you mean, sorry.

IIUC, fundamentally, you have this:

 - at the beginning, a task is tagged with "please collect coverage
   data for this handle"

Write the tag into task local storage, or map indexed by PID.

 - this task creates an SKB, etc, and all of the code that this task
   executes is captured and the coverage data is reported

kprobe the right places to record the skb -> handle mapping.

 - However, the SKB traverses lots of things, gets copied, cloned, or
   whatnot, and eventually leaves the annotated task, say for further
   processing in softirq context or elsewhere.

Which is fine.

Now since the whole point is to see what chaos this SKB created from
beginning (allocation) to end (free), since it was filled with fuzzed
data, you now have to figure out where to pick back up when the SKB is
processed further.

This is what the infrastructure was meant to solve. But note that the
SKB might be further cloned etc, so in order to track it you'd have to
(out-of-band) figure out all the possible places where it could
be reallocated, any time the skb pointer could change.

Ack, you have to figure out all the places anyway, the question is
whether you put probes there or calls in the source code.

Shifting the maintenance burden but also BPF is flexibility.

Then, when you know you've got interesting code on your hands, like in
mac80211 that was annotated in patch 3 here, you basically say

  "oohhh, this SKB was annotated before, let's continue capturing
   coverage data here"

(and turn it off again later by the corresponding kcov_remote_stop().

Yup, the point is you can feed a raw skb pointer (and all other
possible context you may want) to a BPF prog in kcov_remote_start() 
and let BPF/BTF give you the handle it recorded in its maps.

So the only way I could _possibly_ see how to do this would be to

 * capture all possible places where the skb pointer can change
 * still call something like skb_get_kcov_handle() but let it call out
   to a BPF program to query a map or something to figure out if this
   SKB has a handle attached to it

quoted

Or if you don't like BPF or what to KCOV BPF itself in the future you
can roll your own mechanism. The point is - this should be relatively
easily doable out of line...  

Seems pretty complicated to me though ...

It is more complicated. We can go back to an skb field if this work is
expected to yield results for mac80211. Would you mind sending a patch?