From: Heiko Carstens <hca@linux.ibm.com>
To: Philipp Rudo <prudo@redhat.com>
Cc: Baoquan He <bhe@redhat.com>,
linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org,
kexec@lists.infradead.org, dyoung@redhat.com,
akpm@linux-foundation.org
Subject: Re: [PATCH] s390/kexec: fix memory leak of ipl report buffer
Date: Wed, 10 Nov 2021 16:18:26 +0100 [thread overview]
Message-ID: <YYviwi3PVD11xcCs@osiris> (raw)
In-Reply-To: <20211029183132.20839ad0@rhtmp>
On Fri, Oct 29, 2021 at 06:31:32PM +0200, Philipp Rudo wrote:
> Hi Baoquan,
>
> On Fri, 29 Oct 2021 17:26:35 +0800
> Baoquan He <bhe@redhat.com> wrote:
>
> > A memory leak is reported by kmemleak scanning:
...
> > The ipl report buffer is allocated via vmalloc, while has no chance to free
> > if the kexe loading is unloaded. This will cause obvious memory leak
> > when kexec/kdump kernel is reloaded, manually, or triggered, e.g by
> > memory hotplug.
> >
> > Here, add struct kimage_arch to s390 to pass out the ipl report buffer
> > address, and introduce arch dependent function
> > arch_kimage_file_post_load_cleanup() to free it when unloaded.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
>
> other than a missing
>
> Fixes: 99feaa717e55 ("s390/kexec_file: Create ipl report and pass to
> next kernel")
>
> the patch looks good to me.
>
> Reviewed-by: Philipp Rudo <prudo@redhat.com>
...
> > buf.buffer = ipl_report_finish(data->report);
> > buf.bufsz = data->report->size;
> > buf.memsz = buf.bufsz;
> > + image->arch.ipl_buf = buf.buffer;
This seems (still) to be incorrect: ipl_report_finish() may return
-ENOMEN, but there is no error checking anywhere, as far as I can
tell, which would make this:
> > +int arch_kimage_file_post_load_cleanup(struct kimage *image)
> > +{
> > + kvfree(image->arch.ipl_buf);
> > + image->arch.ipl_buf = NULL;
> > +
> > + return kexec_image_post_load_cleanup_default(image);
> > +}
most likely not do what we want. That is: if this code is reached at
all in such a case. I'll check and might add a patch before this to
fix this also.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
WARNING: multiple messages have this Message-ID (diff)
From: Heiko Carstens <hca@linux.ibm.com>
To: Philipp Rudo <prudo@redhat.com>
Cc: Baoquan He <bhe@redhat.com>,
linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org,
kexec@lists.infradead.org, dyoung@redhat.com,
akpm@linux-foundation.org
Subject: Re: [PATCH] s390/kexec: fix memory leak of ipl report buffer
Date: Wed, 10 Nov 2021 16:18:26 +0100 [thread overview]
Message-ID: <YYviwi3PVD11xcCs@osiris> (raw)
In-Reply-To: <20211029183132.20839ad0@rhtmp>
On Fri, Oct 29, 2021 at 06:31:32PM +0200, Philipp Rudo wrote:
> Hi Baoquan,
>
> On Fri, 29 Oct 2021 17:26:35 +0800
> Baoquan He <bhe@redhat.com> wrote:
>
> > A memory leak is reported by kmemleak scanning:
...
> > The ipl report buffer is allocated via vmalloc, while has no chance to free
> > if the kexe loading is unloaded. This will cause obvious memory leak
> > when kexec/kdump kernel is reloaded, manually, or triggered, e.g by
> > memory hotplug.
> >
> > Here, add struct kimage_arch to s390 to pass out the ipl report buffer
> > address, and introduce arch dependent function
> > arch_kimage_file_post_load_cleanup() to free it when unloaded.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
>
> other than a missing
>
> Fixes: 99feaa717e55 ("s390/kexec_file: Create ipl report and pass to
> next kernel")
>
> the patch looks good to me.
>
> Reviewed-by: Philipp Rudo <prudo@redhat.com>
...
> > buf.buffer = ipl_report_finish(data->report);
> > buf.bufsz = data->report->size;
> > buf.memsz = buf.bufsz;
> > + image->arch.ipl_buf = buf.buffer;
This seems (still) to be incorrect: ipl_report_finish() may return
-ENOMEN, but there is no error checking anywhere, as far as I can
tell, which would make this:
> > +int arch_kimage_file_post_load_cleanup(struct kimage *image)
> > +{
> > + kvfree(image->arch.ipl_buf);
> > + image->arch.ipl_buf = NULL;
> > +
> > + return kexec_image_post_load_cleanup_default(image);
> > +}
most likely not do what we want. That is: if this code is reached at
all in such a case. I'll check and might add a patch before this to
fix this also.
next prev parent reply other threads:[~2021-11-10 15:18 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-29 9:26 [PATCH] s390/kexec: fix memory leak of ipl report buffer Baoquan He
2021-10-29 9:26 ` Baoquan He
2021-10-29 16:31 ` Philipp Rudo
2021-10-29 16:31 ` Philipp Rudo
2021-11-10 15:18 ` Heiko Carstens [this message]
2021-11-10 15:18 ` Heiko Carstens
2021-11-11 0:22 ` Baoquan He
2021-11-11 0:22 ` Baoquan He
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YYviwi3PVD11xcCs@osiris \
--to=hca@linux.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=bhe@redhat.com \
--cc=dyoung@redhat.com \
--cc=kexec@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=prudo@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.