* [PATCH] nvme-pci: Avoid leak if pci_p2pmem_virt_to_bus() returns null
@ 2019-06-24 23:57 Alan Mikhak
2019-06-25 7:08 ` Christoph Hellwig
2019-06-25 17:10 ` Heitke, Kenneth
0 siblings, 2 replies; 5+ messages in thread
From: Alan Mikhak @ 2019-06-24 23:57 UTC (permalink / raw)
To: linux-nvme, linux-kernel, keith.busch, axboe, hch, sagi, palmer,
paul.walmsley
Cc: Alan Mikhak
Modify nvme_alloc_sq_cmds() to call pci_free_p2pmem()
to free the memory it allocated using pci_alloc_p2pmem()
in case pci_p2pmem_virt_to_bus() returns null.
Make sure not to call pci_free_p2pmem() if pci_alloc_p2pmem()
returned null which can happen if CONFIG_PCI_P2PDMA is not
configured.
Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>
---
drivers/nvme/host/pci.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 524d6bd6d095..5dfa067f6506 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1456,11 +1456,15 @@ static int nvme_alloc_sq_cmds(struct nvme_dev *dev, struct nvme_queue *nvmeq,
if (qid && dev->cmb_use_sqes && (dev->cmbsz & NVME_CMBSZ_SQS)) {
nvmeq->sq_cmds = pci_alloc_p2pmem(pdev, SQ_SIZE(depth));
- nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
- nvmeq->sq_cmds);
- if (nvmeq->sq_dma_addr) {
- set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
- return 0;
+ if (nvmeq->sq_cmds) {
+ nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
+ nvmeq->sq_cmds);
+ if (nvmeq->sq_dma_addr) {
+ set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
+ return 0;
+ }
+
+ pci_free_p2pmem(pdev, nvmeq->sq_cmds, SQ_SIZE(depth));
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] nvme-pci: Avoid leak if pci_p2pmem_virt_to_bus() returns null
2019-06-24 23:57 [PATCH] nvme-pci: Avoid leak if pci_p2pmem_virt_to_bus() returns null Alan Mikhak
@ 2019-06-25 7:08 ` Christoph Hellwig
2019-06-25 17:27 ` Alan Mikhak
2019-06-25 17:10 ` Heitke, Kenneth
1 sibling, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2019-06-25 7:08 UTC (permalink / raw)
To: Alan Mikhak
Cc: linux-nvme, linux-kernel, keith.busch, axboe, hch, sagi, palmer,
paul.walmsley
On Mon, Jun 24, 2019 at 04:57:22PM -0700, Alan Mikhak wrote:
> Modify nvme_alloc_sq_cmds() to call pci_free_p2pmem()
> to free the memory it allocated using pci_alloc_p2pmem()
> in case pci_p2pmem_virt_to_bus() returns null.
>
> Make sure not to call pci_free_p2pmem() if pci_alloc_p2pmem()
> returned null which can happen if CONFIG_PCI_P2PDMA is not
> configured.
Can you
>
> Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>
> ---
> drivers/nvme/host/pci.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 524d6bd6d095..5dfa067f6506 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1456,11 +1456,15 @@ static int nvme_alloc_sq_cmds(struct nvme_dev *dev, struct nvme_queue *nvmeq,
>
> if (qid && dev->cmb_use_sqes && (dev->cmbsz & NVME_CMBSZ_SQS)) {
> nvmeq->sq_cmds = pci_alloc_p2pmem(pdev, SQ_SIZE(depth));
> - nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
> - nvmeq->sq_cmds);
> - if (nvmeq->sq_dma_addr) {
> - set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
> - return 0;
> + if (nvmeq->sq_cmds) {
> + nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
> + nvmeq->sq_cmds);
> + if (nvmeq->sq_dma_addr) {
> + set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
> + return 0;
> + }
> +
> + pci_free_p2pmem(pdev, nvmeq->sq_cmds, SQ_SIZE(depth));
pci_p2pmem_virt_to_bus should only fail when
pci_p2pmem_virt_to_bus failed. That being said I think doing the
error check on pci_alloc_p2pmem instead of relying on
pci_p2pmem_virt_to_bus "passing through" the error seems odd, I'd
rather check the pci_alloc_p2pmem return value directly.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] nvme-pci: Avoid leak if pci_p2pmem_virt_to_bus() returns null
2019-06-25 7:08 ` Christoph Hellwig
@ 2019-06-25 17:27 ` Alan Mikhak
0 siblings, 0 replies; 5+ messages in thread
From: Alan Mikhak @ 2019-06-25 17:27 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvme, linux-kernel, keith.busch, axboe, sagi,
Palmer Dabbelt, Paul Walmsley
On Tue, Jun 25, 2019 at 12:09 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Jun 24, 2019 at 04:57:22PM -0700, Alan Mikhak wrote:
> > Modify nvme_alloc_sq_cmds() to call pci_free_p2pmem()
> > to free the memory it allocated using pci_alloc_p2pmem()
> > in case pci_p2pmem_virt_to_bus() returns null.
> >
> > Make sure not to call pci_free_p2pmem() if pci_alloc_p2pmem()
> > returned null which can happen if CONFIG_PCI_P2PDMA is not
> > configured.
>
> Can you
>
I mean this patch makes sure not to call pci_free_p2pmem() if nothing
was allocated by pci_alloc_p2pmem().
> >
> > Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>
> > ---
> > drivers/nvme/host/pci.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > index 524d6bd6d095..5dfa067f6506 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -1456,11 +1456,15 @@ static int nvme_alloc_sq_cmds(struct nvme_dev *dev, struct nvme_queue *nvmeq,
> >
> > if (qid && dev->cmb_use_sqes && (dev->cmbsz & NVME_CMBSZ_SQS)) {
> > nvmeq->sq_cmds = pci_alloc_p2pmem(pdev, SQ_SIZE(depth));
> > - nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
> > - nvmeq->sq_cmds);
> > - if (nvmeq->sq_dma_addr) {
> > - set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
> > - return 0;
> > + if (nvmeq->sq_cmds) {
> > + nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
> > + nvmeq->sq_cmds);
> > + if (nvmeq->sq_dma_addr) {
> > + set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
> > + return 0;
> > + }
> > +
> > + pci_free_p2pmem(pdev, nvmeq->sq_cmds, SQ_SIZE(depth));
>
> pci_p2pmem_virt_to_bus should only fail when
> pci_p2pmem_virt_to_bus failed. That being said I think doing the
> error check on pci_alloc_p2pmem instead of relying on
> pci_p2pmem_virt_to_bus "passing through" the error seems odd, I'd
> rather check the pci_alloc_p2pmem return value directly.
Thanks Christoph. I could see the existing code should not leak but only after
inspecting pci_p2pmem_virt_to_bus() and gen_pool_virt_to_phys(). I wondered
what if these functions changed and broke the relation but that seems unlikely.
Checking the return value directly may require less from the reader,
if that's a good
outcome.
Alan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] nvme-pci: Avoid leak if pci_p2pmem_virt_to_bus() returns null
2019-06-24 23:57 [PATCH] nvme-pci: Avoid leak if pci_p2pmem_virt_to_bus() returns null Alan Mikhak
2019-06-25 7:08 ` Christoph Hellwig
@ 2019-06-25 17:10 ` Heitke, Kenneth
2019-06-25 17:37 ` Alan Mikhak
1 sibling, 1 reply; 5+ messages in thread
From: Heitke, Kenneth @ 2019-06-25 17:10 UTC (permalink / raw)
To: Alan Mikhak, linux-nvme, linux-kernel, keith.busch, axboe, hch,
sagi, palmer, paul.walmsley
On 6/24/2019 5:57 PM, Alan Mikhak wrote:
> Modify nvme_alloc_sq_cmds() to call pci_free_p2pmem()
> to free the memory it allocated using pci_alloc_p2pmem()
> in case pci_p2pmem_virt_to_bus() returns null.
>
> Make sure not to call pci_free_p2pmem() if pci_alloc_p2pmem()
> returned null which can happen if CONFIG_PCI_P2PDMA is not
> configured.
>
> Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>
> ---
> drivers/nvme/host/pci.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 524d6bd6d095..5dfa067f6506 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1456,11 +1456,15 @@ static int nvme_alloc_sq_cmds(struct nvme_dev *dev, struct nvme_queue *nvmeq,
>
> if (qid && dev->cmb_use_sqes && (dev->cmbsz & NVME_CMBSZ_SQS)) {
> nvmeq->sq_cmds = pci_alloc_p2pmem(pdev, SQ_SIZE(depth));
> - nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
> - nvmeq->sq_cmds);
> - if (nvmeq->sq_dma_addr) {
> - set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
> - return 0;
> + if (nvmeq->sq_cmds) {
> + nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
> + nvmeq->sq_cmds);
> + if (nvmeq->sq_dma_addr) {
> + set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
> + return 0;
> + }
> +
> + pci_free_p2pmem(pdev, nvmeq->sq_cmds, SQ_SIZE(depth));
Should the pointer be set to NULL here, just in case?
> }
> }
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] nvme-pci: Avoid leak if pci_p2pmem_virt_to_bus() returns null
2019-06-25 17:10 ` Heitke, Kenneth
@ 2019-06-25 17:37 ` Alan Mikhak
0 siblings, 0 replies; 5+ messages in thread
From: Alan Mikhak @ 2019-06-25 17:37 UTC (permalink / raw)
To: Heitke, Kenneth
Cc: linux-nvme, linux-kernel, keith.busch, axboe, Christoph Hellwig,
sagi, Palmer Dabbelt, Paul Walmsley
On Tue, Jun 25, 2019 at 10:10 AM Heitke, Kenneth
<kenneth.heitke@intel.com> wrote:
>
>
>
> On 6/24/2019 5:57 PM, Alan Mikhak wrote:
> > Modify nvme_alloc_sq_cmds() to call pci_free_p2pmem()
> > to free the memory it allocated using pci_alloc_p2pmem()
> > in case pci_p2pmem_virt_to_bus() returns null.
> >
> > Make sure not to call pci_free_p2pmem() if pci_alloc_p2pmem()
> > returned null which can happen if CONFIG_PCI_P2PDMA is not
> > configured.
> >
> > Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>
> > ---
> > drivers/nvme/host/pci.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > index 524d6bd6d095..5dfa067f6506 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -1456,11 +1456,15 @@ static int nvme_alloc_sq_cmds(struct nvme_dev *dev, struct nvme_queue *nvmeq,
> >
> > if (qid && dev->cmb_use_sqes && (dev->cmbsz & NVME_CMBSZ_SQS)) {
> > nvmeq->sq_cmds = pci_alloc_p2pmem(pdev, SQ_SIZE(depth));
> > - nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
> > - nvmeq->sq_cmds);
> > - if (nvmeq->sq_dma_addr) {
> > - set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
> > - return 0;
> > + if (nvmeq->sq_cmds) {
> > + nvmeq->sq_dma_addr = pci_p2pmem_virt_to_bus(pdev,
> > + nvmeq->sq_cmds);
> > + if (nvmeq->sq_dma_addr) {
> > + set_bit(NVMEQ_SQ_CMB, &nvmeq->flags);
> > + return 0;
> > + }
> > +
> > + pci_free_p2pmem(pdev, nvmeq->sq_cmds, SQ_SIZE(depth));
>
> Should the pointer be set to NULL here, just in case?
Thanks Kenneth. The pointer gets immediately reassigned by the return
value of the
code that follows. There is no intervening reference to it between the calls to
pci_free_p2pmem() and dma_alloc_coherent(). It should be safe without
setting it to NULL.
nvmeq->sq_cmds = dma_alloc_coherent(dev->dev, nvmeq->cq_size,
&nvmeq->sq_dma_addr, GFP_KERNEL);
>
> > }
> > }
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-25 17:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-24 23:57 [PATCH] nvme-pci: Avoid leak if pci_p2pmem_virt_to_bus() returns null Alan Mikhak
2019-06-25 7:08 ` Christoph Hellwig
2019-06-25 17:27 ` Alan Mikhak
2019-06-25 17:10 ` Heitke, Kenneth
2019-06-25 17:37 ` Alan Mikhak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox