Skip to content

Commit

Permalink
DMA: PL330: Fix racy mutex unlock
Browse files Browse the repository at this point in the history
pl330_update() stores a pointer to the thrd->req that finished, which
contains a pointer to the corresponding pl330_req.  This is done with
the pl330_lock held.  Then, it iterates through the req_done list,
calling the callback for each of the requests that are done.  The
problem is that the driver releases the lock before calling the
callback for each of the callbacks.  pl330_submit_req() running in
another processor can then acquire the lock and insert another request
in one of the thrd->req that hasn't been processed yet, replacing the
pointer to pl330_req there.  When the callback returns in
pl330_update() and the next rqdone is popped from the list, it
dereferences the pl330_req pointer to the just scheduled pl330_req,
instead of the one that has finished, calling pl330 with the wrong r.

This patch fixes this by storing the pointer to pl330_req directly in
the list.

Signed-off-by: Javi Merino <javi.merino@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Acked-by: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Vinod Koul <vinod.koul@linux.intel.com>
  • Loading branch information
Javi Merino authored and Vinod Koul committed Jun 14, 2012
1 parent 5a67ac5 commit fdec53d
Showing 1 changed file with 10 additions and 16 deletions.
26 changes: 10 additions & 16 deletions drivers/dma/pl330.c
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,8 @@ struct pl330_req {
struct pl330_reqcfg *cfg;
/* Pointer to first xfer in the request. */
struct pl330_xfer *x;
/* Hook to attach to DMAC's list of reqs with due callback */
struct list_head rqd;
};

/*
Expand Down Expand Up @@ -461,8 +463,6 @@ struct _pl330_req {
/* Number of bytes taken to setup MC for the req */
u32 mc_len;
struct pl330_req *r;
/* Hook to attach to DMAC's list of reqs with due callback */
struct list_head rqd;
};

/* ToBeDone for tasklet */
Expand Down Expand Up @@ -1683,7 +1683,7 @@ static void pl330_dotask(unsigned long data)
/* Returns 1 if state was updated, 0 otherwise */
static int pl330_update(const struct pl330_info *pi)
{
struct _pl330_req *rqdone;
struct pl330_req *rqdone, *tmp;
struct pl330_dmac *pl330;
unsigned long flags;
void __iomem *regs;
Expand Down Expand Up @@ -1750,7 +1750,10 @@ static int pl330_update(const struct pl330_info *pi)
if (active == -1) /* Aborted */
continue;

rqdone = &thrd->req[active];
/* Detach the req */
rqdone = thrd->req[active].r;
thrd->req[active].r = NULL;

mark_free(thrd, active);

/* Get going again ASAP */
Expand All @@ -1762,20 +1765,11 @@ static int pl330_update(const struct pl330_info *pi)
}

/* Now that we are in no hurry, do the callbacks */
while (!list_empty(&pl330->req_done)) {
struct pl330_req *r;

rqdone = container_of(pl330->req_done.next,
struct _pl330_req, rqd);

list_del_init(&rqdone->rqd);

/* Detach the req */
r = rqdone->r;
rqdone->r = NULL;
list_for_each_entry_safe(rqdone, tmp, &pl330->req_done, rqd) {
list_del(&rqdone->rqd);

spin_unlock_irqrestore(&pl330->lock, flags);
_callback(r, PL330_ERR_NONE);
_callback(rqdone, PL330_ERR_NONE);
spin_lock_irqsave(&pl330->lock, flags);
}

Expand Down

0 comments on commit fdec53d

Please sign in to comment.