Re: [PATCH] bpf: fix a false positive kmemcheck warning

Re: [PATCH] bpf: fix a false positive kmemcheck warning

[Daniel Borkmann]

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

Re: [PATCH] bpf: fix a false positive kmemcheck warning

[Hannes Frederic Sowa]

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

Re: [PATCH] bpf: fix a false positive kmemcheck warning

[Daniel Borkmann]

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

Re: [PATCH] bpf: fix a false positive kmemcheck warning

[Mikulas Patocka]

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

Re: [PATCH] bpf: fix a false positive kmemcheck warning

[Daniel Borkmann]

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.

Re: [PATCH] bpf: fix a false positive kmemcheck warning

[Alexei Starovoitov]

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.

Re: [PATCH] bpf: fix a false positive kmemcheck warning

[Daniel Borkmann]

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.