netdev - Re: [PATCH RFC net-next 2/7] net: add dst_pending

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 19 Dec 2016 17:53:34 +0100
From:   Hannes Frederic Sowa <hannes@...essinduktion.org>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Julian Anastasov <ja@....bg>, netdev@...r.kernel.org,
        linux-sctp@...r.kernel.org, YueHaibing <yuehaibing@...wei.com>
Subject: Re: [PATCH RFC net-next 2/7] net: add dst_pending_confirm flag to
 skbuff

On 19.12.2016 17:40, Eric Dumazet wrote:
> On Mon, 2016-12-19 at 17:36 +0100, Hannes Frederic Sowa wrote:
>> On 19.12.2016 17:17, Eric Dumazet wrote:
>>> On Sun, 2016-12-18 at 22:56 +0200, Julian Anastasov wrote:
>>>
>>>>  
>>>> +static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
>>>> +{
>>>> +	if (unlikely(skb->dst_pending_confirm)) {
>>>> +		struct sock *sk = skb->sk;
>>>> +		unsigned long now = jiffies;
>>>> +
>>>> +		/* avoid dirtying neighbour */
>>>> +		if (n->confirmed != now)
>>>> +			n->confirmed = now;
>>>> +		if (sk && sk->sk_dst_pending_confirm)
>>>> +			sk->sk_dst_pending_confirm = 0;
>>>> +	}
>>>> +}
>>>> +
>>>
>>> I am still digesting this awesome patch series ;)
>>>
>>> Not sure why you used an unlikely() here. TCP for example would hit this
>>> path quite often.
>>>
>>> So considering sk_dst_pending_confirm might be dirtied quite often,
>>>
>>> I am not sure why you placed it in the cache line that contains
>>> sk_rx_dst (in 1st patch)
>>
>> Because they have to stay synchronized?
>>
>> If we modify sk_rx_dst, we automatically also must clear
>> pending_confirm, otherwise we might end up confirming a wrong neighbor.
> 
> Your answer makes little sense really...
> 
> For most TCP flows, we set sk_rx_dst exactly once.
> 
> Hardly a good reason to have these in the same cache line.

Right :) , and I didn't actually wanted to make an argument in favor of
that. Just noted they are probably semantically grouped together as an
explanation.

Sorry,
Hannes