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: <[email protected]>
>
> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |