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

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] [day] [month] [year] [list]

Message-ID: <CCC74268-5C66-446F-A81F-F9892CC0CAA0@coraid.com>
Date:	Thu, 6 Dec 2012 10:16:02 -0600
From:	Ed Cashin <ecashin@...aid.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/7] aoe: avoid races between device destruction and
 discovery

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

> On Mon, 3 Dec 2012 20:42:56 -0500
> Ed Cashin <ecashin@...aid.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@...aid.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/