[PATCH] net: ipv4: Fix uninitialized pointer warning in fnhe_remove_oldest

[PATCH] net: ipv4: Fix uninitialized pointer warning in fnhe_remove_oldest

[Purva Yeshi]

Fix Smatch-detected issue:
net/ipv4/route.c:605 fnhe_remove_oldest() error:
uninitialized symbol 'oldest_p'.

Initialize oldest_p to NULL to avoid uninitialized pointer warning in
fnhe_remove_oldest.

Check that oldest_p is not NULL after the loop to ensure no dereferencing
of uninitialized pointers.

Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
---
 net/ipv4/route.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 753704f75b2c..2e5159127cb9 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -587,7 +587,7 @@ static void fnhe_flush_routes(struct fib_nh_exception *fnhe)
 
 static void fnhe_remove_oldest(struct fnhe_hash_bucket *hash)
 {
-	struct fib_nh_exception __rcu **fnhe_p, **oldest_p;
+	struct fib_nh_exception __rcu **fnhe_p, **oldest_p = NULL;
 	struct fib_nh_exception *fnhe, *oldest = NULL;
 
 	for (fnhe_p = &hash->chain; ; fnhe_p = &fnhe->fnhe_next) {
@@ -601,9 +601,12 @@ static void fnhe_remove_oldest(struct fnhe_hash_bucket *hash)
 			oldest_p = fnhe_p;
 		}
 	}
-	fnhe_flush_routes(oldest);
-	*oldest_p = oldest->fnhe_next;
-	kfree_rcu(oldest, rcu);
+
+	if (oldest_p) {  /* Ensure to have valid oldest_p element */
+		fnhe_flush_routes(oldest);
+		*oldest_p = oldest->fnhe_next;
+		kfree_rcu(oldest, rcu);
+	}
 }
 
 static u32 fnhe_hashfun(__be32 daddr)
-- 
2.34.1

Re: [PATCH] net: ipv4: Fix uninitialized pointer warning in fnhe_remove_oldest

[Kuniyuki Iwashima]

From: Purva Yeshi <purvayeshi550@gmail.com>
Date: Thu, 17 Apr 2025 15:11:26 +0530
> Fix Smatch-detected issue:
> net/ipv4/route.c:605 fnhe_remove_oldest() error:
> uninitialized symbol 'oldest_p'.
> 
> Initialize oldest_p to NULL to avoid uninitialized pointer warning in
> fnhe_remove_oldest.

How does it remain uninitialised ?

update_or_create_fnhe() ensures the bucket is not empty before
calling fnhe_remove_oldest().


> 
> Check that oldest_p is not NULL after the loop to ensure no dereferencing
> of uninitialized pointers.
> 
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> ---
>  net/ipv4/route.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 753704f75b2c..2e5159127cb9 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -587,7 +587,7 @@ static void fnhe_flush_routes(struct fib_nh_exception *fnhe)
>  
>  static void fnhe_remove_oldest(struct fnhe_hash_bucket *hash)
>  {
> -	struct fib_nh_exception __rcu **fnhe_p, **oldest_p;
> +	struct fib_nh_exception __rcu **fnhe_p, **oldest_p = NULL;
>  	struct fib_nh_exception *fnhe, *oldest = NULL;
>  
>  	for (fnhe_p = &hash->chain; ; fnhe_p = &fnhe->fnhe_next) {
> @@ -601,9 +601,12 @@ static void fnhe_remove_oldest(struct fnhe_hash_bucket *hash)
>  			oldest_p = fnhe_p;
>  		}
>  	}
> -	fnhe_flush_routes(oldest);
> -	*oldest_p = oldest->fnhe_next;
> -	kfree_rcu(oldest, rcu);
> +
> +	if (oldest_p) {  /* Ensure to have valid oldest_p element */
> +		fnhe_flush_routes(oldest);
> +		*oldest_p = oldest->fnhe_next;
> +		kfree_rcu(oldest, rcu);
> +	}
>  }
>  
>  static u32 fnhe_hashfun(__be32 daddr)
> -- 

Re: [PATCH] net: ipv4: Fix uninitialized pointer warning in fnhe_remove_oldest

[David Ahern]

