| Message ID | 20211012110658.10166-1-zhangkaiheb@126.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ipv4: only allow increasing fib_info_hash_size | expand |
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); >
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);
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.
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);
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(-)