lkml.org 
[lkml]   [2016]   [Feb]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] kexec: Make a pair of map/unmap reserved pages in error path
    On 02/26/16 at 04:56pm, Andrew Morton wrote:
    > On Thu, 25 Feb 2016 22:02:40 +0800 Minfei Huang <mhuang@redhat.com> wrote:
    >
    > > From: Minfei Huang <mnfhuang@gmail.com>
    > >
    > > For some arch, kexec shall map the reserved pages, then use them, when
    > > we try to start the kdump service.
    > >
    > > kexec may return directly, without unmaping the reserved pages, if it
    > > fails during starting service. To fix it, we make a pair of map/unmap
    > > reserved pages both in generic path and error path.
    >
    > This patch both refactors the code AND fixes the bug. It is a
    > decent-looking refactoring, but mixing the two together makes it *much*
    > harder to review the bugfix. These two steps should be separated
    > please, with the bugfix patch coming first.

    I will bisect this patch to make it more simple to be reviewed.
    >
    > > --- a/kernel/kexec.c
    > > +++ b/kernel/kexec.c
    > > @@ -103,6 +103,68 @@ out_free_image:
    > > return ret;
    > > }
    > >
    > > +static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
    > > + struct kexec_segment __user *segments, unsigned long flags)
    > > +{
    > > + struct kimage **dest_image, *image;
    > > + unsigned long i;
    > > + int ret;
    > > +
    > > + if (flags & KEXEC_ON_CRASH)
    > > + dest_image = &kexec_crash_image;
    > > + else
    > > + dest_image = &kexec_image;
    > > +
    > > + if (nr_segments == 0) {
    > > + /* Uninstall image */
    > > + kimage_free(xchg(dest_image, NULL));
    > > + return 0;
    > > + }
    > > + if (flags & KEXEC_ON_CRASH) {
    > > + /*
    > > + * Loading another kernel to switch to if this one
    > > + * crashes. Free any current crash dump kernel before
    > > + * we corrupt it.
    > > + */
    > > + kimage_free(xchg(&kexec_crash_image, NULL));
    > > + }
    > > +
    > > + ret = kimage_alloc_init(&image, entry, nr_segments, segments, flags);
    > > + if (ret)
    > > + return ret;
    >
    > This is a bug, isn't it? Missed kimage_free().

    The variable image is NULL, if kimage_alloc_init returns with error. And
    kimage_alloc_init will do the cleanup before returning. So it's fine
    without calling kimage_free.

    Thanks
    Minfei

    \
     
     \ /
      Last update: 2016-02-29 12:41    [W:6.105 / U:0.256 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site