Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object

Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object

[Catalin Marinas]

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

Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object

[Catalin Marinas]

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

Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object

[Valdis.Kletnieks]

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

Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object

[Rik van Riel]

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

Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object

[Rik van Riel]

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

Re: [PATCH] mm:Avoid soft lockup due to possible attempt of double locking object's lock in __delete_object

[Rik van Riel]

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