lkml.org 
[lkml]   [2011]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2.6.38-rc8-tip 11/20] 11: uprobes: slot allocation for uprobes
    Just a couple of minor notes while I was looking at this code...

    > +static struct uprobes_xol_area *xol_alloc_area(void)
    > +{
    > + struct uprobes_xol_area *area = NULL;
    > +
    > + area = kzalloc(sizeof(*area), GFP_USER);
    > + if (unlikely(!area))
    > + return NULL;
    > +
    > + area->bitmap = kzalloc(BITS_TO_LONGS(UINSNS_PER_PAGE) * sizeof(long),
    > + GFP_USER);

    Why GFP_USER? That causes extra allocation limits to be enforced. Given
    that in part 14 you have:

    +/* Prepare to single-step probed instruction out of line. */
    +static int pre_ssout(struct uprobe *uprobe, struct pt_regs *regs,
    + unsigned long vaddr)
    +{
    + xol_get_insn_slot(uprobe, vaddr);
    + BUG_ON(!current->utask->xol_vaddr);

    It seems to me that you really don't want those allocations to fail.

    back to xol_alloc_area():

    > + if (!area->bitmap)
    > + goto fail;
    > +
    > + spin_lock_init(&area->slot_lock);
    > + if (!xol_add_vma(area) && !current->mm->uprobes_xol_area) {
    > + task_lock(current);
    > + if (!current->mm->uprobes_xol_area) {
    > + current->mm->uprobes_xol_area = area;
    > + task_unlock(current);
    > + return area;
    > + }
    > + task_unlock(current);
    > + }
    > +
    > +fail:
    > + if (area) {
    > + if (area->bitmap)
    > + kfree(area->bitmap);
    > + kfree(area);
    > + }

    You've already checked area against NULL, and kfree() can handle null
    pointers, so both of those tests are unneeded.

    > + return current->mm->uprobes_xol_area;
    > +}

    jon


    \
     
     \ /
      Last update: 2011-03-15 20:13    [W:4.203 / U:0.576 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site