netdev - Re: [PATCH] tcp: properly update lost_cnt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 28 Sep 2011 17:12:23 -0700
From:	Nandita Dukkipati <nanditad@...gle.com>
To:	"Yan, Zheng" <zheng.z.yan@...el.com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Yuchung Cheng <ycheng@...gle.com>,
	Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
Subject: Re: [PATCH] tcp: properly update lost_cnt_hint during shifting

Actually don't bother about my question. Your patch is correct. I
convinced myself that it's taking care of diff. cases correctly.

Nandita

On Wed, Sep 28, 2011 at 5:06 PM, Nandita Dukkipati <nanditad@...gle.com> wrote:
> Could you please clarify this case for me-
>
> skb == tp->lost_skb_hint
> If skb is sacked, doesn't tcp_mark_head_lost() already increment
> lost_cnt_hint, in which case you won't need to do it here?
>
> Nandita
>
> On Wed, Sep 28, 2011 at 4:29 AM, Ilpo Järvinen
> <ilpo.jarvinen@...sinki.fi> wrote:
>> On Wed, 28 Sep 2011, Yan, Zheng wrote:
>>
>>> > But is the non-SACKed case really handled right when hint == skb by the
>>> > sacktag_one. We move the seqno in between and then before(x->newseq,
>>> > x->newseq) check returns false?
>>> >
>>> you are right, thank you.
>>>
>>> really hope my patch is correct this time :)
>>> ---
>>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>>> index 21fab3e..a04622e 100644
>>> --- a/net/ipv4/tcp_input.c
>>> +++ b/net/ipv4/tcp_input.c
>>> @@ -1390,8 +1390,7 @@ static int tcp_shifted_skb(struct sock *sk, struct sk_buff *skb,
>>>       BUG_ON(!pcount);
>>>
>>>       /* Tweak before seqno plays */
>>> -     if (!tcp_is_fack(tp) && tcp_is_sack(tp) && tp->lost_skb_hint &&
>>> -         !before(TCP_SKB_CB(tp->lost_skb_hint)->seq, TCP_SKB_CB(skb)->seq))
>>> +     if (!tcp_is_fack(tp) && tcp_is_sack(tp) && tp->lost_skb_hint == skb)
>>>               tp->lost_cnt_hint += pcount;
>>>
>>>       TCP_SKB_CB(prev)->end_seq += shifted;
>>
>> It also looks a lot nicer now and more obvious. According to my current
>> understanding, feel free to add this once doing the proper submission with
>> Signed-off etc. (please also remove the comment too as seqnos have no
>> longer any significance here):
>>
>> Acked-by: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
>>
>> ...but it certainly wouldn't hurt if also somebody else has pair of eyes
>> to spare to confirm that we (both) are now in agreement what the code
>> really says.
>>
>>
>> --
>>  i.
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html