* [PATCH v3] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*()
@ 2024-04-27 16:45 Erick Archer
2024-04-29 17:41 ` Kees Cook
2024-04-29 20:48 ` Christophe JAILLET
0 siblings, 2 replies; 3+ messages in thread
From: Erick Archer @ 2024-04-27 16:45 UTC (permalink / raw
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Thomas Gleixner, Borislav Petkov, Dave Hansen,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
Adrian Hunter, H. Peter Anvin, Liang, Kan, Kees Cook,
Gustavo A. R. Silva, Justin Stitt
Cc: Erick Archer, x86, linux-perf-users, linux-kernel,
linux-hardening
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].
Here the multiplication is obviously safe. However, using kcalloc*()
is more appropriate [2] and improves readability. This patch has no
effect on runtime behavior.
Link: https://github.com/KSPP/linux/issues/162 [1]
Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Erick Archer <erick.archer@outlook.com>
---
Changes in v3:
- Update the commit message to better explain the changes.
- Rebase against linux-next.
Changes in v2:
- Add the "Reviewed-by:" tag.
- Rebase against linux-next.
Previous versions:
v1 -> https://lore.kernel.org/linux-hardening/20240116125813.3754-1-erick.archer@gmx.com
v2 -> https://lore.kernel.org/linux-hardening/AS8PR02MB7237A07D73D6D15EBF72FD8D8B392@AS8PR02MB7237.eurprd02.prod.outlook.com
Hi,
This is a new try. In the v2 version Ingo explained that this change
is nonsense since kzalloc() is a perfectly usable interface and there
is no real overflow here.
Anyway, if we have the 2-factor form of the allocator, I think it is
a good practice to use it.
In this version I have updated the commit message to explain that
the code is obviusly safe in contrast with the last version where the
impression was given that there was a real overlow bug.
I hope this patch can be applied this time.
Regards,
Erick
---
arch/x86/events/amd/uncore.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 4ccb8fa483e6..61c0a2114183 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -479,8 +479,8 @@ static int amd_uncore_ctx_init(struct amd_uncore *uncore, unsigned int cpu)
goto fail;
curr->cpu = cpu;
- curr->events = kzalloc_node(sizeof(*curr->events) *
- pmu->num_counters,
+ curr->events = kcalloc_node(pmu->num_counters,
+ sizeof(*curr->events),
GFP_KERNEL, node);
if (!curr->events) {
kfree(curr);
@@ -928,7 +928,7 @@ int amd_uncore_umc_ctx_init(struct amd_uncore *uncore, unsigned int cpu)
uncore->num_pmus += group_num_pmus[gid];
}
- uncore->pmus = kzalloc(sizeof(*uncore->pmus) * uncore->num_pmus,
+ uncore->pmus = kcalloc(uncore->num_pmus, sizeof(*uncore->pmus),
GFP_KERNEL);
if (!uncore->pmus) {
uncore->num_pmus = 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*()
2024-04-27 16:45 [PATCH v3] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*() Erick Archer
@ 2024-04-29 17:41 ` Kees Cook
2024-04-29 20:48 ` Christophe JAILLET
1 sibling, 0 replies; 3+ messages in thread
From: Kees Cook @ 2024-04-29 17:41 UTC (permalink / raw
To: Erick Archer
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Thomas Gleixner, Borislav Petkov, Dave Hansen,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
Adrian Hunter, H. Peter Anvin, Liang, Kan, Gustavo A. R. Silva,
Justin Stitt, x86, linux-perf-users, linux-kernel,
linux-hardening
On Sat, Apr 27, 2024 at 06:45:23PM +0200, Erick Archer wrote:
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1].
>
> Here the multiplication is obviously safe. However, using kcalloc*()
> is more appropriate [2] and improves readability. This patch has no
> effect on runtime behavior.
>
> Link: https://github.com/KSPP/linux/issues/162 [1]
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> Signed-off-by: Erick Archer <erick.archer@outlook.com>
Thanks!
Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
> Changes in v3:
> - Update the commit message to better explain the changes.
> - Rebase against linux-next.
>
> Changes in v2:
> - Add the "Reviewed-by:" tag.
> - Rebase against linux-next.
>
> Previous versions:
> v1 -> https://lore.kernel.org/linux-hardening/20240116125813.3754-1-erick.archer@gmx.com
> v2 -> https://lore.kernel.org/linux-hardening/AS8PR02MB7237A07D73D6D15EBF72FD8D8B392@AS8PR02MB7237.eurprd02.prod.outlook.com
>
> Hi,
>
> This is a new try. In the v2 version Ingo explained that this change
> is nonsense since kzalloc() is a perfectly usable interface and there
> is no real overflow here.
>
> Anyway, if we have the 2-factor form of the allocator, I think it is
> a good practice to use it.
>
> In this version I have updated the commit message to explain that
> the code is obviusly safe in contrast with the last version where the
> impression was given that there was a real overlow bug.
>
> I hope this patch can be applied this time.
>
> Regards,
> Erick
> ---
> arch/x86/events/amd/uncore.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index 4ccb8fa483e6..61c0a2114183 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -479,8 +479,8 @@ static int amd_uncore_ctx_init(struct amd_uncore *uncore, unsigned int cpu)
> goto fail;
>
> curr->cpu = cpu;
> - curr->events = kzalloc_node(sizeof(*curr->events) *
> - pmu->num_counters,
> + curr->events = kcalloc_node(pmu->num_counters,
> + sizeof(*curr->events),
> GFP_KERNEL, node);
As a general aside to the original code authors, looking at struct
amd_uncore_pmu, I see stuff that should likely be u32 instead of
"int". How is a negtaive num_counters ever sane?
struct amd_uncore_pmu {
...
int num_counters;
int rdpmc_base;
u32 msr_base;
int group;
...
};
--
Kees Cook
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*()
2024-04-27 16:45 [PATCH v3] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*() Erick Archer
2024-04-29 17:41 ` Kees Cook
@ 2024-04-29 20:48 ` Christophe JAILLET
1 sibling, 0 replies; 3+ messages in thread
From: Christophe JAILLET @ 2024-04-29 20:48 UTC (permalink / raw
To: Erick Archer, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Thomas Gleixner,
Borislav Petkov, Dave Hansen, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Ian Rogers, Adrian Hunter, H. Peter Anvin, Liang, Kan,
Kees Cook, Gustavo A. R. Silva, Justin Stitt
Cc: x86, linux-perf-users, linux-kernel, linux-hardening
Le 27/04/2024 à 18:45, Erick Archer a écrit :
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1].
>
> Here the multiplication is obviously safe. However, using kcalloc*()
> is more appropriate [2] and improves readability. This patch has no
> effect on runtime behavior.
>
> Link: https://github.com/KSPP/linux/issues/162 [1]
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> Signed-off-by: Erick Archer <erick.archer@outlook.com>
> ---
> Changes in v3:
> - Update the commit message to better explain the changes.
> - Rebase against linux-next.
>
> Changes in v2:
> - Add the "Reviewed-by:" tag.
> - Rebase against linux-next.
>
> Previous versions:
> v1 -> https://lore.kernel.org/linux-hardening/20240116125813.3754-1-erick.archer@gmx.com
> v2 -> https://lore.kernel.org/linux-hardening/AS8PR02MB7237A07D73D6D15EBF72FD8D8B392@AS8PR02MB7237.eurprd02.prod.outlook.com
>
> Hi,
>
> This is a new try. In the v2 version Ingo explained that this change
> is nonsense since kzalloc() is a perfectly usable interface and there
> is no real overflow here.
>
> Anyway, if we have the 2-factor form of the allocator, I think it is
> a good practice to use it.
>
> In this version I have updated the commit message to explain that
> the code is obviusly safe in contrast with the last version where the
> impression was given that there was a real overlow bug.
>
> I hope this patch can be applied this time.
>
> Regards,
> Erick
> ---
> arch/x86/events/amd/uncore.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index 4ccb8fa483e6..61c0a2114183 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -479,8 +479,8 @@ static int amd_uncore_ctx_init(struct amd_uncore *uncore, unsigned int cpu)
> goto fail;
>
> curr->cpu = cpu;
> - curr->events = kzalloc_node(sizeof(*curr->events) *
> - pmu->num_counters,
> + curr->events = kcalloc_node(pmu->num_counters,
> + sizeof(*curr->events),
> GFP_KERNEL, node);
Hi,
not related to your patch, but amd_uncore_ctx looks like a good
candidate for a flexible array.
This would simplify allocation/freeing of the memory.
struct amd_uncore_ctx {
int refcnt;
int cpu;
struct hlist_node node;
struct perf_event *events[];
};
Just my 2c,
CJ
> if (!curr->events) {
> kfree(curr);
> @@ -928,7 +928,7 @@ int amd_uncore_umc_ctx_init(struct amd_uncore *uncore, unsigned int cpu)
> uncore->num_pmus += group_num_pmus[gid];
> }
>
> - uncore->pmus = kzalloc(sizeof(*uncore->pmus) * uncore->num_pmus,
> + uncore->pmus = kcalloc(uncore->num_pmus, sizeof(*uncore->pmus),
> GFP_KERNEL);
> if (!uncore->pmus) {
> uncore->num_pmus = 0;
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-29 20:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-27 16:45 [PATCH v3] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*() Erick Archer
2024-04-29 17:41 ` Kees Cook
2024-04-29 20:48 ` Christophe JAILLET
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).