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
| ||
|
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/
Powered by blists - more mailing lists