linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held
@ 2012-01-28 12:52 Cong Wang
  2012-01-29  1:15 ` Dave Young
  2012-01-30 20:54 ` Andrew Morton
  0 siblings, 2 replies; 8+ messages in thread
From: Cong Wang @ 2012-01-28 12:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, Prarit Bhargava, WANG Cong, Arnd Bergmann,
	Greg Kroah-Hartman

lkdtm_do_action() may call sleeping functions like kmalloc(),
so do not call it with spin lock held.

Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

---
diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
index 150cd70..28adefe 100644
--- a/drivers/misc/lkdtm.c
+++ b/drivers/misc/lkdtm.c
@@ -354,6 +354,7 @@ static void lkdtm_do_action(enum ctype which)
 static void lkdtm_handler(void)
 {
 	unsigned long flags;
+	bool do_it = false;
 
 	spin_lock_irqsave(&count_lock, flags);
 	count--;
@@ -361,10 +362,13 @@ static void lkdtm_handler(void)
 			cp_name_to_str(cpoint), cp_type_to_str(cptype), count);
 
 	if (count == 0) {
-		lkdtm_do_action(cptype);
+		do_it = true;
 		count = cpoint_count;
 	}
 	spin_unlock_irqrestore(&count_lock, flags);
+
+	if (do_it)
+		lkdtm_do_action(cptype);
 }
 
 static int lkdtm_register_cpoint(enum cname which)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held
  2012-01-28 12:52 [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held Cong Wang
@ 2012-01-29  1:15 ` Dave Young
  2012-01-30 20:54 ` Andrew Morton
  1 sibling, 0 replies; 8+ messages in thread
From: Dave Young @ 2012-01-29  1:15 UTC (permalink / raw)
  To: Cong Wang
  Cc: linux-kernel, Andrew Morton, Prarit Bhargava, Arnd Bergmann,
	Greg Kroah-Hartman

On 01/28/2012 08:52 PM, Cong Wang wrote:

> lkdtm_do_action() may call sleeping functions like kmalloc(),
> so do not call it with spin lock held.
> 
> Cc: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>


Reviewed-by: Dave Young <dyoung@redhat.com>

> 
> ---
> diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
> index 150cd70..28adefe 100644
> --- a/drivers/misc/lkdtm.c
> +++ b/drivers/misc/lkdtm.c
> @@ -354,6 +354,7 @@ static void lkdtm_do_action(enum ctype which)
>  static void lkdtm_handler(void)
>  {
>  	unsigned long flags;
> +	bool do_it = false;
>  
>  	spin_lock_irqsave(&count_lock, flags);
>  	count--;
> @@ -361,10 +362,13 @@ static void lkdtm_handler(void)
>  			cp_name_to_str(cpoint), cp_type_to_str(cptype), count);
>  
>  	if (count == 0) {
> -		lkdtm_do_action(cptype);
> +		do_it = true;
>  		count = cpoint_count;
>  	}
>  	spin_unlock_irqrestore(&count_lock, flags);
> +
> +	if (do_it)
> +		lkdtm_do_action(cptype);
>  }
>  
>  static int lkdtm_register_cpoint(enum cname which)
> --
> 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/



-- 
Thanks
Dave

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held
  2012-01-28 12:52 [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held Cong Wang
  2012-01-29  1:15 ` Dave Young
@ 2012-01-30 20:54 ` Andrew Morton
  2012-01-31 13:25   ` Cong Wang
  1 sibling, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2012-01-30 20:54 UTC (permalink / raw)
  To: Cong Wang
  Cc: linux-kernel, Prarit Bhargava, Arnd Bergmann, Greg Kroah-Hartman

On Sat, 28 Jan 2012 20:52:47 +0800
Cong Wang <xiyou.wangcong@gmail.com> wrote:

> lkdtm_do_action() may call sleeping functions like kmalloc(),
> so do not call it with spin lock held.
> 
> Cc: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
> 
> ---
> diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
> index 150cd70..28adefe 100644
> --- a/drivers/misc/lkdtm.c
> +++ b/drivers/misc/lkdtm.c
> @@ -354,6 +354,7 @@ static void lkdtm_do_action(enum ctype which)
>  static void lkdtm_handler(void)
>  {
>  	unsigned long flags;
> +	bool do_it = false;
>  
>  	spin_lock_irqsave(&count_lock, flags);
>  	count--;
> @@ -361,10 +362,13 @@ static void lkdtm_handler(void)
>  			cp_name_to_str(cpoint), cp_type_to_str(cptype), count);
>  
>  	if (count == 0) {
> -		lkdtm_do_action(cptype);
> +		do_it = true;
>  		count = cpoint_count;
>  	}
>  	spin_unlock_irqrestore(&count_lock, flags);
> +
> +	if (do_it)
> +		lkdtm_do_action(cptype);
>  }
>  
>  static int lkdtm_register_cpoint(enum cname which)

lkdtm_handler() can be called from module IRQ handlers, so perhaps the
same problems can still happen.  The patch does improve things though ;)


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held
  2012-01-30 20:54 ` Andrew Morton
@ 2012-01-31 13:25   ` Cong Wang
  2012-01-31 15:35     ` Arnd Bergmann
  0 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2012-01-31 13:25 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, Prarit Bhargava, Arnd Bergmann, Greg Kroah-Hartman

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

On 01/31/2012 04:54 AM, Andrew Morton wrote:
>
> lkdtm_handler() can be called from module IRQ handlers, so perhaps the
> same problems can still happen.  The patch does improve things though ;)
>

