[Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held

[Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held

[Cong Wang]

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)

Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held

[Dave Young]

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

Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held

[Andrew Morton]

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 ;)

Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held

[Cong Wang]

[-- 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;

Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held

[Arnd Bergmann]

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

Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held

[Cong Wang]

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.

Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held

[Arnd Bergmann]

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

Re: [Patch] lkdtm: avoid calling lkdtm_do_action() with spin lock held

[Cong Wang]

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.