From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rashmica Subject: [PATCH RFCv2 3/6] mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock Date: Mon, 3 Sep 2018 10:36:24 +1000 Message-ID: <70372ef5-e332-6c07-f08c-50f8808bde6d@gmail.com> References: <20180821104418.12710-1-david@redhat.com> <20180821104418.12710-4-david@redhat.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7033227392406503336==" Return-path: In-Reply-To: <20180821104418.12710-4-david@redhat.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" To: David Hildenbrand , linux-mm@kvack.org Cc: Kate Stewart , Michal Hocko , linux-doc@vger.kernel.org, Benjamin Herrenschmidt , Balbir Singh , Heiko Carstens , Paul Mackerras , "K. Y. Srinivasan" , Thomas Gleixner , Michael Neuling , Stephen Hemminger , Michael Ellerman , Pavel Tatashin , linux-acpi@vger.kernel.org, xen-devel@lists.xenproject.org, Len Brown , Haiyang Zhang , Dan Williams , YASUAKI ISHIMATSU , Boris Ostrovsky , Vlastimil Babka , Oscar Salvador , Juergen Gross , Math List-Id: linux-acpi@vger.kernel.org This is a multi-part message in MIME format. --===============7033227392406503336== Content-Type: multipart/alternative; boundary="------------1093E43F969F497286F217F7" Content-Language: en-US This is a multi-part message in MIME format. --------------1093E43F969F497286F217F7 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Hi David, On 21/08/18 20:44, David Hildenbrand wrote: > There seem to be some problems as result of 30467e0b3be ("mm, hotplug: > fix concurrent memory hot-add deadlock"), which tried to fix a possible > lock inversion reported and discussed in [1] due to the two locks > a) device_lock() > b) mem_hotplug_lock > > While add_memory() first takes b), followed by a) during > bus_probe_device(), onlining of memory from user space first took b), > followed by a), exposing a possible deadlock. Do you mean "onlining of memory from user space first took a), followed by b)"? > In [1], and it was decided to not make use of device_hotplug_lock, but > rather to enforce a locking order. > > The problems I spotted related to this: > > 1. Memory block device attributes: While .state first calls > mem_hotplug_begin() and the calls device_online() - which takes > device_lock() - .online does no longer call mem_hotplug_begin(), so > effectively calls online_pages() without mem_hotplug_lock. > > 2. device_online() should be called under device_hotplug_lock, however > onlining memory during add_memory() does not take care of that. > > In addition, I think there is also something wrong about the locking in > > 3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages() > without locks. This was introduced after 30467e0b3be. And skimming over > the code, I assume it could need some more care in regards to locking > (e.g. device_online() called without device_hotplug_lock - but I'll > not touch that for now). Can you mention that you fixed this in later patches? The series looks good to me. Feel free to add my reviewed-by: Reviewed-by: Rashmica Gupta --------------1093E43F969F497286F217F7 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit 
Hi David,


On 21/08/18 20:44, David Hildenbrand wrote:
There seem to be some problems as result of 30467e0b3be ("mm, hotplug:
fix concurrent memory hot-add deadlock"), which tried to fix a possible
lock inversion reported and discussed in [1] due to the two locks
	a) device_lock()
	b) mem_hotplug_lock

While add_memory() first takes b), followed by a) during
bus_probe_device(), onlining of memory from user space first took b),
followed by a), exposing a possible deadlock.
Do you mean "onlining of memory from user space first took a),
followed by b)"? 


In [1], and it was decided to not make use of device_hotplug_lock, but
rather to enforce a locking order.

The problems I spotted related to this:

1. Memory block device attributes: While .state first calls
   mem_hotplug_begin() and the calls device_online() - which takes
   device_lock() - .online does no longer call mem_hotplug_begin(), so
   effectively calls online_pages() without mem_hotplug_lock.

2. device_online() should be called under device_hotplug_lock, however
   onlining memory during add_memory() does not take care of that.

In addition, I think there is also something wrong about the locking in

3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages()
   without locks. This was introduced after 30467e0b3be. And skimming over
   the code, I assume it could need some more care in regards to locking
   (e.g. device_online() called without device_hotplug_lock - but I'll
   not touch that for now).
Can you mention that you fixed this in later patches?



