[PATCH 1/2] NTB: Make _addr functions optional in the API

[Allen Hubbe]

The functions ntb_peer_db_addr and ntb_peer_spad_addr were required by
the api. The functions already support returning an error, so any
existing calling code should already check for it. Any existing code
using drivers that implement the functions will not be affected.

Signed-off-by: Allen Hubbe <Allen...@emc.com>
---
include/linux/ntb.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/ntb.h b/include/linux/ntb.h
index f798e2afba88..6f47562d477b 100644
--- a/include/linux/ntb.h
+++ b/include/linux/ntb.h
@@ -284,7 +284,7 @@ static inline int ntb_dev_ops_is_valid(const struct ntb_dev_ops *ops)
/* ops->db_read_mask && */
ops->db_set_mask &&
ops->db_clear_mask &&
- ops->peer_db_addr &&
+ /* ops->peer_db_addr && */
/* ops->peer_db_read && */
ops->peer_db_set &&
/* ops->peer_db_clear && */
@@ -295,7 +295,7 @@ static inline int ntb_dev_ops_is_valid(const struct ntb_dev_ops *ops)
ops->spad_count &&
ops->spad_read &&
ops->spad_write &&
- ops->peer_spad_addr &&
+ /* ops->peer_spad_addr && */
/* ops->peer_spad_read && */
ops->peer_spad_write &&
1;
@@ -757,6 +757,9 @@ static inline int ntb_peer_db_addr(struct ntb_dev *ntb,
phys_addr_t *db_addr,
resource_size_t *db_size)
{
+ if (!ntb->ops->peer_db_addr)
+ return -EINVAL;
+
return ntb->ops->peer_db_addr(ntb, db_addr, db_size);
}

@@ -948,6 +951,9 @@ static inline int ntb_spad_write(struct ntb_dev *ntb, int idx, u32 val)
static inline int ntb_peer_spad_addr(struct ntb_dev *ntb, int idx,
phys_addr_t *spad_addr)
{
+ if (!ntb->ops->peer_spad_addr)
+ return -EINVAL;
+
return ntb->ops->peer_spad_addr(ntb, idx, spad_addr);
}

--
2.7.0

[Allen Hubbe]

Kernel zero day testing warned about address space confusion. A virtual
iomem address was used where a physical address is expected. The
offending functions implement an optional part of the api, so they are
removed. They can be added later, after testing.

Fixes: a1b3695820aa490e58915d720a1438069813008b

Signed-off-by: Allen Hubbe <Allen...@emc.com>

Cc: Xiangliang Yu <Xiangl...@amd.com>
---
drivers/ntb/hw/amd/ntb_hw_amd.c | 30 ------------------------------
1 file changed, 30 deletions(-)

diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
index 588803ad6847..6ccba0d862df 100644
--- a/drivers/ntb/hw/amd/ntb_hw_amd.c
+++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
@@ -357,20 +357,6 @@ static int amd_ntb_db_clear_mask(struct ntb_dev *ntb, u64 db_bits)
return 0;
}

