2010-07-27 18:42:50

by Kulikov Vasiliy

Subject: [PATCH] dvb: siano: free spinlock before schedule()

Calling schedule() holding spinlock with disables irqs is improper. As
spinlock protects list coredev->buffers, it can be unlocked untill wakeup.
This bug was introduced in a9349315f65cd6a16e8fab1f6cf0fd40f379c4db.

Signed-off-by: Kulikov Vasiliy <[email protected]>
---
drivers/media/dvb/siano/smscoreapi.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/siano/smscoreapi.c b/drivers/media/dvb/siano/smscoreapi.c
index 7f2c94a..d93468c 100644
--- a/drivers/media/dvb/siano/smscoreapi.c
+++ b/drivers/media/dvb/siano/smscoreapi.c
@@ -1113,9 +1113,11 @@ struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
*/

prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
-
- if (list_empty(&coredev->buffers))
+ if (list_empty(&coredev->buffers)) {
+ spin_unlock_irqrestore(&coredev->bufferslock, flags);
schedule();
+ spin_lock_irqsave(&coredev->bufferslock, flags);
+ }

finish_wait(&coredev->buffer_mng_waitq, &wait);

--
1.7.0.4

2010-07-27 22:24:47

by Jiri Slaby

Subject: Re: [PATCH] dvb: siano: free spinlock before schedule()

On 07/27/2010 08:42 PM, Kulikov Vasiliy wrote:
> Calling schedule() holding spinlock with disables irqs is improper. As
> spinlock protects list coredev->buffers, it can be unlocked untill wakeup.
> This bug was introduced in a9349315f65cd6a16e8fab1f6cf0fd40f379c4db.
>
> Signed-off-by: Kulikov Vasiliy <[email protected]>
> ---
> drivers/media/dvb/siano/smscoreapi.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/dvb/siano/smscoreapi.c b/drivers/media/dvb/siano/smscoreapi.c
> index 7f2c94a..d93468c 100644
> --- a/drivers/media/dvb/siano/smscoreapi.c
> +++ b/drivers/media/dvb/siano/smscoreapi.c
> @@ -1113,9 +1113,11 @@ struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
> */
>
> prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
> -
> - if (list_empty(&coredev->buffers))
> + if (list_empty(&coredev->buffers)) {
> + spin_unlock_irqrestore(&coredev->bufferslock, flags);
> schedule();
> + spin_lock_irqsave(&coredev->bufferslock, flags);
> + }
>
> finish_wait(&coredev->buffer_mng_waitq, &wait);

There is a better fix (which fixes the potential NULL dereference):
http://lkml.org/lkml/2010/6/7/175

Richard, could you address the comments there and resend?

regards,
--
js

2010-08-08 16:16:38

by Richard Z

Subject: Re: [PATCH] dvb: siano: free spinlock before schedule()

On Wed, Jul 28, 2010 at 12:24:39AM +0200, Jiri Slaby wrote:

sorry for seeing this so late, was flooded with email lately.

> There is a better fix (which fixes the potential NULL dereference):
> http://lkml.org/lkml/2010/6/7/175

> Richard, could you address the comments there and resend?

I am running this patch since many weeks (after fixing the compile error obviously).
Did not implement your beautification suggestion yet, was doing all kinds of experiments
with IR and had plenty of unrelated issues.

Richard


--- linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c.rz 2010-06-03 21:58:11.000000000 +0200
+++ linux-2.6.34/drivers/media/dvb/siano/smscoreapi.c 2010-06-07 14:32:06.000000000 +0200
@@ -1100,31 +1100,26 @@
*
* @return pointer to descriptor on success, NULL on error.
*/
-struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
+
+struct smscore_buffer_t *get_entry(struct smscore_device_t *coredev)
{
struct smscore_buffer_t *cb = NULL;
unsigned long flags;

- DEFINE_WAIT(wait);
-
spin_lock_irqsave(&coredev->bufferslock, flags);
-
- /* This function must return a valid buffer, since the buffer list is
- * finite, we check that there is an available buffer, if not, we wait
- * until such buffer become available.
- */
-
- prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
-
- if (list_empty(&coredev->buffers))
- schedule();
-
- finish_wait(&coredev->buffer_mng_waitq, &wait);
-
+ if (!list_empty(&coredev->buffers)) {
cb = (struct smscore_buffer_t *) coredev->buffers.next;
list_del(&cb->entry);
-
+ }
spin_unlock_irqrestore(&coredev->bufferslock, flags);
+ return cb;
+}
+
+struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
+{
+ struct smscore_buffer_t *cb = NULL;
+
+ wait_event(coredev->buffer_mng_waitq, (cb = get_entry(coredev)));

return cb;
}

2010-08-24 12:52:37

by Mauro Carvalho Chehab

Subject: Re: [PATCH] dvb: siano: free spinlock before schedule()

Em 08-08-2010 13:10, Richard Zidlicky escreveu:
> On Wed, Jul 28, 2010 at 12:24:39AM +0200, Jiri Slaby wrote:
>
> sorry for seeing this so late, was flooded with email lately.
>
>> There is a better fix (which fixes the potential NULL dereference):
>> http://lkml.org/lkml/2010/6/7/175
>
>> Richard, could you address the comments there and resend?
>
> I am running this patch since many weeks (after fixing the compile error obviously).
> Did not implement your beautification suggestion yet, was doing all kinds of experiments
> with IR and had plenty of unrelated issues.

