Date: Tue, 7 Oct 2003 07:00:37 -0600 From: "Kenneth D. Merry" <ken@kdm.org> To: Thomas Quinot <thomas@FreeBSD.ORG> Cc: freebsd-scsi@FreeBSD.ORG Subject: Re: Defend against calling sysctl_ctx_free on uninitialized context Message-ID: <20031007130037.GA74403@panzer.kdm.org> In-Reply-To: <20030919175032.GA2430@melusine.cuivre.fr.eu.org> References: <20030919175032.GA2430@melusine.cuivre.fr.eu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Sep 19, 2003 at 19:50:32 +0200, Thomas Quinot wrote:
> If a fatal error occurs while cd(4) is attaching, before the sysctl_ctx
> has been initialized, then it must not be freed. The following patch
> resolves this problem, please review.
>
> Thanks,
> Thomas.
>
> Index: scsi_cd.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/cam/scsi/scsi_cd.c,v
> retrieving revision 1.83
> diff -u -r1.83 scsi_cd.c
> --- scsi_cd.c 11 Sep 2003 19:27:24 -0000 1.83
> +++ scsi_cd.c 19 Sep 2003 17:47:03 -0000
> @@ -91,17 +91,18 @@
> } cd_quirks;
>
> typedef enum {
> - CD_FLAG_INVALID = 0x001,
> - CD_FLAG_NEW_DISC = 0x002,
> - CD_FLAG_DISC_LOCKED = 0x004,
> - CD_FLAG_DISC_REMOVABLE = 0x008,
> - CD_FLAG_TAGGED_QUEUING = 0x010,
> - CD_FLAG_CHANGER = 0x040,
> - CD_FLAG_ACTIVE = 0x080,
> - CD_FLAG_SCHED_ON_COMP = 0x100,
> - CD_FLAG_RETRY_UA = 0x200,
> - CD_FLAG_VALID_MEDIA = 0x400,
> - CD_FLAG_VALID_TOC = 0x800
> + CD_FLAG_INVALID = 0x0001,
> + CD_FLAG_NEW_DISC = 0x0002,
> + CD_FLAG_DISC_LOCKED = 0x0004,
> + CD_FLAG_DISC_REMOVABLE = 0x0008,
> + CD_FLAG_TAGGED_QUEUING = 0x0010,
> + CD_FLAG_CHANGER = 0x0040,
> + CD_FLAG_ACTIVE = 0x0080,
> + CD_FLAG_SCHED_ON_COMP = 0x0100,
> + CD_FLAG_RETRY_UA = 0x0200,
> + CD_FLAG_VALID_MEDIA = 0x0400,
> + CD_FLAG_VALID_TOC = 0x0800,
> + CD_FLAG_SCTX_INIT = 0x1000
> } cd_flags;
>
> typedef enum {
> @@ -458,7 +459,8 @@
> xpt_print_path(periph->path);
> printf("removing device entry\n");
>
> - if (sysctl_ctx_free(&softc->sysctl_ctx) != 0) {
> + if ((softc->flags & CD_FLAG_SCTX_INIT) != 0
> + && sysctl_ctx_free(&softc->sysctl_ctx) != 0) {
> xpt_print_path(periph->path);
> printf("can't remove sysctl context\n");
> }
> @@ -622,6 +624,7 @@
> mtx_lock(&Giant);
>
> sysctl_ctx_init(&softc->sysctl_ctx);
> + softc->flags |= CD_FLAG_SCTX_INIT;
> softc->sysctl_tree = SYSCTL_ADD_NODE(&softc->sysctl_ctx,
> SYSCTL_STATIC_CHILDREN(_kern_cam_cd), OID_AUTO,
> tmpstr2, CTLFLAG_RD, 0, tmpstr);
>
> --
> Thomas.Quinot@Cuivre.FR.EU.ORG
This looks fine, feel free to commit.
There is also a potential race condition between the device going away and
the taskqueue running. If the device goes away before the taskqueue runs,
it'll end up shoving sysctl context into freed memory, and will likely
leave the sysctl variable for that cd(4) instance hanging around.
It doesn't look like there is a taskqueue_dequeue(), though, so we can't
check for an active taskqueue request in cdcleanup() and dequeue it.
Ken
--
Kenneth Merry
ken@kdm.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20031007130037.GA74403>