Annoying gcc / rdma / networking warnings

Annoying gcc / rdma / networking warnings

[Linus Torvalds]

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

Re: Annoying gcc / rdma / networking warnings

[Sowmini Varadhan]

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

Re: Annoying gcc / rdma / networking warnings

[Jason Gunthorpe]

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

Re: Annoying gcc / rdma / networking warnings

[David Miller]

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?

Re: Annoying gcc / rdma / networking warnings

[Jason Gunthorpe]

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