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]

Message-ID: <1482165628.1521.9.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Mon, 19 Dec 2016 08:40:28 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Hannes Frederic Sowa <hannes@...essinduktion.org>
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 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.