| Message ID | 20190514005521.GA18085@ziepe.ca (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 641114d2af312d39ca9bbc2369d18a5823da51c6 |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [v2] RDMA: Directly cast the sockaddr union to sockaddr | expand |
On Mon, May 13, 2019 at 09:55:21PM -0300, Jason Gunthorpe wrote: > gcc 9 now does allocation size tracking and thinks that passing the member > of a union and then accessing beyond that member's bounds is an overflow. > > Instead of using the union member, use the entire union with a cast to > get to the sockaddr. gcc will now know that the memory extends the full > size of the union. > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > --- > drivers/infiniband/core/addr.c | 16 ++++++++-------- > drivers/infiniband/hw/ocrdma/ocrdma_ah.c | 5 ++--- > drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 5 ++--- > 3 files changed, 12 insertions(+), 14 deletions(-) > > I missed the ocrdma files in the v1 > > We can revisit what to do with that repetitive union after the merge > window, but this simple patch will eliminate the warnings for now. > > Linus, I'll send this as a PR tomorrow - there is also a bug fix for > the rdma-netlink changes posted that should go too. <2c> I would be very happy to see this revisited in such a way that some use is made of the C type system (instead of casts). </2c>
On Thu, May 16, 2019 at 02:44:28PM +0200, Simon Horman wrote: > On Mon, May 13, 2019 at 09:55:21PM -0300, Jason Gunthorpe wrote: > > gcc 9 now does allocation size tracking and thinks that passing the member > > of a union and then accessing beyond that member's bounds is an overflow. > > > > Instead of using the union member, use the entire union with a cast to > > get to the sockaddr. gcc will now know that the memory extends the full > > size of the union. > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > drivers/infiniband/core/addr.c | 16 ++++++++-------- > > drivers/infiniband/hw/ocrdma/ocrdma_ah.c | 5 ++--- > > drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 5 ++--- > > 3 files changed, 12 insertions(+), 14 deletions(-) > > > > I missed the ocrdma files in the v1 > > > > We can revisit what to do with that repetitive union after the merge > > window, but this simple patch will eliminate the warnings for now. > > > > Linus, I'll send this as a PR tomorrow - there is also a bug fix for > > the rdma-netlink changes posted that should go too. > > <2c> > I would be very happy to see this revisited in such a way > that some use is made of the C type system (instead of casts). > </2c> Well, I was thinking of swapping the union to sockaddr_storage .. Do you propose to add a union to the kernel's sockaddr storage? Jason
On Thu, May 16, 2019 at 12:21:48PM -0300, Jason Gunthorpe wrote: > On Thu, May 16, 2019 at 02:44:28PM +0200, Simon Horman wrote: > > On Mon, May 13, 2019 at 09:55:21PM -0300, Jason Gunthorpe wrote: > > > gcc 9 now does allocation size tracking and thinks that passing the member > > > of a union and then accessing beyond that member's bounds is an overflow. > > > > > > Instead of using the union member, use the entire union with a cast to > > > get to the sockaddr. gcc will now know that the memory extends the full > > > size of the union. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > > drivers/infiniband/core/addr.c | 16 ++++++++-------- > > > drivers/infiniband/hw/ocrdma/ocrdma_ah.c | 5 ++--- > > > drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 5 ++--- > > > 3 files changed, 12 insertions(+), 14 deletions(-) > > > > > > I missed the ocrdma files in the v1 > > > > > > We can revisit what to do with that repetitive union after the merge > > > window, but this simple patch will eliminate the warnings for now. > > > > > > Linus, I'll send this as a PR tomorrow - there is also a bug fix for > > > the rdma-netlink changes posted that should go too. > > > > <2c> > > I would be very happy to see this revisited in such a way > > that some use is made of the C type system (instead of casts). > > </2c> > > Well, I was thinking of swapping the union to sockaddr_storage .. > > Do you propose to add a union to the kernel's sockaddr storage? I understand you have been down that rabbit hole before but, yes, in an ideal world that would be my preference.
diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c index ba01b90c04e775..2f7d14159841f8 100644 --- a/drivers/infiniband/core/addr.c +++ b/drivers/infiniband/core/addr.c @@ -731,8 +731,8 @@ int roce_resolve_route_from_path(struct sa_path_rec *rec, if (rec->roce.route_resolved) return 0; - rdma_gid2ip(&sgid._sockaddr, &rec->sgid); - rdma_gid2ip(&dgid._sockaddr, &rec->dgid); + rdma_gid2ip((struct sockaddr *)&sgid, &rec->sgid); + rdma_gid2ip((struct sockaddr *)&dgid, &rec->dgid); if (sgid._sockaddr.sa_family != dgid._sockaddr.sa_family) return -EINVAL; @@ -743,7 +743,7 @@ int roce_resolve_route_from_path(struct sa_path_rec *rec, dev_addr.net = &init_net; dev_addr.sgid_attr = attr; - ret = addr_resolve(&sgid._sockaddr, &dgid._sockaddr, + ret = addr_resolve((struct sockaddr *)&sgid, (struct sockaddr *)&dgid, &dev_addr, false, true, 0); if (ret) return ret; @@ -815,22 +815,22 @@ int rdma_addr_find_l2_eth_by_grh(const union ib_gid *sgid, struct rdma_dev_addr dev_addr; struct resolve_cb_context ctx; union { - struct sockaddr _sockaddr; struct sockaddr_in _sockaddr_in; struct sockaddr_in6 _sockaddr_in6; } sgid_addr, dgid_addr; int ret; - rdma_gid2ip(&sgid_addr._sockaddr, sgid); - rdma_gid2ip(&dgid_addr._sockaddr, dgid); + rdma_gid2ip((struct sockaddr *)&sgid_addr, sgid); + rdma_gid2ip((struct sockaddr *)&dgid_addr, dgid); memset(&dev_addr, 0, sizeof(dev_addr)); dev_addr.net = &init_net; dev_addr.sgid_attr = sgid_attr; init_completion(&ctx.comp); - ret = rdma_resolve_ip(&sgid_addr._sockaddr, &dgid_addr._sockaddr, - &dev_addr, 1000, resolve_cb, true, &ctx); + ret = rdma_resolve_ip((struct sockaddr *)&sgid_addr, + (struct sockaddr *)&dgid_addr, &dev_addr, 1000, + resolve_cb, true, &ctx); if (ret) return ret; diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_ah.c b/drivers/infiniband/hw/ocrdma/ocrdma_ah.c index 1d4ea135c28f2a..8d3e36d548aae9 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_ah.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_ah.c @@ -83,7 +83,6 @@ static inline int set_av_attr(struct ocrdma_dev *dev, struct ocrdma_ah *ah, struct iphdr ipv4; const struct ib_global_route *ib_grh; union { - struct sockaddr _sockaddr; struct sockaddr_in _sockaddr_in; struct sockaddr_in6 _sockaddr_in6; } sgid_addr, dgid_addr; @@ -133,9 +132,9 @@ static inline int set_av_attr(struct ocrdma_dev *dev, struct ocrdma_ah *ah, ipv4.tot_len = htons(0); ipv4.ttl = ib_grh->hop_limit; ipv4.protocol = nxthdr; - rdma_gid2ip(&sgid_addr._sockaddr, sgid); + rdma_gid2ip((struct sockaddr *)&sgid_addr, sgid); ipv4.saddr = sgid_addr._sockaddr_in.sin_addr.s_addr; - rdma_gid2ip(&dgid_addr._sockaddr, &ib_grh->dgid); + rdma_gid2ip((struct sockaddr*)&dgid_addr, &ib_grh->dgid); ipv4.daddr = dgid_addr._sockaddr_in.sin_addr.s_addr; memcpy((u8 *)ah->av + eth_sz, &ipv4, sizeof(struct iphdr)); } else { diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c index 32674b291f60da..5127e2ea4bdd2d 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c @@ -2499,7 +2499,6 @@ static int ocrdma_set_av_params(struct ocrdma_qp *qp, u16 vlan_id = 0xFFFF; u8 mac_addr[6], hdr_type; union { - struct sockaddr _sockaddr; struct sockaddr_in _sockaddr_in; struct sockaddr_in6 _sockaddr_in6; } sgid_addr, dgid_addr; @@ -2542,8 +2541,8 @@ static int ocrdma_set_av_params(struct ocrdma_qp *qp, hdr_type = rdma_gid_attr_network_type(sgid_attr); if (hdr_type == RDMA_NETWORK_IPV4) { - rdma_gid2ip(&sgid_addr._sockaddr, &sgid_attr->gid); - rdma_gid2ip(&dgid_addr._sockaddr, &grh->dgid); + rdma_gid2ip((struct sockaddr *)&sgid_addr, &sgid_attr->gid); + rdma_gid2ip((struct sockaddr *)&dgid_addr, &grh->dgid); memcpy(&cmd->params.dgid[0], &dgid_addr._sockaddr_in.sin_addr.s_addr, 4); memcpy(&cmd->params.sgid[0],
gcc 9 now does allocation size tracking and thinks that passing the member of a union and then accessing beyond that member's bounds is an overflow. Instead of using the union member, use the entire union with a cast to get to the sockaddr. gcc will now know that the memory extends the full size of the union. Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> --- drivers/infiniband/core/addr.c | 16 ++++++++-------- drivers/infiniband/hw/ocrdma/ocrdma_ah.c | 5 ++--- drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 5 ++--- 3 files changed, 12 insertions(+), 14 deletions(-) I missed the ocrdma files in the v1 We can revisit what to do with that repetitive union after the merge window, but this simple patch will eliminate the warnings for now. Linus, I'll send this as a PR tomorrow - there is also a bug fix for the rdma-netlink changes posted that should go too. Thanks, Jason