All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
@ 2016-08-30 18:35 Nicholas Krause
  2016-08-31  7:54   ` Catalin Marinas
  0 siblings, 1 reply; 13+ messages in thread
From: Nicholas Krause @ 2016-08-30 18:35 UTC (permalink / raw)
  To: catalin.marinas; +Cc: linux-mm, linux-kernel

This fixes a issue in the current locking logic of the function,
__delete_object where we are trying to attempt to lock the passed
object structure's spinlock again after being previously held
elsewhere by the kmemleak code. Fix this by instead of assuming
we are the only one contending for the object's lock their are
possible other users and create two branches, one where we get
the lock when calling spin_trylock_irqsave on the object's lock
and the other when the lock is held else where by kmemleak.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 mm/kmemleak.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 086292f..ad4828f 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -631,12 +631,19 @@ static void __delete_object(struct kmemleak_object *object)
 
 	/*
 	 * Locking here also ensures that the corresponding memory block
-	 * cannot be freed when it is being scanned.
+	 * cannot be freed when it is being scanned. Further more the
+	 * object's lock may have been previously holded by another holder
+	 * in the kmemleak code, therefore attempt to lock the object's lock
+	 * before holding it and unlocking it.
 	 */
-	spin_lock_irqsave(&object->lock, flags);
-	object->flags &= ~OBJECT_ALLOCATED;
-	spin_unlock_irqrestore(&object->lock, flags);
-	put_object(object);
+	if (spin_trylock_irqsave(&object->lock, flags)) {
+		object->flags &= ~OBJECT_ALLOCATED;
+		spin_unlock_irqrestore(&object->lock, flags);
+		put_object(object);
+	} else {
+		object->flags &= ~OBJECT_ALLOCATED;
+		put_object(object);
+	}
 }
 
 /*
-- 
2.7.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
  2016-08-30 18:35 [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object Nicholas Krause
@ 2016-08-31  7:54   ` Catalin Marinas
  0 siblings, 0 replies; 13+ messages in thread
From: Catalin Marinas @ 2016-08-31  7:54 UTC (permalink / raw)
  To: Nicholas Krause; +Cc: linux-mm, linux-kernel

On Tue, Aug 30, 2016 at 02:35:12PM -0400, Nicholas Krause wrote:
> This fixes a issue in the current locking logic of the function,
> __delete_object where we are trying to attempt to lock the passed
> object structure's spinlock again after being previously held
> elsewhere by the kmemleak code. Fix this by instead of assuming
> we are the only one contending for the object's lock their are
> possible other users and create two branches, one where we get
> the lock when calling spin_trylock_irqsave on the object's lock
> and the other when the lock is held else where by kmemleak.

Have you actually got a deadlock that requires this fix?

> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -631,12 +631,19 @@ static void __delete_object(struct kmemleak_object *object)
>  
>  	/*
>  	 * Locking here also ensures that the corresponding memory block
> -	 * cannot be freed when it is being scanned.
> +	 * cannot be freed when it is being scanned. Further more the
> +	 * object's lock may have been previously holded by another holder
> +	 * in the kmemleak code, therefore attempt to lock the object's lock
> +	 * before holding it and unlocking it.
>  	 */
> -	spin_lock_irqsave(&object->lock, flags);
> -	object->flags &= ~OBJECT_ALLOCATED;
> -	spin_unlock_irqrestore(&object->lock, flags);
> -	put_object(object);
> +	if (spin_trylock_irqsave(&object->lock, flags)) {
> +		object->flags &= ~OBJECT_ALLOCATED;
> +		spin_unlock_irqrestore(&object->lock, flags);
> +		put_object(object);
> +	} else {
> +		object->flags &= ~OBJECT_ALLOCATED;
> +		put_object(object);
> +	}

NAK. This lock here is needed, as described in the comment, to prevent
an object being freed while it is being scanned. The scan_object()
function acquires the same lock and checks for OBJECT_ALLOCATED before
accessing the memory (which could be vmalloc'ed for example, so freeing
would cause a page fault).

