On Sat, 2020-11-21 at 10:35 -0800, Jakub Kicinski wrote: > On Sat, 21 Nov 2020 19:12:21 +0100 Johannes Berg wrote: > > > 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" - this task creates an SKB, etc, and all of the code that this task executes is captured and the coverage data is reported - 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. 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. 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(). 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 > 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 ... johannes