-static int amd_ntb_peer_db_addr(struct ntb_dev *ntb,
- phys_addr_t *db_addr,
- resource_size_t *db_size)
-{
- struct amd_ntb_dev *ndev = ntb_ndev(ntb);
-
- if (db_addr)
- *db_addr = (phys_addr_t)(ndev->peer_mmio + AMD_DBREQ_OFFSET);
- if (db_size)
- *db_size = sizeof(u32);
-
- return 0;
-}
-
static int amd_ntb_peer_db_set(struct ntb_dev *ntb, u64 db_bits)
{
struct amd_ntb_dev *ndev = ntb_ndev(ntb);
@@ -415,20 +401,6 @@ static int amd_ntb_spad_write(struct ntb_dev *ntb,
return 0;
}

-static int amd_ntb_peer_spad_addr(struct ntb_dev *ntb, int idx,
- phys_addr_t *spad_addr)
-{
- struct amd_ntb_dev *ndev = ntb_ndev(ntb);
-
- if (idx < 0 || idx >= ndev->spad_count)
- return -EINVAL;
-
- if (spad_addr)
- *spad_addr = (phys_addr_t)(ndev->self_mmio + AMD_SPAD_OFFSET +
- ndev->peer_spad + (idx << 2));
- return 0;
-}
-
static u32 amd_ntb_peer_spad_read(struct ntb_dev *ntb, int idx)
{
struct amd_ntb_dev *ndev = ntb_ndev(ntb);
@@ -472,12 +444,10 @@ static const struct ntb_dev_ops amd_ntb_ops = {
.db_clear = amd_ntb_db_clear,
.db_set_mask = amd_ntb_db_set_mask,
.db_clear_mask = amd_ntb_db_clear_mask,
- .peer_db_addr = amd_ntb_peer_db_addr,
.peer_db_set = amd_ntb_peer_db_set,
.spad_count = amd_ntb_spad_count,
.spad_read = amd_ntb_spad_read,
.spad_write = amd_ntb_spad_write,
- .peer_spad_addr = amd_ntb_peer_spad_addr,
.peer_spad_read = amd_ntb_peer_spad_read,
.peer_spad_write = amd_ntb_peer_spad_write,
};
--
2.7.0

[Jon Mason]

On Mon, Mar 21, 2016 at 4:53 AM, Allen Hubbe <Allen...@emc.com> wrote:
> Kernel zero day testing warned about address space confusion. A virtual
> iomem address was used where a physical address is expected. The
> offending functions implement an optional part of the api, so they are
> removed. They can be added later, after testing.
>
> Fixes: a1b3695820aa490e58915d720a1438069813008b
>
> Signed-off-by: Allen Hubbe <Allen...@emc.com>
>
> Cc: Xiangliang Yu <Xiangl...@amd.com>

I don't see anything wrong with this, but I want Xiangliang to ack it.
So unless I hear from Xiangliang in the next 24 hours, I'm going to
push this fix to the next release (v4.7). Is that acceptable?

Thanks,
Jon

> --
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+...@googlegroups.com.
> To post to this group, send email to linu...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/6a2984f3c623f41aae210f08f23813b680a19d56.1458550127.git.Allen.Hubbe%40emc.com.
> For more options, visit https://groups.google.com/d/optout.

[Yu, Xiangliang]

> -----Original Message-----
> From: Allen Hubbe [mailto:Allen...@emc.com]
> Sent: Monday, March 21, 2016 4:53 PM
> To: linu...@googlegroups.com
> Cc: Jon Mason; Dave Jiang; Allen Hubbe; Yu, Xiangliang
> Subject: [PATCH 2/2] NTB: Remove _addr functions from ntb_hw_amd
>
> Kernel zero day testing warned about address space confusion. A virtual
> iomem address was used where a physical address is expected. The
> offending functions implement an optional part of the api, so they are
> removed. They can be added later, after testing.

Ntb_dev_ops_is_valid will check the related implement and AMD ntb driver can't
Register ntb device if you remove the two functions.
I think this addr should be is iomem address not physical address as address space
Is IO space. Do you map IO space again after getting the physical address?
Please help clarify the usage scenarios.

[Allen Hubbe]

> From: Yu, Xiangliang
> > From: Allen Hubbe

> > Kernel zero day testing warned about address space confusion. A virtual
> > iomem address was used where a physical address is expected. The
> > offending functions implement an optional part of the api, so they are
> > removed. They can be added later, after testing.
>
> Ntb_dev_ops_is_valid will check the related implement and AMD ntb driver can't
> Register ntb device if you remove the two functions.

See [PATCH 1/2].

> I think this addr should be is iomem address not physical address as address space
> Is IO space.

The programming interface specifies to return a physical address. Returning a virtual address is incorrect.

> Do you map IO space again after getting the physical address?
> Please help clarify the usage scenarios.

The use case can be applied in the combination of NTB and a DMA engine to offload memory copy operations. The physical address enables an optimization for that use case. The DMA engine operates in the physical address space, without a virtual memory mapping, so there is no need to map the IO space again.

Using the physical address, a dma descriptor can be inserted into the chain of operations to be executed by the dma hardware. The inserted descriptor would be a write of the appropriate bits to the doorbell register. The write will have the effect of ringing the doorbell.

The dma hardware processes the chain of descriptors asynchronously, and notifies the dma driver as descriptors are completed. By inserting the doorbell write operation in the chain, it is executed asynchronously along with other dma operations, and it does not require the round trip and overhead of processing a dma completion to ring the doorbell.

[Yu, Xiangliang]

> -----Original Message-----
> From: Allen Hubbe [mailto:Allen...@emc.com]

> Sent: Tuesday, March 22, 2016 10:01 PM
> To: Yu, Xiangliang; linu...@googlegroups.com
> Cc: 'Jon Mason'; 'Dave Jiang'
> Subject: RE: [PATCH 2/2] NTB: Remove _addr functions from ntb_hw_amd
>
> > From: Yu, Xiangliang
> > > From: Allen Hubbe
> > > Kernel zero day testing warned about address space confusion. A
> > > virtual iomem address was used where a physical address is expected.
> > > The offending functions implement an optional part of the api, so
> > > they are removed. They can be added later, after testing.
> >
> > Ntb_dev_ops_is_valid will check the related implement and AMD ntb
> > driver can't Register ntb device if you remove the two functions.
>
> See [PATCH 1/2].

I haven't received the mail of [PATCH 1/2].

>
> > I think this addr should be is iomem address not physical address as
> > address space Is IO space.
>
> The programming interface specifies to return a physical address. Returning
> a virtual address is incorrect.
>
> > Do you map IO space again after getting the physical address?
> > Please help clarify the usage scenarios.
>
> The use case can be applied in the combination of NTB and a DMA engine to
> offload memory copy operations. The physical address enables an
> optimization for that use case. The DMA engine operates in the physical
> address space, without a virtual memory mapping, so there is no need to
> map the IO space again.
>
> Using the physical address, a dma descriptor can be inserted into the chain of
> operations to be executed by the dma hardware. The inserted descriptor
> would be a write of the appropriate bits to the doorbell register. The write
> will have the effect of ringing the doorbell.
>
> The dma hardware processes the chain of descriptors asynchronously, and
> notifies the dma driver as descriptors are completed. By inserting the
> doorbell write operation in the chain, it is executed asynchronously along
> with other dma operations, and it does not require the round trip and
> overhead of processing a dma completion to ring the doorbell.

Got it.

[Allen Hubbe]

> From: Yu, Xiangliang
> > From: Allen Hubbe
> > > From: Yu, Xiangliang
> > > > From: Allen Hubbe
> > > > Kernel zero day testing warned about address space confusion. A
> > > > virtual iomem address was used where a physical address is expected.
> > > > The offending functions implement an optional part of the api, so
> > > > they are removed. They can be added later, after testing.
> > >
> > > Ntb_dev_ops_is_valid will check the related implement and AMD ntb
> > > driver can't Register ntb device if you remove the two functions.
> >
> > See [PATCH 1/2].
>
> I haven't received the mail of [PATCH 1/2].

I resent it just to you. Are you not subscribed to the linux-ntb mailing list?

Note: this patch 2/2 just takes the _addr functions out of the driver, which is the only change I am comfortable to make without testing. Since I cannot test the driver, I cannot provide any real fix for the functions. Since you have the ability to test, if you provide a fix for these functions, I expect your patch would be accepted instead of this one. If you provide a fix, it would be best to provide that fix assuming that my patch has not been applied, so that your changes for the fix are clear in the patch.

[Yu, Xiangliang]

> -----Original Message-----
> From: Allen Hubbe [mailto:Allen...@emc.com]
> Sent: Wednesday, March 23, 2016 10:01 PM
> To: Yu, Xiangliang; linu...@googlegroups.com
> Cc: 'Jon Mason'; 'Dave Jiang'
> Subject: RE: [PATCH 2/2] NTB: Remove _addr functions from ntb_hw_amd
>

> > From: Yu, Xiangliang
> > > From: Allen Hubbe
> > > > From: Yu, Xiangliang
> > > > > From: Allen Hubbe
> > > > > Kernel zero day testing warned about address space confusion. A
> > > > > virtual iomem address was used where a physical address is expected.
> > > > > The offending functions implement an optional part of the api,
> > > > > so they are removed. They can be added later, after testing.
> > > >
> > > > Ntb_dev_ops_is_valid will check the related implement and AMD ntb
> > > > driver can't Register ntb device if you remove the two functions.
> > >
> > > See [PATCH 1/2].
> >
> > I haven't received the mail of [PATCH 1/2].
>
> I resent it just to you. Are you not subscribed to the linux-ntb mailing list?

Yes, I have seen it.

> Note: this patch 2/2 just takes the _addr functions out of the driver, which is
> the only change I am comfortable to make without testing. Since I cannot
> test the driver, I cannot provide any real fix for the functions. Since you have
> the ability to test, if you provide a fix for these functions, I expect your patch
> would be accepted instead of this one. If you provide a fix, it would be best
> to provide that fix assuming that my patch has not been applied, so that your
> changes for the fix are clear in the patch.

Ok, let's apply it firstly because I also can't test it now.

Acked-by: Xiangliang Yu <Xiangl...@amd.com>

[Jon Mason]

Applied