-- 
Catalin

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
@ 2016-08-31  7:54   ` Catalin Marinas
  0 siblings, 0 replies; 13+ messages in thread
From: Catalin Marinas @ 2016-08-31  7:54 UTC (permalink / raw)
  To: Nicholas Krause; +Cc: linux-mm, linux-kernel

On Tue, Aug 30, 2016 at 02:35:12PM -0400, Nicholas Krause wrote:
> This fixes a issue in the current locking logic of the function,
> __delete_object where we are trying to attempt to lock the passed
> object structure's spinlock again after being previously held
> elsewhere by the kmemleak code. Fix this by instead of assuming
> we are the only one contending for the object's lock their are
> possible other users and create two branches, one where we get
> the lock when calling spin_trylock_irqsave on the object's lock
> and the other when the lock is held else where by kmemleak.

Have you actually got a deadlock that requires this fix?

> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -631,12 +631,19 @@ static void __delete_object(struct kmemleak_object *object)
>  
>  	/*
>  	 * Locking here also ensures that the corresponding memory block
> -	 * cannot be freed when it is being scanned.
> +	 * cannot be freed when it is being scanned. Further more the
> +	 * object's lock may have been previously holded by another holder
> +	 * in the kmemleak code, therefore attempt to lock the object's lock
> +	 * before holding it and unlocking it.
>  	 */
> -	spin_lock_irqsave(&object->lock, flags);
> -	object->flags &= ~OBJECT_ALLOCATED;
> -	spin_unlock_irqrestore(&object->lock, flags);
> -	put_object(object);
> +	if (spin_trylock_irqsave(&object->lock, flags)) {
> +		object->flags &= ~OBJECT_ALLOCATED;
> +		spin_unlock_irqrestore(&object->lock, flags);
> +		put_object(object);
> +	} else {
> +		object->flags &= ~OBJECT_ALLOCATED;
> +		put_object(object);
> +	}

NAK. This lock here is needed, as described in the comment, to prevent
an object being freed while it is being scanned. The scan_object()
function acquires the same lock and checks for OBJECT_ALLOCATED before
accessing the memory (which could be vmalloc'ed for example, so freeing
would cause a page fault).

-- 
Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
  2016-08-31  7:54   ` Catalin Marinas
  (?)
@ 2016-08-31 13:24   ` nick
  2016-09-07  0:45     ` Rik van Riel
  -1 siblings, 1 reply; 13+ messages in thread
From: nick @ 2016-08-31 13:24 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: linux-mm, linux-kernel



