On Wed, 7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote:
> From: Aleksandr Nogikh <[email protected]>
>
> Remote KCOV coverage collection enables coverage-guided fuzzing of the
> code that is not reachable during normal system call execution. It is
> especially helpful for fuzzing networking subsystems, where it is
> common to perform packet handling in separate work queues even for the
> packets that originated directly from the user space.
>
> Enable coverage-guided frame injection by adding a kcov_handle
> parameter to sk_buff structure. Initialization in __alloc_skb ensures
> that no socket buffer that was generated during a system call will be
> missed.
>
> Code that is of interest and that performs packet processing should be
> annotated with kcov_remote_start()/kcov_remote_stop().
>
> An alternative approach is to determine kcov_handle solely on the
> basis of the device/interface that received the specific socket
> buffer. However, in this case it would be impossible to distinguish
> between packets that originated from normal background network
> processes and those that were intentionally injected from the user
> space.
>
> Signed-off-by: Aleksandr Nogikh <[email protected]>
Could you use skb_extensions for this?
On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <[email protected]> wrote:
>
> On Wed, 7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote:
> > From: Aleksandr Nogikh <[email protected]>
> >
> > Remote KCOV coverage collection enables coverage-guided fuzzing of the
> > code that is not reachable during normal system call execution. It is
> > especially helpful for fuzzing networking subsystems, where it is
> > common to perform packet handling in separate work queues even for the
> > packets that originated directly from the user space.
> >
> > Enable coverage-guided frame injection by adding a kcov_handle
> > parameter to sk_buff structure. Initialization in __alloc_skb ensures
> > that no socket buffer that was generated during a system call will be
> > missed.
> >
> > Code that is of interest and that performs packet processing should be
> > annotated with kcov_remote_start()/kcov_remote_stop().
> >
> > An alternative approach is to determine kcov_handle solely on the
> > basis of the device/interface that received the specific socket
> > buffer. However, in this case it would be impossible to distinguish
> > between packets that originated from normal background network
> > processes and those that were intentionally injected from the user
> > space.
> >
> > Signed-off-by: Aleksandr Nogikh <[email protected]>
>
> Could you use skb_extensions for this?
Why? If for space, this is already under a non-production ifdef.
On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:
> On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <[email protected]> wrote:
> > On Wed, 7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote:
> > > From: Aleksandr Nogikh <[email protected]>
> > >
> > > Remote KCOV coverage collection enables coverage-guided fuzzing of the
> > > code that is not reachable during normal system call execution. It is
> > > especially helpful for fuzzing networking subsystems, where it is
> > > common to perform packet handling in separate work queues even for the
> > > packets that originated directly from the user space.
> > >
> > > Enable coverage-guided frame injection by adding a kcov_handle
> > > parameter to sk_buff structure. Initialization in __alloc_skb ensures
> > > that no socket buffer that was generated during a system call will be
> > > missed.
> > >
> > > Code that is of interest and that performs packet processing should be
> > > annotated with kcov_remote_start()/kcov_remote_stop().
> > >
> > > An alternative approach is to determine kcov_handle solely on the
> > > basis of the device/interface that received the specific socket
> > > buffer. However, in this case it would be impossible to distinguish
> > > between packets that originated from normal background network
> > > processes and those that were intentionally injected from the user
> > > space.
> > >
> > > Signed-off-by: Aleksandr Nogikh <[email protected]>
> >
> > Could you use skb_extensions for this?
>
> Why? If for space, this is already under a non-production ifdef.
I understand, but the skb_ext infra is there for uncommon use cases
like this one. Any particular reason you don't want to use it?
The slight LoC increase?
Is there any precedent for adding the kcov field to other performance
critical structures?
On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <[email protected]> wrote:
>
> On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:
> > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <[email protected]> wrote:
> > > On Wed, 7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote:
> > > > From: Aleksandr Nogikh <[email protected]>
> > > >
> > > > Remote KCOV coverage collection enables coverage-guided fuzzing of the
> > > > code that is not reachable during normal system call execution. It is
> > > > especially helpful for fuzzing networking subsystems, where it is
> > > > common to perform packet handling in separate work queues even for the
> > > > packets that originated directly from the user space.
> > > >
> > > > Enable coverage-guided frame injection by adding a kcov_handle
> > > > parameter to sk_buff structure. Initialization in __alloc_skb ensures
> > > > that no socket buffer that was generated during a system call will be
> > > > missed.
> > > >
> > > > Code that is of interest and that performs packet processing should be
> > > > annotated with kcov_remote_start()/kcov_remote_stop().
> > > >
> > > > An alternative approach is to determine kcov_handle solely on the
> > > > basis of the device/interface that received the specific socket
> > > > buffer. However, in this case it would be impossible to distinguish
> > > > between packets that originated from normal background network
> > > > processes and those that were intentionally injected from the user
> > > > space.
> > > >
> > > > Signed-off-by: Aleksandr Nogikh <[email protected]>
> > >
> > > Could you use skb_extensions for this?
> >
> > Why? If for space, this is already under a non-production ifdef.
>
> I understand, but the skb_ext infra is there for uncommon use cases
> like this one. Any particular reason you don't want to use it?
> The slight LoC increase?
>
> Is there any precedent for adding the kcov field to other performance
> critical structures?
I see. Yes, increase in complexity for no gain.
No, KCOV context wasn't added to anything as critical as sk_buff.
It seems there is no established practice both ways -- I don't see
anything debug-related in sk_buff nor in skb_ext_id...
On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <[email protected]> wrote:
>
> On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <[email protected]> wrote:
> >
> > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote:
> > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <[email protected]> wrote:
[...]
> > > > Could you use skb_extensions for this?
> > >
> > > Why? If for space, this is already under a non-production ifdef.
> >
> > I understand, but the skb_ext infra is there for uncommon use cases
> > like this one. Any particular reason you don't want to use it?
> > The slight LoC increase?
> >
> > Is there any precedent for adding the kcov field to other performance
> > critical structures?
It would be great to come to some conclusion on where exactly to store
kcov_handle. Technically, it is possible to use skb extensions for the
purpose, though it will indeed slightly increase the complexity.
Jakub, you think that kcov_handle should be added as an skb extension,
right?
Though I do not really object to moving the field, it still seems to
me that sk_buff itself is a better place. Right now skb extensions
store values that are local to specific protocols and that are only
meaningful in the context of these protocols (correct me if I'm
wrong). Although this patch only adds remote kcov coverage to the wifi
code, kcov_handle can be meaningful for other protocols as well - just
like the already existing sk_buff fields. So adding kcov_handle to skb
extensions will break this logical separation.
--
Best regards,
Aleksandr