Re: Adding new feature - Kcov
On 04.01.2019 19:56, Maxime Villard wrote: > > Interrupt != exception. When a page fault comes in, there's no flag that is > set in proc/lwp/curcpu, so you can't know if you are in an exception > context; > ci_idepth is unrelated. > > Of course we could add such a flag under #ifdef KCOV and then check for > this > flag in __sanitizer_cov_trace_pc. > > But before that, it would be good to make sure that the extra output is > indeed noise (and not something the fuzzer expects). Because a lof of > things > we do in exception context may contain bugs, and we want to fuzz all of > that. > > Maybe check what Linux does? Linux does not print "side effect" routines from virtual memory layer. If there are no longer any concerns, please import it into src/ and remove kcov(4) entry from src/doc/TODO.sanitizers. In future once we will get GCC 8+ we will be able to add additional modes of execution. Clang already supports more, but short term we can delay it. signature.asc Description: OpenPGP digital signature
Re: Adding new feature - Kcov
Le 04/01/2019 à 16:58, Christos Zoulas a écrit : In article , Siddharth Muralee wrote: On Thu, 3 Jan 2019 at 22:39, Christos Zoulas wrote: In article , Siddharth Muralee wrote: On Wed, 2 Jan 2019 at 23:12, Christos Zoulas wrote: In article , Siddharth Muralee wrote: -=-=-=-=-=- On Wed, 2 Jan 2019 at 09:26, Siddharth Muralee wrote: Hello, I have attached a modified patch for kcov(4). I have modified it to be a per-process lookup instead of the earlier per-unit lookup. It seems to working fine. I have tested it with a couple of system calls. There is however a lot of unnecessary output coming during a simple system call. I have attached below the output of kcov(4) for the system call `read(-1, NULL, 0)`. I would also like to get some input on how to reduce the noise if possible. Seems like the attachments got missed somewhere along the way (Not sure what happened!). kcov diff - https://github.com/R3x/scratch-files/blob/master/kcov/kcov.diff output of kcov(4) for read() - https://github.com/R3x/scratch-files/blob/master/kcov/sample_output.txt What source version do those line numbers correspond it? It is of NetBSD-current probably a week old. I have made this one with the latest version(from GitHub) to avoid any confusion latest output - https://github.com/R3x/scratch-files/blob/master/kcov/latest_output.txt Thanks! The way other profilers summarize the output is to identify common code calling sequences, abstract them and refer to them by id. For example: /src/sys/arch/x86/x86/pmap.c:3069 /src/sys/arch/x86/x86/pmap.c:3080 /repos/obj1/sys/arch/amd64/compile/GENERIC/./x86/pmap.h:411 /src/sys/arch/x86/x86/pmap.c:3028 /src/sys/arch/x86/x86/pmap.c:3029 /src/sys/arch/x86/x86/pmap.c:3031 /src/sys/arch/x86/x86/pmap.c:3028 (discriminator 1) /src/sys/arch/x86/x86/pmap.c:3029 /src/sys/arch/x86/x86/pmap.c:3031 /src/sys/arch/x86/x86/pmap.c:3028 (discriminator 1) /src/sys/arch/x86/x86/pmap.c:3029 /src/sys/arch/x86/x86/pmap.c:3031 /src/sys/arch/x86/x86/pmap.c:3034 /src/sys/arch/x86/x86/pmap.c:3035 /src/sys/arch/x86/x86/pmap.c:3037 /src/sys/arch/x86/x86/pmap.c:3088 /src/sys/arch/x86/x86/pmap.c:3092 /src/sys/arch/x86/x86/pmap.c:3097 /src/sys/arch/x86/x86/pmap.c:3100 is repeated; also putting function names next to line numbers is also helpful to identify stack traces. What tools are other people using to post-process this output? christos Kcov only returns the addresses of all the functions that it traced along the way. I have passed the output to addr2line(1) to get the above output. I can use the -f option that is available as a part of it to print out the function names if necessary. What I meant by excess noise is all the pmap functions and uvm functions that are coming in the output trace. In this case I am trying to predict the output path of the syscall read with the arguments (-1, NULL, 0). The ideal output should only contain the functions that get executed during the system call (The uvm and pmap function aren't really relevant to the system call). The use case of Kcov we wish to take advantage of is to use it for coverage guided fuzzing with syzkaller - mainly in the starting for the system call layer. We only need the coverage for the paths that are executed with reference to the input(arguments of the syscall in this case). You can take a look at the output for the same system call in linux - https://github.com/R3x/scratch-files/blob/master/kcov/linux_output.txt If you look at my trace with functions here - https://github.com/R3x/scratch-files/blob/master/kcov/netbsd_output_with_functions.txt You can see that the topmost functions are similar to what we saw in Linux but there are a lot of uvm and pmap functions after that. Especially the trap part. In this case the pmap functions, mutex functions etc have no relevance to the syscall input and hence we would like to avoid that part. __sanitizer_cov_trace_pc is the function which is compiler instrumented to add the trace coverage to the buffer. In this function - I am disabling the trace during the boot time period and also during interrupts. Currently checking whether we are in an interrupt by ` curcpu()->ci_idepth >= 0 `. This check not working properly could be what is causing the noise. That is what cpu_intr_p() is doing, so it should work... I would try to debug why it does not. Perhaps the pmap functions are called from allocators? Anyway finding a way to create a call graph out of the profiled samples would be useful. christos Interrupt != exception. When a page fault comes in, there's no flag that is set in proc/lwp/curcpu, so you can't know if you are in an exception context; ci_idepth is unrelated. Of course we could add such a flag under #ifdef KCOV and then check for this flag in __sanitizer_cov_trace_pc. But before that, it would be good to make sure that the extra output is indeed noise (and not something the fuzzer expects). Because a lof of things we do in exception context may contain bugs, a
Re: Adding new feature - Kcov
In article , Siddharth Muralee wrote: >On Thu, 3 Jan 2019 at 22:39, Christos Zoulas wrote: >> >> In article >, >> Siddharth Muralee wrote: >> >On Wed, 2 Jan 2019 at 23:12, Christos Zoulas wrote: >> >> >> >> In article >> >, >> >> Siddharth Muralee wrote: >> >> >-=-=-=-=-=- >> >> > >> >> >On Wed, 2 Jan 2019 at 09:26, Siddharth Muralee > >> >> >wrote: >> >> >> >> >> >> Hello, I have attached a modified patch for kcov(4). I have modified >> >> >> it to be a per-process lookup instead of the earlier per-unit lookup. >> >> >> >> >> >> It seems to working fine. I have tested it with a couple of system >> >> >> calls. There is however a lot of unnecessary output coming during a >> >> >> simple system call. I have attached below the output of kcov(4) for >> >> >> the system call `read(-1, NULL, 0)`. I would also like to get some >> >> >> input on how to reduce the noise if possible. >> >> > >> >> >Seems like the attachments got missed somewhere along the way (Not sure >> >> >what happened!). >> >> >kcov diff - >https://github.com/R3x/scratch-files/blob/master/kcov/kcov.diff >> >> >output of kcov(4) for read() - >> >> >https://github.com/R3x/scratch-files/blob/master/kcov/sample_output.txt >> >> >> >> What source version do those line numbers correspond it? >> > >> >It is of NetBSD-current probably a week old. I have made this one with >> >the latest version(from GitHub) to avoid any confusion >> >latest output - >> >https://github.com/R3x/scratch-files/blob/master/kcov/latest_output.txt >> >> Thanks! The way other profilers summarize the output is to identify common >> code calling sequences, abstract them and refer to them by id. For example: >> >> /src/sys/arch/x86/x86/pmap.c:3069 >> /src/sys/arch/x86/x86/pmap.c:3080 >> /repos/obj1/sys/arch/amd64/compile/GENERIC/./x86/pmap.h:411 >> /src/sys/arch/x86/x86/pmap.c:3028 >> /src/sys/arch/x86/x86/pmap.c:3029 >> /src/sys/arch/x86/x86/pmap.c:3031 >> /src/sys/arch/x86/x86/pmap.c:3028 (discriminator 1) >> /src/sys/arch/x86/x86/pmap.c:3029 >> /src/sys/arch/x86/x86/pmap.c:3031 >> /src/sys/arch/x86/x86/pmap.c:3028 (discriminator 1) >> /src/sys/arch/x86/x86/pmap.c:3029 >> /src/sys/arch/x86/x86/pmap.c:3031 >> /src/sys/arch/x86/x86/pmap.c:3034 >> /src/sys/arch/x86/x86/pmap.c:3035 >> /src/sys/arch/x86/x86/pmap.c:3037 >> /src/sys/arch/x86/x86/pmap.c:3088 >> /src/sys/arch/x86/x86/pmap.c:3092 >> /src/sys/arch/x86/x86/pmap.c:3097 >> /src/sys/arch/x86/x86/pmap.c:3100 >> >> is repeated; also putting function names next to line numbers is also helpful >> to identify stack traces. What tools are other people using to post-process >> this output? >> >> christos >> > >Kcov only returns the addresses of all the functions that it traced >along the way. I have passed the output to addr2line(1) to get the >above output. I can use the -f option that is available as a part of >it to print out the function names if necessary. > >What I meant by excess noise is all the pmap functions and uvm >functions that are coming in the output trace. In this case I am >trying to predict the output path of the syscall read with the >arguments (-1, NULL, 0). The ideal output should only contain the >functions that get executed during the system call (The uvm and pmap >function aren't really relevant to the system call). The use case of >Kcov we wish to take advantage of is to use it for coverage guided >fuzzing with syzkaller - mainly in the starting for the system call >layer. We only need the coverage for the paths that are executed with >reference to the input(arguments of the syscall in this case). > >You can take a look at the output for the same system call in linux - >https://github.com/R3x/scratch-files/blob/master/kcov/linux_output.txt > >If you look at my trace with functions here - >https://github.com/R3x/scratch-files/blob/master/kcov/netbsd_output_with_functions.txt >You can see that the topmost functions are similar to what we saw in >Linux but there are a lot of uvm and pmap functions after that. >Especially the trap part. >In this case the pmap functions, mutex functions etc have no relevance >to the syscall input and hence we would like to avoid that part. > >__sanitizer_cov_trace_pc is the function which is compiler >instrumented to add the trace coverage to the buffer. In this function >- I am disabling the trace during the boot time period and also during >interrupts. Currently checking whether we are in an interrupt by ` >curcpu()->ci_idepth >= 0 `. This check not working properly could be >what is causing the noise. That is what cpu_intr_p() is doing, so it should work... I would try to debug why it does not. Perhaps the pmap functions are called from allocators? Anyway finding a way to create a call graph out of the profiled samples would be useful. christos
Re: Adding new feature - Kcov
On Thu, 3 Jan 2019 at 22:39, Christos Zoulas wrote: > > In article > , > Siddharth Muralee wrote: > >On Wed, 2 Jan 2019 at 23:12, Christos Zoulas wrote: > >> > >> In article > >, > >> Siddharth Muralee wrote: > >> >-=-=-=-=-=- > >> > > >> >On Wed, 2 Jan 2019 at 09:26, Siddharth Muralee > >> > > >> >wrote: > >> >> > >> >> Hello, I have attached a modified patch for kcov(4). I have modified > >> >> it to be a per-process lookup instead of the earlier per-unit lookup. > >> >> > >> >> It seems to working fine. I have tested it with a couple of system > >> >> calls. There is however a lot of unnecessary output coming during a > >> >> simple system call. I have attached below the output of kcov(4) for > >> >> the system call `read(-1, NULL, 0)`. I would also like to get some > >> >> input on how to reduce the noise if possible. > >> > > >> >Seems like the attachments got missed somewhere along the way (Not sure > >> >what happened!). > >> >kcov diff - > >> >https://github.com/R3x/scratch-files/blob/master/kcov/kcov.diff > >> >output of kcov(4) for read() - > >> >https://github.com/R3x/scratch-files/blob/master/kcov/sample_output.txt > >> > >> What source version do those line numbers correspond it? > > > >It is of NetBSD-current probably a week old. I have made this one with > >the latest version(from GitHub) to avoid any confusion > >latest output - > >https://github.com/R3x/scratch-files/blob/master/kcov/latest_output.txt > > Thanks! The way other profilers summarize the output is to identify common > code calling sequences, abstract them and refer to them by id. For example: > > /src/sys/arch/x86/x86/pmap.c:3069 > /src/sys/arch/x86/x86/pmap.c:3080 > /repos/obj1/sys/arch/amd64/compile/GENERIC/./x86/pmap.h:411 > /src/sys/arch/x86/x86/pmap.c:3028 > /src/sys/arch/x86/x86/pmap.c:3029 > /src/sys/arch/x86/x86/pmap.c:3031 > /src/sys/arch/x86/x86/pmap.c:3028 (discriminator 1) > /src/sys/arch/x86/x86/pmap.c:3029 > /src/sys/arch/x86/x86/pmap.c:3031 > /src/sys/arch/x86/x86/pmap.c:3028 (discriminator 1) > /src/sys/arch/x86/x86/pmap.c:3029 > /src/sys/arch/x86/x86/pmap.c:3031 > /src/sys/arch/x86/x86/pmap.c:3034 > /src/sys/arch/x86/x86/pmap.c:3035 > /src/sys/arch/x86/x86/pmap.c:3037 > /src/sys/arch/x86/x86/pmap.c:3088 > /src/sys/arch/x86/x86/pmap.c:3092 > /src/sys/arch/x86/x86/pmap.c:3097 > /src/sys/arch/x86/x86/pmap.c:3100 > > is repeated; also putting function names next to line numbers is also helpful > to identify stack traces. What tools are other people using to post-process > this output? > > christos > Kcov only returns the addresses of all the functions that it traced along the way. I have passed the output to addr2line(1) to get the above output. I can use the -f option that is available as a part of it to print out the function names if necessary. What I meant by excess noise is all the pmap functions and uvm functions that are coming in the output trace. In this case I am trying to predict the output path of the syscall read with the arguments (-1, NULL, 0). The ideal output should only contain the functions that get executed during the system call (The uvm and pmap function aren't really relevant to the system call). The use case of Kcov we wish to take advantage of is to use it for coverage guided fuzzing with syzkaller - mainly in the starting for the system call layer. We only need the coverage for the paths that are executed with reference to the input(arguments of the syscall in this case). You can take a look at the output for the same system call in linux - https://github.com/R3x/scratch-files/blob/master/kcov/linux_output.txt If you look at my trace with functions here - https://github.com/R3x/scratch-files/blob/master/kcov/netbsd_output_with_functions.txt You can see that the topmost functions are similar to what we saw in Linux but there are a lot of uvm and pmap functions after that. Especially the trap part. In this case the pmap functions, mutex functions etc have no relevance to the syscall input and hence we would like to avoid that part. __sanitizer_cov_trace_pc is the function which is compiler instrumented to add the trace coverage to the buffer. In this function - I am disabling the trace during the boot time period and also during interrupts. Currently checking whether we are in an interrupt by ` curcpu()->ci_idepth >= 0 `. This check not working properly could be what is causing the noise. -- Regards, Siddharth M Third Year B.Tech (CSE) Student, Amrita School of Engineering, Kollam Blog --- “Most people get ahead during the time that others waste."
Re: Adding new feature - Kcov
In article , Siddharth Muralee wrote: >On Wed, 2 Jan 2019 at 23:12, Christos Zoulas wrote: >> >> In article >, >> Siddharth Muralee wrote: >> >-=-=-=-=-=- >> > >> >On Wed, 2 Jan 2019 at 09:26, Siddharth Muralee >> >wrote: >> >> >> >> Hello, I have attached a modified patch for kcov(4). I have modified >> >> it to be a per-process lookup instead of the earlier per-unit lookup. >> >> >> >> It seems to working fine. I have tested it with a couple of system >> >> calls. There is however a lot of unnecessary output coming during a >> >> simple system call. I have attached below the output of kcov(4) for >> >> the system call `read(-1, NULL, 0)`. I would also like to get some >> >> input on how to reduce the noise if possible. >> > >> >Seems like the attachments got missed somewhere along the way (Not sure >> >what happened!). >> >kcov diff - https://github.com/R3x/scratch-files/blob/master/kcov/kcov.diff >> >output of kcov(4) for read() - >> >https://github.com/R3x/scratch-files/blob/master/kcov/sample_output.txt >> >> What source version do those line numbers correspond it? > >It is of NetBSD-current probably a week old. I have made this one with >the latest version(from GitHub) to avoid any confusion >latest output - >https://github.com/R3x/scratch-files/blob/master/kcov/latest_output.txt Thanks! The way other profilers summarize the output is to identify common code calling sequences, abstract them and refer to them by id. For example: /src/sys/arch/x86/x86/pmap.c:3069 /src/sys/arch/x86/x86/pmap.c:3080 /repos/obj1/sys/arch/amd64/compile/GENERIC/./x86/pmap.h:411 /src/sys/arch/x86/x86/pmap.c:3028 /src/sys/arch/x86/x86/pmap.c:3029 /src/sys/arch/x86/x86/pmap.c:3031 /src/sys/arch/x86/x86/pmap.c:3028 (discriminator 1) /src/sys/arch/x86/x86/pmap.c:3029 /src/sys/arch/x86/x86/pmap.c:3031 /src/sys/arch/x86/x86/pmap.c:3028 (discriminator 1) /src/sys/arch/x86/x86/pmap.c:3029 /src/sys/arch/x86/x86/pmap.c:3031 /src/sys/arch/x86/x86/pmap.c:3034 /src/sys/arch/x86/x86/pmap.c:3035 /src/sys/arch/x86/x86/pmap.c:3037 /src/sys/arch/x86/x86/pmap.c:3088 /src/sys/arch/x86/x86/pmap.c:3092 /src/sys/arch/x86/x86/pmap.c:3097 /src/sys/arch/x86/x86/pmap.c:3100 is repeated; also putting function names next to line numbers is also helpful to identify stack traces. What tools are other people using to post-process this output? christos
Re: Adding new feature - Kcov
On Wed, 2 Jan 2019 at 23:12, Christos Zoulas wrote: > > In article > , > Siddharth Muralee wrote: > >-=-=-=-=-=- > > > >On Wed, 2 Jan 2019 at 09:26, Siddharth Muralee > >wrote: > >> > >> Hello, I have attached a modified patch for kcov(4). I have modified > >> it to be a per-process lookup instead of the earlier per-unit lookup. > >> > >> It seems to working fine. I have tested it with a couple of system > >> calls. There is however a lot of unnecessary output coming during a > >> simple system call. I have attached below the output of kcov(4) for > >> the system call `read(-1, NULL, 0)`. I would also like to get some > >> input on how to reduce the noise if possible. > > > >Seems like the attachments got missed somewhere along the way (Not sure > >what happened!). > >kcov diff - https://github.com/R3x/scratch-files/blob/master/kcov/kcov.diff > >output of kcov(4) for read() - > >https://github.com/R3x/scratch-files/blob/master/kcov/sample_output.txt > > What source version do those line numbers correspond it? It is of NetBSD-current probably a week old. I have made this one with the latest version(from GitHub) to avoid any confusion latest output - https://github.com/R3x/scratch-files/blob/master/kcov/latest_output.txt -- Regards, Siddharth M Third Year B.Tech (CSE) Student, Amrita School of Engineering, Kollam Blog --- “Most people get ahead during the time that others waste."
Re: Adding new feature - Kcov
In article , Siddharth Muralee wrote: >-=-=-=-=-=- > >On Wed, 2 Jan 2019 at 09:26, Siddharth Muralee >wrote: >> >> Hello, I have attached a modified patch for kcov(4). I have modified >> it to be a per-process lookup instead of the earlier per-unit lookup. >> >> It seems to working fine. I have tested it with a couple of system >> calls. There is however a lot of unnecessary output coming during a >> simple system call. I have attached below the output of kcov(4) for >> the system call `read(-1, NULL, 0)`. I would also like to get some >> input on how to reduce the noise if possible. > >Seems like the attachments got missed somewhere along the way (Not sure >what happened!). >kcov diff - https://github.com/R3x/scratch-files/blob/master/kcov/kcov.diff >output of kcov(4) for read() - >https://github.com/R3x/scratch-files/blob/master/kcov/sample_output.txt What source version do those line numbers correspond it? christos
Re: Adding new feature - Kcov
On Wed, 2 Jan 2019 at 09:26, Siddharth Muralee wrote: > > Hello, I have attached a modified patch for kcov(4). I have modified > it to be a per-process lookup instead of the earlier per-unit lookup. > > It seems to working fine. I have tested it with a couple of system > calls. There is however a lot of unnecessary output coming during a > simple system call. I have attached below the output of kcov(4) for > the system call `read(-1, NULL, 0)`. I would also like to get some > input on how to reduce the noise if possible. Seems like the attachments got missed somewhere along the way (Not sure what happened!). kcov diff - https://github.com/R3x/scratch-files/blob/master/kcov/kcov.diff output of kcov(4) for read() - https://github.com/R3x/scratch-files/blob/master/kcov/sample_output.txt -- Regards, Siddharth M Third Year B.Tech (CSE) Student, Amrita School of Engineering, Kollam Blog --- “Most people get ahead during the time that others waste."
Re: Adding new feature - Kcov
Hello, I have attached a modified patch for kcov(4). I have modified it to be a per-process lookup instead of the earlier per-unit lookup. It seems to working fine. I have tested it with a couple of system calls. There is however a lot of unnecessary output coming during a simple system call. I have attached below the output of kcov(4) for the system call `read(-1, NULL, 0)`. I would also like to get some input on how to reduce the noise if possible. On Fri, 14 Dec 2018 at 12:36, Siddharth Muralee wrote: > > Thanks for helping me out. Let me look at the changes you have made and I > will continue working on improving it. I will also test it and get back as > soon as possible. > > On Fri, Dec 14, 2018, 1:15 AM Maxime Villard wrote: >> >> Here is a modified version [1]. Compile-tested only, but it gives an idea. >> >> I realized that there was a problem in your patch: you give a kcov descriptor >> on a per-process basis, but if a proc has several threads you are mixing the >> coverage of each thread in the output buffer. This is not desired, so I >> changed the code to be per-LWP. >> >> Also the lookup in _trace_pc() was bad for two reasons: it is slow, and it >> is racy. The thing is, adding a mutex here is not allowed. So I dropped the >> lookup and made a per-LWP pointer. As a result of that, it unfortunately >> becomes more complicated to free the associated kcov descriptor, typically >> in corner cases such as: >> >> - A thread closes the file descriptor in the process, but another thread >> was being traced; in that case we ask the traced thread to free the kcov >> descriptor when it terminates >> >> - A traced thread is killed; in that case it should disable the kcov >> descriptor but not free it, and this descriptor will get subsequently >> freed when the process terminates >> >> Another concern (that I didn't address) is the fact that you use device >> units. It would seem better to me if it was not per-unit but per-process
Re: Adding new feature - Kcov
Thanks for helping me out. Let me look at the changes you have made and I will continue working on improving it. I will also test it and get back as soon as possible. On Fri, Dec 14, 2018, 1:15 AM Maxime Villard wrote: > Here is a modified version [1]. Compile-tested only, but it gives an idea. > > I realized that there was a problem in your patch: you give a kcov > descriptor > on a per-process basis, but if a proc has several threads you are mixing > the > coverage of each thread in the output buffer. This is not desired, so I > changed the code to be per-LWP. > > Also the lookup in _trace_pc() was bad for two reasons: it is slow, and it > is racy. The thing is, adding a mutex here is not allowed. So I dropped the > lookup and made a per-LWP pointer. As a result of that, it unfortunately > becomes more complicated to free the associated kcov descriptor, typically > in corner cases such as: > > - A thread closes the file descriptor in the process, but another thread > was being traced; in that case we ask the traced thread to free the > kcov > descriptor when it terminates > > - A traced thread is killed; in that case it should disable the kcov > descriptor but not free it, and this descriptor will get subsequently > freed when the process terminates > > Another concern (that I didn't address) is the fact that you use device > units. It would seem better to me if it was not per-unit but per-process. > Ie, when a process opens /dev/kcov, you register the pid in the kcov > descriptor rather than the unit. Then you look up based on the pid, and > you trace based on the thread. > > So, do as you see fit, my version shouldn't be too far from working, and > it's here to give an idea. > > [1] https://m00nbsd.net/garbage/kcov/kcov.diff >
Re: Adding new feature - Kcov
Here is a modified version [1]. Compile-tested only, but it gives an idea. I realized that there was a problem in your patch: you give a kcov descriptor on a per-process basis, but if a proc has several threads you are mixing the coverage of each thread in the output buffer. This is not desired, so I changed the code to be per-LWP. Also the lookup in _trace_pc() was bad for two reasons: it is slow, and it is racy. The thing is, adding a mutex here is not allowed. So I dropped the lookup and made a per-LWP pointer. As a result of that, it unfortunately becomes more complicated to free the associated kcov descriptor, typically in corner cases such as: - A thread closes the file descriptor in the process, but another thread was being traced; in that case we ask the traced thread to free the kcov descriptor when it terminates - A traced thread is killed; in that case it should disable the kcov descriptor but not free it, and this descriptor will get subsequently freed when the process terminates Another concern (that I didn't address) is the fact that you use device units. It would seem better to me if it was not per-unit but per-process. Ie, when a process opens /dev/kcov, you register the pid in the kcov descriptor rather than the unit. Then you look up based on the pid, and you trace based on the thread. So, do as you see fit, my version shouldn't be too far from working, and it's here to give an idea. [1] https://m00nbsd.net/garbage/kcov/kcov.diff
Re: Adding new feature - Kcov
On 12.12.2018 18:04, Mouse wrote: >>> Why would you store pointers in integers at all? Surely the right >>> thing to do is store them as void * (for data pointers) or >>> void (*)() (for function pointers)...? >> kcov utilizes compiler instrumentation that uses low-lever, below the >> C and C++ language (or runtime), thus here it doesn't matter what's >> the exact storage type of a pointer. For simplicity of a kCov >> runtime we store the pointers as integers. > > If it's that machine-dependent, shouldn't this be on the relevant > port-* list(s), rather than tech-kern? Or am I missing something else? > It's supported probably by all GCC and Clang/LLVM backends. signature.asc Description: OpenPGP digital signature
Re: Adding new feature - Kcov
>> Why would you store pointers in integers at all? Surely the right >> thing to do is store them as void * (for data pointers) or >> void (*)() (for function pointers)...? > kcov utilizes compiler instrumentation that uses low-lever, below the > C and C++ language (or runtime), thus here it doesn't matter what's > the exact storage type of a pointer. For simplicity of a kCov > runtime we store the pointers as integers. If it's that machine-dependent, shouldn't this be on the relevant port-* list(s), rather than tech-kern? Or am I missing something else? /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: Adding new feature - Kcov
On 12.12.2018 16:02, Mouse wrote: >> kcov returns kernel pointers, 32-bit program executed by 64-bit >> kernel cannot store them in 32-bit integers. > > Why would you store pointers in integers at all? Surely the right > thing to do is store them as void * (for data pointers) or void (*)() > (for function pointers)...? kcov utilizes compiler instrumentation that uses low-lever, below the C and C++ language (or runtime), thus here it doesn't matter what's the exact storage type of a pointer. For simplicity of a kCov runtime we store the pointers as integers. signature.asc Description: OpenPGP digital signature
Re: Adding new feature - Kcov
> kcov returns kernel pointers, 32-bit program executed by 64-bit > kernel cannot store them in 32-bit integers. Why would you store pointers in integers at all? Surely the right thing to do is store them as void * (for data pointers) or void (*)() (for function pointers)...? /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: Adding new feature - Kcov
Le 12/12/2018 à 12:50, Maxime Villard a écrit :
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.
[...]
16. The flags you're passing to kmem_* are wrong, these are for malloc()
Anyway, I will update your patch with all I said above, and re-send it here
soon.
Re: Adding new feature - Kcov
On 12.12.2018 13:23, Martin Husemann wrote: > On Wed, Dec 12, 2018 at 01:19:40PM +0100, Kamil Rytarowski wrote: >> Upstream (dvyukov) suggested to evaluate whether to use pointers always >> in 64bit types (uint64_t), as this way programs will be easier to reuse >> in 32bit mode on 64-bit kernel mode. > > I fail to parse that - can you give an example of what you mean? > > Martin > kcov returns kernel pointers, 32-bit program executed by 64-bit kernel cannot store them in 32-bit integers. It would need to detect real kernel ABI (and bypass constrains of emulation that fakes it) and handle 32 and 64-bit integers. Switching to 64-bit integers only makes handling of kcov traces in all scenarios easier. signature.asc Description: OpenPGP digital signature
Re: Adding new feature - Kcov
On Wed, Dec 12, 2018 at 01:19:40PM +0100, Kamil Rytarowski wrote: > Upstream (dvyukov) suggested to evaluate whether to use pointers always > in 64bit types (uint64_t), as this way programs will be easier to reuse > in 32bit mode on 64-bit kernel mode. I fail to parse that - can you give an example of what you mean? Martin
Re: Adding new feature - Kcov
On 12.12.2018 12:50, Maxime Villard wrote:
> 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
>
If we are going to rename OpenBSD macros, I propose to switch to the
Linux ones:
KCOV_INIT_TRACE
KCOV_ENABLE
KCOV_DISABLE
https://github.com/torvalds/linux/blob/6f0d349d922ba44e4348a17a78ea51b7135965b1/include/uapi/linux/kcov.h
This way we will be more source-code compatible with the original
implementation.
> 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
Upstream (dvyukov) suggested to evaluate whether to use pointers always
in 64bit types (uint64_t), as this way programs will be easier to reuse
in 32bit mode on 64-bit kernel mode.
Fuzzing 32-bit-on-64-bit-kernel is likely something we want.
We have a similar issues with the ktrace protocol in
32-bit-in-64-bit-kernel.
signature.asc
Description: OpenPGP digital signature
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