On 2016-08-31 03:54 AM, Catalin Marinas wrote:
> On Tue, Aug 30, 2016 at 02:35:12PM -0400, Nicholas Krause wrote:
>> This fixes a issue in the current locking logic of the function,
>> __delete_object where we are trying to attempt to lock the passed
>> object structure's spinlock again after being previously held
>> elsewhere by the kmemleak code. Fix this by instead of assuming
>> we are the only one contending for the object's lock their are
>> possible other users and create two branches, one where we get
>> the lock when calling spin_trylock_irqsave on the object's lock
>> and the other when the lock is held else where by kmemleak.
> 
> Have you actually got a deadlock that requires this fix?
> 
Yes I have got a deadlock that this does fix.
Nick
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -631,12 +631,19 @@ static void __delete_object(struct kmemleak_object *object)
>>  
>>  	/*
>>  	 * Locking here also ensures that the corresponding memory block
>> -	 * cannot be freed when it is being scanned.
>> +	 * cannot be freed when it is being scanned. Further more the
>> +	 * object's lock may have been previously holded by another holder
>> +	 * in the kmemleak code, therefore attempt to lock the object's lock
>> +	 * before holding it and unlocking it.
>>  	 */
>> -	spin_lock_irqsave(&object->lock, flags);
>> -	object->flags &= ~OBJECT_ALLOCATED;
>> -	spin_unlock_irqrestore(&object->lock, flags);
>> -	put_object(object);
>> +	if (spin_trylock_irqsave(&object->lock, flags)) {
>> +		object->flags &= ~OBJECT_ALLOCATED;
>> +		spin_unlock_irqrestore(&object->lock, flags);
>> +		put_object(object);
>> +	} else {
>> +		object->flags &= ~OBJECT_ALLOCATED;
>> +		put_object(object);
>> +	}
> 
> NAK. This lock here is needed, as described in the comment, to prevent
> an object being freed while it is being scanned. The scan_object()
> function acquires the same lock and checks for OBJECT_ALLOCATED before
> accessing the memory (which could be vmalloc'ed for example, so freeing
> would cause a page fault).
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
  2016-08-31  7:54   ` Catalin Marinas
  (?)
  (?)
@ 2016-08-31 13:41   ` nick
  2016-08-31 14:35       ` Catalin Marinas
  -1 siblings, 1 reply; 13+ messages in thread
From: nick @ 2016-08-31 13:41 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: linux-mm, linux-kernel



On 2016-08-31 03:54 AM, Catalin Marinas wrote:
> On Tue, Aug 30, 2016 at 02:35:12PM -0400, Nicholas Krause wrote:
>> This fixes a issue in the current locking logic of the function,
>> __delete_object where we are trying to attempt to lock the passed
>> object structure's spinlock again after being previously held
>> elsewhere by the kmemleak code. Fix this by instead of assuming
>> we are the only one contending for the object's lock their are
>> possible other users and create two branches, one where we get
>> the lock when calling spin_trylock_irqsave on the object's lock
>> and the other when the lock is held else where by kmemleak.
> 
> Have you actually got a deadlock that requires this fix?
> 
Yes it fixes when you run this test, https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ipc/msgctl/msgctl10.c.
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -631,12 +631,19 @@ static void __delete_object(struct kmemleak_object *object)
>>  
>>  	/*
>>  	 * Locking here also ensures that the corresponding memory block
>> -	 * cannot be freed when it is being scanned.
>> +	 * cannot be freed when it is being scanned. Further more the
>> +	 * object's lock may have been previously holded by another holder
>> +	 * in the kmemleak code, therefore attempt to lock the object's lock
>> +	 * before holding it and unlocking it.
>>  	 */
>> -	spin_lock_irqsave(&object->lock, flags);
>> -	object->flags &= ~OBJECT_ALLOCATED;
>> -	spin_unlock_irqrestore(&object->lock, flags);
>> -	put_object(object);
>> +	if (spin_trylock_irqsave(&object->lock, flags)) {
>> +		object->flags &= ~OBJECT_ALLOCATED;
>> +		spin_unlock_irqrestore(&object->lock, flags);
>> +		put_object(object);
>> +	} else {
>> +		object->flags &= ~OBJECT_ALLOCATED;
>> +		put_object(object);
>> +	}
> 
> NAK. This lock here is needed, as described in the comment, to prevent
> an object being freed while it is being scanned. The scan_object()
> function acquires the same lock and checks for OBJECT_ALLOCATED before
> accessing the memory (which could be vmalloc'ed for example, so freeing
> would cause a page fault).
> 
That's the issue, right there. Your double locking in scan_object. If you look at the code:
/*
 * Once the object->lock is acquired, the corresponding memory block
          * cannot be freed (the same lock is acquired in delete_object).
*/
That test case exposes that issue in the logic, what happens if we are running this on separate kernel threads what
happens then ... deadlock. If you would like be to put the lock checking elsewhere that's fine but it does cause
a deadlock.
Regards,
Nick  

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
  2016-08-31 13:41   ` nick
