From 8078943315e04645e89fc530d0a85c888fbffe4d Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 10 Aug 2021 09:54:52 +0000 Subject: [PATCH] wd(4): Fix bugs in softbadsect handling. - Don't copyout kernel virtual addresses (of SLIST entries) that userland won't use anyway. => The structure still has space for this pointer; it's just always null when userland gets it now. - Don't copyout under a lock. - Stop and return error if copyout fails (unless we've already copied some out). - Don't kmem_free under a lock. --- sys/dev/ata/wd.c | 44 ++++++++++++++++++++++++++++++++++++++------ sys/dev/ata/wdvar.h | 16 +++++++++++++++- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/sys/dev/ata/wd.c b/sys/dev/ata/wd.c index 3fb049919eab..1e8f1e74df81 100644 --- a/sys/dev/ata/wd.c +++ b/sys/dev/ata/wd.c @@ -316,6 +316,7 @@ wdattach(device_t parent, device_t self, void *aux) mutex_init(&wd->sc_lock, MUTEX_DEFAULT, IPL_BIO); #ifdef WD_SOFTBADSECT SLIST_INIT(&wd->sc_bslist); + cv_init(&wd->sc_bslist_cv, "wdbadsect"); #endif wd->atabus = adev->adev_bustype; wd->inflight = 0; @@ -587,6 +588,11 @@ wddetach(device_t self, int flags) wd_sysctl_detach(wd); +#ifdef WD_SOFTBADSECT + KASSERT(SLIST_EMPTY(&wd->sc_bslist)); + cv_destroy(&wd->sc_bslist_cv); +#endif + mutex_destroy(&wd->sc_lock); wd->drvp->drive_type = ATA_DRIVET_NONE; /* no drive any more here */ @@ -1262,6 +1268,7 @@ wdioctl(dev_t dev, u_long cmd, void *addr, int flag, struct lwp *l) struct wd_softc *wd = device_lookup_private(&wd_cd, WDUNIT(dev)); struct dk_softc *dksc = &wd->sc_dksc; + int error; ATADEBUG_PRINT(("wdioctl\n"), DEBUG_FUNCS); @@ -1279,11 +1286,10 @@ wdioctl(dev_t dev, u_long cmd, void *addr, int flag, struct lwp *l) return 0; #endif #ifdef WD_SOFTBADSECT - case DIOCBSLIST : - { + case DIOCBSLIST: { uint32_t count, missing, skip; struct disk_badsecinfo dbsi; - struct disk_badsectors *dbs; + struct disk_badsectors *dbs, dbsbuf; size_t available; uint8_t *laddr; @@ -1303,7 +1309,9 @@ wdioctl(dev_t dev, u_long cmd, void *addr, int flag, struct lwp *l) * back to user space whilst the summary is returned via * the struct passed in via the ioctl. */ + error = 0; mutex_enter(&wd->sc_lock); + wd->sc_bslist_inuse++; SLIST_FOREACH(dbs, &wd->sc_bslist, dbs_next) { if (skip > 0) { missing--; @@ -1313,26 +1321,50 @@ wdioctl(dev_t dev, u_long cmd, void *addr, int flag, struct lwp *l) if (available < sizeof(*dbs)) break; available -= sizeof(*dbs); - copyout(dbs, laddr, sizeof(*dbs)); + memset(&dbsbuf, 0, sizeof(dbsbuf)); + dbsbuf.dbs_min = dbs->dbs_min; + dbsbuf.dbs_max = dbs->dbs_max; + dbsbuf.dbs_failedat = dbs->dbs_failedat; + mutex_exit(&wd->sc_lock); + error = copyout(&dbsbuf, laddr, sizeof(dbsbuf)); + mutex_enter(&wd->sc_lock); + if (error) + break; laddr += sizeof(*dbs); missing--; count++; } + if (--wd->sc_bslist_inuse == 0) + cv_broadcast(&wd->sc_bslist_cv); mutex_exit(&wd->sc_lock); dbsi.dbsi_left = missing; dbsi.dbsi_copied = count; *(struct disk_badsecinfo *)addr = dbsi; - return 0; + + /* + * If we copied anything out, ignore error and return + * success -- can't back it out. + */ + return count ? 0 : error; } - case DIOCBSFLUSH : + case DIOCBSFLUSH: /* Clean out the bad sector list */ mutex_enter(&wd->sc_lock); + while (wd->sc_bslist_inuse) { + error = cv_wait_sig(&wd->sc_bslist_cv, &wd->sc_lock); + if (error) { + mutex_exit(&wd->sc_lock); + return error; + } + } while (!SLIST_EMPTY(&wd->sc_bslist)) { struct disk_badsectors *dbs = SLIST_FIRST(&wd->sc_bslist); SLIST_REMOVE_HEAD(&wd->sc_bslist, dbs_next); + mutex_exit(&wd->sc_lock); kmem_free(dbs, sizeof(*dbs)); + mutex_enter(&wd->sc_lock); } mutex_exit(&wd->sc_lock); wd->sc_bscount = 0; diff --git a/sys/dev/ata/wdvar.h b/sys/dev/ata/wdvar.h index 07152ac80d5f..bfbfdbbb4d94 100644 --- a/sys/dev/ata/wdvar.h +++ b/sys/dev/ata/wdvar.h @@ -31,8 +31,20 @@ #include "opt_wd.h" #endif -#include +#include + +#include +#include +#include +#include #include +#include + +#include +#include +#include + +struct sysctllog; struct wd_softc { /* General disk infos */ @@ -64,6 +76,8 @@ struct wd_softc { #ifdef WD_SOFTBADSECT SLIST_HEAD(, disk_badsectors) sc_bslist; u_int sc_bscount; + kcondvar_t sc_bslist_cv; + u_int sc_bslist_inuse; #endif /* Retry/requeue failed transfers */