Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 29 Jul 2020 16:32:46 -0700 (PDT)
From:   David Rientjes <rientjes@google.com>
To:     "Zhang, Qiang" <Qiang.Zhang@windriver.com>
cc:     "cl@linux.com" <cl@linux.com>,
        "penberg@kernel.org" <penberg@kernel.org>,
        "iamjoonsoo.kim@lge.com" <iamjoonsoo.kim@lge.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: =?UTF-8?Q?Re=3A_=E5=9B=9E=E5=A4=8D=3A_=5BPATCH=5D_mm=2Fslab=2Ec=3A?=
 =?UTF-8?Q?_add_node_spinlock_protect_in_=5F=5Fcache=5Ffree=5F?=
 =?UTF-8?Q?alien?=
In-Reply-To: <BYAPR11MB2632BEC45DC867E80F841B82FF700@BYAPR11MB2632.namprd11.prod.outlook.com>
Message-ID: <alpine.DEB.2.23.453.2007291631040.3120473@chino.kir.corp.google.com>
References: <20200728095551.30634-1-qiang.zhang@windriver.com>,<alpine.DEB.2.23.453.2007281238580.2664959@chino.kir.corp.google.com> <BYAPR11MB2632BEC45DC867E80F841B82FF700@BYAPR11MB2632.namprd11.prod.outlook.com>
User-Agent: Alpine 2.23 (DEB 453 2020-06-18)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 29 Jul 2020, Zhang, Qiang wrote:

> > From: Zhang Qiang <qiang.zhang@windriver.com>
> >
> > We should add node spinlock protect "n->alien" which may be
> > assigned to NULL in cpuup_canceled func. cause address access
> > exception.
> >
> 
> >Hi, do you have an example NULL pointer dereference where you have hit
> >this?
> 

If you have a NULL pointer dereference or a GPF that occurred because of 
this, it would be helpful to provide as rationale.

> >This rather looks like something to fix up in cpuup_canceled() since it's
> >currently manipulating the alien cache for the canceled cpu's node.
> 
> yes , it is fix up in cpuup_canceled  it's
> currently manipulating the alien cache for the canceled cpu's node which  may be the same as the node being operated on in the __cache_free_alien func.
> 
> void cpuup_canceled
> {
>         n = get_node(cachep, node);
>         spin_lock_irq(&n->list_lock);
>         ...
>         n->alien = NULL;
>         spin_unlock_irq(&n->list_lock);
>          ....
> }
> 

Right, so the idea is that this should be fixed in cpuup_canceled() 
instead -- why would we invaliate the entire node's alien cache because a 
single cpu failed to come online?