From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753029AbbJFVoL (ORCPT ); Tue, 6 Oct 2015 17:44:11 -0400 Received: from mx2.suse.de ([195.135.220.15]:39519 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752657AbbJFVoK (ORCPT ); Tue, 6 Oct 2015 17:44:10 -0400 Subject: Re: [PATCHv3 1/1] SCSI: update hosts module to use idr index management To: James Bottomley References: <1444159196.10564.11.camel@HansenPartnership.com> Cc: linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, Tejun Heo , Hannes Reinecke , Johannes Thumshirn , Christoph Hellwig From: Lee Duncan X-Enigmail-Draft-Status: N1110 Organization: SUSE Message-ID: <561440A4.1070704@suse.com> Date: Tue, 6 Oct 2015 14:44:04 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <1444159196.10564.11.camel@HansenPartnership.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/06/2015 12:19 PM, James Bottomley wrote: > On Tue, 2015-10-06 at 12:08 -0700, Lee Duncan wrote: >> Update the SCSI hosts module to use idr to manage >> its host_no index instead of an ATOMIC integer. This >> also allows using idr_find() to look up the SCSI >> host structure given the host number. >> >> This means that the SCSI host number will now >> be reclaimable. >> >> Signed-off-by: Lee Duncan >> Reviewed-by: Hannes Reinecke >> --- >> drivers/scsi/hosts.c | 61 ++++++++++++++++++++++++++-------------------------- >> 1 file changed, 31 insertions(+), 30 deletions(- >> >> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c >> index 8bb173e01084..afe7bd962ddb 100644 >> --- a/drivers/scsi/hosts.c >> +++ b/drivers/scsi/hosts.c > [...] >> + spin_lock(&host_index_lock); >> + shost = idr_find(&host_index_idr, hostnum); >> + spin_unlock(&host_index_lock); >> + >> + return shost ? scsi_host_get(shost) : NULL; > > So the thing I don't like here is that there's a race between > scsi_host_get() and the final put. What could happen is that idr_find() > returns the host just before but scsi_host_dev_release() is executed > before the return. In that instance, we'll reference freed memory in > scsi_host_get() ... probably completely harmlessly, but it will show up > occasionally on some of the traces ... particularly the ones doing a > fuzz/stress test around host create/destroy. > > James Good point. The scenario you mention may be a corner case, but I also don't like it. I cannot see another good way to synchronize lookup and removal other than adding a new lock, which would be dumb. I will submit my "host number" patch again, without the change to scsi_host_lookup(). -- Lee Duncan