diff mbox series

[v3,2/8] libperf cpumap: Ensure empty cpumap is NULL from alloc

Message ID 20240202234057.2085863-3-irogers@google.com
State New
Headers
Series Clean up libperf cpumap's empty function |

Commit Message

Ian Rogers Feb. 2, 2024, 11:40 p.m. UTC 
  Potential corner cases could cause a cpumap to be allocated with size
0, but an empty cpumap should be represented as NULL. Add a path in
perf_cpu_map__alloc to ensure this.

Suggested-by: James Clark <james.clark@arm.com>
Closes: https://lore.kernel.org/lkml/2cd09e7c-eb88-6726-6169-647dcd0a8101@arm.com/
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/perf/cpumap.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Namhyung Kim Feb. 17, 2024, 12:25 a.m. UTC | #1 
On Fri, Feb 2, 2024 at 3:41 PM Ian Rogers <irogers@google.com> wrote:
>
> Potential corner cases could cause a cpumap to be allocated with size
> 0, but an empty cpumap should be represented as NULL. Add a path in
> perf_cpu_map__alloc to ensure this.
>
> Suggested-by: James Clark <james.clark@arm.com>
> Closes: https://lore.kernel.org/lkml/2cd09e7c-eb88-6726-6169-647dcd0a8101@arm.com/
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/perf/cpumap.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
> index ba49552952c5..cae799ad44e1 100644
> --- a/tools/lib/perf/cpumap.c
> +++ b/tools/lib/perf/cpumap.c
> @@ -18,9 +18,13 @@ void perf_cpu_map__set_nr(struct perf_cpu_map *map, int nr_cpus)
>
>  struct perf_cpu_map *perf_cpu_map__alloc(int nr_cpus)
>  {
> -       RC_STRUCT(perf_cpu_map) *cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
> +       RC_STRUCT(perf_cpu_map) *cpus;
>         struct perf_cpu_map *result;
>
> +       if (nr_cpus == 0)
> +               return NULL;

But allocation failure also returns NULL.  Then callers should check
what's the expected result.

Thanks,
Namhyung

> +
> +       cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
>         if (ADD_RC_CHK(result, cpus)) {
>                 cpus->nr = nr_cpus;
>                 refcount_set(&cpus->refcnt, 1);
> --
> 2.43.0.594.gd9cf4e227d-goog
>
Ian Rogers Feb. 17, 2024, 12:52 a.m. UTC | #2 
On Fri, Feb 16, 2024 at 4:25 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, Feb 2, 2024 at 3:41 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Potential corner cases could cause a cpumap to be allocated with size
> > 0, but an empty cpumap should be represented as NULL. Add a path in
> > perf_cpu_map__alloc to ensure this.
> >
> > Suggested-by: James Clark <james.clark@arm.com>
> > Closes: https://lore.kernel.org/lkml/2cd09e7c-eb88-6726-6169-647dcd0a8101@arm.com/
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/lib/perf/cpumap.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
> > index ba49552952c5..cae799ad44e1 100644
> > --- a/tools/lib/perf/cpumap.c
> > +++ b/tools/lib/perf/cpumap.c
> > @@ -18,9 +18,13 @@ void perf_cpu_map__set_nr(struct perf_cpu_map *map, int nr_cpus)
> >
> >  struct perf_cpu_map *perf_cpu_map__alloc(int nr_cpus)
> >  {
> > -       RC_STRUCT(perf_cpu_map) *cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
> > +       RC_STRUCT(perf_cpu_map) *cpus;
> >         struct perf_cpu_map *result;
> >
> > +       if (nr_cpus == 0)
> > +               return NULL;
>
> But allocation failure also returns NULL.  Then callers should check
> what's the expected result.

Right, we don't have a habit of just aborting on memory allocation
errors. In the case that NULL is returned it is assumed that an empty
CPU map is appropriate. Adding checks throughout the code base that an
empty CPU map is only returned when 0 is given is beyond the scope of
this patch set.

Thanks,
Ian

> Thanks,
> Namhyung
>
> > +
> > +       cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
> >         if (ADD_RC_CHK(result, cpus)) {
> >                 cpus->nr = nr_cpus;
> >                 refcount_set(&cpus->refcnt, 1);
> > --
> > 2.43.0.594.gd9cf4e227d-goog
> >
James Clark Feb. 19, 2024, 10:38 a.m. UTC | #3 
On 17/02/2024 00:52, Ian Rogers wrote:
> On Fri, Feb 16, 2024 at 4:25 PM Namhyung Kim <namhyung@kernel.org> wrote:
>>
>> On Fri, Feb 2, 2024 at 3:41 PM Ian Rogers <irogers@google.com> wrote:
>>>
>>> Potential corner cases could cause a cpumap to be allocated with size
>>> 0, but an empty cpumap should be represented as NULL. Add a path in
>>> perf_cpu_map__alloc to ensure this.
>>>
>>> Suggested-by: James Clark <james.clark@arm.com>
>>> Closes: https://lore.kernel.org/lkml/2cd09e7c-eb88-6726-6169-647dcd0a8101@arm.com/
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>>  tools/lib/perf/cpumap.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
>>> index ba49552952c5..cae799ad44e1 100644
>>> --- a/tools/lib/perf/cpumap.c
>>> +++ b/tools/lib/perf/cpumap.c
>>> @@ -18,9 +18,13 @@ void perf_cpu_map__set_nr(struct perf_cpu_map *map, int nr_cpus)
>>>
>>>  struct perf_cpu_map *perf_cpu_map__alloc(int nr_cpus)
>>>  {
>>> -       RC_STRUCT(perf_cpu_map) *cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
>>> +       RC_STRUCT(perf_cpu_map) *cpus;
>>>         struct perf_cpu_map *result;
>>>
>>> +       if (nr_cpus == 0)
>>> +               return NULL;
>>
>> But allocation failure also returns NULL.  Then callers should check
>> what's the expected result.>
> Right, we don't have a habit of just aborting on memory allocation

I'm not sure why we don't abort on allocation. It would simplify the
code a lot and wouldn't change the behavior in any meaningful way. And
it would also allow us to print out which line exactly failed which is
much more useful than bubbling up the error and hiding it.

If we're making the decision that an empty map == NULL rather than
non-null but with 0 length then maybe it's time to start thinking about it.

> errors. In the case that NULL is returned it is assumed that an empty
> CPU map is appropriate. Adding checks throughout the code base that an
> empty CPU map is only returned when 0 is given is beyond the scope of
> this patch set.
> 
> Thanks,
> Ian
> 


>> Thanks,
>> Namhyung
>>
>>> +
>>> +       cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
>>>         if (ADD_RC_CHK(result, cpus)) {
>>>                 cpus->nr = nr_cpus;
>>>                 refcount_set(&cpus->refcnt, 1);
>>> --
>>> 2.43.0.594.gd9cf4e227d-goog
>>>
diff mbox series

Patch

diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
index ba49552952c5..cae799ad44e1 100644
--- a/tools/lib/perf/cpumap.c
+++ b/tools/lib/perf/cpumap.c
@@ -18,9 +18,13 @@  void perf_cpu_map__set_nr(struct perf_cpu_map *map, int nr_cpus)
 
 struct perf_cpu_map *perf_cpu_map__alloc(int nr_cpus)
 {
-	RC_STRUCT(perf_cpu_map) *cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
+	RC_STRUCT(perf_cpu_map) *cpus;
 	struct perf_cpu_map *result;
 
+	if (nr_cpus == 0)
+		return NULL;
+
+	cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
 	if (ADD_RC_CHK(result, cpus)) {
 		cpus->nr = nr_cpus;
 		refcount_set(&cpus->refcnt, 1);