* [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).