[PATCH] net: rds: Don't allocate rds_sock on stack

[PATCH] net: rds: Don't allocate rds_sock on stack

[Mark Brown]

From: Mark Brown <broonie@linaro.org>

struct rds_sock is rather large ausing the following warning in an ARM
allmodconfig:

net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]

Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id
instead of allocating it on the stack.

Signed-off-by: Mark Brown <broonie@linaro.org>
---
 net/rds/iw_rdma.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/rds/iw_rdma.c b/net/rds/iw_rdma.c
index a817705..cee5daa 100644
--- a/net/rds/iw_rdma.c
+++ b/net/rds/iw_rdma.c
@@ -180,22 +180,28 @@ int rds_iw_update_cm_id(struct rds_iw_device *rds_iwdev, struct rdma_cm_id *cm_i
 {
 	struct sockaddr_in *src_addr, *dst_addr;
 	struct rds_iw_device *rds_iwdev_old;
-	struct rds_sock rs;
+	struct rds_sock *rs;
 	struct rdma_cm_id *pcm_id;
 	int rc;
 
+	rs = kzalloc(sizeof(*rs), GFP_KERNEL);
+	if (!rs)
+		return -ENOMEM;
+
 	src_addr = (struct sockaddr_in *)&cm_id->route.addr.src_addr;
 	dst_addr = (struct sockaddr_in *)&cm_id->route.addr.dst_addr;
 
-	rs.rs_bound_addr = src_addr->sin_addr.s_addr;
-	rs.rs_bound_port = src_addr->sin_port;
-	rs.rs_conn_addr = dst_addr->sin_addr.s_addr;
-	rs.rs_conn_port = dst_addr->sin_port;
+	rs->rs_bound_addr = src_addr->sin_addr.s_addr;
+	rs->rs_bound_port = src_addr->sin_port;
+	rs->rs_conn_addr = dst_addr->sin_addr.s_addr;
+	rs->rs_conn_port = dst_addr->sin_port;
 
-	rc = rds_iw_get_device(&rs, &rds_iwdev_old, &pcm_id);
+	rc = rds_iw_get_device(rs, &rds_iwdev_old, &pcm_id);
 	if (rc)
 		rds_iw_remove_cm_id(rds_iwdev, cm_id);
 
+	kfree(rs);
+
 	return rds_iw_add_cm_id(rds_iwdev, cm_id);
 }
 
-- 
2.1.0.rc1

Re: [PATCH] net: rds: Don't allocate rds_sock on stack

[Arnd Bergmann]

On Sunday 24 August 2014 19:32:35 Mark Brown wrote:
> From: Mark Brown <broonie@linaro.org>
> 
> struct rds_sock is rather large ausing the following warning in an ARM
> allmodconfig:
> 
> net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id
> instead of allocating it on the stack.
> 
> Signed-off-by: Mark Brown <broonie@linaro.org>
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] net: rds: Don't allocate rds_sock on stack

[David Miller]

From: Mark Brown <broonie@kernel.org>
Date: Sun, 24 Aug 2014 19:32:35 -0500

> From: Mark Brown <broonie@linaro.org>
> 
> struct rds_sock is rather large ausing the following warning in an ARM
> allmodconfig:
> 
> net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id
> instead of allocating it on the stack.
> 
> Signed-off-by: Mark Brown <broonie@linaro.org>

I'd like you to fix this differently.  Creating pseudo instances of
objects, and partially initializing it, just to satisfy an interface
is always a really bad sign.

Create a key structure argument for rds_iw_get_device() and initialize that
and pass it in instead, update the other caller similarly.

Thanks.

Re: [PATCH] net: rds: Don't allocate rds_sock on stack

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

On Mon, Aug 25, 2014 at 12:57:40PM -0700, David Miller wrote:
> From: Mark Brown <broonie@kernel.org>

> > From: Mark Brown <broonie@linaro.org>

> > struct rds_sock is rather large ausing the following warning in an ARM
> > allmodconfig:

> > net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]

> > Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id
> > instead of allocating it on the stack.

> > Signed-off-by: Mark Brown <broonie@linaro.org>

> I'd like you to fix this differently.  Creating pseudo instances of
> objects, and partially initializing it, just to satisfy an interface
> is always a really bad sign.

> Create a key structure argument for rds_iw_get_device() and initialize that
> and pass it in instead, update the other caller similarly.

I agree that the existing code looks like it could be improved even more
but please bear in mind that I'm just looking for a clean build (we've
got less than 20 warnings in allmodconfig including staging at the
minute) rather than actively working on this code in particular - I've
no ability to do more than build testing here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] net: rds: Don't allocate rds_sock on stack

[David Miller]

From: Mark Brown <broonie@kernel.org>
Date: Tue, 26 Aug 2014 07:54:09 +0100

> On Mon, Aug 25, 2014 at 12:57:40PM -0700, David Miller wrote:
>> From: Mark Brown <broonie@kernel.org>
> 
>> > From: Mark Brown <broonie@linaro.org>
> 
>> > struct rds_sock is rather large ausing the following warning in an ARM
>> > allmodconfig:
> 
>> > net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
>> > Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id
>> > instead of allocating it on the stack.
> 
>> > Signed-off-by: Mark Brown <broonie@linaro.org>
> 
>> I'd like you to fix this differently.  Creating pseudo instances of
>> objects, and partially initializing it, just to satisfy an interface
>> is always a really bad sign.
> 
>> Create a key structure argument for rds_iw_get_device() and initialize that
>> and pass it in instead, update the other caller similarly.
> 
> I agree that the existing code looks like it could be improved even more
> but please bear in mind that I'm just looking for a clean build (we've
> got less than 20 warnings in allmodconfig including staging at the
> minute) rather than actively working on this code in particular - I've
> no ability to do more than build testing here.

I understand that, but please fix this bug properly.

Re: [PATCH] net: rds: Don't allocate rds_sock on stack

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On Tue, Aug 26, 2014 at 08:29:06AM -0700, David Miller wrote:
> From: Mark Brown <broonie@kernel.org>

> > I agree that the existing code looks like it could be improved even more
> > but please bear in mind that I'm just looking for a clean build (we've
> > got less than 20 warnings in allmodconfig including staging at the
> > minute) rather than actively working on this code in particular - I've
> > no ability to do more than build testing here.

> I understand that, but please fix this bug properly.

Please set the expectation among the networking developers that their
code should compile cleanly on all architectures; currently the
networking code (mostly drivers rather than the core) is the single
biggest source of warnings outside of staging and if we are requring
things like this then that presents a substantial barrier to others
contributing to addressing the warnings.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]