On Wed, Nov 18, 2020 at 3:21 AM Alexander Bulekov <alx...@bu.edu> wrote: > > On 201116 1805, Andrey Konovalov wrote: > > On Mon, Nov 16, 2020 at 9:35 AM Dmitry Vyukov <dvyu...@google.com> wrote: > > > > > > On Mon, Nov 16, 2020 at 3:39 AM Alexander Bulekov <alx...@bu.edu> wrote: > > > > > > > > Hello, > > > > I'm trying to collect coverage over the syscalls issued by my process, > > > > as well as the kthreads spawned as a result of these syscalls > > > > (eg coverage over vhost ioctls and the worker kthread). Is there a way > > > > to collect coverage with both KCOV_REMOTE_ENABLE(with common_handle) and > > > > KCOV_ENABLE, simultaneously? > > > > > > > > Based on the code it seems that these two modes are mutually > > > > exclusive within a single task, but I don't think this is mentioned in > > > > the Documentation, so I want to make sure I'm not missing something. > > > > > > Hi Alex, > > > > > > Yes, it's probably not supported within a single task. The easiest way > > > to verify is to try it ;) > > > > > > It is possible to collect both coverages, but you will need 2 threads > > > (one just to set up remote KCOV). > > > > > > Unless I am missing any fundamental limitations, I would say it would > > > be reasonable to support this within a single task as well. > > > > I think the reason we did that initially, is because we don't care > > about normal coverage for USB emitting pseudo-syscalls. Filed a bug > > for this: https://bugzilla.kernel.org/show_bug.cgi?id=210225 > > I'm interested in adding support for this. Looking through the code, I > can think of ~two approaches: > > 1.) Allow the same kcov fd to be used to track coverage with both > KCOV_REMOTE_ENABLE and KCOV_ENABLE. If we try to use the same coverage > bitmap for both the remote and the local coverage, I think the local > part would have to deal with the kcov_remote_lock. If the local part > continues to write directly into the user-space coverage-area, as it > does now, it seems it would require locking for each __sanitizer_cov > call. Alternatively, the local and the remote parts could write into > different coverage-bitmaps, but I'm not sure if there is a neat way to > do this.
This has 2 problems: - performance (__sanitizer_cov is by far the most performance critical part of kernel with KCOV=y) - recurions, locks are also traced, so it's not that we really can call anything there > 2.) Allow multiple kcov fds to be used by the same task. In the task, > keep a linked-list of pointers to kcov objects (remote or local). For > each __sanitizer_... call, walk the linked list and check if any of the > kcov objects match the requirements (trace_cmp/trace_pc/remote). This > would also have the side-effect of enabling simultaneous PC and CMP > tracing. Of course, it seems that this would add some overhead (in the > case of a single open fd, there would be extra pointer dereferences to > get the area[], size, etc). Walking linked list in __sanitizer_... has the same performance problems, but I think we don't really need to do it. Assuming we have at most 1 KCOV that traces the task itself we can continue keeping it cached in task_struct: https://elixir.bootlin.com/linux/v5.10-rc4/source/include/linux/sched.h#L1254 and __sanitizer_... will continue using these fields. For the kcov pointer in task struct: https://elixir.bootlin.com/linux/v5.10-rc4/source/include/linux/sched.h#L1257 we either have a linked list, or 1 pointer for local tracking and a separate list for remote kcov's: struct kcov *kcov; // local tracing struct kcov *remote_kcovs; // remote tracing, can be more than 1 Whichever is better I am not sure, it seems that some functions would benefit from a single list (KCOV_DISABLE), while others would benefit from separate fields (KCOV_ENABLE). Maybe the simplest code will be if we use both approaches -- put all kcov's into a list, but also cache the local kcov into a separate field? Then KCOV_DISABLE could just walk the list, but KCOV_ENABLE can continue checking 1 field.
