On Wed, 7 Oct 2020 at 14:48, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > > On Wed, 2020-10-07 at 10:17 +0000, Aleksandr Nogikh wrote: [...] > > Aleksandr Nogikh (2): > > net: store KCOV remote handle in sk_buff > > Can you explain that a bit better? What is a "remote handle"? What does > it do in the SKB? > > I guess I'd have to know more about "kcov_common_handle()" to understand > this bit. Normally, KCOV collects coverage information for the code that is executed inside the system call context. It is easy to identify where that coverage should go and whether it should be collected at all by looking at the current process. If KCOV was enabled on that process, coverage will be stored in a buffer specific to that process. Howerever, it is not always enough as some handling can happen elsewhere (e.g. in separate kernel threads). That is why remote KOV coverage collection was introduced. When it is impossible to infer KCOV-related info just by looking at the currently running process, we need to manually pass some information to the code that is of interest to us. The information takes the form of 64 bit integers (remote handles). Zero is the special value that corresponds to an empty handle. More details on KCOV and remote coverage collection can be found here: Documentation/dev-tools/kcov.rst. In this patch, we obtain the remote handle from KCOV (in this case by executing kcov_common_handle()) and attach it to newly allocated SKBs. If we're in a system call context, the SKB will be tied to the process that issued the syscall (if that process is interested in remote coverage collection). So when kcov_remote_start_common(skb_get_kcov_handle(skb)) is executed, it is possible to determine whether coverage is required and where it should be stored. I have just realized that the default kcov_handle initialization as it was implemented in this patch is not really robust. If an skb is allocated during a hard IRQ, kcov_common_handle() will return a remote handle for the interrupted thread instead of returning 0, and that is not desirable since it will occasionally lead to wrong kcov_handles. I will fix it in the next version of the patch. > > mac80211: add KCOV remote annotations to incoming frame processing > > This seems fine, but a bit too limited? You tagged > only ieee80211_tasklet_handler() which calls ieee80211_rx() > or ieee80211_tx_status(), but > > 1) I'm not even sure ieee80211_tx_status() counts (it's processing > locally generated frames after they round-tripped into the driver > (although in mesh it could be remote originated but retransmitted > frames, so I guess it makes some sense?); and > > 2) there are many other ways that ieee80211_rx() could get called. > > It seems to me it'd make more sense to (also) annotate ieee80211_rx() > itself? Yes, it definitely makes more sense to annotate ieee80211_rx() directly. Collecting coverage for ieee80211_tx_status() does not seem to be needed now and can be added later if there's a use case for it. Thank you for the suggestion. I will implement it in the second version of the patch. -- Best regards, Aleksandr
