diff mbox series

[1/4] selftests/resctrl: Return error if memory is not allocated

Message ID 20230208093016.20670-2-ilpo.jarvinen@linux.intel.com
State New
Headers
Series selftests/resctrl: Fixes to error handling logic |

Commit Message

Ilpo Järvinen Feb. 8, 2023, 9:30 a.m. UTC 
  From: Fenghua Yu <fenghua.yu@intel.com>

malloc_and_init_memory() in fill_buf isn't checking if memalign()
successfully allocated memory or not before accessing the memory.

Check the return value of memalign() and return NULL if allocating
aligned memory fails.

Fixes: a2561b12fe39 ("selftests/resctrl: Add built in benchmark")
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 tools/testing/selftests/resctrl/fill_buf.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Reinette Chatre Feb. 14, 2023, 1 a.m. UTC | #1 
Hi Ilpo,

I do not see a why two patch series are needed for
the resctrl fixes. It may make it easier for everybody if
it is handled as one patch series (with fixes first)?

On 2/8/2023 1:30 AM, Ilpo Järvinen wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> malloc_and_init_memory() in fill_buf isn't checking if memalign()
> successfully allocated memory or not before accessing the memory.
> 
> Check the return value of memalign() and return NULL if allocating
> aligned memory fails.
> 
> Fixes: a2561b12fe39 ("selftests/resctrl: Add built in benchmark")
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

Missing your Signed-off-by?

> ---
>  tools/testing/selftests/resctrl/fill_buf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> index 56ccbeae0638..f4880c962ec4 100644
> --- a/tools/testing/selftests/resctrl/fill_buf.c
> +++ b/tools/testing/selftests/resctrl/fill_buf.c
> @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s)
>  	size_t s64;
>  
>  	void *p = memalign(PAGE_SIZE, s);

This may also be a good time to stop using an obsolete call?

> +	if (!p)
> +		return p;

Could you please return NULL explicitly?

>  
>  	p64 = (uint64_t *)p;
>  	s64 = s / sizeof(uint64_t);


Reinette
Ilpo Järvinen Feb. 14, 2023, 9:32 a.m. UTC | #2 
On Mon, 13 Feb 2023, Reinette Chatre wrote:

> I do not see a why two patch series are needed for
> the resctrl fixes. It may make it easier for everybody if
> it is handled as one patch series (with fixes first)?

Ok, I can put the fixes and cleanups into one series.

> On 2/8/2023 1:30 AM, Ilpo Järvinen wrote:
> > From: Fenghua Yu <fenghua.yu@intel.com>
> > 
> > malloc_and_init_memory() in fill_buf isn't checking if memalign()
> > successfully allocated memory or not before accessing the memory.
> > 
> > Check the return value of memalign() and return NULL if allocating
> > aligned memory fails.
> > 
> > Fixes: a2561b12fe39 ("selftests/resctrl: Add built in benchmark")
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> 
> Missing your Signed-off-by?

These were intentionally. When I didn't modify the original patch at 
all during forward porting it, I just kept the original From and SoB as 
is. But from the doc you pointed me to, I see now x86 wants also handlers 
sobs.

> > ---
> >  tools/testing/selftests/resctrl/fill_buf.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> > index 56ccbeae0638..f4880c962ec4 100644
> > --- a/tools/testing/selftests/resctrl/fill_buf.c
> > +++ b/tools/testing/selftests/resctrl/fill_buf.c
> > @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s)
> >  	size_t s64;
> >  
> >  	void *p = memalign(PAGE_SIZE, s);
> 
> This may also be a good time to stop using an obsolete call?

Sure, I can add another patch to change that to posix_memalign().

> > +	if (!p)
> > +		return p;
> 
> Could you please return NULL explicitly?

I'll change it.

Thanks for you comments.
Reinette Chatre Feb. 14, 2023, 5:01 p.m. UTC | #3 
Hi Ilpo,

On 2/14/2023 1:32 AM, Ilpo Järvinen wrote:
> On Mon, 13 Feb 2023, Reinette Chatre wrote:

>> Missing your Signed-off-by?
> 
> These were intentionally. When I didn't modify the original patch at 
> all during forward porting it, I just kept the original From and SoB as 
> is. But from the doc you pointed me to, I see now x86 wants also handlers 
> sobs.

I do not think this is x86 specific. 
Documentation/process/submitting-patches.rst states:
"Any further SoBs (Signed-off-by:'s) following the author's SoB are from
people handling and transporting the patch, but were not involved in its
development. SoB chains should reflect the **real** route a patch took
as it was propagated to the maintainers and ultimately to Linus, with
the first SoB entry signalling primary authorship of a single author."


> 
>>> ---
>>>  tools/testing/selftests/resctrl/fill_buf.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
>>> index 56ccbeae0638..f4880c962ec4 100644
>>> --- a/tools/testing/selftests/resctrl/fill_buf.c
>>> +++ b/tools/testing/selftests/resctrl/fill_buf.c
>>> @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s)
>>>  	size_t s64;
>>>  
>>>  	void *p = memalign(PAGE_SIZE, s);
>>
>> This may also be a good time to stop using an obsolete call?
> 
> Sure, I can add another patch to change that to posix_memalign().

You can also consider aligned_alloc().

Reinette
diff mbox series

Patch

diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
index 56ccbeae0638..f4880c962ec4 100644
--- a/tools/testing/selftests/resctrl/fill_buf.c
+++ b/tools/testing/selftests/resctrl/fill_buf.c
@@ -68,6 +68,8 @@  static void *malloc_and_init_memory(size_t s)
 	size_t s64;
 
 	void *p = memalign(PAGE_SIZE, s);
+	if (!p)
+		return p;
 
 	p64 = (uint64_t *)p;
 	s64 = s / sizeof(uint64_t);