[PATCH] ipv4: only allow increasing fib_info_hash_size

[PATCH] ipv4: only allow increasing fib_info_hash_size

[zhang kai]

and when failed to allocate memory, check fib_info_hash_size.

Signed-off-by: zhang kai <zhangkaiheb@126.com>
---
 net/ipv4/fib_semantics.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index a632b66bc..7547708a9 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1403,17 +1403,20 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 
 		if (!new_size)
 			new_size = 16;
-		bytes = new_size * sizeof(struct hlist_head *);
-		new_info_hash = fib_info_hash_alloc(bytes);
-		new_laddrhash = fib_info_hash_alloc(bytes);
-		if (!new_info_hash || !new_laddrhash) {
-			fib_info_hash_free(new_info_hash, bytes);
-			fib_info_hash_free(new_laddrhash, bytes);
-		} else
-			fib_info_hash_move(new_info_hash, new_laddrhash, new_size);
-
-		if (!fib_info_hash_size)
-			goto failure;
+
+		if (new_size > fib_info_hash_size) {
+			bytes = new_size * sizeof(struct hlist_head *);
+			new_info_hash = fib_info_hash_alloc(bytes);
+			new_laddrhash = fib_info_hash_alloc(bytes);
+			if (!new_info_hash || !new_laddrhash) {
+				fib_info_hash_free(new_info_hash, bytes);
+				fib_info_hash_free(new_laddrhash, bytes);
+
+				if (!fib_info_hash_size)
+					goto failure;
+			} else
+				fib_info_hash_move(new_info_hash, new_laddrhash, new_size);
+		}
 	}
 
 	fi = kzalloc(struct_size(fi, fib_nh, nhs), GFP_KERNEL);
-- 
2.17.1

Re: [PATCH] ipv4: only allow increasing fib_info_hash_size

[David Ahern]

On 10/12/21 5:06 AM, zhang kai wrote:
> and when failed to allocate memory, check fib_info_hash_size.


what problem are you trying to solve?



> 
> Signed-off-by: zhang kai <zhangkaiheb@126.com>
> ---
>  net/ipv4/fib_semantics.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index a632b66bc..7547708a9 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -1403,17 +1403,20 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
>  
>  		if (!new_size)
>  			new_size = 16;
> -		bytes = new_size * sizeof(struct hlist_head *);
> -		new_info_hash = fib_info_hash_alloc(bytes);
> -		new_laddrhash = fib_info_hash_alloc(bytes);
> -		if (!new_info_hash || !new_laddrhash) {
> -			fib_info_hash_free(new_info_hash, bytes);
> -			fib_info_hash_free(new_laddrhash, bytes);
> -		} else
> -			fib_info_hash_move(new_info_hash, new_laddrhash, new_size);
> -
> -		if (!fib_info_hash_size)
> -			goto failure;
> +
> +		if (new_size > fib_info_hash_size) {
> +			bytes = new_size * sizeof(struct hlist_head *);
> +			new_info_hash = fib_info_hash_alloc(bytes);
> +			new_laddrhash = fib_info_hash_alloc(bytes);
> +			if (!new_info_hash || !new_laddrhash) {
> +				fib_info_hash_free(new_info_hash, bytes);
> +				fib_info_hash_free(new_laddrhash, bytes);
> +
> +				if (!fib_info_hash_size)
> +					goto failure;
> +			} else
> +				fib_info_hash_move(new_info_hash, new_laddrhash, new_size);
> +		}
>  	}
>  
>  	fi = kzalloc(struct_size(fi, fib_nh, nhs), GFP_KERNEL);
> 

Re: [PATCH] ipv4: only allow increasing fib_info_hash_size

[David Ahern]

On 10/12/21 7:10 PM, 张凯 wrote:
> 
> 1) Below shift operation will set fib_info_hash_size to zero if there
> are so many routes: 
>   unsigned int new_size = fib_info_hash_size << 1;
> 
>     This will wrap value of fib_info_hash_size, and a lot of fib_info
> will insert to a small hash bucket.
> so this patch disables above wrap.
> 2) Check whether fib_info_hash_size is zero only needed after allocation
> failed:
> if (!new_info_hash || !new_laddrhash) {
> 
> 

why not something simpler like this:

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 3364cb9c67e0..5c4bd1cebe0a 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1401,6 +1401,10 @@ struct fib_info *fib_create_info(struct
fib_config *cfg,

                if (!new_size)
                        new_size = 16;
+
+               if (new_size < fib_info_hash_size)
+                       goto failure;
+
                bytes = new_size * sizeof(struct hlist_head *);
                new_info_hash = fib_info_hash_alloc(bytes);
                new_laddrhash = fib_info_hash_alloc(bytes);

Re: [PATCH] ipv4: only allow increasing fib_info_hash_size

[David Ahern]

On 10/13/21 2:05 AM, 张凯 wrote:
> Should we let the function still work when the below check is true, not goto failure?
> 
>        if (new_size < fib_info_hash_size)
>                goto failure;
> 
> 

no, it can not.

if (fib_info_cnt >= fib_info_hash_size) {

means the hash table is full. It is going down this path to expand. If
expansion can not happen then you can not add more entries.

This is all theory hence the request for a simpler change; in reality
there should never be so many unique fib_info entries across namespaces
to hit an overflow.