From: Vitaliy Makkoveev Subject: Re: Please test: mpsafe sensors To: tech@openbsd.org Date: Tue, 20 May 2025 02:07:45 +0300 On Wed, Aug 07, 2024 at 06:04:11PM +0300, Vitaliy Makkoveev wrote: > I'm not happy with sensor pointers dereference after sleeping malloc(9) > in sysctl_sensors(), so let's start making them mpsafe. > > New `sensordev_mtx' protects `sensordev_count' and `sensordev_list'. > Reference counters are used to make sensor device and attached sensors > dereference safe. Not all sensors do sensor_detach(), so sensor > 'ksensordev' and 'ksensor' share reference counter. > > Each 'ksensordev' has it's own mutex(9) to protect mutable data. Each > 'ksensor' has mutex too, but since we have many of them, only > SENSOR_MPSAFE marked sensors rely on it. This diff doesn't introduce any > of them, but it could be easily made on top. Anyway, we have many of > sensors so they need to be unlocked separately. > > Please note, except sparc64 only sysctl_sensors() use sensordev_get() > and sensor_find(), drivers have their instances and we don't need to > acquire and release them. > > I'm not asking for OK's, this diff was shared to receive feedback and > help with testing. That's why sensors manual page left unmodified. > I want to start pushing this forward. Updated against the latest sources, but actually the diff is the same. Index: sys/arch/sparc64/dev/tda.c =================================================================== RCS file: /cvs/src/sys/arch/sparc64/dev/tda.c,v retrieving revision 1.9 diff -u -p -r1.9 tda.c --- sys/arch/sparc64/dev/tda.c 24 Oct 2021 17:05:04 -0000 1.9 +++ sys/arch/sparc64/dev/tda.c 19 May 2025 17:42:58 -0000 @@ -115,10 +115,12 @@ tda_attach(struct device *parent, struct tda_setspeed(sc); /* Get the number of sensor devices. */ + mtx_enter(&sensordev_mtx); for (i = 0; ; i++) { - if (sensordev_get(i, &ksens) == ENOENT) + if (sensordev_get_locked(i, &ksens) == ENOENT) break; } + mtx_leave(&sensordev_mtx); sc->sc_nsensors = i; if (!sc->sc_nsensors) { @@ -199,6 +201,7 @@ tda_adjust(void *v) goto out; } ctemp = MAX(ctemp, ks->value); + sensor_rele(ks); err = sensor_find(i, SENSOR_TEMP, SENSOR_TEMP_INT, &ks); if (err) { @@ -207,6 +210,7 @@ tda_adjust(void *v) goto out; } stemp = MAX(stemp, ks->value); + sensor_rele(ks); } if (ctemp < CPU_TEMP_MIN) Index: sys/kern/kern_sensors.c =================================================================== RCS file: /cvs/src/sys/kern/kern_sensors.c,v retrieving revision 1.40 diff -u -p -r1.40 kern_sensors.c --- sys/kern/kern_sensors.c 5 Dec 2022 23:18:37 -0000 1.40 +++ sys/kern/kern_sensors.c 19 May 2025 17:43:01 -0000 @@ -31,18 +31,27 @@ #include #include "hotplug.h" +/* + * Locks used to protect data: + * D sensordev_mtx + */ + +struct mutex sensordev_mtx = MUTEX_INITIALIZER(IPL_MPFLOOR); + struct taskq *sensors_taskq; -int sensordev_count; +int sensordev_count; /* [D] */ SLIST_HEAD(, ksensordev) sensordev_list = - SLIST_HEAD_INITIALIZER(sensordev_list); + SLIST_HEAD_INITIALIZER(sensordev_list); /* [D] */ void sensordev_install(struct ksensordev *sensdev) { struct ksensordev *v, *nv; - int s; - s = splhigh(); + refcnt_init(&sensdev->refcnt); + mtx_init(&sensdev->mtx, IPL_MPFLOOR); + + mtx_enter(&sensordev_mtx); if (sensordev_count == 0) { sensdev->num = 0; SLIST_INSERT_HEAD(&sensordev_list, sensdev, list); @@ -54,8 +63,9 @@ sensordev_install(struct ksensordev *sen sensdev->num = v->num + 1; SLIST_INSERT_AFTER(v, sensdev, list); } + sensordev_count++; - splx(s); + mtx_leave(&sensordev_mtx); #if NHOTPLUG > 0 hotplug_device_attach(DV_DULL, "sensordev"); @@ -67,9 +77,12 @@ sensor_attach(struct ksensordev *sensdev { struct ksensor *v, *nv; struct ksensors_head *sh; - int s, i; + int i; + + sens->parent = sensdev; + mtx_init(&sens->mtx, IPL_MPFLOOR); - s = splhigh(); + mtx_enter(&sensdev->mtx); sh = &sensdev->sensors_list; if (sensdev->sensors_count == 0) { for (i = 0; i < SENSOR_MAX_TYPES; i++) @@ -95,31 +108,30 @@ sensor_attach(struct ksensordev *sensdev if (sensdev->maxnumt[sens->type] == sens->numt) sensdev->maxnumt[sens->type]++; sensdev->sensors_count++; - splx(s); + mtx_leave(&sensdev->mtx); } void sensordev_deinstall(struct ksensordev *sensdev) { - int s; - - s = splhigh(); + mtx_enter(&sensordev_mtx); sensordev_count--; SLIST_REMOVE(&sensordev_list, sensdev, ksensordev, list); - splx(s); + mtx_leave(&sensordev_mtx); #if NHOTPLUG > 0 hotplug_device_detach(DV_DULL, "sensordev"); #endif + + refcnt_finalize(&sensdev->refcnt, "sensordev"); } void sensor_detach(struct ksensordev *sensdev, struct ksensor *sens) { struct ksensors_head *sh; - int s; - s = splhigh(); + mtx_enter(&sensdev->mtx); sh = &sensdev->sensors_list; sensdev->sensors_count--; SLIST_REMOVE(sh, sens, ksensor, list); @@ -128,14 +140,16 @@ sensor_detach(struct ksensordev *sensdev */ if (sens->numt == sensdev->maxnumt[sens->type] - 1) sensdev->maxnumt[sens->type]--; - splx(s); + mtx_leave(&sensdev->mtx); } int -sensordev_get(int num, struct ksensordev **sensdev) +sensordev_get_locked(int num, struct ksensordev **sensdev) { struct ksensordev *sd; + MUTEX_ASSERT_LOCKED(&sensordev_mtx); + SLIST_FOREACH(sd, &sensordev_list, list) { if (sd->num == num) { *sensdev = sd; @@ -148,25 +162,76 @@ sensordev_get(int num, struct ksensordev } int +sensordev_get(int num, struct ksensordev **sensdev) +{ + int error; + + mtx_enter(&sensordev_mtx); + if ((error = sensordev_get_locked(num, sensdev)) == 0) + refcnt_take(&(*sensdev)->refcnt); + mtx_leave(&sensordev_mtx); + + return (error); +} + +void +sensordev_put(struct ksensordev *sensdev) +{ + refcnt_rele_wake(&sensdev->refcnt); +} + +int sensor_find(int dev, enum sensor_type type, int numt, struct ksensor **ksensorp) { struct ksensor *s; struct ksensordev *sensdev; struct ksensors_head *sh; - int ret; + int error; - ret = sensordev_get(dev, &sensdev); - if (ret) - return (ret); + error = sensordev_get(dev, &sensdev); + if (error) + return (error); + mtx_enter(&sensdev->mtx); sh = &sensdev->sensors_list; - SLIST_FOREACH(s, sh, list) - if (s->type == type && s->numt == numt) { - *ksensorp = s; - return (0); - } + SLIST_FOREACH(s, sh, list) { + if (s->type == type && s->numt == numt) + break; + } - return (ENOENT); + if (s) + *ksensorp = s; + else { + sensordev_put(sensdev); + error = ENOENT; + } + mtx_leave(&sensdev->mtx); + + return (error); +} + +void +sensor_rele(struct ksensor *sens) +{ + refcnt_rele_wake(&sens->parent->refcnt); +} + +void +sensor_lock(struct ksensor *sens) +{ + if (sens->flags & SENSOR_MPSAFE) + mtx_enter(&sens->mtx); + else + KERNEL_LOCK(); +} + +void +sensor_unlock(struct ksensor *sens) +{ + if (sens->flags & SENSOR_MPSAFE) + mtx_leave(&sens->mtx); + else + KERNEL_UNLOCK(); } struct sensor_task { Index: sys/kern/kern_sysctl.c =================================================================== RCS file: /cvs/src/sys/kern/kern_sysctl.c,v retrieving revision 1.468 diff -u -p -r1.468 kern_sysctl.c --- sys/kern/kern_sysctl.c 9 May 2025 14:53:22 -0000 1.468 +++ sys/kern/kern_sysctl.c 19 May 2025 17:43:01 -0000 @@ -848,7 +848,6 @@ hw_sysctl(int *name, u_int namelen, void case HW_DISKSTATS: case HW_CPUSPEED: #ifndef SMALL_KERNEL - case HW_SENSORS: case HW_SETPERF: case HW_PERFPOLICY: case HW_BATTERY: @@ -867,6 +866,11 @@ hw_sysctl(int *name, u_int namelen, void sysctl_vsunlock(oldp, savelen); return (err); } +#ifndef SMALL_KERNEL + case HW_SENSORS: + return (sysctl_sensors(name + 1, namelen - 1, oldp, oldlenp, + newp, newlen)); +#endif /* !SMALL_KERNEL */ case HW_VENDOR: if (hw_vendor) return (sysctl_rdstring(oldp, oldlenp, newp, @@ -937,9 +941,6 @@ hw_sysctl_locked(int *name, u_int namele return err; return (sysctl_rdint(oldp, oldlenp, newp, cpuspeed)); #ifndef SMALL_KERNEL - case HW_SENSORS: - return (sysctl_sensors(name + 1, namelen - 1, oldp, oldlenp, - newp, newlen)); case HW_SETPERF: return (sysctl_hwsetperf(oldp, oldlenp, newp, newlen)); case HW_PERFPOLICY: @@ -2786,10 +2787,15 @@ sysctl_sensors(int *name, u_int namelen, /* Grab a copy, to clear the kernel pointers */ usd = malloc(sizeof(*usd), M_TEMP, M_WAITOK|M_ZERO); + + mtx_enter(&ksd->mtx); usd->num = ksd->num; strlcpy(usd->xname, ksd->xname, sizeof(usd->xname)); memcpy(usd->maxnumt, ksd->maxnumt, sizeof(usd->maxnumt)); usd->sensors_count = ksd->sensors_count; + mtx_leave(&ksd->mtx); + + sensordev_put(ksd); ret = sysctl_rdstruct(oldp, oldlenp, newp, usd, sizeof(struct sensordev)); @@ -2807,13 +2813,18 @@ sysctl_sensors(int *name, u_int namelen, /* Grab a copy, to clear the kernel pointers */ us = malloc(sizeof(*us), M_TEMP, M_WAITOK|M_ZERO); + + sensor_lock(ks); memcpy(us->desc, ks->desc, sizeof(us->desc)); us->tv = ks->tv; us->value = ks->value; us->type = ks->type; us->status = ks->status; us->numt = ks->numt; - us->flags = ks->flags; + us->flags = ks->flags & ~SENSOR_MPSAFE; + sensor_unlock(ks); + + sensor_rele(ks); ret = sysctl_rdstruct(oldp, oldlenp, newp, us, sizeof(struct sensor)); Index: sys/sys/sensors.h =================================================================== RCS file: /cvs/src/sys/sys/sensors.h,v retrieving revision 1.37 diff -u -p -r1.37 sensors.h --- sys/sys/sensors.h 15 Jul 2020 07:13:57 -0000 1.37 +++ sys/sys/sensors.h 19 May 2025 17:43:01 -0000 @@ -119,6 +119,7 @@ struct sensor { int flags; /* sensor flags */ #define SENSOR_FINVALID 0x0001 /* sensor is invalid */ #define SENSOR_FUNKNOWN 0x0002 /* sensor value is unknown */ +#define SENSOR_MPSAFE 0x0004 /* sensor is mpsafe */ }; /* Sensor device data: @@ -133,13 +134,29 @@ struct sensordev { #ifdef _KERNEL +#include + +/* + * Locks used to protect data: + * I immutable after creation + * D sensordev_mtx + * d ksensordev's mtx + */ + +struct ksensordev; + +extern struct mutex sensordev_mtx; + /* Sensor data */ struct ksensor { - SLIST_ENTRY(ksensor) list; /* device-scope list */ - char desc[32]; /* sensor description, may be empty */ + struct mutex mtx; + struct ksensordev *parent; /* [I] parent ksensordev */ + SLIST_ENTRY(ksensor) list; /* [d] device-scope list */ + char desc[32]; /* [I] sensor description, may be + empty */ struct timeval tv; /* sensor value last change time */ int64_t value; /* current value */ - enum sensor_type type; /* sensor type */ + enum sensor_type type; /* [I] sensor type */ enum sensor_status status; /* sensor status */ int numt; /* sensor number of .type type */ int flags; /* sensor flags, ie. SENSOR_FINVALID */ @@ -148,23 +165,31 @@ SLIST_HEAD(ksensors_head, ksensor); /* Sensor device data */ struct ksensordev { - SLIST_ENTRY(ksensordev) list; - int num; /* sensordev number */ - char xname[16]; /* unix device name */ - int maxnumt[SENSOR_MAX_TYPES]; - int sensors_count; - struct ksensors_head sensors_list; + struct refcnt refcnt; + struct mutex mtx; + SLIST_ENTRY(ksensordev) list; /* [D] */ + int num; /* [I] sensordev number */ + char xname[16]; /* [I] unix device name */ + int maxnumt[SENSOR_MAX_TYPES]; /* [d] */ + int sensors_count; /* [d] */ + struct ksensors_head sensors_list; /* [d] */ }; /* struct ksensordev */ void sensordev_install(struct ksensordev *); void sensordev_deinstall(struct ksensordev *); +int sensordev_get_locked(int, struct ksensordev **); int sensordev_get(int, struct ksensordev **); +void sensordev_put(struct ksensordev *); +void sensor_lock(struct ksensor *); +void sensor_unlock(struct ksensor *); /* struct ksensor */ void sensor_attach(struct ksensordev *, struct ksensor *); void sensor_detach(struct ksensordev *, struct ksensor *); -int sensor_find(int, enum sensor_type, int, struct ksensor **); +int sensor_find(int, enum sensor_type, int, + struct ksensor **); +void sensor_rele(struct ksensor *); /* task scheduling */ struct sensor_task;