| Message ID | 20220918092146.4060783-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/slub: clean up create_unique_id() | expand |
On Sun, Sep 18, 2022 at 05:21:46PM +0800, 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. > > 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); > > - BUG_ON(p > name + ID_STR_LENGTH - 1); > + if (p > name + ID_STR_LENGTH - 1) { > + kfree(name); > + return ERR_PTR(-EINVAL); > + } > return name; IIUC 16 bytes will be still safe ;) Anyway, for either size: Looks good to me, Reviewed-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> Thanks! > } > > -- > 2.25.1 >
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. > 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? > - BUG_ON(p > name + ID_STR_LENGTH - 1); > + if (p > name + ID_STR_LENGTH - 1) { > + kfree(name); > + return ERR_PTR(-EINVAL); > + } > return name; > } >
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; >> } >> >
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); - BUG_ON(p > name + ID_STR_LENGTH - 1); + if (p > name + ID_STR_LENGTH - 1) { + kfree(name); + return ERR_PTR(-EINVAL); + } return name; }