The series looks good to me. Feel free to add my reviewed-by:

Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>
--------------1093E43F969F497286F217F7-- --===============7033227392406503336== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVucHJvamVjdC5vcmcKaHR0cHM6Ly9saXN0 cy54ZW5wcm9qZWN0Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL3hlbi1kZXZlbA== --===============7033227392406503336==-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-f198.google.com (mail-pg1-f198.google.com [209.85.215.198]) by kanga.kvack.org (Postfix) with ESMTP id 495DE6B650F for ; Sun, 2 Sep 2018 20:36:40 -0400 (EDT) Received: by mail-pg1-f198.google.com with SMTP id l125-v6so930329pga.1 for ; Sun, 02 Sep 2018 17:36:40 -0700 (PDT) Received: from mail-sor-f41.google.com (mail-sor-f41.google.com. [209.85.220.41]) by mx.google.com with SMTPS id u28-v6sor3804709pgn.114.2018.09.02.17.36.38 for (Google Transport Security); Sun, 02 Sep 2018 17:36:39 -0700 (PDT) From: Rashmica Subject: [PATCH RFCv2 3/6] mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock References: <20180821104418.12710-1-david@redhat.com> <20180821104418.12710-4-david@redhat.com> Message-ID: <70372ef5-e332-6c07-f08c-50f8808bde6d@gmail.com> Date: Mon, 3 Sep 2018 10:36:24 +1000 MIME-Version: 1.0 In-Reply-To: <20180821104418.12710-4-david@redhat.com> Content-Type: multipart/alternative; boundary="------------1093E43F969F497286F217F7" Content-Language: en-US Sender: owner-linux-mm@kvack.org List-ID: To: David Hildenbrand , linux-mm@kvack.org Cc: linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-acpi@vger.kernel.org, xen-devel@lists.xenproject.org, devel@linuxdriverproject.org, Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , "Rafael J. Wysocki" , Len Brown , Greg Kroah-Hartman , "K. Y. Srinivasan" , Haiyang Zhang , Stephen Hemminger , Martin Schwidefsky , Heiko Carstens , Boris Ostrovsky , Juergen Gross , Michael Neuling , Balbir Singh , Kate Stewart , Thomas Gleixner , Philippe Ombredanne , Andrew Morton , Michal Hocko , Pavel Tatashin , Vlastimil Babka , Dan Williams , Oscar Salvador , YASUAKI ISHIMATSU , Mathieu Malaterre This is a multi-part message in MIME format. --------------1093E43F969F497286F217F7 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Hi David, On 21/08/18 20:44, David Hildenbrand wrote: > There seem to be some problems as result of 30467e0b3be ("mm, hotplug: > fix concurrent memory hot-add deadlock"), which tried to fix a possible > lock inversion reported and discussed in [1] due to the two locks > a) device_lock() > b) mem_hotplug_lock > > While add_memory() first takes b), followed by a) during > bus_probe_device(), onlining of memory from user space first took b), > followed by a), exposing a possible deadlock. Do you mean "onlining of memory from user space first took a), followed by b)"? > In [1], and it was decided to not make use of device_hotplug_lock, but > rather to enforce a locking order. > > The problems I spotted related to this: > > 1. Memory block device attributes: While .state first calls > mem_hotplug_begin() and the calls device_online() - which takes > device_lock() - .online does no longer call mem_hotplug_begin(), so > effectively calls online_pages() without mem_hotplug_lock. > > 2. device_online() should be called under device_hotplug_lock, however > onlining memory during add_memory() does not take care of that. > > In addition, I think there is also something wrong about the locking in > > 3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages() > without locks. This was introduced after 30467e0b3be. And skimming over > the code, I assume it could need some more care in regards to locking > (e.g. device_online() called without device_hotplug_lock - but I'll > not touch that for now). Can you mention that you fixed this in later patches? The series looks good to me. Feel free to add my reviewed-by: Reviewed-by: Rashmica Gupta --------------1093E43F969F497286F217F7 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit 
Hi David,


On 21/08/18 20:44, David Hildenbrand wrote:
There seem to be some problems as result of 30467e0b3be ("mm, hotplug:
fix concurrent memory hot-add deadlock"), which tried to fix a possible
lock inversion reported and discussed in [1] due to the two locks
	a) device_lock()
	b) mem_hotplug_lock

While add_memory() first takes b), followed by a) during
bus_probe_device(), onlining of memory from user space first took b),
followed by a), exposing a possible deadlock.
Do you mean "onlining of memory from user space first took a),
followed by b)"? 


In [1], and it was decided to not make use of device_hotplug_lock, but
rather to enforce a locking order.

The problems I spotted related to this:

1. Memory block device attributes: While .state first calls
   mem_hotplug_begin() and the calls device_online() - which takes
   device_lock() - .online does no longer call mem_hotplug_begin(), so
   effectively calls online_pages() without mem_hotplug_lock.

2. device_online() should be called under device_hotplug_lock, however
   onlining memory during add_memory() does not take care of that.

In addition, I think there is also something wrong about the locking in

3. arch/powerpc/platforms/powernv/memtrace.c calls offline_pages()
   without locks. This was introduced after 30467e0b3be. And skimming over
   the code, I assume it could need some more care in regards to locking
   (e.g. device_online() called without device_hotplug_lock - but I'll
   not touch that for now).
Can you mention that you fixed this in later patches?



The series looks good to me. Feel free to add my reviewed-by:

Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>
--------------1093E43F969F497286F217F7--