@ 2016-08-31 14:35       ` Catalin Marinas
  0 siblings, 0 replies; 13+ messages in thread
From: Catalin Marinas @ 2016-08-31 14:35 UTC (permalink / raw)
  To: nick; +Cc: linux-mm, linux-kernel

On Wed, Aug 31, 2016 at 09:41:23AM -0400, nick wrote:
> On 2016-08-31 03:54 AM, Catalin Marinas wrote:
> > On Tue, Aug 30, 2016 at 02:35:12PM -0400, Nicholas Krause wrote:
> >> This fixes a issue in the current locking logic of the function,
> >> __delete_object where we are trying to attempt to lock the passed
> >> object structure's spinlock again after being previously held
> >> elsewhere by the kmemleak code. Fix this by instead of assuming
> >> we are the only one contending for the object's lock their are
> >> possible other users and create two branches, one where we get
> >> the lock when calling spin_trylock_irqsave on the object's lock
> >> and the other when the lock is held else where by kmemleak.
> > 
> > Have you actually got a deadlock that requires this fix?
> 
> Yes it fixes when you run this test,
> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ipc/msgctl/msgctl10.c.

I haven't read the subject properly, you meant soft lockup, so no
deadlock here. Do you have any kernel message that you can post on the
list, showing the soft lockup?

> >> --- a/mm/kmemleak.c
> >> +++ b/mm/kmemleak.c
> >> @@ -631,12 +631,19 @@ static void __delete_object(struct kmemleak_object *object)
> >>  
> >>  	/*
> >>  	 * Locking here also ensures that the corresponding memory block
> >> -	 * cannot be freed when it is being scanned.
> >> +	 * cannot be freed when it is being scanned. Further more the
> >> +	 * object's lock may have been previously holded by another holder
> >> +	 * in the kmemleak code, therefore attempt to lock the object's lock
> >> +	 * before holding it and unlocking it.
> >>  	 */
> >> -	spin_lock_irqsave(&object->lock, flags);
> >> -	object->flags &= ~OBJECT_ALLOCATED;
> >> -	spin_unlock_irqrestore(&object->lock, flags);
> >> -	put_object(object);
> >> +	if (spin_trylock_irqsave(&object->lock, flags)) {
> >> +		object->flags &= ~OBJECT_ALLOCATED;
> >> +		spin_unlock_irqrestore(&object->lock, flags);
> >> +		put_object(object);
> >> +	} else {
> >> +		object->flags &= ~OBJECT_ALLOCATED;
> >> +		put_object(object);
> >> +	}
> > 
> > NAK. This lock here is needed, as described in the comment, to prevent
> > an object being freed while it is being scanned. The scan_object()
> > function acquires the same lock and checks for OBJECT_ALLOCATED before
> > accessing the memory (which could be vmalloc'ed for example, so freeing
> > would cause a page fault).
> 
> That's the issue, right there. Your double locking in scan_object. If
> you look at the code:
> /*
>  * Once the object->lock is acquired, the corresponding memory block
>           * cannot be freed (the same lock is acquired in delete_object).
> */
> That test case exposes that issue in the logic, what happens if we are
> running this on separate kernel threads what happens then ...
> deadlock.

There cannot be any deadlock since you can't have two threads on the
same CPU trying to acquire this spinlock, nor any dependency on another
lock that I'm aware of. As for the soft lockup, scan_object() in
kmemleak tries to avoid it by releasing this lock periodically and
calling cond_resched().

-- 
Catalin

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
@ 2016-08-31 14:35       ` Catalin Marinas
  0 siblings, 0 replies; 13+ messages in thread
From: Catalin Marinas @ 2016-08-31 14:35 UTC (permalink / raw)
  To: nick; +Cc: linux-mm, linux-kernel

On Wed, Aug 31, 2016 at 09:41:23AM -0400, nick wrote:
> On 2016-08-31 03:54 AM, Catalin Marinas wrote:
> > On Tue, Aug 30, 2016 at 02:35:12PM -0400, Nicholas Krause wrote:
> >> This fixes a issue in the current locking logic of the function,
> >> __delete_object where we are trying to attempt to lock the passed
> >> object structure's spinlock again after being previously held
> >> elsewhere by the kmemleak code. Fix this by instead of assuming
> >> we are the only one contending for the object's lock their are
> >> possible other users and create two branches, one where we get
> >> the lock when calling spin_trylock_irqsave on the object's lock
> >> and the other when the lock is held else where by kmemleak.
> > 
> > Have you actually got a deadlock that requires this fix?
> 
> Yes it fixes when you run this test,
> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ipc/msgctl/msgctl10.c.

I haven't read the subject properly, you meant soft lockup, so no
deadlock here. Do you have any kernel message that you can post on the
list, showing the soft lockup?

> >> --- a/mm/kmemleak.c
> >> +++ b/mm/kmemleak.c
> >> @@ -631,12 +631,19 @@ static void __delete_object(struct kmemleak_object *object)
> >>  
> >>  	/*
> >>  	 * Locking here also ensures that the corresponding memory block
> >> -	 * cannot be freed when it is being scanned.
> >> +	 * cannot be freed when it is being scanned. Further more the
> >> +	 * object's lock may have been previously holded by another holder
> >> +	 * in the kmemleak code, therefore attempt to lock the object's lock
> >> +	 * before holding it and unlocking it.
> >>  	 */
> >> -	spin_lock_irqsave(&object->lock, flags);
> >> -	object->flags &= ~OBJECT_ALLOCATED;
> >> -	spin_unlock_irqrestore(&object->lock, flags);
> >> -	put_object(object);
> >> +	if (spin_trylock_irqsave(&object->lock, flags)) {
> >> +		object->flags &= ~OBJECT_ALLOCATED;
> >> +		spin_unlock_irqrestore(&object->lock, flags);
> >> +		put_object(object);
> >> +	} else {
> >> +		object->flags &= ~OBJECT_ALLOCATED;
> >> +		put_object(object);
> >> +	}
> > 
> > NAK. This lock here is needed, as described in the comment, to prevent
> > an object being freed while it is being scanned. The scan_object()
> > function acquires the same lock and checks for OBJECT_ALLOCATED before
> > accessing the memory (which could be vmalloc'ed for example, so freeing
> > would cause a page fault).
> 
> That's the issue, right there. Your double locking in scan_object. If
> you look at the code:
> /*
>  * Once the object->lock is acquired, the corresponding memory block
>           * cannot be freed (the same lock is acquired in delete_object).
> */
> That test case exposes that issue in the logic, what happens if we are
> running this on separate kernel threads what happens then ...
> deadlock.

