Messages in this thread | | | Date | Sun, 25 Sep 2022 23:24:55 +0800 | Subject | Re: [PATCH] mm/slub: clean up create_unique_id() | From | Chao Yu <> |
| |
On 2022/9/23 4:47, Vlastimil Babka wrote: > On 9/18/22 11:21, Chao Yu wrote: >> From: Chao Yu <chao.yu@oppo.com> >> >> As Christophe JAILLET suggested: >> >> In create_unique_id(), >> >> "looks that ID_STR_LENGTH could even be reduced to 32 or 16. >> >> The 2nd BUG_ON at the end of the function could certainly be just >> removed as well or remplaced by a: >> if (p > name + ID_STR_LENGTH - 1) { >> kfree(name); >> return -E<something>; >> } >> " >> >> According to above suggestion, let's do below cleanups: >> 1. reduce ID_STR_LENGTH to 32, as the buffer size should be enough; >> 2. remove BUG_ON() and return error if check condition is true. > > I'd leave a WARN_ON there as we really don't expect this to happen, so if it > does, we should be loud about it and not silently fail.
Agreed.
> >> Link: https://lore.kernel.org/linux-mm/2025305d-16db-abdf-6cd3-1fb93371c2b4@wanadoo.fr/ >> Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> Fixes: 81819f0fc828 ("SLUB core") >> Signed-off-by: Chao Yu <chao.yu@oppo.com> >> --- >> mm/slub.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/mm/slub.c b/mm/slub.c >> index 5ba6db62a5ab..a045c1ca8772 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -5883,7 +5883,7 @@ static inline struct kset *cache_kset(struct kmem_cache *s) >> return slab_kset; >> } >> >> -#define ID_STR_LENGTH 64 >> +#define ID_STR_LENGTH 32 >> >> /* Create a unique string id for a slab cache: >> * >> @@ -5919,7 +5919,10 @@ static char *create_unique_id(struct kmem_cache *s) >> *p++ = '-'; >> p += sprintf(p, "%07u", s->size); > > Hm but sprintf() will happily overflow, so if we only detect that > afterwards, it's kinda too late to gracefully fail. > Should use snprintf() then?
Correct.
Updated in v2.
Thanks,
> >> - BUG_ON(p > name + ID_STR_LENGTH - 1); >> + if (p > name + ID_STR_LENGTH - 1) { >> + kfree(name); >> + return ERR_PTR(-EINVAL); >> + } >> return name; >> } >> >
| |