Yeah, what do you think about patch below (untested)?

---

Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

[-- Attachment #2: lkdtm-2.diff --]
[-- Type: text/x-patch, Size: 1030 bytes --]

diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
index 28adefe..5cbd740 100644
--- a/drivers/misc/lkdtm.c
+++ b/drivers/misc/lkdtm.c
@@ -315,7 +315,7 @@ static void lkdtm_do_action(enum ctype which)
 	}
 	case CT_OVERWRITE_ALLOCATION: {
 		size_t len = 1020;
-		u32 *data = kmalloc(len, GFP_KERNEL);
+		u32 *data = kmalloc(len, GFP_ATOMIC);
 
 		data[1024 / sizeof(u32)] = 0x12345678;
 		kfree(data);
@@ -323,14 +323,16 @@ static void lkdtm_do_action(enum ctype which)
 	}
 	case CT_WRITE_AFTER_FREE: {
 		size_t len = 1024;
-		u32 *data = kmalloc(len, GFP_KERNEL);
+		u32 *data = kmalloc(len, GFP_ATOMIC);
 
 		kfree(data);
-		schedule();
+		udelay(100);
 		memset(data, 0x78, len);
 		break;
 	}
 	case CT_SOFTLOCKUP:
+		if (in_interrupt())
+			break;
 		preempt_disable();
 		for (;;)
 			cpu_relax();
@@ -341,6 +343,8 @@ static void lkdtm_do_action(enum ctype which)
 			cpu_relax();
 		break;
 	case CT_HUNG_TASK:
+		if (in_interrupt())
+			break;
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		schedule();
 		break;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held
  2012-01-31 13:25   ` Cong Wang
@ 2012-01-31 15:35     ` Arnd Bergmann
  2012-02-01  3:01       ` Cong Wang
  0 siblings, 1 reply; 8+ messages in thread
From: Arnd Bergmann @ 2012-01-31 15:35 UTC (permalink / raw)
  To: Cong Wang
  Cc: Andrew Morton, linux-kernel, Prarit Bhargava, Greg Kroah-Hartman

On Tuesday 31 January 2012, Cong Wang wrote:
> @@ -323,14 +323,16 @@ static void lkdtm_do_action(enum ctype which)
>         }
>         case CT_WRITE_AFTER_FREE: {
>                 size_t len = 1024;
> -               u32 *data = kmalloc(len, GFP_KERNEL);
> +               u32 *data = kmalloc(len, GFP_ATOMIC);
>  
>                 kfree(data);
> -               schedule();
> +               udelay(100);
>                 memset(data, 0x78, len);
>                 break;
>         }

I can't think of why the udelay would have any positive effect here,
if the idea of the schedule was to let some other process allocate and
use the memory.

Can't you just get rid of the count_lock if you use an atomic_t for the
count and use appropriate accesses on it?

	Arnd

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held
  2012-01-31 15:35     ` Arnd Bergmann
@ 2012-02-01  3:01       ` Cong Wang
  2012-02-01 15:29         ` Arnd Bergmann
  0 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2012-02-01  3:01 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Morton, linux-kernel, Prarit Bhargava, Greg Kroah-Hartman

