| Message ID | 1418744596-25251-1-git-send-email-jinpu.wang@profitbricks.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
> If user attach private data for AF_IB, the first byte will > be overwritten, because we always set the cma version no matter > family is AF_IB, so move the version set inside if condition. > > Reported-by: Fabian Holler <fabian.holler@profitbricks.com> > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com> > --- > drivers/infiniband/core/cma.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index d570030..22a22e2 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -2618,10 +2618,10 @@ static int cma_format_hdr(void *hdr, struct > rdma_id_private *id_priv) > struct cma_hdr *cma_hdr; > > cma_hdr = hdr; > - cma_hdr->cma_version = CMA_VERSION; > if (cma_family(id_priv) == AF_INET) { > struct sockaddr_in *src4, *dst4; > > + cma_hdr->cma_version = CMA_VERSION; > src4 = (struct sockaddr_in *) cma_src_addr(id_priv); > dst4 = (struct sockaddr_in *) cma_dst_addr(id_priv); > > @@ -2632,6 +2632,7 @@ static int cma_format_hdr(void *hdr, struct > rdma_id_private *id_priv) > } else if (cma_family(id_priv) == AF_INET6) { > struct sockaddr_in6 *src6, *dst6; > > + cma_hdr->cma_version = CMA_VERSION; > src6 = (struct sockaddr_in6 *) cma_src_addr(id_priv); > dst6 = (struct sockaddr_in6 *) cma_dst_addr(id_priv); I don't think this is sufficient. The RDMA CM private data header is defined by the IB spec. If the service ID starts with the prefix 0x0000000001, it's reasonable to assume that the header is part of the private data. The receive side should probably even check the version and discard any unknown values. - Sean -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sean, Thanks for looking into this. I checked nfiniBand TM Architecture Release 1.2.1 VOLUME 1 - GENERAL SPECIFICATIONS, for RDMA IP CM service, the private data for CM REQ is defined in: Table 532 IP Addressing CM REQ Message Private Data Format So you mean even for RDMA IB, the private data format is also required, could you point me a little more detail? On Tue, Dec 16, 2014 at 10:29 PM, Hefty, Sean <sean.hefty@intel.com> wrote: >> If user attach private data for AF_IB, the first byte will >> be overwritten, because we always set the cma version no matter >> family is AF_IB, so move the version set inside if condition. >> >> Reported-by: Fabian Holler <fabian.holler@profitbricks.com> >> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com> >> --- >> drivers/infiniband/core/cma.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c >> index d570030..22a22e2 100644 >> --- a/drivers/infiniband/core/cma.c >> +++ b/drivers/infiniband/core/cma.c >> @@ -2618,10 +2618,10 @@ static int cma_format_hdr(void *hdr, struct >> rdma_id_private *id_priv) >> struct cma_hdr *cma_hdr; >> >> cma_hdr = hdr; >> - cma_hdr->cma_version = CMA_VERSION; >> if (cma_family(id_priv) == AF_INET) { >> struct sockaddr_in *src4, *dst4; >> >> + cma_hdr->cma_version = CMA_VERSION; >> src4 = (struct sockaddr_in *) cma_src_addr(id_priv); >> dst4 = (struct sockaddr_in *) cma_dst_addr(id_priv); >> >> @@ -2632,6 +2632,7 @@ static int cma_format_hdr(void *hdr, struct >> rdma_id_private *id_priv) >> } else if (cma_family(id_priv) == AF_INET6) { >> struct sockaddr_in6 *src6, *dst6; >> >> + cma_hdr->cma_version = CMA_VERSION; >> src6 = (struct sockaddr_in6 *) cma_src_addr(id_priv); >> dst6 = (struct sockaddr_in6 *) cma_dst_addr(id_priv); > > I don't think this is sufficient. The RDMA CM private data header is defined by the IB spec. If the service ID starts with the prefix 0x0000000001, it's reasonable to assume that the header is part of the private data. The receive side should probably even check the version and discard any unknown values. > > - Sean
On 12/16/2014 5:43 PM, Jack Wang wrote: > If user attach private data for AF_IB, Just out of curiosity, can you share what was the motivation for you to use AF_IB? Or. > the first byte will > be overwritten, because we always set the cma version no matter > family is AF_IB, so move the version set inside if condition. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Or, We have some trouble in the past about IPoIB support about neighbour discovery, AF_IB doesn't relay on IPoIB, so we think have another option maybe give us more safe in some cases. On Wed, Dec 17, 2014 at 11:40 AM, Or Gerlitz <ogerlitz@mellanox.com> wrote: > On 12/16/2014 5:43 PM, Jack Wang wrote: >> >> If user attach private data for AF_IB, > > > Just out of curiosity, can you share what was the motivation for you to use > AF_IB? > > > Or. > > >> the first byte will >> be overwritten, because we always set the cma version no matter >> family is AF_IB, so move the version set inside if condition. > >
> I checked nfiniBand TM Architecture Release 1.2.1 VOLUME 1 - GENERAL > SPECIFICATIONS, for RDMA IP CM service, > the private data for CM REQ is defined in: > Table 532 IP Addressing CM REQ Message Private Data Format > So you mean even for RDMA IB, the private data format is also > required, could you point me a little more detail? The RDMA IP CM service Annex in the IB spec defines a range of service IDs that apply to the RDMA CM. If a service ID falls into this range, the private data format should also match. For example, an app could start with AF_INET, use an alternate method to resolve the IP address to a GID (such as a user space daemon), then provide the mapped address directly to the kernel using AF_IB. This prevents the kernel from needing to perform a duplicate address resolution. The listening side of the connection may still be using AF_INET. In this example, the client side is still required to format the private data as indicated in the Annex. I.e. the original IP addressing that was used should appear as part of the private data. The port space (which maps to the service ID) needs to be included as part of the check that determines the format of the private data, and not simply the address family. - Sean
Hi Sean, On Wed, Dec 17, 2014 at 6:58 PM, Hefty, Sean <sean.hefty@intel.com> wrote: >> I checked nfiniBand TM Architecture Release 1.2.1 VOLUME 1 - GENERAL >> SPECIFICATIONS, for RDMA IP CM service, >> the private data for CM REQ is defined in: >> Table 532 IP Addressing CM REQ Message Private Data Format >> So you mean even for RDMA IB, the private data format is also >> required, could you point me a little more detail? > > The RDMA IP CM service Annex in the IB spec defines a range of service IDs that apply to the RDMA CM. If a service ID falls into this range, the private data format should also match. > > For example, an app could start with AF_INET, use an alternate method to resolve the IP address to a GID (such as a user space daemon), then provide the mapped address directly to the kernel using AF_IB. This prevents the kernel from needing to perform a duplicate address resolution. The listening side of the connection may still be using AF_INET. In this example, the client side is still required to format the private data as indicated in the Annex. I.e. the original IP addressing that was used should appear as part of the private data. So it's possible on server side listen using AF_INET, but client rdma_resolve_addr/rdma_resolve_route/rdma_connect with AF_IB? Jack > > The port space (which maps to the service ID) needs to be included as part of the check that determines the format of the private data, and not simply the address family. > > - Sean
> So it's possible on server side listen using AF_INET, but client > rdma_resolve_addr/rdma_resolve_route/rdma_connect with AF_IB? Yes
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index d570030..22a22e2 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -2618,10 +2618,10 @@ static int cma_format_hdr(void *hdr, struct rdma_id_private *id_priv) struct cma_hdr *cma_hdr; cma_hdr = hdr; - cma_hdr->cma_version = CMA_VERSION; if (cma_family(id_priv) == AF_INET) { struct sockaddr_in *src4, *dst4; + cma_hdr->cma_version = CMA_VERSION; src4 = (struct sockaddr_in *) cma_src_addr(id_priv); dst4 = (struct sockaddr_in *) cma_dst_addr(id_priv); @@ -2632,6 +2632,7 @@ static int cma_format_hdr(void *hdr, struct rdma_id_private *id_priv) } else if (cma_family(id_priv) == AF_INET6) { struct sockaddr_in6 *src6, *dst6; + cma_hdr->cma_version = CMA_VERSION; src6 = (struct sockaddr_in6 *) cma_src_addr(id_priv); dst6 = (struct sockaddr_in6 *) cma_dst_addr(id_priv);
If user attach private data for AF_IB, the first byte will be overwritten, because we always set the cma version no matter family is AF_IB, so move the version set inside if condition. Reported-by: Fabian Holler <fabian.holler@profitbricks.com> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com> --- drivers/infiniband/core/cma.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)