dev->read_buffer_length is otherwise only ever touched under
dev->buflock (by adu_interrupt_in_callback() and adu_read()), per the
locking scheme documented in the comment above struct adu_device.
adu_open() resets it to 0 without holding buflock.

In the current code this is not a reachable race. adu_open() and
adu_release() are fully serialized by adutux_mutex, and
adu_release_internal() calls usb_kill_urb() (via
adu_abort_transfers()) before that mutex is dropped, which blocks
until any in-flight adu_interrupt_in_callback() has returned. The
write in adu_open() also precedes the urb (re)submission in program
order, so the callback cannot observe or race with it there either.

This change brings the assignment under buflock purely so the
field's locking is locally consistent with the driver's documented
scheme, making the invariant easy to verify without having to reason
across adu_open(), adu_release_internal(), and usb_kill_urb()'s
blocking semantics. No behavioral or functional change intended.

v2:
 - Retitled and reframed from "fix unlocked read_buffer_length write
   in adu_open() (data race)" to a lock-discipline consistency
   change. Discussion on the RFC (with Oliver Neukum) established
   that adutux_mutex serialization plus usb_kill_urb()'s blocking
   semantics rule out any runtime-reachable race here, so this is
   no longer presented as a bugfix. (Oliver Neukum, Greg
   Kroah-Hartman)

Signed-off-by: Kanishka De Silva <kpskanna1915@gmail.com>
---
 drivers/usb/misc/adutux.c | 3 ++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index 1111111..2222222 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -337,7 +337,8 @@ static int adu_open(struct inode *inode, struct file *file)
 	file->private_data = dev;
 
 	/* initialize in direction */
-	dev->read_buffer_length = 0;
+	spin_lock_irq(&dev->buflock);
+	dev->read_buffer_length = 0;
+	spin_unlock_irq(&dev->buflock);
 
 	/* fixup first read by having urb waiting for it */
 	usb_fill_int_urb(dev->interrupt_in_urb, dev->udev,
--
2.43.0

On Mon, Jul 13, 2026 at 01:26:16PM +0530, Kanishka De Silva wrote:
> dev->read_buffer_length is otherwise only ever touched under
> dev->buflock (by adu_interrupt_in_callback() and adu_read()), per the
> locking scheme documented in the comment above struct adu_device.
> adu_open() resets it to 0 without holding buflock.
> 
> In the current code this is not a reachable race. adu_open() and
> adu_release() are fully serialized by adutux_mutex, and
> adu_release_internal() calls usb_kill_urb() (via
> adu_abort_transfers()) before that mutex is dropped, which blocks
> until any in-flight adu_interrupt_in_callback() has returned. The
> write in adu_open() also precedes the urb (re)submission in program
> order, so the callback cannot observe or race with it there either.
> 
> This change brings the assignment under buflock purely so the
> field's locking is locally consistent with the driver's documented
> scheme, making the invariant easy to verify without having to reason
> across adu_open(), adu_release_internal(), and usb_kill_urb()'s
> blocking semantics. No behavioral or functional change intended.
> 
> v2:
>  - Retitled and reframed from "fix unlocked read_buffer_length write
>    in adu_open() (data race)" to a lock-discipline consistency
>    change. Discussion on the RFC (with Oliver Neukum) established
>    that adutux_mutex serialization plus usb_kill_urb()'s blocking
>    semantics rule out any runtime-reachable race here, so this is
>    no longer presented as a bugfix. (Oliver Neukum, Greg
>    Kroah-Hartman)

Version info goes below the --- line please.

> 
> Signed-off-by: Kanishka De Silva <kpskanna1915@gmail.com>
> ---
>  drivers/usb/misc/adutux.c | 3 ++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
> index 1111111..2222222 100644
> --- a/drivers/usb/misc/adutux.c
> +++ b/drivers/usb/misc/adutux.c
> @@ -337,7 +337,8 @@ static int adu_open(struct inode *inode, struct file *file)
>  	file->private_data = dev;
>  
>  	/* initialize in direction */
> -	dev->read_buffer_length = 0;
> +	spin_lock_irq(&dev->buflock);
> +	dev->read_buffer_length = 0;
> +	spin_unlock_irq(&dev->buflock);

This makes no sense, the single write here isn't going to need a lock,
it can't "tear" and if someone else is doing something with it it can
change right after the lock is released, right?

So what exactly is this supposed to be "fixing"?

Ah, your changelog says "not really fixing anything", so then it's not
needed at all, so why add this at all?

thanks,

greg k-h

On Mon, Jul 13, 2026 at 02:03:03PM +0200, Greg KH wrote:
> On Mon, Jul 13, 2026 at 01:26:16PM +0530, Kanishka De Silva wrote:
> > dev->read_buffer_length is otherwise only ever touched under
> > dev->buflock (by adu_interrupt_in_callback() and adu_read()), per the
> > locking scheme documented in the comment above struct adu_device.
> > adu_open() resets it to 0 without holding buflock.
> > 
> > In the current code this is not a reachable race. adu_open() and
> > adu_release() are fully serialized by adutux_mutex, and
> > adu_release_internal() calls usb_kill_urb() (via
> > adu_abort_transfers()) before that mutex is dropped, which blocks
> > until any in-flight adu_interrupt_in_callback() has returned. The
> > write in adu_open() also precedes the urb (re)submission in program
> > order, so the callback cannot observe or race with it there either.
> > 
> > This change brings the assignment under buflock purely so the
> > field's locking is locally consistent with the driver's documented
> > scheme, making the invariant easy to verify without having to reason
> > across adu_open(), adu_release_internal(), and usb_kill_urb()'s
> > blocking semantics. No behavioral or functional change intended.
> > 
> > v2:
> >  - Retitled and reframed from "fix unlocked read_buffer_length write
> >    in adu_open() (data race)" to a lock-discipline consistency
> >    change. Discussion on the RFC (with Oliver Neukum) established
> >    that adutux_mutex serialization plus usb_kill_urb()'s blocking
> >    semantics rule out any runtime-reachable race here, so this is
> >    no longer presented as a bugfix. (Oliver Neukum, Greg
> >    Kroah-Hartman)
> 
> Version info goes below the --- line please.
> 
> > 
> > Signed-off-by: Kanishka De Silva <kpskanna1915@gmail.com>
> > ---
> >  drivers/usb/misc/adutux.c | 3 ++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
> > index 1111111..2222222 100644
> > --- a/drivers/usb/misc/adutux.c
> > +++ b/drivers/usb/misc/adutux.c
> > @@ -337,7 +337,8 @@ static int adu_open(struct inode *inode, struct file *file)
> >  	file->private_data = dev;
> >  
> >  	/* initialize in direction */
> > -	dev->read_buffer_length = 0;
> > +	spin_lock_irq(&dev->buflock);
> > +	dev->read_buffer_length = 0;
> > +	spin_unlock_irq(&dev->buflock);
> 
> This makes no sense, the single write here isn't going to need a lock,
> it can't "tear" and if someone else is doing something with it it can
> change right after the lock is released, right?
> 
> So what exactly is this supposed to be "fixing"?
> 
> Ah, your changelog says "not really fixing anything", so then it's not
> needed at all, so why add this at all?

A better solution would be to leave the code as it is, and change the 
comment instead.  Say that buflock protects the things touched by IRQ 
handlers, but only after they have been initialized in adu_open().

Alan Stern