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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 18 Jul 2012 10:28:26 +0300 (EEST)
From:	Julian Anastasov <ja@....bg>
To:	Eric Dumazet <eric.dumazet@...il.com>
cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH 0/5] Long term PMTU/redirect storage in ipv4.


	Hello,

On Wed, 18 Jul 2012, Eric Dumazet wrote:

> Hi Julian
> 
> I find this patch too complex.
> 
> You could only change fnhe_lock to a seqlock

	I'll change it, I was not sure if we are going to
use some array of seqlocks and also without adding locks in
the struct fib_nh.

> -static DEFINE_SPINLOCK(fnhe_lock);
> +static DEFINE_SEQLOCK(fnhe_seqlock);
>  
>  static struct fib_nh_exception *fnhe_oldest(struct fnhe_hash_bucket *hash, __be32 daddr)
>  {
> @@ -1454,11 +1454,11 @@ static void __ip_do_redirect(struct rtable *rt, struct sk_buff *skb, struct flow
>  				struct fib_nh *nh = &FIB_RES_NH(res);
>  				struct fib_nh_exception *fnhe;
>  
> -				spin_lock_bh(&fnhe_lock);
> +				write_seqlock_bh(&fnhe_seqlock);
>  				fnhe = find_or_create_fnhe(nh, fl4->daddr);
>  				if (fnhe)
>  					fnhe->fnhe_gw = new_gw;
> -				spin_unlock_bh(&fnhe_lock);
> +				write_sequnlock_bh(&fnhe_seqlock);
>  			}
>  			rt->rt_gateway = new_gw;
>  			rt->rt_flags |= RTCF_REDIRECTED;
> @@ -1665,13 +1665,13 @@ static void __ip_rt_update_pmtu(struct rtable *rt, struct flowi4 *fl4, u32 mtu)
>  		struct fib_nh *nh = &FIB_RES_NH(res);
>  		struct fib_nh_exception *fnhe;
>  
> -		spin_lock_bh(&fnhe_lock);
> +		write_seqlock_bh(&fnhe_seqlock);
>  		fnhe = find_or_create_fnhe(nh, fl4->daddr);
>  		if (fnhe) {
>  			fnhe->fnhe_pmtu = mtu;
>  			fnhe->fnhe_expires = jiffies + ip_rt_mtu_expires;
>  		}
> -		spin_unlock_bh(&fnhe_lock);
> +		write_sequnlock_bh(&fnhe_seqlock);
>  	}
>  	rt->rt_pmtu = mtu;
>  	dst_set_expires(&rt->dst, ip_rt_mtu_expires);
> @@ -1904,18 +1904,29 @@ static void rt_bind_exception(struct rtable *rt, struct fib_nh *nh, __be32 daddr
>  
>  	for (fnhe = rcu_dereference(hash[hval].chain); fnhe;
>  	     fnhe = rcu_dereference(fnhe->fnhe_next)) {
> -		if (fnhe->fnhe_daddr == daddr) {
> -			if (fnhe->fnhe_pmtu) {
> -				unsigned long expires = fnhe->fnhe_expires;
> -				unsigned long diff = jiffies - expires;
> +		unsigned int seq;
> +		__be32 fnhe_daddr, gw;
> +		u32 pmtu;
> +		unsigned long expires;
> +
> +		do {
> +			seq = read_seqbegin(&fnhe_seqlock);
> +			fnhe_daddr = fnhe->fnhe_daddr;
> +			gw = fnhe->fnhe_gw;
> +			pmtu = fnhe->fnhe_pmtu;
> +			expires = fnhe->fnhe_expires;
> +		} while (read_seqretry(&fnhe_seqlock, seq));

	This is going to read all values in the chain
before reaching daddr? Or may be FNHE_RECLAIM_DEPTH is
small and nobody will increase it. May be I can create
some func that searches daddr in chain instead. Do you still
prefer to remove the first daddr check or it is only
that the code is intended too much?

> +		if (fnhe_daddr == daddr) {

	Also, do we need some rcu locking in
__ip_rt_update_pmtu or may be ipv4_update_pmtu is
called always under rcu lock?

Regards

--
Julian Anastasov <ja@....bg>
--
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

Powered by blists - more mailing lists