* Re: [PATCH] bpf: fix a false positive kmemcheck warning [not found] <alpine.LRH.2.02.1409051157440.5269@file01.intranet.prod.int.rdu2.redhat.com> @ 2014-09-05 16:20 ` Daniel Borkmann 2014-09-05 17:00 ` Hannes Frederic Sowa 0 siblings, 1 reply; 7+ messages in thread From: Daniel Borkmann @ 2014-09-05 16:20 UTC (permalink / raw) To: Mikulas Patocka Cc: Alexei Starovoitov, Pablo Neira Ayuso, David S. Miller, linux-kernel, netdev Hi Mikulas, On 09/05/2014 06:01 PM, Mikulas Patocka wrote: > This patch fixes false positive kmemcheck warning in bpf. > > When we try to write the variable len, the compiler generates a code that > reads the 32-bit word, modifies the bits belonging to "len" and writes the > 32-bit word back. The reading of the word results in kmemcheck warning due > to reading uninitialized memory. This patch fixes it by avoiding using bit > fields when kmemcheck is enabled. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> You need to submit this patch to netdev (Cc'ed). > --- > include/linux/filter.h | 5 +++++ > 1 file changed, 5 insertions(+) > > Index: linux-2.6/include/linux/filter.h > =================================================================== > --- linux-2.6.orig/include/linux/filter.h 2014-09-04 23:04:26.000000000 +0200 > +++ linux-2.6/include/linux/filter.h 2014-09-04 23:43:05.000000000 +0200 > @@ -325,8 +325,13 @@ struct sock; > struct seccomp_data; > > struct bpf_prog { > +#ifdef CONFIG_KMEMCHECK > + bool jited; > + u32 len; > +#else > u32 jited:1, /* Is our filter JIT'ed? */ > len:31; /* Number of filter blocks */ > +#endif > struct sock_fprog_kern *orig_prog; /* Original BPF program */ > unsigned int (*bpf_func)(const struct sk_buff *skb, > const struct bpf_insn *filter); I don't really like this if-def. If you really want to fix it, can't you just use : kmemcheck_bitfield_begin(bpf_anc_data) ... kmemcheck_bitfield_end(bpf_anc_data) et al infrastructure as container (in case in future we will add some more bit flags, since len doesn't really need all 31 bit universe)? Note, there are currently some patches pending in patchwork that also fill the u32 hole thus the extra bool would introduce a new hole after that. Therefore, I think above would be better. Thanks, Daniel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bpf: fix a false positive kmemcheck warning 2014-09-05 16:20 ` [PATCH] bpf: fix a false positive kmemcheck warning Daniel Borkmann @ 2014-09-05 17:00 ` Hannes Frederic Sowa 2014-09-05 17:10 ` Daniel Borkmann 0 siblings, 1 reply; 7+ messages in thread From: Hannes Frederic Sowa @ 2014-09-05 17:00 UTC (permalink / raw) To: Daniel Borkmann Cc: Mikulas Patocka, Alexei Starovoitov, Pablo Neira Ayuso, David S. Miller, linux-kernel, netdev On Fr, 2014-09-05 at 18:20 +0200, Daniel Borkmann wrote: > Hi Mikulas, > > On 09/05/2014 06:01 PM, Mikulas Patocka wrote: > > This patch fixes false positive kmemcheck warning in bpf. > > > > When we try to write the variable len, the compiler generates a code that > > reads the 32-bit word, modifies the bits belonging to "len" and writes the > > 32-bit word back. The reading of the word results in kmemcheck warning due > > to reading uninitialized memory. This patch fixes it by avoiding using bit > > fields when kmemcheck is enabled. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > You need to submit this patch to netdev (Cc'ed). > > > --- > > include/linux/filter.h | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > Index: linux-2.6/include/linux/filter.h > > =================================================================== > > --- linux-2.6.orig/include/linux/filter.h 2014-09-04 23:04:26.000000000 +0200 > > +++ linux-2.6/include/linux/filter.h 2014-09-04 23:43:05.000000000 +0200 > > @@ -325,8 +325,13 @@ struct sock; > > struct seccomp_data; > > > > struct bpf_prog { > > +#ifdef CONFIG_KMEMCHECK > > + bool jited; > > + u32 len; > > +#else > > u32 jited:1, /* Is our filter JIT'ed? */ > > len:31; /* Number of filter blocks */ > > +#endif > > struct sock_fprog_kern *orig_prog; /* Original BPF program */ > > unsigned int (*bpf_func)(const struct sk_buff *skb, > > const struct bpf_insn *filter); > > I don't really like this if-def. If you really want to fix it, can't > you just use : > > kmemcheck_bitfield_begin(bpf_anc_data) > ... > kmemcheck_bitfield_end(bpf_anc_data) you also need to annotate the bitfield after allocation: struct bpf_prog *prog = kalloc(...); kmemcheck_annotate_bitfield(prog, bpf_anc_data); Bye, Hannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bpf: fix a false positive kmemcheck warning 2014-09-05 17:00 ` Hannes Frederic Sowa @ 2014-09-05 17:10 ` Daniel Borkmann 2014-09-05 17:13 ` Mikulas Patocka 0 siblings, 1 reply; 7+ messages in thread From: Daniel Borkmann @ 2014-09-05 17:10 UTC (permalink / raw) To: Hannes Frederic Sowa Cc: Mikulas Patocka, Alexei Starovoitov, Pablo Neira Ayuso, David S. Miller, linux-kernel, netdev On 09/05/2014 07:00 PM, Hannes Frederic Sowa wrote: > On Fr, 2014-09-05 at 18:20 +0200, Daniel Borkmann wrote: >> Hi Mikulas, >> >> On 09/05/2014 06:01 PM, Mikulas Patocka wrote: >>> This patch fixes false positive kmemcheck warning in bpf. >>> >>> When we try to write the variable len, the compiler generates a code that >>> reads the 32-bit word, modifies the bits belonging to "len" and writes the >>> 32-bit word back. The reading of the word results in kmemcheck warning due >>> to reading uninitialized memory. This patch fixes it by avoiding using bit >>> fields when kmemcheck is enabled. >>> >>> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> >> >> You need to submit this patch to netdev (Cc'ed). >> >>> --- >>> include/linux/filter.h | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> Index: linux-2.6/include/linux/filter.h >>> =================================================================== >>> --- linux-2.6.orig/include/linux/filter.h 2014-09-04 23:04:26.000000000 +0200 >>> +++ linux-2.6/include/linux/filter.h 2014-09-04 23:43:05.000000000 +0200 >>> @@ -325,8 +325,13 @@ struct sock; >>> struct seccomp_data; >>> >>> struct bpf_prog { >>> +#ifdef CONFIG_KMEMCHECK >>> + bool jited; >>> + u32 len; >>> +#else >>> u32 jited:1, /* Is our filter JIT'ed? */ >>> len:31; /* Number of filter blocks */ >>> +#endif >>> struct sock_fprog_kern *orig_prog; /* Original BPF program */ >>> unsigned int (*bpf_func)(const struct sk_buff *skb, >>> const struct bpf_insn *filter); >> >> I don't really like this if-def. If you really want to fix it, can't >> you just use : >> >> kmemcheck_bitfield_begin(bpf_anc_data) >> ... >> kmemcheck_bitfield_end(bpf_anc_data) > > you also need to annotate the bitfield after allocation: > struct bpf_prog *prog = kalloc(...); > kmemcheck_annotate_bitfield(prog, bpf_anc_data); Yes, sure, sorry if that was not clear from my side, that was what I intended to say with kmemcheck /infrastructure/. :) > Bye, > Hannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bpf: fix a false positive kmemcheck warning 2014-09-05 17:10 ` Daniel Borkmann @ 2014-09-05 17:13 ` Mikulas Patocka 2014-09-05 17:17 ` Daniel Borkmann 0 siblings, 1 reply; 7+ messages in thread From: Mikulas Patocka @ 2014-09-05 17:13 UTC (permalink / raw) To: Daniel Borkmann Cc: Hannes Frederic Sowa, Alexei Starovoitov, Pablo Neira Ayuso, David S. Miller, linux-kernel, netdev On Fri, 5 Sep 2014, Daniel Borkmann wrote: > On 09/05/2014 07:00 PM, Hannes Frederic Sowa wrote: > > On Fr, 2014-09-05 at 18:20 +0200, Daniel Borkmann wrote: > > > Hi Mikulas, > > > > > > On 09/05/2014 06:01 PM, Mikulas Patocka wrote: > > > > This patch fixes false positive kmemcheck warning in bpf. > > > > > > > > When we try to write the variable len, the compiler generates a code > > > > that > > > > reads the 32-bit word, modifies the bits belonging to "len" and writes > > > > the > > > > 32-bit word back. The reading of the word results in kmemcheck warning > > > > due > > > > to reading uninitialized memory. This patch fixes it by avoiding using > > > > bit > > > > fields when kmemcheck is enabled. > > > > > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > > > > > You need to submit this patch to netdev (Cc'ed). > > > > > > > --- > > > > include/linux/filter.h | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > Index: linux-2.6/include/linux/filter.h > > > > =================================================================== > > > > --- linux-2.6.orig/include/linux/filter.h 2014-09-04 23:04:26.000000000 > > > > +0200 > > > > +++ linux-2.6/include/linux/filter.h 2014-09-04 23:43:05.000000000 > > > > +0200 > > > > @@ -325,8 +325,13 @@ struct sock; > > > > struct seccomp_data; > > > > > > > > struct bpf_prog { > > > > +#ifdef CONFIG_KMEMCHECK > > > > + bool jited; > > > > + u32 len; > > > > +#else > > > > u32 jited:1, /* Is our filter > > > > JIT'ed? */ > > > > len:31; /* Number of filter > > > > blocks */ > > > > +#endif > > > > struct sock_fprog_kern *orig_prog; /* Original BPF > > > > program */ > > > > unsigned int (*bpf_func)(const struct sk_buff *skb, > > > > const struct bpf_insn > > > > *filter); > > > > > > I don't really like this if-def. If you really want to fix it, can't > > > you just use : > > > > > > kmemcheck_bitfield_begin(bpf_anc_data) > > > ... > > > kmemcheck_bitfield_end(bpf_anc_data) > > > > you also need to annotate the bitfield after allocation: > > struct bpf_prog *prog = kalloc(...); > > kmemcheck_annotate_bitfield(prog, bpf_anc_data); > > Yes, sure, sorry if that was not clear from my side, that was what I > intended to say with kmemcheck /infrastructure/. :) So, change it to use these markings. I'm not an expert in this area, so I don't know all the places where this structure could be allocated. If you know them all, mark it in this way. Mikulas ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bpf: fix a false positive kmemcheck warning 2014-09-05 17:13 ` Mikulas Patocka @ 2014-09-05 17:17 ` Daniel Borkmann 2014-09-05 17:21 ` Alexei Starovoitov 0 siblings, 1 reply; 7+ messages in thread From: Daniel Borkmann @ 2014-09-05 17:17 UTC (permalink / raw) To: Mikulas Patocka Cc: Hannes Frederic Sowa, Alexei Starovoitov, Pablo Neira Ayuso, David S. Miller, linux-kernel, netdev On 09/05/2014 07:13 PM, Mikulas Patocka wrote: > On Fri, 5 Sep 2014, Daniel Borkmann wrote: >> On 09/05/2014 07:00 PM, Hannes Frederic Sowa wrote: >>> On Fr, 2014-09-05 at 18:20 +0200, Daniel Borkmann wrote: >>>> Hi Mikulas, >>>> >>>> On 09/05/2014 06:01 PM, Mikulas Patocka wrote: >>>>> This patch fixes false positive kmemcheck warning in bpf. >>>>> >>>>> When we try to write the variable len, the compiler generates a code >>>>> that >>>>> reads the 32-bit word, modifies the bits belonging to "len" and writes >>>>> the >>>>> 32-bit word back. The reading of the word results in kmemcheck warning >>>>> due >>>>> to reading uninitialized memory. This patch fixes it by avoiding using >>>>> bit >>>>> fields when kmemcheck is enabled. >>>>> >>>>> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> >>>> >>>> You need to submit this patch to netdev (Cc'ed). >>>> >>>>> --- >>>>> include/linux/filter.h | 5 +++++ >>>>> 1 file changed, 5 insertions(+) >>>>> >>>>> Index: linux-2.6/include/linux/filter.h >>>>> =================================================================== >>>>> --- linux-2.6.orig/include/linux/filter.h 2014-09-04 23:04:26.000000000 >>>>> +0200 >>>>> +++ linux-2.6/include/linux/filter.h 2014-09-04 23:43:05.000000000 >>>>> +0200 >>>>> @@ -325,8 +325,13 @@ struct sock; >>>>> struct seccomp_data; >>>>> >>>>> struct bpf_prog { >>>>> +#ifdef CONFIG_KMEMCHECK >>>>> + bool jited; >>>>> + u32 len; >>>>> +#else >>>>> u32 jited:1, /* Is our filter >>>>> JIT'ed? */ >>>>> len:31; /* Number of filter >>>>> blocks */ >>>>> +#endif >>>>> struct sock_fprog_kern *orig_prog; /* Original BPF >>>>> program */ >>>>> unsigned int (*bpf_func)(const struct sk_buff *skb, >>>>> const struct bpf_insn >>>>> *filter); >>>> >>>> I don't really like this if-def. If you really want to fix it, can't >>>> you just use : >>>> >>>> kmemcheck_bitfield_begin(bpf_anc_data) >>>> ... >>>> kmemcheck_bitfield_end(bpf_anc_data) >>> >>> you also need to annotate the bitfield after allocation: >>> struct bpf_prog *prog = kalloc(...); >>> kmemcheck_annotate_bitfield(prog, bpf_anc_data); >> >> Yes, sure, sorry if that was not clear from my side, that was what I >> intended to say with kmemcheck /infrastructure/. :) > > So, change it to use these markings. I'm not an expert in this area, so I > don't know all the places where this structure could be allocated. If you > know them all, mark it in this way. Ok, fine by me. I have some pending items, so I'll put it on top of them. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bpf: fix a false positive kmemcheck warning 2014-09-05 17:17 ` Daniel Borkmann @ 2014-09-05 17:21 ` Alexei Starovoitov 2014-09-05 17:34 ` Daniel Borkmann 0 siblings, 1 reply; 7+ messages in thread From: Alexei Starovoitov @ 2014-09-05 17:21 UTC (permalink / raw) To: Daniel Borkmann Cc: Mikulas Patocka, Hannes Frederic Sowa, Pablo Neira Ayuso, David S. Miller, LKML, Network Development On Fri, Sep 5, 2014 at 10:17 AM, Daniel Borkmann <dborkman@redhat.com> wrote: > On 09/05/2014 07:13 PM, Mikulas Patocka wrote: >> >> On Fri, 5 Sep 2014, Daniel Borkmann wrote: >>> >>> On 09/05/2014 07:00 PM, Hannes Frederic Sowa wrote: >>>> >>>> On Fr, 2014-09-05 at 18:20 +0200, Daniel Borkmann wrote: >>>>> >>>>> Hi Mikulas, >>>>> >>>>> On 09/05/2014 06:01 PM, Mikulas Patocka wrote: >>>>>> >>>>>> This patch fixes false positive kmemcheck warning in bpf. >>>>>> >>>>>> When we try to write the variable len, the compiler generates a code >>>>>> that >>>>>> reads the 32-bit word, modifies the bits belonging to "len" and writes >>>>>> the >>>>>> 32-bit word back. The reading of the word results in kmemcheck warning >>>>>> due >>>>>> to reading uninitialized memory. This patch fixes it by avoiding using >>>>>> bit >>>>>> fields when kmemcheck is enabled. >>>>>> >>>>>> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> >>>>> >>>>> >>>>> You need to submit this patch to netdev (Cc'ed). >>>>> >>>>>> --- >>>>>> include/linux/filter.h | 5 +++++ >>>>>> 1 file changed, 5 insertions(+) >>>>>> >>>>>> Index: linux-2.6/include/linux/filter.h >>>>>> =================================================================== >>>>>> --- linux-2.6.orig/include/linux/filter.h 2014-09-04 >>>>>> 23:04:26.000000000 >>>>>> +0200 >>>>>> +++ linux-2.6/include/linux/filter.h 2014-09-04 23:43:05.000000000 >>>>>> +0200 >>>>>> @@ -325,8 +325,13 @@ struct sock; >>>>>> struct seccomp_data; >>>>>> >>>>>> struct bpf_prog { >>>>>> +#ifdef CONFIG_KMEMCHECK >>>>>> + bool jited; >>>>>> + u32 len; >>>>>> +#else >>>>>> u32 jited:1, /* Is our filter >>>>>> JIT'ed? */ >>>>>> len:31; /* Number of filter >>>>>> blocks */ >>>>>> +#endif >>>>>> struct sock_fprog_kern *orig_prog; /* Original BPF >>>>>> program */ >>>>>> unsigned int (*bpf_func)(const struct sk_buff *skb, >>>>>> const struct bpf_insn >>>>>> *filter); >>>>> >>>>> >>>>> I don't really like this if-def. If you really want to fix it, can't >>>>> you just use : >>>>> >>>>> kmemcheck_bitfield_begin(bpf_anc_data) >>>>> ... >>>>> kmemcheck_bitfield_end(bpf_anc_data) >>>> >>>> >>>> you also need to annotate the bitfield after allocation: >>>> struct bpf_prog *prog = kalloc(...); >>>> kmemcheck_annotate_bitfield(prog, bpf_anc_data); >>> >>> >>> Yes, sure, sorry if that was not clear from my side, that was what I >>> intended to say with kmemcheck /infrastructure/. :) >> >> >> So, change it to use these markings. I'm not an expert in this area, so I >> don't know all the places where this structure could be allocated. If you >> know them all, mark it in this way. > > > Ok, fine by me. I have some pending items, so I'll put it > on top of them. imo it's cleaner to convert to bool unconditionally instead of annotating things everywhere. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bpf: fix a false positive kmemcheck warning 2014-09-05 17:21 ` Alexei Starovoitov @ 2014-09-05 17:34 ` Daniel Borkmann 0 siblings, 0 replies; 7+ messages in thread From: Daniel Borkmann @ 2014-09-05 17:34 UTC (permalink / raw) To: Alexei Starovoitov Cc: Mikulas Patocka, Hannes Frederic Sowa, Pablo Neira Ayuso, David S. Miller, LKML, Network Development On 09/05/2014 07:21 PM, Alexei Starovoitov wrote: ... > imo it's cleaner to convert to bool unconditionally instead > of annotating things everywhere. Will do, fine as well. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-05 17:34 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <alpine.LRH.2.02.1409051157440.5269@file01.intranet.prod.int.rdu2.redhat.com>
2014-09-05 16:20 ` [PATCH] bpf: fix a false positive kmemcheck warning Daniel Borkmann
2014-09-05 17:00 ` Hannes Frederic Sowa
2014-09-05 17:10 ` Daniel Borkmann
2014-09-05 17:13 ` Mikulas Patocka
2014-09-05 17:17 ` Daniel Borkmann
2014-09-05 17:21 ` Alexei Starovoitov
2014-09-05 17:34 ` Daniel Borkmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).