| Message ID | 20250417094126.34352-1-purvayeshi550@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipv4: Fix uninitialized pointer warning in fnhe_remove_oldest | expand |
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) > --
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.
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) >> --
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
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)
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(-)