struct comedi_file holds per file object settings for comedi devices to allow user space to change the "read" and "write" comedi subdevices locally to the open file description, using the COMEDI_SETWSUBD and COMEDI_SETWSUBD ioctl commands. If there are any subsequent changes to the status of the attachment of the comedi device to a low-level comedi driver, the references to the "read" and "write" subdevices in the struct comedi_file become stale and need to be reset to the current defaults for the comedi device. That is done by comedi_file_reset() when comedi_file_check() (called by comedi_file_read_subdevice() and comedi_file_write_subdevice()) has determined that they have become stale. The tasks calling these functions will have either locked the main mutex for the comedi device, or will have read-locked the "attach_lock" for the comedi device. In the latter case, there may be several tasks executing the functions at the same time. Unfortunately, there is a race condition in the access to the struct comedi_file members that can leave it having stale pointers to "read" and/or "write" subdevices. Reorder the accesses to the struct comedi_file members and add SMP memory barriers to fix the problem. Fixes: 20f083c07565 ("staging: comedi: prepare support for per-file read and write subdevices") Reported-by: Eulgyu Kim Reported-by: Jaeyoung Chung Closes: https://lore.kernel.org/lkml/20260819174614.3857878-1-jjy600901@snu.ac.kr Cc: stable@vger.kernel.org Signed-off-by: Ian Abbott --- drivers/comedi/comedi_fops.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c index 68eddf853465b..069656bab7e2c 100644 --- a/drivers/comedi/comedi_fops.c +++ b/drivers/comedi/comedi_fops.c @@ -66,7 +66,7 @@ struct comedi_file { struct comedi_subdevice *read_subdev; struct comedi_subdevice *write_subdev; unsigned int last_detach_count; - unsigned int last_attached:1; + unsigned char last_attached; }; #define COMEDI_NUM_MINORS 0x100 @@ -293,10 +293,17 @@ static void comedi_file_reset(struct file *file) if (!s || s->subdev_flags & SDF_CMD_WRITE) write_s = s; } - cfp->last_attached = dev->attached; - cfp->last_detach_count = dev->detach_count; WRITE_ONCE(cfp->read_subdev, read_s); WRITE_ONCE(cfp->write_subdev, write_s); + /* + * All tasks that call this function simultaneously will store the same + * values in the cfp members. The last_attached and last_detach_count + * members will be checked for not being stale before assuming the + * read_subdev and write_subdev members are valid. + */ + smp_wmb(); /* matched with smp_rmb() in comedi_file_check() */ + WRITE_ONCE(cfp->last_attached, dev->attached); + WRITE_ONCE(cfp->last_detach_count, dev->detach_count); } static void comedi_file_check(struct file *file) @@ -304,9 +311,11 @@ static void comedi_file_check(struct file *file) struct comedi_file *cfp = file->private_data; struct comedi_device *dev = cfp->dev; - if (cfp->last_attached != dev->attached || - cfp->last_detach_count != dev->detach_count) + if (READ_ONCE(cfp->last_attached) != dev->attached || + READ_ONCE(cfp->last_detach_count) != dev->detach_count) comedi_file_reset(file); + /* caller is about to load cfp->read_subdev or cfp->write_subdev */ + smp_rmb(); } static struct comedi_subdevice *comedi_file_read_subdevice(struct file *file) @@ -2227,6 +2236,8 @@ static int do_setrsubd_ioctl(struct comedi_device *dev, unsigned long arg, return -EBUSY; WRITE_ONCE(cfp->read_subdev, s_new); + /* comedi_file_read_subdevice() => comedi_file_check() => smp_rmb() */ + smp_wmb(); return 0; } @@ -2270,6 +2281,8 @@ static int do_setwsubd_ioctl(struct comedi_device *dev, unsigned long arg, return -EBUSY; WRITE_ONCE(cfp->write_subdev, s_new); + /* comedi_file_write_subdevice() => comedi_file_check() => smp_rmb() */ + smp_wmb(); return 0; } -- 2.53.0