* [Patch net-next] icmp: avoid allocating large struct on stack
@ 2013-06-01 3:24 Cong Wang
2013-06-01 3:34 ` Joe Perches
2013-06-01 5:22 ` Eric Dumazet
0 siblings, 2 replies; 5+ messages in thread
From: Cong Wang @ 2013-06-01 3:24 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Cong Wang
From: Cong Wang <amwang@redhat.com>
struct icmp_bxm is a large struct, reduce stack usage
by allocating it on heap.
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
net/ipv4/icmp.c | 37 +++++++++++++++++++++----------------
1 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 5d0d379..8157684 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -482,7 +482,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
{
struct iphdr *iph;
int room;
- struct icmp_bxm icmp_param;
+ struct icmp_bxm *icmp_param;
struct rtable *rt = skb_rtable(skb_in);
struct ipcm_cookie ipc;
struct flowi4 fl4;
@@ -558,6 +558,10 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
}
}
+ icmp_param = kzalloc(sizeof(*icmp_param), GFP_ATOMIC);
+ if (!icmp_param)
+ return;
+
sk = icmp_xmit_lock(net);
if (sk == NULL)
return;
@@ -586,7 +590,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
IPTOS_PREC_INTERNETCONTROL) :
iph->tos;
- if (ip_options_echo(&icmp_param.replyopts.opt.opt, skb_in))
+ if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in))
goto out_unlock;
@@ -594,19 +598,19 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
* Prepare data for ICMP header.
*/
- icmp_param.data.icmph.type = type;
- icmp_param.data.icmph.code = code;
- icmp_param.data.icmph.un.gateway = info;
- icmp_param.data.icmph.checksum = 0;
- icmp_param.skb = skb_in;
- icmp_param.offset = skb_network_offset(skb_in);
+ icmp_param->data.icmph.type = type;
+ icmp_param->data.icmph.code = code;
+ icmp_param->data.icmph.un.gateway = info;
+ icmp_param->data.icmph.checksum = 0;
+ icmp_param->skb = skb_in;
+ icmp_param->offset = skb_network_offset(skb_in);
inet_sk(sk)->tos = tos;
ipc.addr = iph->saddr;
- ipc.opt = &icmp_param.replyopts.opt;
+ ipc.opt = &icmp_param->replyopts.opt;
ipc.tx_flags = 0;
rt = icmp_route_lookup(net, &fl4, skb_in, iph, saddr, tos,
- type, code, &icmp_param);
+ type, code, icmp_param);
if (IS_ERR(rt))
goto out_unlock;
@@ -618,19 +622,20 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
room = dst_mtu(&rt->dst);
if (room > 576)
room = 576;
- room -= sizeof(struct iphdr) + icmp_param.replyopts.opt.opt.optlen;
+ room -= sizeof(struct iphdr) + icmp_param->replyopts.opt.opt.optlen;
room -= sizeof(struct icmphdr);
- icmp_param.data_len = skb_in->len - icmp_param.offset;
- if (icmp_param.data_len > room)
- icmp_param.data_len = room;
- icmp_param.head_len = sizeof(struct icmphdr);
+ icmp_param->data_len = skb_in->len - icmp_param->offset;
+ if (icmp_param->data_len > room)
+ icmp_param->data_len = room;
+ icmp_param->head_len = sizeof(struct icmphdr);
- icmp_push_reply(&icmp_param, &fl4, &ipc, &rt);
+ icmp_push_reply(icmp_param, &fl4, &ipc, &rt);
ende:
ip_rt_put(rt);
out_unlock:
icmp_xmit_unlock(sk);
+ kfree(icmp_param);
out:;
}
EXPORT_SYMBOL(icmp_send);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch net-next] icmp: avoid allocating large struct on stack
2013-06-01 3:24 [Patch net-next] icmp: avoid allocating large struct on stack Cong Wang
@ 2013-06-01 3:34 ` Joe Perches
2013-06-01 3:42 ` Cong Wang
2013-06-01 5:22 ` Eric Dumazet
1 sibling, 1 reply; 5+ messages in thread
From: Joe Perches @ 2013-06-01 3:34 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, David S. Miller
On Sat, 2013-06-01 at 11:24 +0800, Cong Wang wrote:
> struct icmp_bxm is a large struct, reduce stack usage
> by allocating it on heap.
[]
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
[]
> @@ -558,6 +558,10 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
> }
> }
>
> + icmp_param = kzalloc(sizeof(*icmp_param), GFP_ATOMIC);
why kzalloc if the previous stack was uninitialized?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch net-next] icmp: avoid allocating large struct on stack
2013-06-01 3:34 ` Joe Perches
@ 2013-06-01 3:42 ` Cong Wang
0 siblings, 0 replies; 5+ messages in thread
From: Cong Wang @ 2013-06-01 3:42 UTC (permalink / raw)
To: Joe Perches; +Cc: netdev, David S. Miller
On Fri, 2013-05-31 at 20:34 -0700, Joe Perches wrote:
> On Sat, 2013-06-01 at 11:24 +0800, Cong Wang wrote:
> > struct icmp_bxm is a large struct, reduce stack usage
> > by allocating it on heap.
> []
> > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> []
> > @@ -558,6 +558,10 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
> > }
> > }
> >
> > + icmp_param = kzalloc(sizeof(*icmp_param), GFP_ATOMIC);
>
> why kzalloc if the previous stack was uninitialized?
>
>
Right, kmalloc() should be sufficient here...
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch net-next] icmp: avoid allocating large struct on stack
2013-06-01 3:24 [Patch net-next] icmp: avoid allocating large struct on stack Cong Wang
2013-06-01 3:34 ` Joe Perches
@ 2013-06-01 5:22 ` Eric Dumazet
2013-06-02 1:14 ` Cong Wang
1 sibling, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2013-06-01 5:22 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, David S. Miller
On Sat, 2013-06-01 at 11:24 +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
>
> struct icmp_bxm is a large struct, reduce stack usage
> by allocating it on heap.
>
Strange, I posted a patch like that some days ago.
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
>
> ---
> net/ipv4/icmp.c | 37 +++++++++++++++++++++----------------
> 1 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 5d0d379..8157684 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -482,7 +482,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
> {
> struct iphdr *iph;
> int room;
> - struct icmp_bxm icmp_param;
> + struct icmp_bxm *icmp_param;
> struct rtable *rt = skb_rtable(skb_in);
> struct ipcm_cookie ipc;
> struct flowi4 fl4;
> @@ -558,6 +558,10 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
> }
> }
>
> + icmp_param = kzalloc(sizeof(*icmp_param), GFP_ATOMIC);
> + if (!icmp_param)
> + return;
> +
> sk = icmp_xmit_lock(net);
> if (sk == NULL)
> return;
Please.
Read carefully this code and tell me if you do not leak memory.
For the record my patch was saving more stack space :
net/ipv4/icmp.c | 72
+++++++++++++++++++++++++++++---------------------------
1 file changed, 38 insertions(+), 34 deletions(-)
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 76e10b4..e33f3b0 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -208,7 +208,7 @@ static struct sock *icmp_sk(struct net *net)
return net->ipv4.icmp_sk[smp_processor_id()];
}
-static inline struct sock *icmp_xmit_lock(struct net *net)
+static struct sock *icmp_xmit_lock(struct net *net)
{
struct sock *sk;
@@ -226,7 +226,7 @@ static inline struct sock *icmp_xmit_lock(struct net *net)
return sk;
}
-static inline void icmp_xmit_unlock(struct sock *sk)
+static void icmp_xmit_unlock(struct sock *sk)
{
spin_unlock_bh(&sk->sk_lock.slock);
}
@@ -235,8 +235,8 @@ static inline void icmp_xmit_unlock(struct sock *sk)
* Send an ICMP frame.
*/
-static inline bool icmpv4_xrlim_allow(struct net *net, struct rtable *rt,
- struct flowi4 *fl4, int type, int code)
+static bool icmpv4_xrlim_allow(struct net *net, struct rtable *rt,
+ struct flowi4 *fl4, int type, int code)
{
struct dst_entry *dst = &rt->dst;
bool rc = true;
@@ -375,19 +375,22 @@ out_unlock:
icmp_xmit_unlock(sk);
}
-static struct rtable *icmp_route_lookup(struct net *net,
- struct flowi4 *fl4,
- struct sk_buff *skb_in,
- const struct iphdr *iph,
- __be32 saddr, u8 tos,
- int type, int code,
- struct icmp_bxm *param)
+struct icmp_send_data {
+ struct icmp_bxm icmp_param;
+ struct ipcm_cookie ipc;
+ struct flowi4 fl4;
+};
+
+static noinline_for_stack struct rtable *
+icmp_route_lookup(struct net *net, struct flowi4 *fl4,
+ struct sk_buff *skb_in, const struct iphdr *iph,
+ __be32 saddr, u8 tos, int type, int code,
+ struct icmp_bxm *param)
{
struct rtable *rt, *rt2;
struct flowi4 fl4_dec;
int err;
- memset(fl4, 0, sizeof(*fl4));
fl4->daddr = (param->replyopts.opt.opt.srr ?
param->replyopts.opt.opt.faddr : iph->saddr);
fl4->saddr = saddr;
@@ -482,14 +485,12 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
{
struct iphdr *iph;
int room;
- struct icmp_bxm icmp_param;
struct rtable *rt = skb_rtable(skb_in);
- struct ipcm_cookie ipc;
- struct flowi4 fl4;
__be32 saddr;
u8 tos;
struct net *net;
struct sock *sk;
+ struct icmp_send_data *data = NULL;
if (!rt)
goto out;
@@ -585,7 +586,11 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
IPTOS_PREC_INTERNETCONTROL) :
iph->tos;
- if (ip_options_echo(&icmp_param.replyopts.opt.opt, skb_in))
+ data = kzalloc(sizeof(*data), GFP_ATOMIC);
+ if (!data)
+ goto out_unlock;
+
+ if (ip_options_echo(&data->icmp_param.replyopts.opt.opt, skb_in))
goto out_unlock;
@@ -593,23 +598,21 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
* Prepare data for ICMP header.
*/
- icmp_param.data.icmph.type = type;
- icmp_param.data.icmph.code = code;
- icmp_param.data.icmph.un.gateway = info;
- icmp_param.data.icmph.checksum = 0;
- icmp_param.skb = skb_in;
- icmp_param.offset = skb_network_offset(skb_in);
+ data->icmp_param.data.icmph.type = type;
+ data->icmp_param.data.icmph.code = code;
+ data->icmp_param.data.icmph.un.gateway = info;
+ data->icmp_param.skb = skb_in;
+ data->icmp_param.offset = skb_network_offset(skb_in);
inet_sk(sk)->tos = tos;
- ipc.addr = iph->saddr;
- ipc.opt = &icmp_param.replyopts.opt;
- ipc.tx_flags = 0;
+ data->ipc.addr = iph->saddr;
+ data->ipc.opt = &data->icmp_param.replyopts.opt;
- rt = icmp_route_lookup(net, &fl4, skb_in, iph, saddr, tos,
- type, code, &icmp_param);
+ rt = icmp_route_lookup(net, &data->fl4, skb_in, iph, saddr, tos,
+ type, code, &data->icmp_param);
if (IS_ERR(rt))
goto out_unlock;
- if (!icmpv4_xrlim_allow(net, rt, &fl4, type, code))
+ if (!icmpv4_xrlim_allow(net, rt, &data->fl4, type, code))
goto ende;
/* RFC says return as much as we can without exceeding 576 bytes. */
@@ -617,19 +620,20 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
room = dst_mtu(&rt->dst);
if (room > 576)
room = 576;
- room -= sizeof(struct iphdr) + icmp_param.replyopts.opt.opt.optlen;
+ room -= sizeof(struct iphdr) + data->icmp_param.replyopts.opt.opt.optlen;
room -= sizeof(struct icmphdr);
- icmp_param.data_len = skb_in->len - icmp_param.offset;
- if (icmp_param.data_len > room)
- icmp_param.data_len = room;
- icmp_param.head_len = sizeof(struct icmphdr);
+ data->icmp_param.data_len = skb_in->len - data->icmp_param.offset;
+ if (data->icmp_param.data_len > room)
+ data->icmp_param.data_len = room;
+ data->icmp_param.head_len = sizeof(struct icmphdr);
- icmp_push_reply(&icmp_param, &fl4, &ipc, &rt);
+ icmp_push_reply(&data->icmp_param, &data->fl4, &data->ipc, &rt);
ende:
ip_rt_put(rt);
out_unlock:
icmp_xmit_unlock(sk);
+ kfree(data);
out:;
}
EXPORT_SYMBOL(icmp_send);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch net-next] icmp: avoid allocating large struct on stack
2013-06-01 5:22 ` Eric Dumazet
@ 2013-06-02 1:14 ` Cong Wang
0 siblings, 0 replies; 5+ messages in thread
From: Cong Wang @ 2013-06-02 1:14 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David S. Miller
On Fri, 2013-05-31 at 22:22 -0700, Eric Dumazet wrote:
> On Sat, 2013-06-01 at 11:24 +0800, Cong Wang wrote:
> > From: Cong Wang <amwang@redhat.com>
> >
> > struct icmp_bxm is a large struct, reduce stack usage
> > by allocating it on heap.
> >
>
> Strange, I posted a patch like that some days ago.
Strange, why it isn't merged. :)
>
> Please.
>
> Read carefully this code and tell me if you do not leak memory.
Right, I missed some kfree.
>
> For the record my patch was saving more stack space :
Please submit it as a normal patch?
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-06-02 1:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-01 3:24 [Patch net-next] icmp: avoid allocating large struct on stack Cong Wang
2013-06-01 3:34 ` Joe Perches
2013-06-01 3:42 ` Cong Wang
2013-06-01 5:22 ` Eric Dumazet
2013-06-02 1:14 ` Cong Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).