[PATCH 1/2] lkdtm: use atomic_t to replace count_lock

[PATCH 1/2] lkdtm: use atomic_t to replace count_lock

[Cong Wang]

Andrew, this patch replaces
lkdtm-avoid-calling-lkdtm_do_action-with-spin-lock-held.patch
in your tree.

---------->

The spin lock count_lock only protects count, it can be removed by
using atomic_t. Suggested by Arnd.

Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <greg@kroah.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

---
 drivers/misc/lkdtm.c |   19 ++++---------------
 1 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
index 150cd70..afdef2e 100644
--- a/drivers/misc/lkdtm.c
+++ b/drivers/misc/lkdtm.c
@@ -119,8 +119,7 @@ static int recur_count = REC_NUM_DEFAULT;
 
 static enum cname cpoint = CN_INVALID;
 static enum ctype cptype = CT_NONE;
-static int count = DEFAULT_COUNT;
-static DEFINE_SPINLOCK(count_lock);
+static atomic_t count = ATOMIC_INIT(DEFAULT_COUNT);
 
 module_param(recur_count, int, 0644);
 MODULE_PARM_DESC(recur_count, " Recursion level for the stack overflow test, "\
@@ -231,14 +230,11 @@ static const char *cp_name_to_str(enum cname name)
 static int lkdtm_parse_commandline(void)
 {
 	int i;
-	unsigned long flags;
 
 	if (cpoint_count < 1 || recur_count < 1)
 		return -EINVAL;
 
-	spin_lock_irqsave(&count_lock, flags);
-	count = cpoint_count;
-	spin_unlock_irqrestore(&count_lock, flags);
+	atomic_set(&count, cpoint_count);
 
 	/* No special parameters */
 	if (!cpoint_type && !cpoint_name)
@@ -353,18 +349,11 @@ static void lkdtm_do_action(enum ctype which)
 
 static void lkdtm_handler(void)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&count_lock, flags);
-	count--;
 	printk(KERN_INFO "lkdtm: Crash point %s of type %s hit, trigger in %d rounds\n",
-			cp_name_to_str(cpoint), cp_type_to_str(cptype), count);
+			cp_name_to_str(cpoint), cp_type_to_str(cptype), atomic_dec_return(&count));
 
-	if (count == 0) {
+	if (!atomic_cmpxchg(&count, 0, cpoint_count))
 		lkdtm_do_action(cptype);
-		count = cpoint_count;
-	}
-	spin_unlock_irqrestore(&count_lock, flags);
 }
 
 static int lkdtm_register_cpoint(enum cname which)
-- 
1.7.7.6

[PATCH 2/2] lkdtm: avoid calling sleeping functions in interrupt context

[Cong Wang]

lkdtm_do_action() could be called in interrupt context,
but it also calls sleeping functions like schedule(),
kmalloc(GFP_KERNEL) etc., for such cases, avoid calling them
if we are in interrupt context.

BTW, check the return value of kmalloc().

Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <greg@kroah.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

---
 drivers/misc/lkdtm.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
index afdef2e..63b23a4 100644
--- a/drivers/misc/lkdtm.c
+++ b/drivers/misc/lkdtm.c
@@ -311,22 +311,31 @@ 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);
 
+		if (!data)
+			break;
 		data[1024 / sizeof(u32)] = 0x12345678;
 		kfree(data);
 		break;
 	}
 	case CT_WRITE_AFTER_FREE: {
 		size_t len = 1024;
-		u32 *data = kmalloc(len, GFP_KERNEL);
+		u32 *data;
 
+		if (in_interrupt())
+			break;
+		data = kmalloc(len, GFP_KERNEL);
+		if (!data)
+			break;
 		kfree(data);
 		schedule();
 		memset(data, 0x78, len);
 		break;
 	}
 	case CT_SOFTLOCKUP:
+		if (in_interrupt())
+			break;
 		preempt_disable();
 		for (;;)
 			cpu_relax();
@@ -337,6 +346,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;
-- 
1.7.7.6

Re: [PATCH 1/2] lkdtm: use atomic_t to replace count_lock

[Arnd Bergmann]

On Wednesday 01 February 2012, Cong Wang wrote:
>  static void lkdtm_handler(void)
>  {
> -       unsigned long flags;
> -
> -       spin_lock_irqsave(&count_lock, flags);
> -       count--;
>         printk(KERN_INFO "lkdtm: Crash point %s of type %s hit, trigger in %d rounds\n",
> -                       cp_name_to_str(cpoint), cp_type_to_str(cptype), count);
> +                       cp_name_to_str(cpoint), cp_type_to_str(cptype), atomic_dec_return(&count));
>  
> -       if (count == 0) {
> +       if (!atomic_cmpxchg(&count, 0, cpoint_count))
>                 lkdtm_do_action(cptype);
> -               count = cpoint_count;
> -       }
> -       spin_unlock_irqrestore(&count_lock, flags);
>  }

This use is not atomic, you could have two threads doing atomic_dec_return
at the same time, and after that the value will be -1 so the atomic_cmpxchg
does not trigger.

In order to have an atomic here, you have to use a loop around
atomic_cmpxchg, like


	int old, new;
	old = atomic_read(&count);
	do {
		new = old ? old - 1 : cpoint_count;
		old = cmpxchg(&count, old, new);
	} while (old != new);

