Date: Tue, 10 Dec 2002 17:15:28 +0300
From: Yar Tikhiy <yar@FreeBSD.ORG>
To: Nate Lawson <nate@root.org>
Cc: "Kenneth D. Merry" <ken@kdm.org>, freebsd-scsi@FreeBSD.ORG
Subject: Re: {da,sa,...}open bug?
Message-ID: <20021210171528.B27181@comp.chem.msu.su>
In-Reply-To: <Pine.BSF.4.21.0212091323240.25027-100000@root.org>; from nate@root.org on Mon, Dec 09, 2002 at 01:50:14PM -0800
References: <20021208170756.A75509@comp.chem.msu.su> <Pine.BSF.4.21.0212091323240.25027-100000@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Dec 09, 2002 at 01:50:14PM -0800, Nate Lawson wrote:
> 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).
I mean that if two program parts hold pointers to the same structure
and one of the parts free()'s the structure, the second part won't
have its pointer set to NULL automagically :-)
That is, we don't need to worry whether the "periph" argument is
NULL after we have reached behind cam_periph_lock() even if some
bogus piece of code would rip the actual periph from under us.
> > 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
Justin's point sounds reasonable indeed. If a piece of code were
to do something with periph after cam_periph_lock() had failed on
it, the code should use this order. On the other hand, it's a bad
thing to do anything with unlocked periph's internal data.
> 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.
Excuse me, but I see little reason in this patch. "periph" is a
function argument, so its value cannot be changed anywhere else,
so there is no need for splsoftcam() before the NULL check.
--
Yar
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?20021210171528.B27181>