Re: [PATCH v4 3/5] nvme: refactor nvme_submit_user_cmd()

[Jens Axboe <axboe@kernel.dk>]

On 5/5/22 1:03 PM, Jens Axboe wrote:
> On 5/5/22 12:37 PM, Clay Mayers wrote:
>>> From: Kanchan Joshi
>>> Sent: Wednesday, May 4, 2022 11:06 PM
>>> ---
>>
>>>  drivers/nvme/host/ioctl.c | 47 ++++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 42 insertions(+), 5 deletions(-)
>>>
>>> +static int nvme_execute_user_rq(struct request *req, void __user
>>> *meta_buffer,
>>> +		unsigned meta_len, u64 *result)
>>> +{
>>> +	struct bio *bio = req->bio;
>>> +	bool write = bio_op(bio) == REQ_OP_DRV_OUT;
>>
>> I'm getting a NULL ptr access on the first ioctl(NVME_IOCTL_ADMIN64_CMD)
>> I send - it has no ubuffer so I think there's no req->bio.
> 
> Does this work?

This might be better, though you'd only notice if you had integrity
enabled. Christoph, I'm folding this in with patch 3...


diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 8fe7ad18a709..3d827789b536 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -21,9 +21,13 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
 
 static inline void *nvme_meta_from_bio(struct bio *bio)
 {
-	struct bio_integrity_payload *bip = bio_integrity(bio);
+	if (bio) {
+		struct bio_integrity_payload *bip = bio_integrity(bio);
 
-	return bip ? bvec_virt(bip->bip_vec) : NULL;
+		return bip ? bvec_virt(bip->bip_vec) : NULL;
+	}
+
+	return NULL;
 }
 
 /*
@@ -205,19 +209,20 @@ static int nvme_execute_user_rq(struct request *req, void __user *meta_buffer,
 		unsigned meta_len, u64 *result)
 {
 	struct bio *bio = req->bio;
-	bool write = bio_op(bio) == REQ_OP_DRV_OUT;
-	int ret;
 	void *meta = nvme_meta_from_bio(bio);
+	int ret;
 
 	ret = nvme_execute_passthru_rq(req);
 
 	if (result)
 		*result = le64_to_cpu(nvme_req(req)->result.u64);
-	if (meta && !ret && !write) {
-		if (copy_to_user(meta_buffer, meta, meta_len))
+	if (meta) {
+		bool write = bio_op(bio) == REQ_OP_DRV_OUT;
+
+		if (!ret && !write && copy_to_user(meta_buffer, meta, meta_len))
 			ret = -EFAULT;
+		kfree(meta);
 	}
-	kfree(meta);
 	if (bio)
 		blk_rq_unmap_user(bio);
 	blk_mq_free_request(req);

-- 
Jens Axboe