On 01/31/2012 11:35 PM, Arnd Bergmann wrote:
> On Tuesday 31 January 2012, Cong Wang wrote:
>> @@ -323,14 +323,16 @@ static void lkdtm_do_action(enum ctype which)
>>          }
>>          case CT_WRITE_AFTER_FREE: {
>>                  size_t len = 1024;
>> -               u32 *data = kmalloc(len, GFP_KERNEL);
>> +               u32 *data = kmalloc(len, GFP_ATOMIC);
>>
>>                  kfree(data);
>> -               schedule();
>> +               udelay(100);
>>                  memset(data, 0x78, len);
>>                  break;
>>          }
>
> I can't think of why the udelay would have any positive effect here,
> if the idea of the schedule was to let some other process allocate and
> use the memory.


Hmm, on SMP udelay on this CPU will give a chance to other CPU's to use 
that memory, right?

>
> Can't you just get rid of the count_lock if you use an atomic_t for the
> count and use appropriate accesses on it?
>

Good idea, will do.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held
  2012-02-01  3:01       ` Cong Wang
@ 2012-02-01 15:29         ` Arnd Bergmann
  2012-02-02 13:31           ` Cong Wang
  0 siblings, 1 reply; 8+ messages in thread
From: Arnd Bergmann @ 2012-02-01 15:29 UTC (permalink / raw)
  To: Cong Wang
  Cc: Andrew Morton, linux-kernel, Prarit Bhargava, Greg Kroah-Hartman

On Wednesday 01 February 2012, Cong Wang wrote:
> On 01/31/2012 11:35 PM, Arnd Bergmann wrote:
> > On Tuesday 31 January 2012, Cong Wang wrote:
> >> @@ -323,14 +323,16 @@ static void lkdtm_do_action(enum ctype which)
> >>          }
> >>          case CT_WRITE_AFTER_FREE: {
> >>                  size_t len = 1024;
> >> -               u32 *data = kmalloc(len, GFP_KERNEL);
> >> +               u32 *data = kmalloc(len, GFP_ATOMIC);
> >>
> >>                  kfree(data);
> >> -               schedule();
> >> +               udelay(100);
> >>                  memset(data, 0x78, len);
> >>                  break;
> >>          }
> >
> > I can't think of why the udelay would have any positive effect here,
> > if the idea of the schedule was to let some other process allocate and
> > use the memory.
> 
> 
> Hmm, on SMP udelay on this CPU will give a chance to other CPU's to use 
> that memory, right?
> 

There is a small chance for that, but it's much less likely than it would
be using another process on the same CPU, plus it requires SMP.

	Arnd

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held
  2012-02-01 15:29         ` Arnd Bergmann
@ 2012-02-02 13:31           ` Cong Wang
  0 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2012-02-02 13:31 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Morton, linux-kernel, Prarit Bhargava, Greg Kroah-Hartman

On 02/01/2012 11:29 PM, Arnd Bergmann wrote:
> On Wednesday 01 February 2012, Cong Wang wrote:
>> On 01/31/2012 11:35 PM, Arnd Bergmann wrote:
>>> On Tuesday 31 January 2012, Cong Wang wrote:
>>>> @@ -323,14 +323,16 @@ static void lkdtm_do_action(enum ctype which)
>>>>           }
>>>>           case CT_WRITE_AFTER_FREE: {
>>>>                   size_t len = 1024;
>>>> -               u32 *data = kmalloc(len, GFP_KERNEL);
>>>> +               u32 *data = kmalloc(len, GFP_ATOMIC);
>>>>
>>>>                   kfree(data);
>>>> -               schedule();
>>>> +               udelay(100);
>>>>                   memset(data, 0x78, len);
>>>>                   break;
>>>>           }
>>>
>>> I can't think of why the udelay would have any positive effect here,
>>> if the idea of the schedule was to let some other process allocate and
>>> use the memory.
>>
>>
>> Hmm, on SMP udelay on this CPU will give a chance to other CPU's to use
>> that memory, right?
>>
>
> There is a small chance for that, but it's much less likely than it would
> be using another process on the same CPU, plus it requires SMP.

Yeah, I updated this in [PATCH 2/2] lkdtm: avoid calling sleeping 
functions in interrupt context.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2012-02-02 13:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-28 12:52 [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held Cong Wang
2012-01-29  1:15 ` Dave Young
2012-01-30 20:54 ` Andrew Morton
2012-01-31 13:25   ` Cong Wang
2012-01-31 15:35     ` Arnd Bergmann
2012-02-01  3:01       ` Cong Wang
2012-02-01 15:29         ` Arnd Bergmann
2012-02-02 13:31           ` Cong Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).