netdev - Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver

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, 25 Feb 2008 13:05:08 +0000
From:	Jarek Poplawski <jarkao2@...il.com>
To:	James Chapman <jchapman@...alix.com>
Cc:	David Miller <davem@...emloft.net>,
	Paul Mackerras <paulus@...ba.org>, netdev@...r.kernel.org
Subject: Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver

On Mon, Feb 25, 2008 at 12:19:50PM +0000, James Chapman wrote:
> Jarek Poplawski wrote:
>> Jarek Poplawski wrote, On 02/21/2008 01:08 PM:
>> ...
>>
>>> Another, probably simpler way would be to move almost all pppol2tp_xmit
>> ...
>>
>> Actually, the simplest off all seems to be now this old idea to maybe make
>> sk_dst_lock globally softirq immune. At least I think it's worth of testing,
>> to check for these other possible lockdep warnings. It should only need to
>> change all write_ and read_lock(&sk->sk_dst_lock) in very few places:
>> include/net/sock.h, include/net/ip6_route.h, and net/ipv6/ipv6_sockglue.c.
>> This could be tested together with you full _bh locking patch (maybe except
>> these other changes in pppol2tp_xmit).
>
> I did this and all lockdep errors have now gone. Tests ran all weekend.  
> See attached patch.
>
> Is this an acceptable solution? If so, I'll prepare and send official  
> patches.

IMHO this should be acceptable because I can't see any reason for
changing properly working code if there is so simple and not costly
solution. But maybe David or somebody else finds some cons? Since
this patch isn't very big I think you could try to send this
officially and we will simply see...

Regards,
Jarek P.


>
>
> -- 
> James Chapman
> Katalix Systems Ltd
> http://www.katalix.com
> Catalysts for your Embedded Linux software development
>

> Index: linux-2.6.24.2/include/net/ip6_route.h
> ===================================================================
> --- linux-2.6.24.2.orig/include/net/ip6_route.h
> +++ linux-2.6.24.2/include/net/ip6_route.h
> @@ -150,9 +150,9 @@ static inline void __ip6_dst_store(struc
>  static inline void ip6_dst_store(struct sock *sk, struct dst_entry *dst,
>  				 struct in6_addr *daddr, struct in6_addr *saddr)
>  {
> -	write_lock(&sk->sk_dst_lock);
> +	write_lock_bh(&sk->sk_dst_lock);
>  	__ip6_dst_store(sk, dst, daddr, saddr);
> -	write_unlock(&sk->sk_dst_lock);
> +	write_unlock_bh(&sk->sk_dst_lock);
>  }
>  
>  static inline int ipv6_unicast_destination(struct sk_buff *skb)
> Index: linux-2.6.24.2/include/net/sock.h
> ===================================================================
> --- linux-2.6.24.2.orig/include/net/sock.h
> +++ linux-2.6.24.2/include/net/sock.h
> @@ -1058,11 +1058,11 @@ sk_dst_get(struct sock *sk)
>  {
>  	struct dst_entry *dst;
>  
> -	read_lock(&sk->sk_dst_lock);
> +	read_lock_bh(&sk->sk_dst_lock);
>  	dst = sk->sk_dst_cache;
>  	if (dst)
>  		dst_hold(dst);
> -	read_unlock(&sk->sk_dst_lock);
> +	read_unlock_bh(&sk->sk_dst_lock);
>  	return dst;
>  }
>  
> @@ -1079,9 +1079,9 @@ __sk_dst_set(struct sock *sk, struct dst
>  static inline void
>  sk_dst_set(struct sock *sk, struct dst_entry *dst)
>  {
> -	write_lock(&sk->sk_dst_lock);
> +	write_lock_bh(&sk->sk_dst_lock);
>  	__sk_dst_set(sk, dst);
> -	write_unlock(&sk->sk_dst_lock);
> +	write_unlock_bh(&sk->sk_dst_lock);
>  }
>  
>  static inline void
> @@ -1097,9 +1097,9 @@ __sk_dst_reset(struct sock *sk)
>  static inline void
>  sk_dst_reset(struct sock *sk)
>  {
> -	write_lock(&sk->sk_dst_lock);
> +	write_lock_bh(&sk->sk_dst_lock);
>  	__sk_dst_reset(sk);
> -	write_unlock(&sk->sk_dst_lock);
> +	write_unlock_bh(&sk->sk_dst_lock);
>  }
>  
>  extern struct dst_entry *__sk_dst_check(struct sock *sk, u32 cookie);
> Index: linux-2.6.24.2/net/ipv6/ipv6_sockglue.c
> ===================================================================
> --- linux-2.6.24.2.orig/net/ipv6/ipv6_sockglue.c
> +++ linux-2.6.24.2/net/ipv6/ipv6_sockglue.c
> @@ -440,9 +440,9 @@ static int do_ipv6_setsockopt(struct soc
>  			opt = xchg(&np->opt, opt);
>  			sk_dst_reset(sk);
>  		} else {
> -			write_lock(&sk->sk_dst_lock);
> +			write_lock_bh(&sk->sk_dst_lock);
>  			opt = xchg(&np->opt, opt);
> -			write_unlock(&sk->sk_dst_lock);
> +			write_unlock_bh(&sk->sk_dst_lock);
>  			sk_dst_reset(sk);
>  		}
>  sticky_done:
> @@ -504,9 +504,9 @@ update:
>  			opt = xchg(&np->opt, opt);
>  			sk_dst_reset(sk);
>  		} else {
> -			write_lock(&sk->sk_dst_lock);
> +			write_lock_bh(&sk->sk_dst_lock);
>  			opt = xchg(&np->opt, opt);
> -			write_unlock(&sk->sk_dst_lock);
> +			write_unlock_bh(&sk->sk_dst_lock);
>  			sk_dst_reset(sk);
>  		}
>  

--
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