* [PATCH] kunit: alloc_string_stream_fragment error handling bug fix
@ 2022-10-28 14:42 YoungJun.park
2022-10-29 10:35 ` Maíra Canal
2022-10-30 3:20 ` David Gow
0 siblings, 2 replies; 4+ messages in thread
From: YoungJun.park @ 2022-10-28 14:42 UTC (permalink / raw)
To: Brendan Higgins
Cc: David Gow, linux-kselftest, kunit-dev, linux-kernel, YoungJun.park
When it fails to allocate fragment, it does not free and return error.
And check the pointer inappropriately.
Signed-off-by: YoungJun.park <her0gyugyu@gmail.com>
---
lib/kunit/string-stream.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index 72659a9773e3..0228fe814e96 100644
--- a/lib/kunit/string-stream.c
+++ b/lib/kunit/string-stream.c
@@ -23,8 +23,10 @@ static struct string_stream_fragment *alloc_string_stream_fragment(
return ERR_PTR(-ENOMEM);
frag->fragment = kunit_kmalloc(test, len, gfp);
- if (!frag->fragment)
+ if (!frag->fragment) {
+ kunit_kfree(test, frag);
return ERR_PTR(-ENOMEM);
+ }
return frag;
}
@@ -56,7 +58,7 @@ int string_stream_vadd(struct string_stream *stream,
frag_container = alloc_string_stream_fragment(stream->test,
len,
stream->gfp);
- if (!frag_container)
+ if (IS_ERR(frag_container))
return -ENOMEM;
len = vsnprintf(frag_container->fragment, len, fmt, args);
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] kunit: alloc_string_stream_fragment error handling bug fix
2022-10-28 14:42 [PATCH] kunit: alloc_string_stream_fragment error handling bug fix YoungJun.park
@ 2022-10-29 10:35 ` Maíra Canal
2022-10-30 3:20 ` David Gow
1 sibling, 0 replies; 4+ messages in thread
From: Maíra Canal @ 2022-10-29 10:35 UTC (permalink / raw)
To: YoungJun.park, Brendan Higgins
Cc: David Gow, linux-kselftest, kunit-dev, linux-kernel
On 10/28/22 11:42, YoungJun.park wrote:
> When it fails to allocate fragment, it does not free and return error.
> And check the pointer inappropriately.
>
> Signed-off-by: YoungJun.park <her0gyugyu@gmail.com>
> ---
> lib/kunit/string-stream.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index 72659a9773e3..0228fe814e96 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -23,8 +23,10 @@ static struct string_stream_fragment *alloc_string_stream_fragment(
> return ERR_PTR(-ENOMEM);
>
> frag->fragment = kunit_kmalloc(test, len, gfp);
> - if (!frag->fragment)
> + if (!frag->fragment) {
> + kunit_kfree(test, frag);
I don't believe that kunit_kfree is necessary here, because
kunit_kmalloc is like kmalloc, but the allocation is test managed, which
means that the allocation is managed by the test case and is
automatically cleaned up after the test case concludes.
So, the original version seems correct: if the allocation fails,
alloc_string_stream_fragment will return NULL and string_stream_vadd
will check if frag_container is different than NULL.
Best Regards,
- Maíra Canal
> return ERR_PTR(-ENOMEM);
> + }
>
> return frag;
> }
> @@ -56,7 +58,7 @@ int string_stream_vadd(struct string_stream *stream,
> frag_container = alloc_string_stream_fragment(stream->test,
> len,
> stream->gfp);
> - if (!frag_container)
> + if (IS_ERR(frag_container))
> return -ENOMEM;
>
> len = vsnprintf(frag_container->fragment, len, fmt, args);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kunit: alloc_string_stream_fragment error handling bug fix
2022-10-28 14:42 [PATCH] kunit: alloc_string_stream_fragment error handling bug fix YoungJun.park
2022-10-29 10:35 ` Maíra Canal
@ 2022-10-30 3:20 ` David Gow
2022-10-31 20:47 ` Daniel Latypov
1 sibling, 1 reply; 4+ messages in thread
From: David Gow @ 2022-10-30 3:20 UTC (permalink / raw)
To: YoungJun.park; +Cc: Brendan Higgins, linux-kselftest, kunit-dev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]
On Fri, Oct 28, 2022 at 10:43 PM YoungJun.park <her0gyugyu@gmail.com> wrote:
>
> When it fails to allocate fragment, it does not free and return error.
> And check the pointer inappropriately.
>
> Signed-off-by: YoungJun.park <her0gyugyu@gmail.com>
> ---
Thanks! As Maíra points out, the added kunit_kfree() call isn't
strictly necessary, though it definitely doesn't hurt (and it's
probably a nice thing to free memory early if we're already in a
pretty dire memory situation). So I think it's an improvement.
The IS_ERR check is definitely a fix, though.
Reviewed-by: David Gow <davidgow@google.com>
Cheers,
-- David
> lib/kunit/string-stream.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index 72659a9773e3..0228fe814e96 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -23,8 +23,10 @@ static struct string_stream_fragment *alloc_string_stream_fragment(
> return ERR_PTR(-ENOMEM);
>
> frag->fragment = kunit_kmalloc(test, len, gfp);
> - if (!frag->fragment)
> + if (!frag->fragment) {
> + kunit_kfree(test, frag);
> return ERR_PTR(-ENOMEM);
> + }
>
> return frag;
> }
> @@ -56,7 +58,7 @@ int string_stream_vadd(struct string_stream *stream,
> frag_container = alloc_string_stream_fragment(stream->test,
> len,
> stream->gfp);
> - if (!frag_container)
> + if (IS_ERR(frag_container))
> return -ENOMEM;
>
> len = vsnprintf(frag_container->fragment, len, fmt, args);
> --
> 2.25.1
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20221028144241.634012-1-her0gyugyu%40gmail.com.
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kunit: alloc_string_stream_fragment error handling bug fix
2022-10-30 3:20 ` David Gow
@ 2022-10-31 20:47 ` Daniel Latypov
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Latypov @ 2022-10-31 20:47 UTC (permalink / raw)
To: David Gow
Cc: YoungJun.park, Brendan Higgins, linux-kselftest, kunit-dev, linux-kernel
On Sat, Oct 29, 2022 at 8:20 PM 'David Gow' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> On Fri, Oct 28, 2022 at 10:43 PM YoungJun.park <her0gyugyu@gmail.com> wrote:
> >
> > When it fails to allocate fragment, it does not free and return error.
> > And check the pointer inappropriately.
> >
> > Signed-off-by: YoungJun.park <her0gyugyu@gmail.com>
> > ---
>
> Thanks! As Maíra points out, the added kunit_kfree() call isn't
> strictly necessary, though it definitely doesn't hurt (and it's
> probably a nice thing to free memory early if we're already in a
> pretty dire memory situation). So I think it's an improvement.
>
> The IS_ERR check is definitely a fix, though.
Note: the IS_ERR check was fixed already in
https://patchwork.kernel.org/project/linux-kselftest/patch/Y0kt1aCTHO4r2CmL@kili/
That change has made its way into torvalds/master.
So we could rebase this patch and reword it to talk just about the
early kfree().
Re free memory early:
It'll save us sizeof(struct string_stream_fragment) + sizeof(struct
kunit_resource), i.e. 24 + 56 = 80 bytes (on UML/x86_64).
So it's not much, but I guess it could help in edge cases.
Daniel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-10-31 20:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-28 14:42 [PATCH] kunit: alloc_string_stream_fragment error handling bug fix YoungJun.park
2022-10-29 10:35 ` Maíra Canal
2022-10-30 3:20 ` David Gow
2022-10-31 20:47 ` Daniel Latypov
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 NNTP newsgroup(s).