From owner-freebsd-scsi Mon Dec 9 13:50:16 2002 Delivered-To: freebsd-scsi@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 491EE37B404 for ; Mon, 9 Dec 2002 13:50:14 -0800 (PST) Received: from rootlabs.com (root.org [67.118.192.226]) by mx1.FreeBSD.org (Postfix) with SMTP id B221E43EC5 for ; Mon, 9 Dec 2002 13:50:13 -0800 (PST) (envelope-from nate@rootlabs.com) Received: (qmail 25662 invoked by uid 1000); 9 Dec 2002 21:50:14 -0000 Date: Mon, 9 Dec 2002 13:50:14 -0800 (PST) From: Nate Lawson To: Yar Tikhiy Cc: "Kenneth D. Merry" , freebsd-scsi@FreeBSD.ORG Subject: Re: {da,sa,...}open bug? In-Reply-To: <20021208170756.A75509@comp.chem.msu.su> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-scsi@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org 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