There cannot be any deadlock since you can't have two threads on the
same CPU trying to acquire this spinlock, nor any dependency on another
lock that I'm aware of. As for the soft lockup, scan_object() in
kmemleak tries to avoid it by releasing this lock periodically and
calling cond_resched().

-- 
Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
  2016-08-31  7:54   ` Catalin Marinas
                     ` (2 preceding siblings ...)
  (?)
@ 2016-08-31 21:08   ` Valdis.Kletnieks
  2016-08-31 21:28     ` nick
  -1 siblings, 1 reply; 13+ messages in thread
From: Valdis.Kletnieks @ 2016-08-31 21:08 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: Nicholas Krause, linux-mm, linux-kernel

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

On Wed, 31 Aug 2016 08:54:21 +0100, Catalin Marinas said:
> On Tue, Aug 30, 2016 at 02:35:12PM -0400, Nicholas Krause wrote:
> > This fixes a issue in the current locking logic of the function,
> > __delete_object where we are trying to attempt to lock the passed
> > object structure's spinlock again after being previously held
> > elsewhere by the kmemleak code. Fix this by instead of assuming
> > we are the only one contending for the object's lock their are
> > possible other users and create two branches, one where we get
> > the lock when calling spin_trylock_irqsave on the object's lock
> > and the other when the lock is held else where by kmemleak.
>
> Have you actually got a deadlock that requires this fix?

Almost certainly not, but that's never stopped Nicholas before.  He's a well-known
submitter of bad patches, usually totally incorrect, not even compile tested.

He's infamous enough that he's not allowed to post to any list hosted at vger.

