Re: [PATCH v4] nvme: fix corruption for passthrough meta/data

[Kanchan Joshi <joshi.k@xxxxxxxxxxx>]

On 10/13/2023 3:44 PM, Kanchan Joshi wrote:
> On 10/13/2023 10:56 AM, Christoph Hellwig wrote:
>> On Fri, Oct 13, 2023 at 10:44:58AM +0530, Kanchan Joshi wrote:
>>> Changes since v3:
>>> - Block only unprivileged user
>>
>> That's not really what at least I had in mind.  I'd much rather
>> completely disable unprivileged passthrough for now as an easy
>> backportable patch.  And then only re-enable it later in a way
>> where it does require using SGLs for all data transfers.
>>
> 
> I did not get how forcing SGLs can solve the issue at hand.
> The problem happened because (i) user specified short buffer/len, and
> (ii) kernel allocated buffer. Whether the buffer is fed to device using
> PRP or SGL does not seem to solve the large DMA problem.
> 

FWIW, this is the test-patch I wrote to force passthrough to use SGL.

---
  drivers/nvme/host/ioctl.c | 2 ++
  drivers/nvme/host/nvme.h  | 1 +
  drivers/nvme/host/pci.c   | 8 +++++---
  3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index d8ff796fd5f2..508a813b349e 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -202,6 +202,8 @@ static int nvme_map_user_request(struct request 
*req, u64 ubuffer,
                 }
                 *metap = meta;
         }
+       /* force sgl for data transfer */
+       nvme_req(req)->flags |= NVME_REQ_FORCE_SGL;

         return ret;

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index f35647c470af..9fe91d25cfdd 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -184,6 +184,7 @@ enum {
         NVME_REQ_CANCELLED              = (1 << 0),
         NVME_REQ_USERCMD                = (1 << 1),
         NVME_MPATH_IO_STATS             = (1 << 2),
+       NVME_REQ_FORCE_SGL              = (1 << 3),
  };

  static inline struct nvme_request *nvme_req(struct request *req)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 60a08dfe8d75..e28d3b7b14ef 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -772,18 +772,20 @@ static blk_status_t nvme_map_data(struct nvme_dev 
*dev, struct request *req,
         struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
         blk_status_t ret = BLK_STS_RESOURCE;
         int rc;
+       bool force_sgl = nvme_req(req)->flags & NVME_REQ_FORCE_SGL;

         if (blk_rq_nr_phys_segments(req) == 1) {
                 struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
                 struct bio_vec bv = req_bvec(req);

                 if (!is_pci_p2pdma_page(bv.bv_page)) {
-                       if (bv.bv_offset + bv.bv_len <= 
NVME_CTRL_PAGE_SIZE * 2)
+                       if (!force_sgl &&
+                           bv.bv_offset + bv.bv_len <= 
NVME_CTRL_PAGE_SIZE * 2)
                                 return nvme_setup_prp_simple(dev, req,
                                                              &cmnd->rw, 
&bv);

-                       if (nvmeq->qid && sgl_threshold &&
-                           nvme_ctrl_sgl_supported(&dev->ctrl))
+                       if (nvmeq->qid && 
nvme_ctrl_sgl_supported(&dev->ctrl)
+                           && (sgl_threshold || force_sgl))
                                 return nvme_setup_sgl_simple(dev, req,
                                                              &cmnd->rw, 
&bv);
                 }