Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 12 Oct 2021 06:13:52 -0700
From:   Keith Busch <kbusch@kernel.org>
To:     Christoph Hellwig <hch@lst.de>
Cc:     Jens Axboe <axboe@kernel.dk>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        "J. Bruce Fields" <bfields@fieldses.org>,
        Chuck Lever <chuck.lever@oracle.com>,
        linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
        target-devel@vger.kernel.org, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 2/7] sd: implement ->get_unique_id
Message-ID: <20211012131352.GC635062@dhcp-10-100-145-180.wdc.com>
References: <20211012120445.861860-1-hch@lst.de>
 <20211012120445.861860-3-hch@lst.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211012120445.861860-3-hch@lst.de>
Precedence: bulk

On Tue, Oct 12, 2021 at 02:04:40PM +0200, Christoph Hellwig wrote:
> +static int sd_get_unique_id(struct gendisk *disk, u8 id[16], u8 type)
> +{
> +	struct scsi_device *sdev = scsi_disk(disk)->device;
> +	const struct scsi_vpd *vpd;
> +	const unsigned char *d;
> +	int len = -ENXIO;
> +
> +	rcu_read_lock();
> +	vpd = rcu_dereference(sdev->vpd_pg83);
> +	if (!vpd)
> +		goto out_unlock;
> +
> +	len = -EINVAL;
> +	for (d = vpd->data + 4; d < vpd->data + vpd->len; d += d[3] + 4) {
> +		/* we only care about designators with LU association */
> +		if (((d[1] >> 4) & 0x3) != 0x00)
> +			continue;
> +		if ((d[1] & 0xf) != type)
> +			continue;
> +
> +		/*
> +		 * Only exit early if a 16-byte descriptor was found.  Otherwise
> +		 * keep looking as one with more entropy might still show up.
> +		 */
> +		len = d[3];
> +		if (len != 8 && len != 12 && len != 16)
> +			continue;

I think you need a temporary variable instead of assigning directly to
'len' here. Otherwise, the 'len' returned will be whatever the last
iteration was, which may not be then len that was copied into the 'id'.

> +		memcpy(id, d + 4, len);
> +		if (len == 16)
> +			break;
> +	}
> +out_unlock:
> +	rcu_read_unlock();
> +	return len;
> +}