lkml.org 
[lkml]   [2006]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] ipr: don't doublefree pages from scatterlist
    Hugh Dickins wrote:
    > On some architectures, mapping the scatterlist may coalesce entries:
    > if that coalesced list is then used for freeing the pages afterwards,
    > there's a danger that pages may be doubly freed (and others leaked).

    I don't understand why this is necessary... Comparing the bug you fixed
    in st with ipr's usage of scatterlists, there is a bit of a difference.
    st is dealing with user pages and calling page_cache_release to release
    the pages, and I can begin to understand why there might be a problem
    in that code. page_cache_release looks at the pages themselves to figure
    out how many compound pages there are. This could certainly result in
    double free's occurring.

    Looking at ipr, however, it is simply doing alloc_pages
    and __free_pages. __free_pages passes in the page allocation order, so
    I don't think I would have the double free problem.

    As I understand it, pci_map_sg only modifies the dma_address and dma_length
    fields when things get coalesced. If it were to coalese pages by
    turning them into compound pages then I would agree that ipr might have
    a problem, but I don't think this to be the case...


    Brian

    >
    > Fix Power RAID's ipr_free_ucode_buffer by freeing from a separate array
    > beyond the scatterlist.
    >
    > Signed-off-by: Hugh Dickins <hugh@veritas.com>
    > ---
    > Warning: untested!
    >
    > drivers/scsi/ipr.c | 12 ++++++++++--
    > 1 files changed, 10 insertions(+), 2 deletions(-)
    >
    > --- 2.6.16-rc2/drivers/scsi/ipr.c 2006-02-03 09:32:24.000000000 +0000
    > +++ linux/drivers/scsi/ipr.c 2006-02-03 09:59:37.000000000 +0000
    > @@ -2538,6 +2538,7 @@ static struct ipr_sglist *ipr_alloc_ucod
    > int sg_size, order, bsize_elem, num_elem, i, j;
    > struct ipr_sglist *sglist;
    > struct scatterlist *scatterlist;
    > + struct page **sg_pages;
    > struct page *page;
    >
    > /* Get the minimum size per scatter/gather element */
    > @@ -2557,7 +2558,8 @@ static struct ipr_sglist *ipr_alloc_ucod
    >
    > /* Allocate a scatter/gather list for the DMA */
    > sglist = kzalloc(sizeof(struct ipr_sglist) +
    > - (sizeof(struct scatterlist) * (num_elem - 1)),
    > + (sizeof(struct scatterlist) * (num_elem - 1)) +
    > + (sizeof(struct page *) * num_elem),
    > GFP_KERNEL);
    >
    > if (sglist == NULL) {
    > @@ -2566,6 +2568,8 @@ static struct ipr_sglist *ipr_alloc_ucod
    > }
    >
    > scatterlist = sglist->scatterlist;
    > + /* Save pages to be freed in array beyond scatterlist */
    > + sg_pages = (struct page **) (scatterlist + num_elem);
    >
    > sglist->order = order;
    > sglist->num_sg = num_elem;
    > @@ -2584,6 +2588,7 @@ static struct ipr_sglist *ipr_alloc_ucod
    > }
    >
    > scatterlist[i].page = page;
    > + sg_pages[i] = page;
    > }
    >
    > return sglist;
    > @@ -2601,10 +2606,13 @@ static struct ipr_sglist *ipr_alloc_ucod
    > **/
    > static void ipr_free_ucode_buffer(struct ipr_sglist *sglist)
    > {
    > + struct page **sg_pages;
    > int i;
    >
    > + /* Scatterlist entries may have been coalesced: free saved pagelist */
    > + sg_pages = (struct page **) (sglist->scatterlist + sglist->num_sg);
    > for (i = 0; i < sglist->num_sg; i++)
    > - __free_pages(sglist->scatterlist[i].page, sglist->order);
    > + __free_pages(sg_pages[i], sglist->order);
    >
    > kfree(sglist);
    > }
    >


    --
    Brian King
    eServer Storage I/O
    IBM Linux Technology Center
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-02-03 23:09    [W:0.038 / U:0.476 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site