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 <[email protected]>
Cc: Jassi Brar <[email protected]>
---
drivers/dma/pl330.c | 26 ++++++++++----------------
1 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index cbcc28e..b1d93b0 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -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;
};
/*
@@ -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 */
@@ -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;
@@ -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 */
@@ -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);
}
--
1.7.0.4
On 13 June 2012 19:37, Javi Merino <[email protected]> wrote:
> 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.
>
.....
> @@ -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;
> @@ -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;
> +
Doesn't this movement of "Detach the req" chunk effectively remain the
same? Since that was already protected by the same lock. I thought I
deliberately took care of that already.
Do you see some real problem fixed by this patch? Info about that
could help me better understand if I missed something here.
Thanks.
On Wed 13 Jun 2012 16:13:05 BST, Jassi Brar wrote:
> On 13 June 2012 19:37, Javi Merino<[email protected]> wrote:
>> 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.
>>
> .....
>> @@ -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;
>> @@ -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;
>> +
> Doesn't this movement of "Detach the req" chunk effectively remain the
> same? Since that was already protected by the same lock. I thought I
> deliberately took care of that already.
Yes, but you release the lock before calling the first callback, so the
subsequent dereferences of rqdone->r are not protected by the lock.
> Do you see some real problem fixed by this patch? Info about that
> could help me better understand if I missed something here.
Ok, the description sucks. Let me try to describe it with the scenario
that failed:
Core 0:
- Two DMA transactions finish, in channels 0 and 1.
- pl330_update() is called, the "Event-Interrupt Status Register" (ES)
is 0x3.
- In the "for (ev = 0;..." loop
+ two pointers are stored in pl330->req_done:
pl330->channels[0]->req[0] and pl330->channels[1]->req[0]
- In the "while (!list_empty.." loop,
+ r = pl330->channels[0]->req[0]->r
+ Release the pl330_lock and call _callback()
Core 1:
- pl330_submit_req() for channel 1
- Grab the pl330_lock
- Hook a request in pl330->channels[1]->req[0]->r
- Release the pl330_lock
Core 0:
- _callback() returns
- Acquire the pl330_lock again
- second iteration of "while (!list_empty.." loop,
+ r = pl330->channels[1]->req[0]->r , but you get the r that has just
been scheduled, not the one that finished.
Hope it's now clearer,
Javi
On 14 June 2012 00:06, Javi Merino <[email protected]> wrote:
>
> Ok, the description sucks. ?Let me try to describe it with the scenario that
> failed:
>
> Core 0:
> - Two DMA transactions finish, in channels 0 and 1.
> - pl330_update() is called, the "Event-Interrupt Status Register" (ES)
> ?is 0x3.
> - In the "for (ev = 0;..." loop
> ?+ two pointers are stored in pl330->req_done:
> ? ?pl330->channels[0]->req[0] and pl330->channels[1]->req[0]
> - In the "while (!list_empty.." loop,
> ?+ r = pl330->channels[0]->req[0]->r
> ?+ Release the pl330_lock and call _callback()
>
> Core 1:
> - pl330_submit_req() for channel 1
> - Grab the pl330_lock
> - Hook a request in pl330->channels[1]->req[0]->r
> - Release the pl330_lock
>
> Core 0:
> - _callback() returns
> - Acquire the pl330_lock again
> - second iteration of "while (!list_empty.." loop,
> ?+ r = pl330->channels[1]->req[0]->r , but you get the r that has just
> ? ?been scheduled, not the one that finished.
>
Thanks for detailed explanation of the bug.
I see, it's not safe across channels. Another option could be taking
channel's lock in update and submit.
Acked-by: Jassi Brar <[email protected]>
On Wed, 2012-06-13 at 15:07 +0100, Javi Merino wrote:
> 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 <[email protected]>
> Cc: Jassi Brar <[email protected]>
Applied, thanks
> ---
> drivers/dma/pl330.c | 26 ++++++++++----------------
> 1 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index cbcc28e..b1d93b0 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -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;
> };
>
> /*
> @@ -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 */
> @@ -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;
> @@ -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 */
> @@ -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);
> }
>
--
~Vinod