| Message ID | 20201007101726.3149375-2-a.nogikh@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | net, mac80211: enable KCOV remote coverage collection for 802.11 frame handling | expand |
On Wed, 7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote: > From: Aleksandr Nogikh <nogikh@google.com> > > 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 <nogikh@google.com> Could you use skb_extensions for this?
On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote: > > From: Aleksandr Nogikh <nogikh@google.com> > > > > 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 <nogikh@google.com> > > 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 <kuba@kernel.org> wrote: > > On Wed, 7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote: > > > From: Aleksandr Nogikh <nogikh@google.com> > > > > > > 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 <nogikh@google.com> > > > > 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 <kuba@kernel.org> wrote: > > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote: > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > On Wed, 7 Oct 2020 10:17:25 +0000 Aleksandr Nogikh wrote: > > > > From: Aleksandr Nogikh <nogikh@google.com> > > > > > > > > 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 <nogikh@google.com> > > > > > > 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 Wed, 2020-10-07 at 10:17 +0000, Aleksandr Nogikh wrote: > > @@ -904,6 +905,10 @@ struct sk_buff { > __u16 network_header; > __u16 mac_header; > > +#ifdef CONFIG_KCOV > + u64 kcov_handle; > +#endif [...] > @@ -233,6 +233,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, > skb->end = skb->tail + size; > skb->mac_header = (typeof(skb->mac_header))~0U; > skb->transport_header = (typeof(skb->transport_header))~0U; > + skb_set_kcov_handle(skb, kcov_common_handle()); Btw, you're only setting this here. It seems to me it would make sense to copy it when the skb is copied, rather than then having it set to the kcov handle of the (interrupted) task that was copying the skb? johannes
On Mon, 12 Oct 2020 at 10:12, Johannes Berg <johannes@sipsolutions.net> wrote: [...] > > @@ -233,6 +233,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, > > skb->end = skb->tail + size; > > skb->mac_header = (typeof(skb->mac_header))~0U; > > skb->transport_header = (typeof(skb->transport_header))~0U; > > + skb_set_kcov_handle(skb, kcov_common_handle()); > > Btw, you're only setting this here. It seems to me it would make sense > to copy it when the skb is copied, rather than then having it set to the > kcov handle of the (interrupted) task that was copying the skb? > > johannes > The field is added to the part of sk_buff that is between headers_start and headers_end, therefore skb_copy will copy the field to the newly created buffer. So in the case of copying it will be initialized, and then overwritten. Probably, it's not the most efficient way, but it seems to be the same for some other fields that are initialized in __alloc_skb.
On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <dvyukov@google.com> wrote: > > On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote: > > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> 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
On Tue, 13 Oct 2020 18:59:28 +0300 Aleksandr Nogikh wrote: > On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <dvyukov@google.com> wrote: > > > > On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote: > > > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> 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? That'd be preferable. I understand with current use cases it doesn't really matter, but history shows people come up with all sort of wonderful use cases down the line. And when they do they rarely go back and fix such fundamental minutiae. > 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. It's not as much protocols as subsystems. The values are meaningful to a subsystem which inserts them, that doesn't mean single layer of the stack. If it was about storing layer's context we would just use skb->cb. So I think the kcov use matches pretty well. Florian, what's your take?
On Tue, Oct 13, 2020 at 12:50 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 13 Oct 2020 18:59:28 +0300 Aleksandr Nogikh wrote: > > On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <dvyukov@google.com> wrote: > > > > > > On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > > > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote: > > > > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> 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? > > That'd be preferable. I understand with current use cases it doesn't > really matter, but history shows people come up with all sort of > wonderful use cases down the line. And when they do they rarely go back > and fix such fundamental minutiae. > > > 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. > > It's not as much protocols as subsystems. The values are meaningful to > a subsystem which inserts them, that doesn't mean single layer of the > stack. If it was about storing layer's context we would just use > skb->cb. > > So I think the kcov use matches pretty well. skb_extensions was the first thing that came to mind when I read this patchset too. It is not specific to protocols. We have long stopped growing sk_buff size.
On Thu, Oct 15, 2020 at 10:04 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Tue, Oct 13, 2020 at 12:50 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Tue, 13 Oct 2020 18:59:28 +0300 Aleksandr Nogikh wrote: > > > On Mon, 12 Oct 2020 at 09:04, Dmitry Vyukov <dvyukov@google.com> wrote: > > > > > > > > On Sat, Oct 10, 2020 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > > > > > On Sat, 10 Oct 2020 09:54:57 +0200 Dmitry Vyukov wrote: > > > > > > On Sat, Oct 10, 2020 at 1:16 AM Jakub Kicinski <kuba@kernel.org> 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? > > > > That'd be preferable. I understand with current use cases it doesn't > > really matter, but history shows people come up with all sort of > > wonderful use cases down the line. And when they do they rarely go back > > and fix such fundamental minutiae. > > > > > 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. > > > > It's not as much protocols as subsystems. The values are meaningful to > > a subsystem which inserts them, that doesn't mean single layer of the > > stack. If it was about storing layer's context we would just use > > skb->cb. > > > > So I think the kcov use matches pretty well. > > skb_extensions was the first thing that came to mind when I read this > patchset too. It is not specific to protocols. > > We have long stopped growing sk_buff size. Thank you all for your comments. I'll use skb extensions in v3 of the series.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index a828cf99c521..5639f27e05ef 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -701,6 +701,7 @@ typedef unsigned char *sk_buff_data_t; * @transport_header: Transport layer header * @network_header: Network layer header * @mac_header: Link layer header + * @kcov_handle: KCOV remote handle for remote coverage collection * @tail: Tail pointer * @end: End pointer * @head: Head of buffer @@ -904,6 +905,10 @@ struct sk_buff { __u16 network_header; __u16 mac_header; +#ifdef CONFIG_KCOV + u64 kcov_handle; +#endif + /* private: */ __u32 headers_end[0]; /* public: */ @@ -4605,5 +4610,21 @@ static inline void skb_reset_redirect(struct sk_buff *skb) #endif } +static inline void skb_set_kcov_handle(struct sk_buff *skb, const u64 kcov_handle) +{ +#ifdef CONFIG_KCOV + skb->kcov_handle = kcov_handle; +#endif +} + +static inline u64 skb_get_kcov_handle(struct sk_buff *skb) +{ +#ifdef CONFIG_KCOV + return skb->kcov_handle; +#else + return 0; +#endif +} + #endif /* __KERNEL__ */ #endif /* _LINUX_SKBUFF_H */ diff --git a/net/core/skbuff.c b/net/core/skbuff.c index f67631faa9aa..e7acd7d45b03 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -233,6 +233,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, skb->end = skb->tail + size; skb->mac_header = (typeof(skb->mac_header))~0U; skb->transport_header = (typeof(skb->transport_header))~0U; + skb_set_kcov_handle(skb, kcov_common_handle()); /* make sure we initialize shinfo sequentially */ shinfo = skb_shinfo(skb);