This patch seems a way better than the previous patch. I've rebased it against
the current tree (and fixed the identation).

The only missing issue on it is the lack of your Signed-off-by. Richard, could you
please send it to us?

---
drivers/media/dvb/siano/smscoreapi.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)

--- patchwork.orig/drivers/media/dvb/siano/smscoreapi.c
+++ patchwork/drivers/media/dvb/siano/smscoreapi.c
@@ -1098,31 +1098,26 @@ EXPORT_SYMBOL_GPL(smscore_onresponse);
*
* @return pointer to descriptor on success, NULL on error.
*/
-struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
+
+struct smscore_buffer_t *get_entry(struct smscore_device_t *coredev)
{
struct smscore_buffer_t *cb = NULL;
unsigned long flags;

- DEFINE_WAIT(wait);
-
spin_lock_irqsave(&coredev->bufferslock, flags);
+ if (!list_empty(&coredev->buffers)) {
+ cb = (struct smscore_buffer_t *) coredev->buffers.next;
+ list_del(&cb->entry);
+ }
+ spin_unlock_irqrestore(&coredev->bufferslock, flags);
+ return cb;
+}

- /* This function must return a valid buffer, since the buffer list is
- * finite, we check that there is an available buffer, if not, we wait
- * until such buffer become available.
- */
-
- prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
-
- if (list_empty(&coredev->buffers))
- schedule();
-
- finish_wait(&coredev->buffer_mng_waitq, &wait);
-
- cb = (struct smscore_buffer_t *) coredev->buffers.next;
- list_del(&cb->entry);
+struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
+{
+ struct smscore_buffer_t *cb = NULL;

- spin_unlock_irqrestore(&coredev->bufferslock, flags);
+ wait_event(coredev->buffer_mng_waitq, (cb = get_entry(coredev)));

return cb;
}

2010-08-24 15:53:42

by Richard Z

Subject: Re: [PATCH] dvb: siano: free spinlock before schedule()

On Tue, Aug 24, 2010 at 09:52:36AM -0300, Mauro Carvalho Chehab wrote:
> Em 08-08-2010 13:10, Richard Zidlicky escreveu:
> > On Wed, Jul 28, 2010 at 12:24:39AM +0200, Jiri Slaby wrote:
> >
> > sorry for seeing this so late, was flooded with email lately.
> >
> >> There is a better fix (which fixes the potential NULL dereference):
> >> http://lkml.org/lkml/2010/6/7/175
> >
> >> Richard, could you address the comments there and resend?
> >
> > I am running this patch since many weeks (after fixing the compile error obviously).
> > Did not implement your beautification suggestion yet, was doing all kinds of experiments
> > with IR and had plenty of unrelated issues.
>
> This patch seems a way better than the previous patch. I've rebased it against
> the current tree (and fixed the identation).

thanks for looking at it.

> The only missing issue on it is the lack of your Signed-off-by. Richard, could you
> please send it to us?


Signed-off-by: Richard Zidlicky <[email protected]>

PS: I will be away for a while.

>
> ---
> drivers/media/dvb/siano/smscoreapi.c | 31 +++++++++++++------------------
> 1 file changed, 13 insertions(+), 18 deletions(-)
>
> --- patchwork.orig/drivers/media/dvb/siano/smscoreapi.c
> +++ patchwork/drivers/media/dvb/siano/smscoreapi.c
> @@ -1098,31 +1098,26 @@ EXPORT_SYMBOL_GPL(smscore_onresponse);
> *
> * @return pointer to descriptor on success, NULL on error.
> */
> -struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
> +
> +struct smscore_buffer_t *get_entry(struct smscore_device_t *coredev)
> {
> struct smscore_buffer_t *cb = NULL;
> unsigned long flags;
>
> - DEFINE_WAIT(wait);
> -
> spin_lock_irqsave(&coredev->bufferslock, flags);
> + if (!list_empty(&coredev->buffers)) {
> + cb = (struct smscore_buffer_t *) coredev->buffers.next;
> + list_del(&cb->entry);
> + }
> + spin_unlock_irqrestore(&coredev->bufferslock, flags);
> + return cb;
> +}
>
> - /* This function must return a valid buffer, since the buffer list is
> - * finite, we check that there is an available buffer, if not, we wait
> - * until such buffer become available.
> - */
> -
> - prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
> -
> - if (list_empty(&coredev->buffers))
> - schedule();
> -
> - finish_wait(&coredev->buffer_mng_waitq, &wait);
> -
> - cb = (struct smscore_buffer_t *) coredev->buffers.next;
> - list_del(&cb->entry);
> +struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
> +{
> + struct smscore_buffer_t *cb = NULL;
>
> - spin_unlock_irqrestore(&coredev->bufferslock, flags);
> + wait_event(coredev->buffer_mng_waitq, (cb = get_entry(coredev)));
>
> return cb;
> }