* [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
@ 2012-07-02 3:18 roy.qing.li
2012-07-02 3:19 ` [PATCH net-next 2/2] dccp: remove unnecessary codes in ipv6.c roy.qing.li
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: roy.qing.li @ 2012-07-02 3:18 UTC (permalink / raw)
To: netdev
From: RongQing.Li <roy.qing.li@gmail.com>
opt always equals np->opts, so it is meaningless to define opt, and
check if opt does not equal np->opts and then try to free opt.
Signed-off-by: RongQing.Li <roy.qing.li@gmail.com>
---
net/ipv6/tcp_ipv6.c | 16 +++-------------
1 files changed, 3 insertions(+), 13 deletions(-)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 9c06eaf..6cc67ed 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -486,7 +486,6 @@ static int tcp_v6_send_synack(struct sock *sk, struct dst_entry *dst,
struct inet6_request_sock *treq = inet6_rsk(req);
struct ipv6_pinfo *np = inet6_sk(sk);
struct sk_buff * skb;
- struct ipv6_txoptions *opt = np->opt;
int err = -ENOMEM;
/* First, grab a route. */
@@ -500,13 +499,11 @@ static int tcp_v6_send_synack(struct sock *sk, struct dst_entry *dst,
fl6->daddr = treq->rmt_addr;
skb_set_queue_mapping(skb, queue_mapping);
- err = ip6_xmit(sk, skb, fl6, opt, np->tclass);
+ err = ip6_xmit(sk, skb, fl6, np->opt, np->tclass);
err = net_xmit_eval(err);
}
done:
- if (opt && opt != np->opt)
- sock_kfree_s(sk, opt, opt->tot_len);
return err;
}
@@ -1229,7 +1226,6 @@ static struct sock * tcp_v6_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
struct inet_sock *newinet;
struct tcp_sock *newtp;
struct sock *newsk;
- struct ipv6_txoptions *opt;
#ifdef CONFIG_TCP_MD5SIG
struct tcp_md5sig_key *key;
#endif
@@ -1290,7 +1286,6 @@ static struct sock * tcp_v6_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
}
treq = inet6_rsk(req);
- opt = np->opt;
if (sk_acceptq_is_full(sk))
goto out_overflow;
@@ -1359,11 +1354,8 @@ static struct sock * tcp_v6_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
but we make one more one thing there: reattach optmem
to newsk.
*/
- if (opt) {
- newnp->opt = ipv6_dup_options(newsk, opt);
- if (opt != np->opt)
- sock_kfree_s(sk, opt, opt->tot_len);
- }
+ if (np->opt)
+ newnp->opt = ipv6_dup_options(newsk, np->opt);
inet_csk(newsk)->icsk_ext_hdr_len = 0;
if (newnp->opt)
@@ -1410,8 +1402,6 @@ static struct sock * tcp_v6_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
out_overflow:
NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS);
out_nonewsk:
- if (opt && opt != np->opt)
- sock_kfree_s(sk, opt, opt->tot_len);
dst_release(dst);
out:
NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_LISTENDROPS);
--
1.7.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next 2/2] dccp: remove unnecessary codes in ipv6.c
2012-07-02 3:18 [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c roy.qing.li
@ 2012-07-02 3:19 ` roy.qing.li
2012-07-02 3:26 ` David Miller
2012-07-02 9:08 ` Eric Dumazet
2012-07-02 3:26 ` [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c David Miller
2012-07-02 9:07 ` Eric Dumazet
2 siblings, 2 replies; 12+ messages in thread
From: roy.qing.li @ 2012-07-02 3:19 UTC (permalink / raw)
To: netdev
From: RongQing.Li <roy.qing.li@gmail.com>
opt always equals np->opts, so it is meaningless to define opt, and
check if opt does not equal np->opts and then try to free opt.
Signed-off-by: RongQing.Li <roy.qing.li@gmail.com>
---
net/dccp/ipv6.c | 21 +++++----------------
1 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index 9991be0..02162cf 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -239,7 +239,6 @@ static int dccp_v6_send_response(struct sock *sk, struct request_sock *req,
struct inet6_request_sock *ireq6 = inet6_rsk(req);
struct ipv6_pinfo *np = inet6_sk(sk);
struct sk_buff *skb;
- struct ipv6_txoptions *opt = NULL;
struct in6_addr *final_p, final;
struct flowi6 fl6;
int err = -1;
@@ -255,9 +254,8 @@ static int dccp_v6_send_response(struct sock *sk, struct request_sock *req,
fl6.fl6_sport = inet_rsk(req)->loc_port;
security_req_classify_flow(req, flowi6_to_flowi(&fl6));
- opt = np->opt;
- final_p = fl6_update_dst(&fl6, opt, &final);
+ final_p = fl6_update_dst(&fl6, np->opt, &final);
dst = ip6_dst_lookup_flow(sk, &fl6, final_p, false);
if (IS_ERR(dst)) {
@@ -274,13 +272,11 @@ static int dccp_v6_send_response(struct sock *sk, struct request_sock *req,
&ireq6->loc_addr,
&ireq6->rmt_addr);
fl6.daddr = ireq6->rmt_addr;
- err = ip6_xmit(sk, skb, &fl6, opt, np->tclass);
+ err = ip6_xmit(sk, skb, &fl6, np->opt, np->tclass);
err = net_xmit_eval(err);
}
done:
- if (opt != NULL && opt != np->opt)
- sock_kfree_s(sk, opt, opt->tot_len);
dst_release(dst);
return err;
}
@@ -475,7 +471,6 @@ static struct sock *dccp_v6_request_recv_sock(struct sock *sk,
struct inet_sock *newinet;
struct dccp6_sock *newdp6;
struct sock *newsk;
- struct ipv6_txoptions *opt;
if (skb->protocol == htons(ETH_P_IP)) {
/*
@@ -520,7 +515,6 @@ static struct sock *dccp_v6_request_recv_sock(struct sock *sk,
return newsk;
}
- opt = np->opt;
if (sk_acceptq_is_full(sk))
goto out_overflow;
@@ -532,7 +526,7 @@ static struct sock *dccp_v6_request_recv_sock(struct sock *sk,
memset(&fl6, 0, sizeof(fl6));
fl6.flowi6_proto = IPPROTO_DCCP;
fl6.daddr = ireq6->rmt_addr;
- final_p = fl6_update_dst(&fl6, opt, &final);
+ final_p = fl6_update_dst(&fl6, np->opt, &final);
fl6.saddr = ireq6->loc_addr;
fl6.flowi6_oif = sk->sk_bound_dev_if;
fl6.fl6_dport = inet_rsk(req)->rmt_port;
@@ -597,11 +591,8 @@ static struct sock *dccp_v6_request_recv_sock(struct sock *sk,
* Yes, keeping reference count would be much more clever, but we make
* one more one thing there: reattach optmem to newsk.
*/
- if (opt != NULL) {
- newnp->opt = ipv6_dup_options(newsk, opt);
- if (opt != np->opt)
- sock_kfree_s(sk, opt, opt->tot_len);
- }
+ if (np->opt != NULL)
+ newnp->opt = ipv6_dup_options(newsk, np->opt);
inet_csk(newsk)->icsk_ext_hdr_len = 0;
if (newnp->opt != NULL)
@@ -627,8 +618,6 @@ out_nonewsk:
dst_release(dst);
out:
NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_LISTENDROPS);
- if (opt != NULL && opt != np->opt)
- sock_kfree_s(sk, opt, opt->tot_len);
return NULL;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
2012-07-02 3:18 [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c roy.qing.li
2012-07-02 3:19 ` [PATCH net-next 2/2] dccp: remove unnecessary codes in ipv6.c roy.qing.li
@ 2012-07-02 3:26 ` David Miller
2012-07-02 5:23 ` RongQing Li
2012-07-02 8:13 ` Eric Dumazet
2012-07-02 9:07 ` Eric Dumazet
2 siblings, 2 replies; 12+ messages in thread
From: David Miller @ 2012-07-02 3:26 UTC (permalink / raw)
To: roy.qing.li; +Cc: netdev
From: roy.qing.li@gmail.com
Date: Mon, 2 Jul 2012 11:18:59 +0800
> - if (opt) {
> - newnp->opt = ipv6_dup_options(newsk, opt);
> - if (opt != np->opt)
> - sock_kfree_s(sk, opt, opt->tot_len);
This is bogus, if we copy the options into a new copy in
ipv6_dup_options() we have to free the old one or else we
leak it.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] dccp: remove unnecessary codes in ipv6.c
2012-07-02 3:19 ` [PATCH net-next 2/2] dccp: remove unnecessary codes in ipv6.c roy.qing.li
@ 2012-07-02 3:26 ` David Miller
2012-07-02 9:08 ` Eric Dumazet
1 sibling, 0 replies; 12+ messages in thread
From: David Miller @ 2012-07-02 3:26 UTC (permalink / raw)
To: roy.qing.li; +Cc: netdev
From: roy.qing.li@gmail.com
Date: Mon, 2 Jul 2012 11:19:00 +0800
> - if (opt != NULL) {
> - newnp->opt = ipv6_dup_options(newsk, opt);
> - if (opt != np->opt)
> - sock_kfree_s(sk, opt, opt->tot_len);
> - }
Same bug as your other patch, this leaks options.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
2012-07-02 3:26 ` [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c David Miller
@ 2012-07-02 5:23 ` RongQing Li
2012-07-02 5:37 ` David Miller
2012-07-02 8:13 ` Eric Dumazet
1 sibling, 1 reply; 12+ messages in thread
From: RongQing Li @ 2012-07-02 5:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev
2012/7/2 David Miller <davem@davemloft.net>:
> From: roy.qing.li@gmail.com
> Date: Mon, 2 Jul 2012 11:18:59 +0800
>
>> - if (opt) {
>> - newnp->opt = ipv6_dup_options(newsk, opt);
>> - if (opt != np->opt)
>> - sock_kfree_s(sk, opt, opt->tot_len);
>
> This is bogus, if we copy the options into a new copy in
> ipv6_dup_options() we have to free the old one or else we
> leak it.
Do you mean I should free newnp->opt firstly ?
If I understand it right, I think we do not need to free it. the
process is below:
newsk = tcp_v4_syn_recv_sock(sk, skb, req, dst);
..
newnp = inet6_sk(newsk);
..
memcpy(newnp, np, sizeof(struct ipv6_pinfo));
..
newnp->opt = NULL;
So newnp->opt is not a effective memory.
Thanks.
-Roy
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
2012-07-02 5:23 ` RongQing Li
@ 2012-07-02 5:37 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2012-07-02 5:37 UTC (permalink / raw)
To: roy.qing.li; +Cc: netdev
From: RongQing Li <roy.qing.li@gmail.com>
Date: Mon, 2 Jul 2012 13:23:09 +0800
> 2012/7/2 David Miller <davem@davemloft.net>:
>> From: roy.qing.li@gmail.com
>> Date: Mon, 2 Jul 2012 11:18:59 +0800
>>
>>> - if (opt) {
>>> - newnp->opt = ipv6_dup_options(newsk, opt);
>>> - if (opt != np->opt)
>>> - sock_kfree_s(sk, opt, opt->tot_len);
>>
>> This is bogus, if we copy the options into a new copy in
>> ipv6_dup_options() we have to free the old one or else we
>> leak it.
>
> Do you mean I should free newnp->opt firstly ?
>
> If I understand it right, I think we do not need to free it. the
> process is below:
>
> newsk = tcp_v4_syn_recv_sock(sk, skb, req, dst);
> ..
> newnp = inet6_sk(newsk);
> ..
> memcpy(newnp, np, sizeof(struct ipv6_pinfo));
> ..
> newnp->opt = NULL;
>
> So newnp->opt is not a effective memory.
ipv6_dup_options() allocates new memory for the options and this call
statement assigns that new pointer to np->opt.
If you do not free the old (before ipv6_dup_options()) np->opt memory
here, it is lost forever.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
2012-07-02 3:26 ` [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c David Miller
2012-07-02 5:23 ` RongQing Li
@ 2012-07-02 8:13 ` Eric Dumazet
2012-07-02 8:54 ` Eric Dumazet
1 sibling, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2012-07-02 8:13 UTC (permalink / raw)
To: David Miller; +Cc: roy.qing.li, netdev
On Sun, 2012-07-01 at 20:26 -0700, David Miller wrote:
> From: roy.qing.li@gmail.com
> Date: Mon, 2 Jul 2012 11:18:59 +0800
>
> > - if (opt) {
> > - newnp->opt = ipv6_dup_options(newsk, opt);
> > - if (opt != np->opt)
> > - sock_kfree_s(sk, opt, opt->tot_len);
>
> This is bogus, if we copy the options into a new copy in
> ipv6_dup_options() we have to free the old one or else we
> leak it.
Note that the old one is the np->opt of the listener, not the child.
I dont understand how np->opt could change under us, since we have
the listener socket locked.
If np->opt can change under us, we are doomed and need to add refcounts.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
2012-07-02 8:13 ` Eric Dumazet
@ 2012-07-02 8:54 ` Eric Dumazet
0 siblings, 0 replies; 12+ messages in thread
From: Eric Dumazet @ 2012-07-02 8:54 UTC (permalink / raw)
To: David Miller; +Cc: roy.qing.li, netdev
On Mon, 2012-07-02 at 10:13 +0200, Eric Dumazet wrote:
> Note that the old one is the np->opt of the listener, not the child.
>
> I dont understand how np->opt could change under us, since we have
> the listener socket locked.
>
> If np->opt can change under us, we are doomed and need to add refcounts.
Hmm, it seems net/ipv6/udp.c uses np->opt outside of the
lock_sock()/release_sock(sk) boundary.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
2012-07-02 3:18 [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c roy.qing.li
2012-07-02 3:19 ` [PATCH net-next 2/2] dccp: remove unnecessary codes in ipv6.c roy.qing.li
2012-07-02 3:26 ` [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c David Miller
@ 2012-07-02 9:07 ` Eric Dumazet
2012-07-05 10:13 ` David Miller
2 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2012-07-02 9:07 UTC (permalink / raw)
To: roy.qing.li; +Cc: netdev
On Mon, 2012-07-02 at 11:18 +0800, roy.qing.li@gmail.com wrote:
> From: RongQing.Li <roy.qing.li@gmail.com>
>
> opt always equals np->opts, so it is meaningless to define opt, and
> check if opt does not equal np->opts and then try to free opt.
>
> Signed-off-by: RongQing.Li <roy.qing.li@gmail.com>
> ---
> net/ipv6/tcp_ipv6.c | 16 +++-------------
> 1 files changed, 3 insertions(+), 13 deletions(-)
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] dccp: remove unnecessary codes in ipv6.c
2012-07-02 3:19 ` [PATCH net-next 2/2] dccp: remove unnecessary codes in ipv6.c roy.qing.li
2012-07-02 3:26 ` David Miller
@ 2012-07-02 9:08 ` Eric Dumazet
2012-07-05 10:13 ` David Miller
1 sibling, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2012-07-02 9:08 UTC (permalink / raw)
To: roy.qing.li; +Cc: netdev
On Mon, 2012-07-02 at 11:19 +0800, roy.qing.li@gmail.com wrote:
> From: RongQing.Li <roy.qing.li@gmail.com>
>
> opt always equals np->opts, so it is meaningless to define opt, and
> check if opt does not equal np->opts and then try to free opt.
>
> Signed-off-by: RongQing.Li <roy.qing.li@gmail.com>
> ---
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
2012-07-02 9:07 ` Eric Dumazet
@ 2012-07-05 10:13 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2012-07-05 10:13 UTC (permalink / raw)
To: eric.dumazet; +Cc: roy.qing.li, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Jul 2012 11:07:47 +0200
> On Mon, 2012-07-02 at 11:18 +0800, roy.qing.li@gmail.com wrote:
>> From: RongQing.Li <roy.qing.li@gmail.com>
>>
>> opt always equals np->opts, so it is meaningless to define opt, and
>> check if opt does not equal np->opts and then try to free opt.
>>
>> Signed-off-by: RongQing.Li <roy.qing.li@gmail.com>
>> ---
>> net/ipv6/tcp_ipv6.c | 16 +++-------------
>> 1 files changed, 3 insertions(+), 13 deletions(-)
>
> Acked-by: Eric Dumazet <edumazet@google.com>
Ok I now understand better why these changes are correct,
applied.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] dccp: remove unnecessary codes in ipv6.c
2012-07-02 9:08 ` Eric Dumazet
@ 2012-07-05 10:13 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2012-07-05 10:13 UTC (permalink / raw)
To: eric.dumazet; +Cc: roy.qing.li, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Jul 2012 11:08:50 +0200
> On Mon, 2012-07-02 at 11:19 +0800, roy.qing.li@gmail.com wrote:
>> From: RongQing.Li <roy.qing.li@gmail.com>
>>
>> opt always equals np->opts, so it is meaningless to define opt, and
>> check if opt does not equal np->opts and then try to free opt.
>>
>> Signed-off-by: RongQing.Li <roy.qing.li@gmail.com>
>> ---
>
> Acked-by: Eric Dumazet <edumazet@google.com>
Also applied, thank you.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-07-05 10:13 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-02 3:18 [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c roy.qing.li
2012-07-02 3:19 ` [PATCH net-next 2/2] dccp: remove unnecessary codes in ipv6.c roy.qing.li
2012-07-02 3:26 ` David Miller
2012-07-02 9:08 ` Eric Dumazet
2012-07-05 10:13 ` David Miller
2012-07-02 3:26 ` [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c David Miller
2012-07-02 5:23 ` RongQing Li
2012-07-02 5:37 ` David Miller
2012-07-02 8:13 ` Eric Dumazet
2012-07-02 8:54 ` Eric Dumazet
2012-07-02 9:07 ` Eric Dumazet
2012-07-05 10:13 ` David Miller
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.