Date: Mon, 9 Dec 2002 13:50:14 -0800 (PST)
From: Nate Lawson <nate@root.org>
To: Yar Tikhiy <yar@FreeBSD.ORG>
Cc: "Kenneth D. Merry" <ken@kdm.org>, freebsd-scsi@FreeBSD.ORG
Subject: Re: {da,sa,...}open bug?
Message-ID: <Pine.BSF.4.21.0212091323240.25027-100000@root.org>
In-Reply-To: <20021208170756.A75509@comp.chem.msu.su>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 8 Dec 2002, Yar Tikhiy wrote:
> On Fri, Dec 06, 2002 at 12:34:01PM -0700, Kenneth D. Merry wrote:
> > cam_periph_acquire() only returns an error when periph == NULL. If, for
> > some wild reason, a locked periph manages to get freed (thus causing
> > cam_periph_acquire() to return CAM_REQ_CMP_ERR), cam_periph_unlock()
> > will panic (NULL pointer deference) when called with a NULL periph.
>
> First, I'd like to point out that freeing a periph doesn't cause a
> pointer to it to become NULL. Therefore, cam_periph_unlock() is
> extremely unlikely to panic since cam_periph_lock(), which in turn
> calls cam_periph_acquire(), has been successful (i.e., periph != NULL)
A little confusing here. What do you mean by "freeing a periph"? If you
mean remove a reference to a periph, that's in cam_periph_release(). It
is always an error to deref a periph pointer after cam_periph_release even
though the poniter may still be valid (based on refcount).
However, you are right in that if cam_periph_lock has succeeded, it is
extremely unlikely a later cam_periph_acquire will fail.
> My general impression is that that NULL check at the beginning of
> cam_periph_acquire() is one of safety belts for unwary programmers
> still left out there. I think it should be changed to something
> like this:
>
> KASSERT(periph != NULL, ("cam_periph_acquire: null periph"));
>
> After this change, it will be OK to unlock a periph on cam_periph_acquire()
> failure (granted someone have added code able to fail to the latter
> function.)
See Justin's email re: acquire first, lock second. In that case, acquire
should continue to return an error since it is first in line in the *open
function and ensures we have a referenced periph, allowing us to always
back state out of that periph if things later fail. Also, I think the
NULL check should be moved inside the splsoftcam to ensure exclusive
access. In the future, we'll use mutex(9) for implementing lock and
acquire.
--- /sys/cam/cam_periph.c 14 Nov 2002 05:35:57 -0000 1.43
+++ /sys/cam/cam_periph.c 9 Dec 2002 21:38:02 -0000
@@ -262,15 +262,17 @@
cam_periph_acquire(struct cam_periph *periph)
{
int s;
+ cam_status status;
- if (periph == NULL)
- return(CAM_REQ_CMP_ERR);
-
+ cam_status = CAM_REQ_CMP;
s = splsoftcam();
- periph->refcount++;
+ if (periph != NULL)
+ periph->refcount++;
+ else
+ status = CAM_REQ_CMP_ERR;
splx(s);
- return(CAM_REQ_CMP);
+ return (status);
}
void
If we reorder things according to Justin's suggestion, then we should do
the obvious thing and always unroll state on all returns.
-Nate
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-scsi" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0212091323240.25027-100000>