[-- Attachment #2: Type: application/pgp-signature, Size: 830 bytes --]

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
  2016-08-31 21:08   ` Valdis.Kletnieks
@ 2016-08-31 21:28     ` nick
  2016-09-07  0:51       ` Rik van Riel
  0 siblings, 1 reply; 13+ messages in thread
From: nick @ 2016-08-31 21:28 UTC (permalink / raw)
  To: Valdis.Kletnieks, Catalin Marinas; +Cc: linux-mm, linux-kernel



On 2016-08-31 05:08 PM, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 31 Aug 2016 08:54:21 +0100, Catalin Marinas said:
>> On Tue, Aug 30, 2016 at 02:35:12PM -0400, Nicholas Krause wrote:
>>> This fixes a issue in the current locking logic of the function,
>>> __delete_object where we are trying to attempt to lock the passed
>>> object structure's spinlock again after being previously held
>>> elsewhere by the kmemleak code. Fix this by instead of assuming
>>> we are the only one contending for the object's lock their are
>>> possible other users and create two branches, one where we get
>>> the lock when calling spin_trylock_irqsave on the object's lock
>>> and the other when the lock is held else where by kmemleak.
>>
>> Have you actually got a deadlock that requires this fix?
> 
> Almost certainly not, but that's never stopped Nicholas before.  He's a well-known
> submitter of bad patches, usually totally incorrect, not even compile tested.
> 
> He's infamous enough that he's not allowed to post to any list hosted at vger.
>
Valdis,
Rather then argue since that will go nowhere. I am posing actual patches that have been tested on
hardware. Yes I known that is surprising but it's true.

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
  2016-08-31 13:24   ` nick
@ 2016-09-07  0:45     ` Rik van Riel
  0 siblings, 0 replies; 13+ messages in thread
From: Rik van Riel @ 2016-09-07  0:45 UTC (permalink / raw)
  To: nick, Catalin Marinas; +Cc: linux-mm, linux-kernel

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

On Wed, 2016-08-31 at 09:24 -0400, nick wrote:
> 
> On 2016-08-31 03:54 AM, Catalin Marinas wrote:
> > On Tue, Aug 30, 2016 at 02:35:12PM -0400, Nicholas Krause wrote:
> > > This fixes a issue in the current locking logic of the function,
> > > __delete_object where we are trying to attempt to lock the passed
> > > object structure's spinlock again after being previously held
> > > elsewhere by the kmemleak code. Fix this by instead of assuming
> > > we are the only one contending for the object's lock their are
> > > possible other users and create two branches, one where we get
> > > the lock when calling spin_trylock_irqsave on the object's lock
> > > and the other when the lock is held else where by kmemleak.
> > 
> > Have you actually got a deadlock that requires this fix?
> > 
> Yes I have got a deadlock that this does fix.

Why don't you share the backtrace with us?

Claiming you have a deadlock, but not sharing
it on the list means nobody can see what the
problem is you are trying to address.

It would be good if every email with a patch
that you post starts with an actual detailed
problem description.

Can you do that?

-- 

All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
  2016-08-31 21:28     ` nick
@ 2016-09-07  0:51       ` Rik van Riel
  2016-09-07  1:12         ` nick
  0 siblings, 1 reply; 13+ messages in thread
From: Rik van Riel @ 2016-09-07  0:51 UTC (permalink / raw)
  To: nick, Valdis.Kletnieks, Catalin Marinas; +Cc: linux-mm, linux-kernel

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

On Wed, 2016-08-31 at 17:28 -0400, nick wrote:
> 
> Rather then argue since that will go nowhere. I am posing actual
> patches that have been tested on
> hardware. 

But not by you, apparently.

The patch below was first posted by somebody else
in 2013: https://lkml.org/lkml/2013/7/11/93

When re-posting somebody else's patch, you need to
preserve their From: and Signed-off-by: headers.

See Documentation/SubmittingPatches for the details
on that.

Pretending that other people's code is your own
is not only very impolite, it also means that
the origin of the code, and permission to distribute
it under the GPL, are in question.

Will you promise to not claim other people's code as
your own?

Otherwise there is another very good reason to refuse
ever accepting code posted by you into the kernel.
We cannot merge code when there is no clear permission
from the actual author to distribute it under the GPL.

> From 719ad39496679523c70c7dda006e6da31d9004b3 Mon Sep 17 00:00:00
> 2001
> From: Nicholas Krause <xerofoify@gmail.com>
> Date: Wed, 24 Aug 2016 02:09:39 -0400
> Subject: [PATCH] pciehp: Avoid not bringing up device if already
> existing on
>  bus
> 
> This fixes pcihp_resume to now avoid incorrectly bailing out if the
> device is already live in the pci bus but currently suspended.
> Further
> more this issue is caused by incorrectly checking the status of the
> device adapter directly, instead since this adapter can be shared
> we must instead also check if the pci_bus has no more links to this
> adapter by checking if the pci_bus used by this adapter's device list
> is also empty before enabling it. Finally do the opposite of checking
> that the list is not empty before disabling in order to avoid the
> same issue on disabling the slot instead.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/pci/hotplug/pciehp_core.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c
> b/drivers/pci/hotplug/pciehp_core.c
> index ac531e6..1ce725e 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -291,7 +291,7 @@ static int pciehp_resume(struct pcie_device *dev)
>  	struct controller *ctrl;
>  	struct slot *slot;
>  	u8 status;
> -
> +	struct pci_bus *pbus = dev->port->subordinate;
>  	ctrl = get_service_data(dev);
>  
>  	/* reinitialize the chipset's event detection logic */
> @@ -302,10 +302,12 @@ static int pciehp_resume(struct pcie_device
> *dev)
>  	/* Check if slot is occupied */
>  	pciehp_get_adapter_status(slot, &status);
>  	mutex_lock(&slot->hotplug_lock);
> -	if (status)
> -		pciehp_enable_slot(slot);
> -	else
> +	if (status) {
> +		if (list_empty(&pbus->devices))
> +			pciehp_enable_slot(slot);
> +	} else if (!list_empty(&pbus->devices))
>  		pciehp_disable_slot(slot);
> +
>  	mutex_unlock(&slot->hotplug_lock);
>  	return 0;
>  }
> 
-- 

