lkml.org 
[lkml]   [2014]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 03/13] kexec: Move segment verification code in a separate function
    On Wed, Jun 04, 2014 at 11:32:55AM +0200, Borislav Petkov wrote:
    > On Tue, Jun 03, 2014 at 09:06:52AM -0400, Vivek Goyal wrote:
    > > Previously do_kimage_alloc() will allocate a kimage structure, copy
    > > segment list from user space and then do the segment list sanity verification.
    > >
    > > Break down this function in 3 parts. do_kimage_alloc_init() to do actual
    > > allocation and basic initialization of kimage structure.
    > > copy_user_segment_list() to copy segment list from user space and
    > > sanity_check_segment_list() to verify the sanity of segment list as passed
    > > by user space.
    > >
    > > In later patches, I need to only allocate kimage and not copy segment
    > > list from user space. So breaking down in smaller functions enables
    > > re-use of code at other places.
    >
    > I haven't seen what's going on further in the patchset but from looking at
    > kimage_normal_alloc() and kimage_crash_alloc()'s guts, they look very
    > similar and could probably share a common __kimage_alloc which does
    > do_kimage_alloc_init, copy_user_segment_list, sanity_check_segment_list
    > and kimage_alloc_control_pages...
    >
    > One probably would have to actually write it down to see whether it
    > makes sense though and is not too ugly :-)

    Hi Boris,

    Agreed. kimage_normal_alloc() and kimage_crash_alloc() are sharing
    lot of code and it should make sense to write a common function for
    shared code and let both call that function. I will give it a try
    and if it makes sense will make it part of next version of posting.

    >
    > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
    > > ---
    >
    > In any case, it looks ok, just two nitpicks below:
    >
    > Acked-by: Borislav Petkov <bp@suse.de>
    >
    > > kernel/kexec.c | 182 ++++++++++++++++++++++++++++++++-------------------------
    > > 1 file changed, 101 insertions(+), 81 deletions(-)
    >
    > ...
    >
    > > +static struct kimage *do_kimage_alloc_init(void)
    > > +{
    > > + struct kimage *image;
    > > +
    > > + /* Allocate a controlling structure */
    > > + image = kzalloc(sizeof(*image), GFP_KERNEL);
    > > + if (!image)
    > > + return NULL;
    > > +
    > > + image->head = 0;
    > > + image->entry = &image->head;
    > > + image->last_entry = &image->head;
    > > + image->control_page = ~0; /* By default this does not apply */
    > > + image->type = KEXEC_TYPE_DEFAULT;
    > > +
    > > + /* Initialize the list of control pages */
    > > + INIT_LIST_HEAD(&image->control_pages);
    > > +
    > > + /* Initialize the list of destination pages */
    > > + INIT_LIST_HEAD(&image->dest_pages);
    > > +
    > > + /* Initialize the list of unusable pages */
    > > + INIT_LIST_HEAD(&image->unuseable_pages);
    >
    > If the "e" in "unuseable" bugs you too, like me, you could add this one
    > to your patchset :-)
    >
    > http://lkml.kernel.org/r/1392819695-24116-1-git-send-email-bp@alien8.de
    >

    Hmm..., Interesting. I never noticed it. So google search seems to say
    that unuseable is also not wrong.

    I am not feeling very strongly about it, so I will leave this cleanup
    for some other day.

    > ...
    >
    > > @@ -258,22 +292,23 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
    > > get_order(KEXEC_CONTROL_PAGE_SIZE));
    > > if (!image->control_code_page) {
    > > printk(KERN_ERR "Could not allocate control_code_buffer\n");
    > > - goto out_free;
    > > + goto out_free_image;
    > > }
    > >
    > > image->swap_page = kimage_alloc_control_pages(image, 0);
    > > if (!image->swap_page) {
    > > printk(KERN_ERR "Could not allocate swap buffer\n");
    > > - goto out_free;
    > > + goto out_free_control_pages;
    > > }
    > >
    > > *rimage = image;
    > > return 0;
    > >
    > > -out_free:
    > > +
    >
    > Superfluous newline.

    Will remove.

    Thanks
    Vivek


    \
     
     \ /
      Last update: 2014-06-04 21:41    [W:3.229 / U:0.124 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site