* [Patch net] ipv4: clear all TCP_SKB_CB before passing to network layer
@ 2014-10-17 17:49 Cong Wang
2014-10-17 18:18 ` Eric Dumazet
0 siblings, 1 reply; 3+ messages in thread
From: Cong Wang @ 2014-10-17 17:49 UTC (permalink / raw)
To: netdev; +Cc: davem, Cong Wang, Krzysztof Kolasa, Eric Dumazet, Cong Wang
From: Cong Wang <cwang@twopensource.com>
Probably not a big deal, but IP is not the only network protocol,
don't clear skb->cb just for IP.
Also, IPv6 header is not always defined in struct tcp_skb_cb.
Cc: Krzysztof Kolasa <kkolasa@winsoft.pl>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Cong Wang <cwang@twopensource.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/ipv4/tcp_output.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e13d778..ee356e5 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1005,9 +1005,7 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
/* Our usage of tstamp should remain private */
skb->tstamp.tv64 = 0;
- /* Cleanup our debris for IP stacks */
- memset(skb->cb, 0, max(sizeof(struct inet_skb_parm),
- sizeof(struct inet6_skb_parm)));
+ memset(TCP_SKB_CB(skb), 0, sizeof(*TCP_SKB_CB(skb)));
err = icsk->icsk_af_ops->queue_xmit(sk, skb, &inet->cork.fl);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Patch net] ipv4: clear all TCP_SKB_CB before passing to network layer
2014-10-17 17:49 [Patch net] ipv4: clear all TCP_SKB_CB before passing to network layer Cong Wang
@ 2014-10-17 18:18 ` Eric Dumazet
2014-10-17 18:26 ` Cong Wang
0 siblings, 1 reply; 3+ messages in thread
From: Eric Dumazet @ 2014-10-17 18:18 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, davem, Krzysztof Kolasa, Eric Dumazet, Cong Wang
On Fri, 2014-10-17 at 10:49 -0700, Cong Wang wrote:
> From: Cong Wang <cwang@twopensource.com>
>
> Probably not a big deal, but IP is not the only network protocol,
> don't clear skb->cb just for IP.
>
> Also, IPv6 header is not always defined in struct tcp_skb_cb.
>
> Cc: Krzysztof Kolasa <kkolasa@winsoft.pl>
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Cong Wang <cwang@twopensource.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> net/ipv4/tcp_output.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index e13d778..ee356e5 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1005,9 +1005,7 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
> /* Our usage of tstamp should remain private */
> skb->tstamp.tv64 = 0;
>
> - /* Cleanup our debris for IP stacks */
> - memset(skb->cb, 0, max(sizeof(struct inet_skb_parm),
> - sizeof(struct inet6_skb_parm)));
> + memset(TCP_SKB_CB(skb), 0, sizeof(*TCP_SKB_CB(skb)));
>
> err = icsk->icsk_af_ops->queue_xmit(sk, skb, &inet->cork.fl);
>
Usually, each layer is responsible for clearing skb->cb[] at its entry
point. Or more exactly it does not care of previous garbage.
There is no evidence your patch is needed.
I was maybe too defensive when I added this, because I wanted to make
only TCP changes.
We should instead remove the memset() in TCP and fix IP/IPv6 if
necessary.
But this should wait net-next being open.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Patch net] ipv4: clear all TCP_SKB_CB before passing to network layer
2014-10-17 18:18 ` Eric Dumazet
@ 2014-10-17 18:26 ` Cong Wang
0 siblings, 0 replies; 3+ messages in thread
From: Cong Wang @ 2014-10-17 18:26 UTC (permalink / raw)
To: Eric Dumazet
Cc: Cong Wang, netdev, David Miller, Krzysztof Kolasa, Eric Dumazet
On Fri, Oct 17, 2014 at 11:18 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2014-10-17 at 10:49 -0700, Cong Wang wrote:
>> From: Cong Wang <cwang@twopensource.com>
>>
>> Probably not a big deal, but IP is not the only network protocol,
>> don't clear skb->cb just for IP.
>>
>> Also, IPv6 header is not always defined in struct tcp_skb_cb.
>>
>> Cc: Krzysztof Kolasa <kkolasa@winsoft.pl>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Signed-off-by: Cong Wang <cwang@twopensource.com>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ---
>> net/ipv4/tcp_output.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>> index e13d778..ee356e5 100644
>> --- a/net/ipv4/tcp_output.c
>> +++ b/net/ipv4/tcp_output.c
>> @@ -1005,9 +1005,7 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
>> /* Our usage of tstamp should remain private */
>> skb->tstamp.tv64 = 0;
>>
>> - /* Cleanup our debris for IP stacks */
>> - memset(skb->cb, 0, max(sizeof(struct inet_skb_parm),
>> - sizeof(struct inet6_skb_parm)));
>> + memset(TCP_SKB_CB(skb), 0, sizeof(*TCP_SKB_CB(skb)));
>>
>> err = icsk->icsk_af_ops->queue_xmit(sk, skb, &inet->cork.fl);
>>
>
>
> Usually, each layer is responsible for clearing skb->cb[] at its entry
> point. Or more exactly it does not care of previous garbage.
>
> There is no evidence your patch is needed.
This is why I said "probably not a big deal" in changelog, I never
say it fixes anything, just a cleanup.
>
> I was maybe too defensive when I added this, because I wanted to make
> only TCP changes.
>
> We should instead remove the memset() in TCP and fix IP/IPv6 if
> necessary.
That works too and sounds better.
>
> But this should wait net-next being open.
>
>
Yeah, agreed, I will update and resend when net-next is reopen.
Thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-10-17 18:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-17 17:49 [Patch net] ipv4: clear all TCP_SKB_CB before passing to network layer Cong Wang
2014-10-17 18:18 ` Eric Dumazet
2014-10-17 18:26 ` 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).