Re: [PATCH 2/7] aoe: avoid races between device destruction and discovery

[Ed Cashin <ecashin@coraid.com>]

On Dec 4, 2012, at 6:45 PM, Andrew Morton wrote:

> On Mon, 3 Dec 2012 20:42:56 -0500
> Ed Cashin <ecashin@coraid.com> wrote:
> 
>> This change avoids a race that could result in a NULL pointer
>> derference following a WARNing from kobject_add_internal, "don't
>> try to register things with the same name in the same directory."
...
>> The check for a bad aoedev pointer remains from a time when about
>> half of this patch was done, and it was possible for the
>> bdev->bd_disk->private_data to become corrupted.  The check
>> should be removed eventually, but it is not expected to add
>> significant overhead, occurring in the aoeblk_open routine.
...
>> --- a/drivers/block/aoe/aoeblk.c
>> +++ b/drivers/block/aoe/aoeblk.c
>> @@ -147,9 +147,18 @@ aoeblk_open(struct block_device *bdev, fmode_t mode)
>> 	struct aoedev *d = bdev->bd_disk->private_data;
>> 	ulong flags;
>> 
>> +	if (!virt_addr_valid(d)) {
>> +		pr_crit("aoe: invalid device pointer in %s\n",
>> +			__func__);
>> +		WARN_ON(1);
>> +		return -ENODEV;
>> +	}
> 
> Can this ever happen?

This is the check mentioned in the last paragraph of the changelog
message.  I don't think it can happen now.  Folks have been using
it like this and nobody has seen the invalid device pointer message.

I'll go ahead and remove the check and resubmit the patch.

...
>> @@ -259,6 +268,18 @@ aoeblk_gdalloc(void *vp)
>> 	struct request_queue *q;
>> 	enum { KB = 1024, MB = KB * KB, READ_AHEAD = 2 * MB, };
>> 	ulong flags;
>> +	int late = 0;
>> +
>> +	spin_lock_irqsave(&d->lock, flags);
>> +	if (d->flags & DEVFL_GDALLOC
>> +	&& !(d->flags & DEVFL_TKILL)
>> +	&& !(d->flags & DEVFL_GD_NOW))
> 
> That's pretty sickly-looking code layout.
> 
> We often do
> 
> 	if ((d->flags & (DEVFL_GDALLOC|DEVFL_TKILL|DEVFL_GD_NOW)) ==
> 		DEVFL_GDALLOC)
> 
> in these cases.

OK.  When I'm resubmitting these patches, I'm planning to submit
the series of 7 patches and include info in the cover letter about
what has changed in the resubmission.

-- 
  Ed Cashin
  ecashin@coraid.com