All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
  2016-09-07  0:51       ` Rik van Riel
@ 2016-09-07  1:12         ` nick
  2016-09-07  1:22           ` Rik van Riel
  0 siblings, 1 reply; 13+ messages in thread
From: nick @ 2016-09-07  1:12 UTC (permalink / raw)
  To: Rik van Riel, Valdis.Kletnieks, Catalin Marinas; +Cc: linux-mm, linux-kernel



On 2016-09-06 08:51 PM, Rik van Riel wrote:
> On Wed, 2016-08-31 at 17:28 -0400, nick wrote:
>>  
>> Rather then argue since that will go nowhere. I am posing actual
>> patches that have been tested on
>> hardware. 
> 
> But not by you, apparently.
> 
> The patch below was first posted by somebody else
> in 2013: https://lkml.org/lkml/2013/7/11/93
> 
> When re-posting somebody else's patch, you need to
> preserve their From: and Signed-off-by: headers.
> 
> See Documentation/SubmittingPatches for the details
> on that.
> 
> Pretending that other people's code is your own
> is not only very impolite, it also means that
> the origin of the code, and permission to distribute
> it under the GPL, are in question.
> 
> Will you promise to not claim other people's code as
> your own?
> 
I wasn't aware of that. Seems it was fixed before I got to 
it but was never merged. Next time I will double check if the
patch work is already out there. Also have this patch but the
commit message needs to be reworked:

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

* Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object
  2016-09-07  1:12         ` nick
@ 2016-09-07  1:22           ` Rik van Riel
  0 siblings, 0 replies; 13+ messages in thread
From: Rik van Riel @ 2016-09-07  1:22 UTC (permalink / raw)
  To: nick, Valdis.Kletnieks, Catalin Marinas; +Cc: linux-mm, linux-kernel

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

On Tue, 2016-09-06 at 21:12 -0400, nick wrote:
> 
> On 2016-09-06 08:51 PM, Rik van Riel wrote:
> > On Wed, 2016-08-31 at 17:28 -0400, nick wrote:
> > >  
> > > Rather then argue since that will go nowhere. I am posing actual
> > > patches that have been tested on
> > > hardware. 
> > 
> > But not by you, apparently.
> > 
> > The patch below was first posted by somebody else
> > in 2013: https://lkml.org/lkml/2013/7/11/93
> > 
> > When re-posting somebody else's patch, you need to
> > preserve their From: and Signed-off-by: headers.
> > 
> > See Documentation/SubmittingPatches for the details
> > on that.
> > 
> > Pretending that other people's code is your own
> > is not only very impolite, it also means that
> > the origin of the code, and permission to distribute
> > it under the GPL, are in question.
> > 
> > Will you promise to not claim other people's code as
> > your own?
> > 
> I wasn't aware of that. Seems it was fixed before I got to 
> it but was never merged. Next time I will double check if the
> patch work is already out there. Also have this patch but the
> commit message needs to be reworked:

Can you tell us what hardware you tested this
patch on?

What kind of system did you plug the ninja32
controller into?

> From: Nicholas Krause <xerofoify@gmail.com>
> Date: Wed, 31 Aug 2016 17:20:10 -0400
> Subject: [PATCH] ata:Fix incorrect function call ordering in
>  pata_ninja32_init_one
> 
> This fixes a incorrect function call ordering making cards using
> this driver not being able to be read or written to due to the
> incorrect calling of pci_set_master before other parts of the
> card are registered before the pci master bus should be registered.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/ata/pata_ninja32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_ninja32.c b/drivers/ata/pata_ninja32.c
> index 44f97ad..89320c9 100644
> --- a/drivers/ata/pata_ninja32.c
> +++ b/drivers/ata/pata_ninja32.c

-- 

All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

end of thread, other threads:[~2016-09-07  1:22 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-30 18:35 [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object Nicholas Krause
2016-08-31  7:54 ` Catalin Marinas
2016-08-31  7:54   ` Catalin Marinas
2016-08-31 13:24   ` nick
2016-09-07  0:45     ` Rik van Riel
2016-08-31 13:41   ` nick
2016-08-31 14:35     ` Catalin Marinas
2016-08-31 14:35       ` Catalin Marinas
2016-08-31 21:08   ` Valdis.Kletnieks
2016-08-31 21:28     ` nick
2016-09-07  0:51       ` Rik van Riel
2016-09-07  1:12         ` nick
2016-09-07  1:22           ` Rik van Riel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.