Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <b6169317-4714-4240-87ab-3e80b2aac2c4@grimberg.me>
Date:   Wed, 13 Dec 2023 15:40:43 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC] nvmet: configfs: use ctrl->instance to track passthru
 subsystems
Content-Language: en-US
To:     Evan Burgess <evan.burgess@seagate.com>,
        Christoph Hellwig <hch@lst.de>,
        Chaitanya Kulkarni <kch@nvidia.com>
Cc:     "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <SN7PR20MB6609903F74075383EC400BFC808FA@SN7PR20MB6609.namprd20.prod.outlook.com>
From:   Sagi Grimberg <sagi@grimberg.me>
In-Reply-To: <SN7PR20MB6609903F74075383EC400BFC808FA@SN7PR20MB6609.namprd20.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


> To prevent enabling more than one passthrough subsystem per NVMe
> controller, passthru.c maintains an xarray indexed by cntlid values.
> An nvmet subsystem cannot enable passthrough via configfs if its
> passthru_ctrl->cntlid value is already accounted for by the xarray.
> 
> However, according to the NVMe base spec (rev 2.0c, p.145), "The
> Controller ID (CNTLID) value returned in the Identify Controller data
> structure may be used to uniquely identify a controller within an NVM
> subsystem," meaning that cntlid values are not guaranteed to be
> globally unique across multiple subsystems. Instead, the cntlid only
> uniquely identifies multiple controllers _within_ a subsystem.
> 
> As a result, multiple unique & valid NVMe targets can be blocked from
> enabling passthrough at the same time if their controllers share cntlid
> values, a behavior (seemingly) allowed by the spec. This could be
> remedied by indexing the xarray with passthru_ctrl->instance values,
> which are allocated per controller by IDA and thus should act as truly
> unique controller identifiers.
> 
> I have seen this issue in practice, but have found a suspicious lack of
> corroboration across this ML and elsewhere (so far). So, I am not
> discounting a possible misunderstanding of the spec and/or code here.

You are correct AFAICT.
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>