On 4/17/25 4:00 PM, Kuniyuki Iwashima wrote:
> From: Purva Yeshi <purvayeshi550@gmail.com>
> Date: Thu, 17 Apr 2025 15:11:26 +0530
>> Fix Smatch-detected issue:
>> net/ipv4/route.c:605 fnhe_remove_oldest() error:
>> uninitialized symbol 'oldest_p'.
>>
>> Initialize oldest_p to NULL to avoid uninitialized pointer warning in
>> fnhe_remove_oldest.
> 
> How does it remain uninitialised ?
> 
> update_or_create_fnhe() ensures the bucket is not empty before
> calling fnhe_remove_oldest().
> 

agreed. Not the simplest logic, but I do not see how oldest_p can be
unset after the loop.

Re: [PATCH] net: ipv4: Fix uninitialized pointer warning in fnhe_remove_oldest

[Purva Yeshi]

On 18/04/25 10:33, David Ahern wrote:
> On 4/17/25 4:00 PM, Kuniyuki Iwashima wrote:
>> From: Purva Yeshi <purvayeshi550@gmail.com>
>> Date: Thu, 17 Apr 2025 15:11:26 +0530
>>> Fix Smatch-detected issue:
>>> net/ipv4/route.c:605 fnhe_remove_oldest() error:
>>> uninitialized symbol 'oldest_p'.
>>>
>>> Initialize oldest_p to NULL to avoid uninitialized pointer warning in
>>> fnhe_remove_oldest.
>>
>> How does it remain uninitialised ?
>>
>> update_or_create_fnhe() ensures the bucket is not empty before
>> calling fnhe_remove_oldest().
>>
> 
> agreed. Not the simplest logic, but I do not see how oldest_p can be
> unset after the loop.

Hi David,

The loop always sets oldest_p when the list has at least one entry, 
which the caller guarantees. Smatch doesn't catch that context, so it 
flags a false positive.

Best regards,
Purva

Re: [PATCH] net: ipv4: Fix uninitialized pointer warning in fnhe_remove_oldest

[Purva Yeshi]

On 18/04/25 03:30, Kuniyuki Iwashima wrote:
> From: Purva Yeshi <purvayeshi550@gmail.com>
> Date: Thu, 17 Apr 2025 15:11:26 +0530
>> Fix Smatch-detected issue:
>> net/ipv4/route.c:605 fnhe_remove_oldest() error:
>> uninitialized symbol 'oldest_p'.
>>
>> Initialize oldest_p to NULL to avoid uninitialized pointer warning in
>> fnhe_remove_oldest.
> 
> How does it remain uninitialised ?
> 
> update_or_create_fnhe() ensures the bucket is not empty before
> calling fnhe_remove_oldest().

Hi Kuniyuki,

Thanks for the feedback.

Smatch reports this because oldest_p is conditionally assigned inside 
the loop, and it doesn't reason about the guarantees made by 
update_or_create_fnhe() (i.e., that the list is non-empty). So it flags 
oldest_p as potentially uninitialized when dereferenced.

I understand your point, logically, the code is safe, and the warning is 
a false positive.

> 
> 
>>
>> Check that oldest_p is not NULL after the loop to ensure no dereferencing
>> of uninitialized pointers.
>>
>> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
>> ---
>>   net/ipv4/route.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>> index 753704f75b2c..2e5159127cb9 100644
>> --- a/net/ipv4/route.c
>> +++ b/net/ipv4/route.c
>> @@ -587,7 +587,7 @@ static void fnhe_flush_routes(struct fib_nh_exception *fnhe)
>>   
>>   static void fnhe_remove_oldest(struct fnhe_hash_bucket *hash)
>>   {
>> -	struct fib_nh_exception __rcu **fnhe_p, **oldest_p;
>> +	struct fib_nh_exception __rcu **fnhe_p, **oldest_p = NULL;
>>   	struct fib_nh_exception *fnhe, *oldest = NULL;
>>   
>>   	for (fnhe_p = &hash->chain; ; fnhe_p = &fnhe->fnhe_next) {
>> @@ -601,9 +601,12 @@ static void fnhe_remove_oldest(struct fnhe_hash_bucket *hash)
>>   			oldest_p = fnhe_p;
>>   		}
>>   	}
>> -	fnhe_flush_routes(oldest);
>> -	*oldest_p = oldest->fnhe_next;
>> -	kfree_rcu(oldest, rcu);
>> +
>> +	if (oldest_p) {  /* Ensure to have valid oldest_p element */
>> +		fnhe_flush_routes(oldest);
>> +		*oldest_p = oldest->fnhe_next;
>> +		kfree_rcu(oldest, rcu);
>> +	}
>>   }
>>   
>>   static u32 fnhe_hashfun(__be32 daddr)
>> --