netdev - Re: [PATCH] cnic: don't take the rtnl_read_lock in cnic_rcv

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:	Fri, 30 May 2014 10:58:11 -0700
From:	Michael Chan <mchan@...adcom.com>
To:	Neil Horman <nhorman@...driver.com>
CC:	<netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>,
	<fcoe-devel@...n-fcoe.org>, Robert Love <robert.w.love@...el.com>,
	Vasu Dev <vasu.dev@...el.com>
Subject: Re: [PATCH] cnic: don't take the rtnl_read_lock in
 cnic_rcv_netevent

On Fri, 2014-05-30 at 11:00 -0400, Neil Horman wrote: 
> The Cnic driver handles lots of ulp operations in its netdevice event hanlder.
> To do this, it accesses the ulp_ops array, which is an rcu protected array.
> However, some ulp operations (like bnx2fc_indicate_netevent) try to lock
> mutexes, which might sleep (somthing that you can't do while holding rcu read
> side locks if you've configured non-preemptive rcu.
> 
> Fix this by changing the dereference method.  All accesses to the ulp_ops array
> for a cnic dev are modified under the protection of the rtnl lock, and so we can
> safely just use rcu_dereference_rtnl, and remove the rcu_read_lock here

Because the bnx2fc function can sleep, we need a more complete fix to
prevent the ulp_ops from going away when the device is unregistered.
synchronize_rcu() won't be able to protect it.  I'll post the patch
later today.  Thanks.

> 
> Signed-off-by: Neil Horman <nhorman@...driver.com>
> CC: "David S. Miller" <davem@...emloft.net>
> CC: Michael Chan <mchan@...adcom.com>
> CC: fcoe-devel@...n-fcoe.org
> CC: Robert Love <robert.w.love@...el.com>
> CC: Vasu Dev <vasu.dev@...el.com>
> ---
>  drivers/net/ethernet/broadcom/cnic.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
> index f58a8b8..bfdb9f0 100644
> --- a/drivers/net/ethernet/broadcom/cnic.c
> +++ b/drivers/net/ethernet/broadcom/cnic.c
> @@ -5622,12 +5622,11 @@ static void cnic_rcv_netevent(struct cnic_local *cp, unsigned long event,
>  {
>  	int if_type;
>  
> -	rcu_read_lock();
>  	for (if_type = 0; if_type < MAX_CNIC_ULP_TYPE; if_type++) {
>  		struct cnic_ulp_ops *ulp_ops;
>  		void *ctx;
>  
> -		ulp_ops = rcu_dereference(cp->ulp_ops[if_type]);
> +		ulp_ops = rcu_dereference_rtnl(cp->ulp_ops[if_type]);
>  		if (!ulp_ops || !ulp_ops->indicate_netevent)
>  			continue;
>  
> @@ -5635,7 +5634,6 @@ static void cnic_rcv_netevent(struct cnic_local *cp, unsigned long event,
>  
>  		ulp_ops->indicate_netevent(ctx, event, vlan_id);
>  	}
> -	rcu_read_unlock();
>  }
>  
>  /* netdev event handler */


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