I suppose you could also just keep the spinlock and move lkdtm_do_action()
outside of it?

	Arnd

Re: [PATCH 1/2] lkdtm: use atomic_t to replace count_lock

[Cong Wang]

On 02/01/2012 11:27 PM, Arnd Bergmann wrote:
> On Wednesday 01 February 2012, Cong Wang wrote:
>>   static void lkdtm_handler(void)
>>   {
>> -       unsigned long flags;
>> -
>> -       spin_lock_irqsave(&count_lock, flags);
>> -       count--;
>>          printk(KERN_INFO "lkdtm: Crash point %s of type %s hit, trigger in %d rounds\n",
>> -                       cp_name_to_str(cpoint), cp_type_to_str(cptype), count);
>> +                       cp_name_to_str(cpoint), cp_type_to_str(cptype), atomic_dec_return(&count));
>>
>> -       if (count == 0) {
>> +       if (!atomic_cmpxchg(&count, 0, cpoint_count))
>>                  lkdtm_do_action(cptype);
>> -               count = cpoint_count;
>> -       }
>> -       spin_unlock_irqrestore(&count_lock, flags);
>>   }
>
> This use is not atomic, you could have two threads doing atomic_dec_return
> at the same time, and after that the value will be -1 so the atomic_cmpxchg
> does not trigger.


Yeah, simply combining two atomic operations is not atomic. :-/

>
> In order to have an atomic here, you have to use a loop around
> atomic_cmpxchg, like
>
>
> 	int old, new;
> 	old = atomic_read(&count);
> 	do {
> 		new = old ? old - 1 : cpoint_count;
> 		old = cmpxchg(&count, old, new);
> 	} while (old != new);
>
> I suppose you could also just keep the spinlock and move lkdtm_do_action()
> outside of it?

If we still need spinlock, I think we don't need to bother atomic_t at all.

Re: [PATCH 1/2] lkdtm: use atomic_t to replace count_lock

[Arnd Bergmann]

On Thursday 02 February 2012, Cong Wang wrote:
> > In order to have an atomic here, you have to use a loop around
> > atomic_cmpxchg, like
> >
> >
> >       int old, new;
> >       old = atomic_read(&count);
> >       do {
> >               new = old ? old - 1 : cpoint_count;
> >               old = cmpxchg(&count, old, new);
> >       } while (old != new);
> >
> > I suppose you could also just keep the spinlock and move lkdtm_do_action()
> > outside of it?
> 
> If we still need spinlock, I think we don't need to bother atomic_t at all.

Yes, it's one or the other: If you use the cmpxchg loop, you don't need a
spinlock and vice versa.

	Arnd

Re: [PATCH 1/2] lkdtm: use atomic_t to replace count_lock

[Cong Wang]

On 02/02/2012 09:44 PM, Arnd Bergmann wrote:
> On Thursday 02 February 2012, Cong Wang wrote:
>>> In order to have an atomic here, you have to use a loop around
>>> atomic_cmpxchg, like
>>>
>>>
>>>        int old, new;
>>>        old = atomic_read(&count);
>>>        do {
>>>                new = old ? old - 1 : cpoint_count;
>>>                old = cmpxchg(&count, old, new);
>>>        } while (old != new);
>>>
>>> I suppose you could also just keep the spinlock and move lkdtm_do_action()
>>> outside of it?
>>
>> If we still need spinlock, I think we don't need to bother atomic_t at all.
>
> Yes, it's one or the other: If you use the cmpxchg loop, you don't need a
> spinlock and vice versa.
>

The cmpxchg loop is for comparing and assigning to 'count', but still 
there is a printk() above that needs to read 'count'. Combining these 
two operations means we have to use a spinlock, correct? Because there 
is a chance that another process could change 'count' in between.

Thanks.

Re: [PATCH 1/2] lkdtm: use atomic_t to replace count_lock

[Arnd Bergmann]

On Thursday 02 February 2012, Cong Wang wrote:
> On 02/02/2012 09:44 PM, Arnd Bergmann wrote:
> > On Thursday 02 February 2012, Cong Wang wrote:
> >>> In order to have an atomic here, you have to use a loop around
> >>> atomic_cmpxchg, like
> >>>
> >>>
> >>>        int old, new;
> >>>        old = atomic_read(&count);
> >>>        do {
> >>>                new = old ? old - 1 : cpoint_count;
> >>>                old = cmpxchg(&count, old, new);

		    ^^^^^^^
I guess I meant "new = cmpxchg(...)" here, sorry.

> >>>        } while (old != new);
> >>>
> >>> I suppose you could also just keep the spinlock and move lkdtm_do_action()
> >>> outside of it?
> >>
> >> If we still need spinlock, I think we don't need to bother atomic_t at all.
> >
> > Yes, it's one or the other: If you use the cmpxchg loop, you don't need a
> > spinlock and vice versa.
> >
> 
> The cmpxchg loop is for comparing and assigning to 'count', but still 
> there is a printk() above that needs to read 'count'. Combining these 
> two operations means we have to use a spinlock, correct? Because there 
> is a chance that another process could change 'count' in between.

No, you can just print the value of "old" in the above example,
which was atomically read.

	Arnd