tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Adding new feature - Kcov



Le 12/12/2018 à 11:01, Siddharth Muralee a écrit :
Hello,
    I have attached a patch which adds the Kernel Code Coverage(KCov)
to the NetBSD kernel.
This is one of the foundational steps to get Syzkaller (Kernel Fuzzer)
fully functional on NetBSD.

The option can be enabled by uncommenting the following lines in amd64/GENERIC
makeoptions KCOV=1
options KCOV

The patch also contains a man page and a ATF test for the same.

Please let me know if there are any issues with the same.

What I can think of:

 1.  Please use proper KNF in the code example of the man page, and also in
     the ATF test

 2.  Don't use capital letters for bool, just !pmap_extract

 3.  kd_alloc, kd_lookup, etc, should be static, there shouldn't be any
     shared function or variable

 4.  The __sanitizer_cov_trace_pc() prototype should be in kcov.c, and the
     kcov_exit() prototype around "#ifdef _KERNEL" (since it is included from
     userland)

 5.  You probably don't need the HAVE_GCC in Makefile.amd64, also it shouldn't
     be in Makefile.amd64 but rather in bsd.sys.mk (take KLEAK as example)

 6.  The driver is not modulable, so there's no point making a kcov directory
     in sys/modules/

 7.  I would drop the debugging code, there's no interest, it is a very small
     and basic driver, so nothing particular to debug

 8.  I would call the include kcov_ioctl.h, and I would put it in sys/dev/

 9.  By the way, the values used in the ioctl are confusing IMO, I would use
     KIOSETBUFSIZE -> KCOV_IOC_SETBUFSIZE
     KIOENABLE     -> KCOV_IOC_ENABLE
     KIODISABLE    -> KCOV_IOC_DISABLE
     Otherwise it sounds like "Kernel I/O", which really has nothing to do
     with kcov

 10. In kern_exit.c you probably need a #include "opt_kcov.h"

 11. What does 'nmemb' stand for? I would use something like 'nentries', it
     sounds more explicit. Also the 'kd' name sounds wrong to me, this looks
     like 'Kernel Descriptor', I would just use 'kcov', eg, 'kcov_lookup_pid'

 12. kcovopen() seems racy to me, if two processes open /dev/kcov at the
     same time TAILQ_INSERT_TAIL() will clobber pointers; same problem in
     kd_lookup_pid(), and basically the rest

 13. Given that kd_buf[0] seems to be an index, I would declare it as a
     struct { uint64_t n; uintptr_t pc[]; }

 14. Please use consistent types, eg in kd_alloc() it should be uintptr_t
     and not vaddr_t

 15. Keep in mind that __sanitizer_cov_trace_pc() is a big hot path, and that
     everything called from there should be optimized for performance. So I
     would add a __predict_false() on 'cold', and I would bring together the
     'kd_mode', 'kd_pid' and 'kd_entry' fields to optimize the lookup.

In short, I think that for really simple drivers like that, it is better to
write one from scratch, with clear names, no bugs, and limited slowdown,
rather than taking code from the outside (here OpenBSD, which they themselves
took from Linux).

Maxime


Home | Main Index | Thread Index | Old Index