From: Chao Yu <[email protected]>
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. use WARN_ON instead of BUG_ON() and return error if check condition
is true;
3. use snprintf instead of sprintf to avoid overflow.
Link: https://lore.kernel.org/linux-mm/[email protected]/
Fixes: 81819f0fc828 ("SLUB core")
Suggested-by: Christophe JAILLET <[email protected]>
Reviewed-by: Hyeonggon Yoo <[email protected]>
Signed-off-by: Chao Yu <[email protected]>
---
v2:
- add WARN_ON() instead of return error silently;
- use snprintf instead of sprintf to avoid overflow.
mm/slub.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/mm/slub.c b/mm/slub.c
index 4b98dff9be8e..3d37a8a7b965 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5890,7 +5890,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:
*
@@ -5924,9 +5924,13 @@ static char *create_unique_id(struct kmem_cache *s)
*p++ = 'A';
if (p != name + 1)
*p++ = '-';
- p += sprintf(p, "%07u", s->size);
+ p += snprintf(p, ID_STR_LENGTH - 1 - (p - name), "%07u", s->size);
- BUG_ON(p > name + ID_STR_LENGTH - 1);
+ if (p > name + ID_STR_LENGTH - 1) {
+ WARN_ON(1);
+ kfree(name);
+ return ERR_PTR(-EINVAL);
+ }
return name;
}
--
2.36.1
On 2022/9/26 16:22, Vlastimil Babka wrote:
> On 9/25/22 09:12, Chao Yu wrote:
>> From: Chao Yu <[email protected]>
>>
>> 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. use WARN_ON instead of BUG_ON() and return error if check condition
>> is true;
>> 3. use snprintf instead of sprintf to avoid overflow.
>>
>> Link: https://lore.kernel.org/linux-mm/[email protected]/
>> Fixes: 81819f0fc828 ("SLUB core")
>> Suggested-by: Christophe JAILLET <[email protected]>
>> Reviewed-by: Hyeonggon Yoo <[email protected]>
>> Signed-off-by: Chao Yu <[email protected]>
>> ---
>> v2:
>> - add WARN_ON() instead of return error silently;
>> - use snprintf instead of sprintf to avoid overflow.
>> mm/slub.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 4b98dff9be8e..3d37a8a7b965 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -5890,7 +5890,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:
>> *
>> @@ -5924,9 +5924,13 @@ static char *create_unique_id(struct kmem_cache *s)
>> *p++ = 'A';
>> if (p != name + 1)
>> *p++ = '-';
>> - p += sprintf(p, "%07u", s->size);
>> + p += snprintf(p, ID_STR_LENGTH - 1 - (p - name), "%07u", s->size);
>
> I think we don't need "- 1" here as snprintf() says:
> @size: The size of the buffer, including the trailing null space
Oops, my bad.
>
>> - BUG_ON(p > name + ID_STR_LENGTH - 1);
>> + if (p > name + ID_STR_LENGTH - 1) {
>> + WARN_ON(1);
>
> This would be shorter: if (WARN_ON(p > name...))
Thanks for the comments, will update soon.
Thanks,
>
>> + kfree(name);
>> + return ERR_PTR(-EINVAL);
>> + }
>> return name;
>> }
>
On 9/25/22 09:12, Chao Yu wrote:
> From: Chao Yu <[email protected]>
>
> 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. use WARN_ON instead of BUG_ON() and return error if check condition
> is true;
> 3. use snprintf instead of sprintf to avoid overflow.
>
> Link: https://lore.kernel.org/linux-mm/[email protected]/
> Fixes: 81819f0fc828 ("SLUB core")
> Suggested-by: Christophe JAILLET <[email protected]>
> Reviewed-by: Hyeonggon Yoo <[email protected]>
> Signed-off-by: Chao Yu <[email protected]>
> ---
> v2:
> - add WARN_ON() instead of return error silently;
> - use snprintf instead of sprintf to avoid overflow.
> mm/slub.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 4b98dff9be8e..3d37a8a7b965 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5890,7 +5890,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:
> *
> @@ -5924,9 +5924,13 @@ static char *create_unique_id(struct kmem_cache *s)
> *p++ = 'A';
> if (p != name + 1)
> *p++ = '-';
> - p += sprintf(p, "%07u", s->size);
> + p += snprintf(p, ID_STR_LENGTH - 1 - (p - name), "%07u", s->size);
I think we don't need "- 1" here as snprintf() says:
@size: The size of the buffer, including the trailing null space
>
> - BUG_ON(p > name + ID_STR_LENGTH - 1);
> + if (p > name + ID_STR_LENGTH - 1) {
> + WARN_ON(1);
This would be shorter: if (WARN_ON(p > name...))
> + kfree(name);
> + return ERR_PTR(-EINVAL);
> + }
> return name;
> }
>