* Annoying gcc / rdma / networking warnings @ 2019-05-11 16:52 Linus Torvalds 2019-05-11 20:02 ` Sowmini Varadhan 2019-05-13 1:11 ` Jason Gunthorpe 0 siblings, 2 replies; 7+ messages in thread From: Linus Torvalds @ 2019-05-11 16:52 UTC (permalink / raw) To: Jason Gunthorpe, David Miller Cc: Doug Ledford, linux-rdma, Linux List Kernel Mailing, Netdev Jason and Davem, with gcc-9, I'm now seeing a number of annoying warnings from the rdma layer. I think it depends on the exact gcc version, because I'm seeing them on my laptop but didn't see them on my desktop, probably due to updating at different times. The warning is because gcc now looks at pointer types for some allocation sizes, and will do things like this: In function ‘memset’, inlined from ‘rdma_gid2ip’ at ./include/rdma/ib_addr.h:168:3, inlined from ‘roce_resolve_route_from_path’ at drivers/infiniband/core/addr.c:735:2: ./include/linux/string.h:344:9: warning: ‘__builtin_memset’ offset [17, 28] from the object at ‘dgid’ is out of the bounds of referenced subobject ‘_sockaddr’ with type ‘struct sockaddr’ at offset 0 [-Warray-bounds] 344 | return __builtin_memset(p, c, size); because the "memset()" is done onto a "sockaddr_in6" (where it's not out of bounds), but the rdma_gid2ip() function was passed a "sockaddr" type (where it *is* out of bounds. All the cases I found actually have good *allocations* for the underlying storage, using a union of the different sockaddr types, and includes a "sockaddr_in6". So the warning actually looks bogus from an actual allocation standpoint, but at the same time, I think the warning does show a real issue in the networking code. In particular, a "struct sockaddr" is supposed to be a valid superset of the different sockaddr types, and the rdma use is in that sense the rdma use of "struct sockaddr *" is entirely sane. BUT. The Linux kernel sockaddr is actually just 16 bytes. While a sockaddr_int is about twice that. So if you look at the types like gcc does, then the rdma layer really is passing a pointer to a 16-byte sockaddr, and then filling it with (much bigger) sockaddr_ip6 data. Arguably gcc is being stupid, and it should look at the actual allocation, but that's not what it does. And I do think what gcc does is at least understandable. So David, arguably the kernel "struct sockaddr" is simply wrong, if it can't contain a "struct sockaddr_in6". No? Is extending it a huge problem for other users that don't need it (mainly stack, I assume..)? Also equally arguably, the rdma code could just use a "struct sockaddr_in6 for this use and avoid the gcc issue, couldn't it? It has the type right there, and rdma_gid2ip() could just take that type instead, since that's what it actually then uses anyway. Comments? Linus ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Annoying gcc / rdma / networking warnings 2019-05-11 16:52 Annoying gcc / rdma / networking warnings Linus Torvalds @ 2019-05-11 20:02 ` Sowmini Varadhan 2019-05-13 1:11 ` Jason Gunthorpe 1 sibling, 0 replies; 7+ messages in thread From: Sowmini Varadhan @ 2019-05-11 20:02 UTC (permalink / raw) To: Linus Torvalds Cc: Jason Gunthorpe, David Miller, Doug Ledford, linux-rdma, Linux List Kernel Mailing, Netdev On (05/11/19 12:52), Linus Torvalds wrote: > > So David, arguably the kernel "struct sockaddr" is simply wrong, if it > can't contain a "struct sockaddr_in6". No? Is extending it a huge > problem for other users that don't need it (mainly stack, I assume..)? The ipv6 working group came up with sockaddr_storage to solve this. See RFC 2553. However, in practice, since sizeof(struct sockaddr_storage) is much larger than simply creating a union of sockaddr_in and sockaaddr_in6, most userspace networking applications will do the latter. The strucut sockaddr is the mereely the generic pointer cast that is expected to be used for the common posix fucntions like bind/connect etc. > Also equally arguably, the rdma code could just use a "struct > sockaddr_in6 for this use and avoid the gcc issue, couldn't it? It has Yes, that would be the right solution. --Sowmini ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Annoying gcc / rdma / networking warnings 2019-05-11 16:52 Annoying gcc / rdma / networking warnings Linus Torvalds @ 2019-05-13 1:11 ` Jason Gunthorpe 2019-05-13 1:11 ` Jason Gunthorpe 1 sibling, 0 replies; 7+ messages in thread From: Jason Gunthorpe @ 2019-05-13 1:11 UTC (permalink / raw) To: Linus Torvalds Cc: David Miller, Doug Ledford, linux-rdma, Linux List Kernel Mailing, Netdev On Sat, May 11, 2019 at 12:52:06PM -0400, Linus Torvalds wrote: > Jason and Davem, > with gcc-9, I'm now seeing a number of annoying warnings from the > rdma layer. I think it depends on the exact gcc version, because I'm > seeing them on my laptop but didn't see them on my desktop, probably > due to updating at different times. I can see them too on a latest FC30 compiler.. It is pretty amazing FC30 shipped this month with a compiler that was just released 10 days ago. :) Also a lot of of 'taking address of a packed member' warnings in RDMA code for structs that probably should be aligned(4) not packed. I'll have to look at those more carefully this week and see what can be done. > So if you look at the types like gcc does, then the rdma layer really > is passing a pointer to a 16-byte sockaddr, and then filling it with > (much bigger) sockaddr_ip6 data. It looks like gcc is assuming that since we started with the _sockaddr union member that the memory is actually bounded to that specific member. This doesn't seem unreasonable and matches a lot of uses of unions. However I wonder how that sort of analysis is going to work in the kernel, considering our container_of idiom breaks it in the same way, but maybe that is special case'd.. So if we tell gcc the sockaddr memory is actually the whole union then it becomes happy, see below. > So David, arguably the kernel "struct sockaddr" is simply wrong, if it > can't contain a "struct sockaddr_in6". No? Is extending it a huge > problem for other users that don't need it (mainly stack, I assume..)? We have sockaddr_storage for this, and arguably this code is just over-optimizing to save a few bytes of stack vs sockaddr_storage.. > Also equally arguably, the rdma code could just use a "struct > sockaddr_in6 for this use and avoid the gcc issue, couldn't it? I think the specific sockaddr types should only ever be used if we *know* the sa_family is that type. If the sa_family is not known then it should be sockaddr or sockaddr_storage. Otherwise things get very confusing. When using sockaddr_storage code always has the cast to sockaddr anyhow, as it is not a union, so this jaunty cast is not out of place in sockets code. Below silences the warning, and the warning continues to work in other cases, ie if I remove _sockaddr_in6 from the union I do get compile warnings about out of bound references. Jason >From 38a4d7e4644a13378c11381feb3936aa1faf9f58 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe <jgg@mellanox.com> Date: Sun, 12 May 2019 21:57:57 -0300 Subject: [PATCH] RDMA: Directly cast the sockaddr union to sockaddr 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 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c index 0dce94e3c49561..d0b04b0d309fa6 100644 --- a/drivers/infiniband/core/addr.c +++ b/drivers/infiniband/core/addr.c @@ -730,8 +730,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; @@ -742,7 +742,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; @@ -814,22 +814,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; -- 2.21.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: Annoying gcc / rdma / networking warnings @ 2019-05-13 1:11 ` Jason Gunthorpe 0 siblings, 0 replies; 7+ messages in thread From: Jason Gunthorpe @ 2019-05-13 1:11 UTC (permalink / raw) To: Linus Torvalds Cc: David Miller, Doug Ledford, linux-rdma, Linux List Kernel Mailing, Netdev On Sat, May 11, 2019 at 12:52:06PM -0400, Linus Torvalds wrote: > Jason and Davem, > with gcc-9, I'm now seeing a number of annoying warnings from the > rdma layer. I think it depends on the exact gcc version, because I'm > seeing them on my laptop but didn't see them on my desktop, probably > due to updating at different times. I can see them too on a latest FC30 compiler.. It is pretty amazing FC30 shipped this month with a compiler that was just released 10 days ago. :) Also a lot of of 'taking address of a packed member' warnings in RDMA code for structs that probably should be aligned(4) not packed. I'll have to look at those more carefully this week and see what can be done. > So if you look at the types like gcc does, then the rdma layer really > is passing a pointer to a 16-byte sockaddr, and then filling it with > (much bigger) sockaddr_ip6 data. It looks like gcc is assuming that since we started with the _sockaddr union member that the memory is actually bounded to that specific member. This doesn't seem unreasonable and matches a lot of uses of unions. However I wonder how that sort of analysis is going to work in the kernel, considering our container_of idiom breaks it in the same way, but maybe that is special case'd.. So if we tell gcc the sockaddr memory is actually the whole union then it becomes happy, see below. > So David, arguably the kernel "struct sockaddr" is simply wrong, if it > can't contain a "struct sockaddr_in6". No? Is extending it a huge > problem for other users that don't need it (mainly stack, I assume..)? We have sockaddr_storage for this, and arguably this code is just over-optimizing to save a few bytes of stack vs sockaddr_storage.. > Also equally arguably, the rdma code could just use a "struct > sockaddr_in6 for this use and avoid the gcc issue, couldn't it? I think the specific sockaddr types should only ever be used if we *know* the sa_family is that type. If the sa_family is not known then it should be sockaddr or sockaddr_storage. Otherwise things get very confusing. When using sockaddr_storage code always has the cast to sockaddr anyhow, as it is not a union, so this jaunty cast is not out of place in sockets code. Below silences the warning, and the warning continues to work in other cases, ie if I remove _sockaddr_in6 from the union I do get compile warnings about out of bound references. Jason From 38a4d7e4644a13378c11381feb3936aa1faf9f58 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe <jgg@mellanox.com> Date: Sun, 12 May 2019 21:57:57 -0300 Subject: [PATCH] RDMA: Directly cast the sockaddr union to sockaddr 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 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c index 0dce94e3c49561..d0b04b0d309fa6 100644 --- a/drivers/infiniband/core/addr.c +++ b/drivers/infiniband/core/addr.c @@ -730,8 +730,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; @@ -742,7 +742,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; @@ -814,22 +814,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; -- 2.21.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: Annoying gcc / rdma / networking warnings 2019-05-13 1:11 ` Jason Gunthorpe @ 2019-05-13 3:17 ` David Miller -1 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2019-05-13 3:17 UTC (permalink / raw) To: jgg; +Cc: torvalds, dledford, linux-rdma, linux-kernel, netdev From: Jason Gunthorpe <jgg@mellanox.com> Date: Mon, 13 May 2019 01:11:42 +0000 > I think the specific sockaddr types should only ever be used if we > *know* the sa_family is that type. If the sa_family is not known then > it should be sockaddr or sockaddr_storage. Otherwise things get very > confusing. > > When using sockaddr_storage code always has the cast to sockaddr > anyhow, as it is not a union, so this jaunty cast is not out of place > in sockets code. >From what I can see, each and every call side of these helpers like rdma_gid2ip() et al. redefine this union type over and over and over again in the local function. That's the real problem. It seems that if we just defined it explicitly in one place, like include/rdma/ib_addr.h, then we could have tdma_gid2ip(), addr_resolve(), and rdma_resolve_ip() take that type explcitily. No? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Annoying gcc / rdma / networking warnings @ 2019-05-13 3:17 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2019-05-13 3:17 UTC (permalink / raw) To: jgg; +Cc: torvalds, dledford, linux-rdma, linux-kernel, netdev From: Jason Gunthorpe <jgg@mellanox.com> Date: Mon, 13 May 2019 01:11:42 +0000 > I think the specific sockaddr types should only ever be used if we > *know* the sa_family is that type. If the sa_family is not known then > it should be sockaddr or sockaddr_storage. Otherwise things get very > confusing. > > When using sockaddr_storage code always has the cast to sockaddr > anyhow, as it is not a union, so this jaunty cast is not out of place > in sockets code. From what I can see, each and every call side of these helpers like rdma_gid2ip() et al. redefine this union type over and over and over again in the local function. That's the real problem. It seems that if we just defined it explicitly in one place, like include/rdma/ib_addr.h, then we could have tdma_gid2ip(), addr_resolve(), and rdma_resolve_ip() take that type explcitily. No? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Annoying gcc / rdma / networking warnings 2019-05-13 3:17 ` David Miller (?) @ 2019-05-13 18:30 ` Jason Gunthorpe -1 siblings, 0 replies; 7+ messages in thread From: Jason Gunthorpe @ 2019-05-13 18:30 UTC (permalink / raw) To: David Miller, g; +Cc: torvalds, dledford, linux-rdma, linux-kernel, netdev On Sun, May 12, 2019 at 08:17:01PM -0700, David Miller wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > Date: Mon, 13 May 2019 01:11:42 +0000 > > > I think the specific sockaddr types should only ever be used if we > > *know* the sa_family is that type. If the sa_family is not known then > > it should be sockaddr or sockaddr_storage. Otherwise things get very > > confusing. > > > > When using sockaddr_storage code always has the cast to sockaddr > > anyhow, as it is not a union, so this jaunty cast is not out of place > > in sockets code. > > From what I can see, each and every call side of these helpers like > rdma_gid2ip() et al. redefine this union type over and over and over > again in the local function. Yes, the repeated union is very ugly and could be consolidated - or should just use sockaddr_storage in the first place. > It seems that if we just defined it explicitly in one place, like > include/rdma/ib_addr.h, then we could have tdma_gid2ip(), addr_resolve(), > and rdma_resolve_ip() take that type explcitily. I pulled on this thread for a while and the number of places that would need to convert to use a global 'union rdma_sockaddr_inet' started to become pretty silly and weird. I eventually reached a point where I had to cast a sockaddr * to the union - which is something that makes no sense and I gave up. So, I think this feels simpler to follow the usual sockaddr_storage pattern and only use the union to declare the initial storage. Then everything else just uses the sockaddr * plus casts.. Jason ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-13 18:30 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-11 16:52 Annoying gcc / rdma / networking warnings Linus Torvalds 2019-05-11 20:02 ` Sowmini Varadhan 2019-05-13 1:11 ` Jason Gunthorpe 2019-05-13 1:11 ` Jason Gunthorpe 2019-05-13 3:17 ` David Miller 2019-05-13 3:17 ` David Miller 2019-05-13 18:30 ` Jason Gunthorpe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.