[PATCH] fib_trie: Fix potential null pointer dereference

[PATCH] fib_trie: Fix potential null pointer dereference

[Firo Yang]

A smatch warning.
When kmem_cache_alloc() failed to alloc memory, a null pointer
will be returned. Redeference null pointer will generate
an unnecessary oops. So, use it after check.

Signed-off-by: Firo Yang <firogm@gmail.com>
---
 net/ipv4/fib_trie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 01bce15..34094c7 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
 static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
 {
 	struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
-	struct key_vector *l = kv->kv;
+	struct key_vector *l;
 
 	if (!kv)
 		return NULL;
 
 	/* initialize key vector */
+	l = kv->kv;
 	l->key = key;
 	l->pos = 0;
 	l->bits = 0;
-- 
2.4.2

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[Eric Dumazet]

On Sat, 2015-06-06 at 19:35 +0800, Firo Yang wrote:
> A smatch warning.
> When kmem_cache_alloc() failed to alloc memory, a null pointer
> will be returned. Redeference null pointer will generate


Dereferencing a null pointer will crash.

> an unnecessary oops. So, use it after check.
> 
> Signed-off-by: Firo Yang <firogm@gmail.com>
> ---
>  net/ipv4/fib_trie.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 01bce15..34094c7 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
>  static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
>  {
>  	struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
> -	struct key_vector *l = kv->kv;
> +	struct key_vector *l;
>  
>  	if (!kv)
>  		return NULL;
>  
>  	/* initialize key vector */
> +	l = kv->kv;
>  	l->key = key;
>  	l->pos = 0;
>  	l->bits = 0;

Fixes: dc35dbeda3e0 ("fib_trie: Add tnode struct as a container for fields not needed in key_vector")
Acked-by: Eric Dumazet <edumazet@google.com>

Thanks.

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[walter harms]

Am 06.06.2015 13:35, schrieb Firo Yang:
> A smatch warning.
> When kmem_cache_alloc() failed to alloc memory, a null pointer
> will be returned. Redeference null pointer will generate
> an unnecessary oops. So, use it after check.
> 
> Signed-off-by: Firo Yang <firogm@gmail.com>
> ---
>  net/ipv4/fib_trie.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 01bce15..34094c7 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
>  static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
>  {
>  	struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
> -	struct key_vector *l = kv->kv;
> +	struct key_vector *l;

It is a good custom to have action and check close together, so this may be more
obvious for future readers:
	struct tnode *kv;
	struct key_vector *l;

        kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
  	if (!kv)
  		return NULL;


re,
 wh

>  	/* initialize key vector */
> +	l = kv->kv;
>  	l->key = key;
>  	l->pos = 0;
>  	l->bits = 0;

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[Alexander Duyck]

On 06/06/2015 05:05 AM, Eric Dumazet wrote:
> On Sat, 2015-06-06 at 19:35 +0800, Firo Yang wrote:
>> A smatch warning.
>> When kmem_cache_alloc() failed to alloc memory, a null pointer
>> will be returned. Redeference null pointer will generate
>
>
> Dereferencing a null pointer will crash.
>
>> an unnecessary oops. So, use it after check.
>>
>> Signed-off-by: Firo Yang <firogm@gmail.com>
>> ---
>>   net/ipv4/fib_trie.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>> index 01bce15..34094c7 100644
>> --- a/net/ipv4/fib_trie.c
>> +++ b/net/ipv4/fib_trie.c
>> @@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
>>   static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
>>   {
>>   	struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
>> -	struct key_vector *l = kv->kv;
>> +	struct key_vector *l;
>>
>>   	if (!kv)
>>   		return NULL;
>>
>>   	/* initialize key vector */
>> +	l = kv->kv;
>>   	l->key = key;
>>   	l->pos = 0;
>>   	l->bits = 0;
>
> Fixes: dc35dbeda3e0 ("fib_trie: Add tnode struct as a container for fields not needed in key_vector")
> Acked-by: Eric Dumazet <edumazet@google.com>
>
> Thanks.

kv->kv isn't a dereference.  kv is an array contained within the tnode. 
  Below is the layout of struct tnode:


struct tnode {
	struct rcu_head rcu;
	t_key empty_children;		/* KEYLENGTH bits needed */
	t_key full_children;		/* KEYLENGTH bits needed */
	struct key_vector __rcu *parent;
	struct key_vector kv[1];
#define tn_bits kv[0].bits
};


As such kv->kv doesn't actually dereference anything, it is simply a 
means for getting the offset to the array from the pointer kv.  It 
should be equivalent to &kv->kv[0] which last I knew was how the 
offsetof macro basically worked and it uses a NULL pointer.  The first 
actual derference occurs at "l->key = key" which should be safe since 
writing to a memory location has side effects so the compiler cannot 
reorder the writes before the !kv check.

The patch itself is fine.  However what it is fixing is the smatch false 
positive, not a null pointer deference.  As such you may need to update 
the comments to reflect that.

- Alex

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[Firo Yang]

On Sat, Jun 06, 2015 at 08:16:50PM -0700, Alexander Duyck wrote:
>On 06/06/2015 05:05 AM, Eric Dumazet wrote:
>>On Sat, 2015-06-06 at 19:35 +0800, Firo Yang wrote:
>>>A smatch warning.
>>>When kmem_cache_alloc() failed to alloc memory, a null pointer
>>>will be returned. Redeference null pointer will generate
>>
>>Dereferencing a null pointer will crash.
>>
>>>an unnecessary oops. So, use it after check.
>>>
>>>Signed-off-by: Firo Yang <firogm@gmail.com>
>>>---
>>>  net/ipv4/fib_trie.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>>>index 01bce15..34094c7 100644
>>>--- a/net/ipv4/fib_trie.c
>>>+++ b/net/ipv4/fib_trie.c
>>>@@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
>>>  static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
>>>  {
>>>  	struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
>>>-	struct key_vector *l = kv->kv;
>>>+	struct key_vector *l;
>>>  	if (!kv)
>>>  		return NULL;
>>>  	/* initialize key vector */
>>>+	l = kv->kv;
>>>  	l->key = key;
>>>  	l->pos = 0;
>>>  	l->bits = 0;
>>Fixes: dc35dbeda3e0 ("fib_trie: Add tnode struct as a container for fields not needed in key_vector")
>>Acked-by: Eric Dumazet <edumazet@google.com>
>>
>>Thanks.
>
>kv->kv isn't a dereference.  kv is an array contained within the tnode.
>Below is the layout of struct tnode:
>
>|
>struct  tnode{
>	struct  rcu_head rcu;
>	t_key empty_children;		/* KEYLENGTH bits needed */
>	t_key full_children;		/* KEYLENGTH bits needed */
>	struct  key_vector __rcu*parent;
>	struct  key_vector kv[1];
>#define tn_bits kv[0].bits
>};|
>
>
>As such kv->kv doesn't actually dereference anything, it is simply a means
>for getting the offset to the array from the pointer kv.  It should be
>equivalent to &kv->kv[0] which last I knew was how the offsetof macro
>basically worked and it uses a NULL pointer.  The first actual derference
>occurs at "l->key = key" which should be safe since writing to a memory
>location has side effects so the compiler cannot reorder the writes before
>the !kv check.
>
>The patch itself is fine.  However what it is fixing is the smatch false
>positive, not a null pointer deference.  As such you may need to update the
>comments to reflect that.

Thanks Alex for firguring out my false cognition!
I will update the patch with new comments.

Firo

>
>- Alex
>
>

-- 

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[Firo Yang]

On Sat, Jun 06, 2015 at 03:39:38PM +0200, walter harms wrote:
>
>
>Am 06.06.2015 13:35, schrieb Firo Yang:
>> A smatch warning.
>> When kmem_cache_alloc() failed to alloc memory, a null pointer
>> will be returned. Redeference null pointer will generate
>> an unnecessary oops. So, use it after check.
>> 
>> Signed-off-by: Firo Yang <firogm@gmail.com>
>> ---
>>  net/ipv4/fib_trie.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>> index 01bce15..34094c7 100644
>> --- a/net/ipv4/fib_trie.c
>> +++ b/net/ipv4/fib_trie.c
>> @@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
>>  static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
>>  {
>>  	struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
>> -	struct key_vector *l = kv->kv;
>> +	struct key_vector *l;
>
>It is a good custom to have action and check close together, so this may be more
>obvious for future readers:
>	struct tnode *kv;
>	struct key_vector *l;
>
>        kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
>  	if (!kv)
>  		return NULL;
>
>
>re,
> wh
Thanks walter harms, I will update the patch like this.
>
>>  	/* initialize key vector */
>> +	l = kv->kv;
>>  	l->key = key;
>>  	l->pos = 0;
>>  	l->bits = 0;

-- 

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[Firo Yang]

On Sat, Jun 06, 2015 at 05:05:04AM -0700, Eric Dumazet wrote:
>On Sat, 2015-06-06 at 19:35 +0800, Firo Yang wrote:
>> A smatch warning.
>> When kmem_cache_alloc() failed to alloc memory, a null pointer
>> will be returned. Redeference null pointer will generate
>
>
>Dereferencing a null pointer will crash.
>
>> an unnecessary oops. So, use it after check.
>> 
>> Signed-off-by: Firo Yang <firogm@gmail.com>
>> ---
>>  net/ipv4/fib_trie.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>> index 01bce15..34094c7 100644
>> --- a/net/ipv4/fib_trie.c
>> +++ b/net/ipv4/fib_trie.c
>> @@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
>>  static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
>>  {
>>  	struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
>> -	struct key_vector *l = kv->kv;
>> +	struct key_vector *l;
>>  
>>  	if (!kv)
>>  		return NULL;
>>  
>>  	/* initialize key vector */
>> +	l = kv->kv;
>>  	l->key = key;
>>  	l->pos = 0;
>>  	l->bits = 0;
>
>Fixes: dc35dbeda3e0 ("fib_trie: Add tnode struct as a container for fields not needed in key_vector")
>Acked-by: Eric Dumazet <edumazet@google.com>
>
>Thanks.

Hi Eric,
Please discard this useless patch figured out by Alexander Duyck. 
I will send a patch to Smatch for eliminating the negative warning.

>
>

-- 

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[Julia Lawall]

On Sun, 7 Jun 2015, Firo Yang wrote:

> On Sat, Jun 06, 2015 at 05:05:04AM -0700, Eric Dumazet wrote:
> >On Sat, 2015-06-06 at 19:35 +0800, Firo Yang wrote:
> >> A smatch warning.
> >> When kmem_cache_alloc() failed to alloc memory, a null pointer
> >> will be returned. Redeference null pointer will generate
> >
> >
> >Dereferencing a null pointer will crash.
> >
> >> an unnecessary oops. So, use it after check.
> >> 
> >> Signed-off-by: Firo Yang <firogm@gmail.com>
> >> ---
> >>  net/ipv4/fib_trie.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> >> index 01bce15..34094c7 100644
> >> --- a/net/ipv4/fib_trie.c
> >> +++ b/net/ipv4/fib_trie.c
> >> @@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
> >>  static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
> >>  {
> >>  	struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
> >> -	struct key_vector *l = kv->kv;
> >> +	struct key_vector *l;
> >>  
> >>  	if (!kv)
> >>  		return NULL;
> >>  
> >>  	/* initialize key vector */
> >> +	l = kv->kv;
> >>  	l->key = key;
> >>  	l->pos = 0;
> >>  	l->bits = 0;
> >
> >Fixes: dc35dbeda3e0 ("fib_trie: Add tnode struct as a container for fields not needed in key_vector")
> >Acked-by: Eric Dumazet <edumazet@google.com>
> >
> >Thanks.
> 
> Hi Eric,
> Please discard this useless patch figured out by Alexander Duyck. 
> I will send a patch to Smatch for eliminating the negative warning.

I think that many people would make the same mistake when looking at the 
code.  The change doesn't seem to hurt anything?

julia

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[Firo Yang]

On Sun, Jun 07, 2015 at 08:36:45AM +0200, Julia Lawall wrote:
>
>
>On Sun, 7 Jun 2015, Firo Yang wrote:
>
>> On Sat, Jun 06, 2015 at 05:05:04AM -0700, Eric Dumazet wrote:
>> >On Sat, 2015-06-06 at 19:35 +0800, Firo Yang wrote:
>> >> A smatch warning.
>> >> When kmem_cache_alloc() failed to alloc memory, a null pointer
>> >> will be returned. Redeference null pointer will generate
>> >
>> >
>> >Dereferencing a null pointer will crash.
>> >
>> >> an unnecessary oops. So, use it after check.
>> >> 
>> >> Signed-off-by: Firo Yang <firogm@gmail.com>
>> >> ---
>> >>  net/ipv4/fib_trie.c | 3 ++-
>> >>  1 file changed, 2 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>> >> index 01bce15..34094c7 100644
>> >> --- a/net/ipv4/fib_trie.c
>> >> +++ b/net/ipv4/fib_trie.c
>> >> @@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
>> >>  static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
>> >>  {
>> >>  	struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
>> >> -	struct key_vector *l = kv->kv;
>> >> +	struct key_vector *l;
>> >>  
>> >>  	if (!kv)
>> >>  		return NULL;
>> >>  
>> >>  	/* initialize key vector */
>> >> +	l = kv->kv;
>> >>  	l->key = key;
>> >>  	l->pos = 0;
>> >>  	l->bits = 0;
>> >
>> >Fixes: dc35dbeda3e0 ("fib_trie: Add tnode struct as a container for fields not needed in key_vector")
>> >Acked-by: Eric Dumazet <edumazet@google.com>
>> >
>> >Thanks.
>> 
>> Hi Eric,
>> Please discard this useless patch figured out by Alexander Duyck. 
>> I will send a patch to Smatch for eliminating the negative warning.
>
>I think that many people would make the same mistake when looking at the 
>code.  The change doesn't seem to hurt anything?
Actually, yes. But it does imporve nothing but coding style.
I think Alexander's code style make function more compact is also a
good code style. So, just keep it original.

Firo
>
>julia

-- 

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[Julia Lawall]

> >I think that many people would make the same mistake when looking at the
> >code.  The change doesn't seem to hurt anything?
> Actually, yes. But it does imporve nothing but coding style.
> I think Alexander's code style make function more compact is also a
> good code style. So, just keep it original.

Still it makes noise when other people look at the code and find that it
looks odd.  Removing the false positive in smatch won't completely help,
because people could look at the code for other reasons.

But, as you like.

julia

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[Firo Yang]

On Sun, Jun 07, 2015 at 03:23:57PM +0200, Julia Lawall wrote:
>> >I think that many people would make the same mistake when looking at the
>> >code.  The change doesn't seem to hurt anything?
>> Actually, yes. But it does imporve nothing but coding style.
>> I think Alexander's code style make function more compact is also a
>> good code style. So, just keep it original.
>
>Still it makes noise when other people look at the code and find that it
>looks odd.  Removing the false positive in smatch won't completely help,
>because people could look at the code for other reasons.

You said it! I will do the improvement.

Firo

>
>But, as you like.
>
>julia

-- 

Re: [PATCH] fib_trie: Fix potential null pointer dereference

[Alexander Duyck]

On 06/07/2015 06:23 AM, Julia Lawall wrote:
>>> I think that many people would make the same mistake when looking at the
>>> code.  The change doesn't seem to hurt anything?
>> Actually, yes. But it does imporve nothing but coding style.
>> I think Alexander's code style make function more compact is also a
>> good code style. So, just keep it original.
>
> Still it makes noise when other people look at the code and find that it
> looks odd.  Removing the false positive in smatch won't completely help,
> because people could look at the code for other reasons.

I agree.  The patch is good.  My only comment was about the patch 
description.  You are fixing a coding style issue reported by smatch, 
not a NULL pointer dereference.  This is a code clean-up rather than a 
fix, but still the patch is just as useful.

While you are at it you could probably also incorporate the suggestion 
from Walter as that helps to improve the readability further.

- Alex