rcu read-side protection

rcu read-side protection

[Suzanne Wood]

In dn_neigh_construct() of linux-2.6.12/net/decnet/dn_neigh.c

static int dn_neigh_construct(struct neighbour *neigh)
{
 struct net_device *dev = neigh->dev;
 struct dn_neigh *dn = (struct dn_neigh *)neigh;
 struct dn_dev *dn_db;
 struct neigh_parms *parms;

 rcu_read_lock();
 dn_db = rcu_dereference(dev->dn_ptr);
 if (dn_db == NULL) {
  rcu_read_unlock();
  return -EINVAL;
 }

 parms = dn_db->neigh_parms;
 if (!parms) {
  rcu_read_unlock();
  return -EINVAL;
 }

 __neigh_parms_put(neigh->parms);
 neigh->parms = neigh_parms_clone(parms);
 rcu_read_unlock();

 if (dn_db->use_long)
  neigh->ops = &dn_long_ops;
 else
  neigh->ops = &dn_short_ops;

 if (dn->flags & DN_NDFLAG_P3)
  neigh->ops = &dn_phase3_ops;

 neigh->nud_state = NUD_NOARP;
 neigh->output = neigh->ops->connected_output;

 if ((dev->type == ARPHRD_IPGRE) || (dev->flags & IFF_POINTOPOINT))
  memcpy(neigh->ha, dev->broadcast, dev->addr_len);
 else if ((dev->type == ARPHRD_ETHER) || (dev->type == ARPHRD_LOOPBACK))
  dn_dn2eth(neigh->ha, dn->addr);
 else {
  if (net_ratelimit())
   printk(KERN_DEBUG "Trying to create neigh for hw %d\n",  dev->type);
  return -EINVAL;
 }

A read-side critical section is marked to protect the dereference of the 
dn_ptr and assignment to dn_db which is a pointer to a dn_dev.  (struct 
net_device is defined in /linux/netdevice.h and its dn_ptr in 
/include/net/dn_dev.h)  Should this rcu-protection be extended to the line 
following rcu_read_lock()?  Even though use_long is a simple char, it 
appears to be a member of an rcu-protected structure.

Thank you.

Re: rcu read-side protection

[Paul E. McKenney]

On Tue, Aug 16, 2005 at 05:09:29PM -0700, Suzanne Wood wrote:
[ . . . ]
> A read-side critical section is marked to protect the dereference of the 
> dn_ptr and assignment to dn_db which is a pointer to a dn_dev.  (struct 
> net_device is defined in /linux/netdevice.h and its dn_ptr in 
> /include/net/dn_dev.h)  Should this rcu-protection be extended to the line 
> following rcu_read_lock()?  Even though use_long is a simple char, it 
> appears to be a member of an rcu-protected structure.

Looks to me that this could indeed be a problem -- the structure
pointed to by dn_db could potentially be freed immediately after the
rcu_read_unlock(), unless there is some other non-obvious locking
mechanism protecting it.  In which case, why the rcu_read_lock()
and rcu_read_unlock()...

						Thanx, Paul

Re: rcu read-side protection

[Steven Whitehouse]

Hi,

On Tue, Aug 16, 2005 at 07:01:57PM -0700, Paul E. McKenney wrote:
> On Tue, Aug 16, 2005 at 05:09:29PM -0700, Suzanne Wood wrote:
> [ . . . ]
> > A read-side critical section is marked to protect the dereference of the 
> > dn_ptr and assignment to dn_db which is a pointer to a dn_dev.  (struct 
> > net_device is defined in /linux/netdevice.h and its dn_ptr in 
> > /include/net/dn_dev.h)  Should this rcu-protection be extended to the line 
> > following rcu_read_lock()?  Even though use_long is a simple char, it 
> > appears to be a member of an rcu-protected structure.
> 
> Looks to me that this could indeed be a problem -- the structure
> pointed to by dn_db could potentially be freed immediately after the
> rcu_read_unlock(), unless there is some other non-obvious locking
> mechanism protecting it.  In which case, why the rcu_read_lock()
> and rcu_read_unlock()...
> 
> 						Thanx, Paul

The dev->dn_ptr points to the DECnet specific portion of a net device which
is allocated in dn_dev.c/dn_dev_up and freed in dn_dev.c/dn_dev_delete when
the net device goes up and down.

So I think you are right in that as far as I can see, its possible for a
net device going down to race with this, but the window of opportunity is
very small indeed (in fact possibly zero?) due to the ordering of operations
in dn_dev_delete where dev->dn_ptr is set to NULL (esentially preventing
any more DECnet packets being received on that device) before flushing all
neighbours and only then releasing dn_db.

Also, Patrick Caulfield is maintaining this code now, so I've added him to
the CC list. Thanks for the report though,

Steve.

Re: rcu read-side protection

[Paul E. McKenney]

On Wed, Aug 17, 2005 at 09:25:52AM +0100, Steven Whitehouse wrote:
> Hi,
> 
> On Tue, Aug 16, 2005 at 07:01:57PM -0700, Paul E. McKenney wrote:
> > On Tue, Aug 16, 2005 at 05:09:29PM -0700, Suzanne Wood wrote:
> > [ . . . ]
> > > A read-side critical section is marked to protect the dereference of the 
> > > dn_ptr and assignment to dn_db which is a pointer to a dn_dev.  (struct 
> > > net_device is defined in /linux/netdevice.h and its dn_ptr in 
> > > /include/net/dn_dev.h)  Should this rcu-protection be extended to the line 
> > > following rcu_read_lock()?  Even though use_long is a simple char, it 
> > > appears to be a member of an rcu-protected structure.
> > 
> > Looks to me that this could indeed be a problem -- the structure
> > pointed to by dn_db could potentially be freed immediately after the
> > rcu_read_unlock(), unless there is some other non-obvious locking
> > mechanism protecting it.  In which case, why the rcu_read_lock()
> > and rcu_read_unlock()...
> > 
> > 						Thanx, Paul
> 
> The dev->dn_ptr points to the DECnet specific portion of a net device which
> is allocated in dn_dev.c/dn_dev_up and freed in dn_dev.c/dn_dev_delete when
> the net device goes up and down.
> 
> So I think you are right in that as far as I can see, its possible for a
> net device going down to race with this, but the window of opportunity is
> very small indeed (in fact possibly zero?) due to the ordering of operations
> in dn_dev_delete where dev->dn_ptr is set to NULL (esentially preventing
> any more DECnet packets being received on that device) before flushing all
> neighbours and only then releasing dn_db.

I agree that the window is quite small, but suppose that there was a
lengthy interrupt received just after the rcu_read_unlock()?

> Also, Patrick Caulfield is maintaining this code now, so I've added him to
> the CC list. Thanks for the report though,

How about the following patch?  Untested, but seems pretty straightforward.

							Thanx, Paul

Fix RCU race condition in dn_neigh_construct().

---

Signed-off-by: <paulmck@us.ibm.com>

diff -urpNa -X dontdiff linux-2.6.13-rc6/net/decnet/dn_neigh.c linux-2.6.13-rc6-db_db/net/decnet/dn_neigh.c
--- linux-2.6.13-rc6/net/decnet/dn_neigh.c	2005-08-08 19:59:25.000000000 -0700
+++ linux-2.6.13-rc6-db_db/net/decnet/dn_neigh.c	2005-08-17 07:08:10.000000000 -0700
@@ -148,12 +148,12 @@ static int dn_neigh_construct(struct nei
 
 	__neigh_parms_put(neigh->parms);
 	neigh->parms = neigh_parms_clone(parms);
-	rcu_read_unlock();
 
 	if (dn_db->use_long)
 		neigh->ops = &dn_long_ops;
 	else
 		neigh->ops = &dn_short_ops;
+	rcu_read_unlock();
 
 	if (dn->flags & DN_NDFLAG_P3)
 		neigh->ops = &dn_phase3_ops;

Re: rcu read-side protection

[Steven Whitehouse]

Hi,

On Wed, Aug 17, 2005 at 07:14:38AM -0700, Paul E. McKenney wrote:
[snip]
> How about the following patch?  Untested, but seems pretty straightforward.
> 
> 							Thanx, Paul
> 


That would be my preferred fix. If Patrick is happy with that, then please
pass it on to Dave M with our respective blessings :-) Thanks,

Steve.

Re: rcu read-side protection

[Patrick Caulfield]

Paul E. McKenney wrote:
> On Wed, Aug 17, 2005 at 09:25:52AM +0100, Steven Whitehouse wrote:
> 
>>Hi,
>>
>>On Tue, Aug 16, 2005 at 07:01:57PM -0700, Paul E. McKenney wrote:
>>
>>>On Tue, Aug 16, 2005 at 05:09:29PM -0700, Suzanne Wood wrote:
>>>[ . . . ]
>>>
>>>>A read-side critical section is marked to protect the dereference of the 
>>>>dn_ptr and assignment to dn_db which is a pointer to a dn_dev.  (struct 
>>>>net_device is defined in /linux/netdevice.h and its dn_ptr in 
>>>>/include/net/dn_dev.h)  Should this rcu-protection be extended to the line 
>>>>following rcu_read_lock()?  Even though use_long is a simple char, it 
>>>>appears to be a member of an rcu-protected structure.
>>>
>>>Looks to me that this could indeed be a problem -- the structure
>>>pointed to by dn_db could potentially be freed immediately after the
>>>rcu_read_unlock(), unless there is some other non-obvious locking
>>>mechanism protecting it.  In which case, why the rcu_read_lock()
>>>and rcu_read_unlock()...
>>>
>>>						Thanx, Paul
>>
>>The dev->dn_ptr points to the DECnet specific portion of a net device which
>>is allocated in dn_dev.c/dn_dev_up and freed in dn_dev.c/dn_dev_delete when
>>the net device goes up and down.
>>
>>So I think you are right in that as far as I can see, its possible for a
>>net device going down to race with this, but the window of opportunity is
>>very small indeed (in fact possibly zero?) due to the ordering of operations
>>in dn_dev_delete where dev->dn_ptr is set to NULL (esentially preventing
>>any more DECnet packets being received on that device) before flushing all
>>neighbours and only then releasing dn_db.
> 
> 
> I agree that the window is quite small, but suppose that there was a
> lengthy interrupt received just after the rcu_read_unlock()?
> 
> 
>>Also, Patrick Caulfield is maintaining this code now, so I've added him to
>>the CC list. Thanks for the report though,
> 
> 
> How about the following patch?  Untested, but seems pretty straightforward.
> 
> 							Thanx, Paul
> 
> Fix RCU race condition in dn_neigh_construct().
> 
> ---
> 
> Signed-off-by: <paulmck@us.ibm.com>
> 
> diff -urpNa -X dontdiff linux-2.6.13-rc6/net/decnet/dn_neigh.c linux-2.6.13-rc6-db_db/net/decnet/dn_neigh.c
> --- linux-2.6.13-rc6/net/decnet/dn_neigh.c	2005-08-08 19:59:25.000000000 -0700
> +++ linux-2.6.13-rc6-db_db/net/decnet/dn_neigh.c	2005-08-17 07:08:10.000000000 -0700
> @@ -148,12 +148,12 @@ static int dn_neigh_construct(struct nei
>  
>  	__neigh_parms_put(neigh->parms);
>  	neigh->parms = neigh_parms_clone(parms);
> -	rcu_read_unlock();
>  
>  	if (dn_db->use_long)
>  		neigh->ops = &dn_long_ops;
>  	else
>  		neigh->ops = &dn_short_ops;
> +	rcu_read_unlock();
>  
>  	if (dn->flags & DN_NDFLAG_P3)
>  		neigh->ops = &dn_phase3_ops;
> 
> 

Looks fine to me. I've done a quick test and it doesn't seem to interfere - not
that I expected it to :)

Thanks.

-- 

patrick

Re: rcu read-side protection

[David S. Miller]

From: "Paul E. McKenney" <paulmck@us.ibm.com>
Date: Wed, 17 Aug 2005 07:14:38 -0700

> Fix RCU race condition in dn_neigh_construct().

Applied, thanks Paul.