[PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()

[PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()

[Ginger Li]

max3100_work() reads s->rts when it processes a pending RTS update and when
it transmits a character, both times without holding s->conf_lock.
max3100_set_mctrl() updates s->rts under that lock when the requested RTS
state changes, so the two accesses race and the work handler can program the
old RTS state in the hardware.

Take the value of s->rts in the s->conf_lock protected snapshot at the top of
the loop, like the other configuration fields.

Fixes: 7831d56b0a35 ("tty: MAX3100")
Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
---
 drivers/tty/serial/max3100.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -236,6 +236,7 @@ static void max3100_work(struct work_struct *w)
 	struct tty_port *tport = &s->port.state->port;
 	unsigned char ch;
 	int conf, cconf, cloopback, crts;
+	bool rts;
 	int rxchars;
 	u16 tx, rx;
 
@@ -251,6 +252,7 @@ static void max3100_work(struct work_struct *w)
 		s->loopback_commit = 0;
 		crts = s->rts_commit;
 		s->rts_commit = 0;
+		rts = s->rts;
 		spin_unlock(&s->conf_lock);
 		if (cconf)
 			max3100_sr(s, MAX3100_WC | conf, &rx);
@@ -258,7 +260,7 @@ static void max3100_work(struct work_struct *w)
 			max3100_sr(s, 0x4001, &rx);
 		if (crts) {
 			max3100_sr(s, MAX3100_WD | MAX3100_TE |
-				   (s->rts ? MAX3100_RTS : 0), &rx);
+				   (rts ? MAX3100_RTS : 0), &rx);
 			rxchars += max3100_handlerx(s, rx);
 		}
 
@@ -277,7 +279,7 @@ static void max3100_work(struct work_struct *w)
 			}
 			if (tx != 0xffff) {
 				max3100_calc_parity(s, &tx);
-				tx |= MAX3100_WD | (s->rts ? MAX3100_RTS : 0);
+				tx |= MAX3100_WD | (rts ? MAX3100_RTS : 0);
 				max3100_sr(s, tx, &rx);
 				rxchars += max3100_handlerx(s, rx);
 			}
-- 
2.43.0

Re: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()

[sashiko-bot]

> max3100_work() reads s->rts when it processes a pending RTS update and when
> it transmits a character, both times without holding s->conf_lock.
> max3100_set_mctrl() updates s->rts under that lock when the requested RTS
> state changes, so the two accesses race and the work handler can program the
> old RTS state in the hardware.
> 
> Take the value of s->rts in the s->conf_lock protected snapshot at the top of
> the loop, like the other configuration fields.
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922053151.11260-1-ginger.jzllee@gmail.com?part=1

RE: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()

[Maarten Brock]

Hello Ginger Li,

> -----Original Message-----
> From: Ginger Li <ginger.jzllee@gmail.com>
> Sent: Tuesday 22 September 2026 7:32
> To: jirislaby@kernel.org; gregkh@linuxfoundation.org
> Cc: linux-serial@vger.kernel.org
> Subject: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()
> 
> max3100_work() reads s->rts when it processes a pending RTS update and when
> it transmits a character, both times without holding s->conf_lock.
> max3100_set_mctrl() updates s->rts under that lock when the requested RTS
> state changes, so the two accesses race and the work handler can program the
> old RTS state in the hardware.
> 
> Take the value of s->rts in the s->conf_lock protected snapshot at the top of
> the loop, like the other configuration fields.
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
> ---
>  drivers/tty/serial/max3100.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -236,6 +236,7 @@ static void max3100_work(struct work_struct *w)
>  	struct tty_port *tport = &s->port.state->port;
>  	unsigned char ch;
>  	int conf, cconf, cloopback, crts;
> +	bool rts;
>  	int rxchars;
>  	u16 tx, rx;
> 
> @@ -251,6 +252,7 @@ static void max3100_work(struct work_struct *w)
>  		s->loopback_commit = 0;
>  		crts = s->rts_commit;
>  		s->rts_commit = 0;
> +		rts = s->rts;
>  		spin_unlock(&s->conf_lock);

This is not going to help. Now s->rts is read _earlier_ under the lock and then
the lock is released. So s->rts now has even more chance to be modified and
the old value to be programmed.

>  		if (cconf)
>  			max3100_sr(s, MAX3100_WC | conf, &rx);
> @@ -258,7 +260,7 @@ static void max3100_work(struct work_struct *w)
>  			max3100_sr(s, 0x4001, &rx);
>  		if (crts) {
>  			max3100_sr(s, MAX3100_WD | MAX3100_TE |
> -				   (s->rts ? MAX3100_RTS : 0), &rx);
> +				   (rts ? MAX3100_RTS : 0), &rx);
>  			rxchars += max3100_handlerx(s, rx);
>  		}
> 
> @@ -277,7 +279,7 @@ static void max3100_work(struct work_struct *w)
>  			}
>  			if (tx != 0xffff) {
>  				max3100_calc_parity(s, &tx);
> -				tx |= MAX3100_WD | (s->rts ? MAX3100_RTS : 0);
> +				tx |= MAX3100_WD | (rts ? MAX3100_RTS : 0);
>  				max3100_sr(s, tx, &rx);
>  				rxchars += max3100_handlerx(s, rx);
>  			}
> --
> 2.43.0
> 

Kind regards,
Maarten

Re: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()

[Ginger]

Hello Maarten Brock,

Sorry that my previous reply did not use plain text, so I am sending
this email again.

Thanks for your reply. Then, in that case, how about just directly
wrapping the reads of
s->rts with s->conf_lock in max3100_work()?

Sincerely,
Ginger

On Tue, Sep 22, 2026 at 4:17 PM Maarten Brock <Maarten.Brock@sttls.nl> wrote:
>
> Hello Ginger Li,
>
> > -----Original Message-----
> > From: Ginger Li <ginger.jzllee@gmail.com>
> > Sent: Tuesday 22 September 2026 7:32
> > To: jirislaby@kernel.org; gregkh@linuxfoundation.org
> > Cc: linux-serial@vger.kernel.org
> > Subject: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()
> >
> > max3100_work() reads s->rts when it processes a pending RTS update and when
> > it transmits a character, both times without holding s->conf_lock.
> > max3100_set_mctrl() updates s->rts under that lock when the requested RTS
> > state changes, so the two accesses race and the work handler can program the
> > old RTS state in the hardware.
> >
> > Take the value of s->rts in the s->conf_lock protected snapshot at the top of
> > the loop, like the other configuration fields.
> >
> > Fixes: 7831d56b0a35 ("tty: MAX3100")
> > Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
> > ---
> >  drivers/tty/serial/max3100.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> > --- a/drivers/tty/serial/max3100.c
> > +++ b/drivers/tty/serial/max3100.c
> > @@ -236,6 +236,7 @@ static void max3100_work(struct work_struct *w)
> >       struct tty_port *tport = &s->port.state->port;
> >       unsigned char ch;
> >       int conf, cconf, cloopback, crts;
> > +     bool rts;
> >       int rxchars;
> >       u16 tx, rx;
> >
> > @@ -251,6 +252,7 @@ static void max3100_work(struct work_struct *w)
> >               s->loopback_commit = 0;
> >               crts = s->rts_commit;
> >               s->rts_commit = 0;
> > +             rts = s->rts;
> >               spin_unlock(&s->conf_lock);
>
> This is not going to help. Now s->rts is read _earlier_ under the lock and then
> the lock is released. So s->rts now has even more chance to be modified and
> the old value to be programmed.
>
> >               if (cconf)
> >                       max3100_sr(s, MAX3100_WC | conf, &rx);
> > @@ -258,7 +260,7 @@ static void max3100_work(struct work_struct *w)
> >                       max3100_sr(s, 0x4001, &rx);
> >               if (crts) {
> >                       max3100_sr(s, MAX3100_WD | MAX3100_TE |
> > -                                (s->rts ? MAX3100_RTS : 0), &rx);
> > +                                (rts ? MAX3100_RTS : 0), &rx);
> >                       rxchars += max3100_handlerx(s, rx);
> >               }
> >
> > @@ -277,7 +279,7 @@ static void max3100_work(struct work_struct *w)
> >                       }
> >                       if (tx != 0xffff) {
> >                               max3100_calc_parity(s, &tx);
> > -                             tx |= MAX3100_WD | (s->rts ? MAX3100_RTS : 0);
> > +                             tx |= MAX3100_WD | (rts ? MAX3100_RTS : 0);
> >                               max3100_sr(s, tx, &rx);
> >                               rxchars += max3100_handlerx(s, rx);
> >                       }
> > --
> > 2.43.0
> >
>
> Kind regards,
> Maarten
>

RE: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()

[Maarten Brock]

Hello Ginger,

I had missed the meaning of s->rts_commit, but it is this flag that indicates
s->rts has changed. So your fix is correct after all, but the commit message
can be improved by mentioning s->rts_commit.

I suggest to read s->rts into rts before assigning crts under the spinlock,
just like conf is read before cconf is assigned.

Side note: personally I would not have constructed this difficult
s->rts_commit flag but combined s->rts and s->rts_commit into one byte,
making an assignment atomic in itself without lock.

Kind regards,
Maarten

> -----Original Message-----
> From: Ginger <ginger.jzllee@gmail.com>
> Sent: Tuesday 22 September 2026 10:27
> To: Maarten Brock <Maarten.Brock@sttls.nl>
> Cc: jirislaby@kernel.org; gregkh@linuxfoundation.org; linux-serial@vger.kernel.org
> Subject: Re: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()
> 
> Hello Maarten Brock,
> 
> Sorry that my previous reply did not use plain text, so I am sending
> this email again.
> 
> Thanks for your reply. Then, in that case, how about just directly
> wrapping the reads of
> s->rts with s->conf_lock in max3100_work()?
> 
> Sincerely,
> Ginger
> 
> On Tue, Sep 22, 2026 at 4:17 PM Maarten Brock <Maarten.Brock@sttls.nl> wrote:
> >
> > Hello Ginger Li,
> >
> > > -----Original Message-----
> > > From: Ginger Li <ginger.jzllee@gmail.com>
> > > Sent: Tuesday 22 September 2026 7:32
> > > To: jirislaby@kernel.org; gregkh@linuxfoundation.org
> > > Cc: linux-serial@vger.kernel.org
> > > Subject: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()
> > >
> > > max3100_work() reads s->rts when it processes a pending RTS update and when
> > > it transmits a character, both times without holding s->conf_lock.
> > > max3100_set_mctrl() updates s->rts under that lock when the requested RTS
> > > state changes, so the two accesses race and the work handler can program the
> > > old RTS state in the hardware.
> > >
> > > Take the value of s->rts in the s->conf_lock protected snapshot at the top of
> > > the loop, like the other configuration fields.
> > >
> > > Fixes: 7831d56b0a35 ("tty: MAX3100")
> > > Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
> > > ---
> > >  drivers/tty/serial/max3100.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> > > --- a/drivers/tty/serial/max3100.c
> > > +++ b/drivers/tty/serial/max3100.c
> > > @@ -236,6 +236,7 @@ static void max3100_work(struct work_struct *w)
> > >       struct tty_port *tport = &s->port.state->port;
> > >       unsigned char ch;
> > >       int conf, cconf, cloopback, crts;
> > > +     bool rts;
> > >       int rxchars;
> > >       u16 tx, rx;
> > >
> > > @@ -251,6 +252,7 @@ static void max3100_work(struct work_struct *w)
> > >               s->loopback_commit = 0;
> > >               crts = s->rts_commit;
> > >               s->rts_commit = 0;
> > > +             rts = s->rts;
> > >               spin_unlock(&s->conf_lock);
> >
> > This is not going to help. Now s->rts is read _earlier_ under the lock and then
> > the lock is released. So s->rts now has even more chance to be modified and
> > the old value to be programmed.
> >
> > >               if (cconf)
> > >                       max3100_sr(s, MAX3100_WC | conf, &rx);
> > > @@ -258,7 +260,7 @@ static void max3100_work(struct work_struct *w)
> > >                       max3100_sr(s, 0x4001, &rx);
> > >               if (crts) {
> > >                       max3100_sr(s, MAX3100_WD | MAX3100_TE |
> > > -                                (s->rts ? MAX3100_RTS : 0), &rx);
> > > +                                (rts ? MAX3100_RTS : 0), &rx);
> > >                       rxchars += max3100_handlerx(s, rx);
> > >               }
> > >
> > > @@ -277,7 +279,7 @@ static void max3100_work(struct work_struct *w)
> > >                       }
> > >                       if (tx != 0xffff) {
> > >                               max3100_calc_parity(s, &tx);
> > > -                             tx |= MAX3100_WD | (s->rts ? MAX3100_RTS : 0);
> > > +                             tx |= MAX3100_WD | (rts ? MAX3100_RTS : 0);
> > >                               max3100_sr(s, tx, &rx);
> > >                               rxchars += max3100_handlerx(s, rx);
> > >                       }
> > > --
> > > 2.43.0
> > >
> >
> > Kind regards,
> > Maarten
> >

[PATCH v2] serial: max3100: Fix a data race on s->rts in max3100_work()

[Ginger Li]

max3100_set_mctrl() stores the new RTS state and raises s->rts_commit to tell
max3100_work() that it has to program it:

	spin_lock(&s->conf_lock);
	if (s->rts != rts) {
		s->rts = rts;
		s->rts_commit = 1;
	}
	if (s->loopback_commit || s->rts_commit)
		max3100_dowork(s);
	spin_unlock(&s->conf_lock);

max3100_work() consumes s->rts_commit under s->conf_lock, but reads s->rts
itself outside of the lock, both when it services that pending update and when
it later transmits a character, so the state it programs into the hardware can
be stale.

Read s->rts into a local variable in the s->conf_lock protected snapshot at
the top of the loop, before s->rts_commit is consumed, in the same way as
s->conf is read before s->conf_commit.

Fixes: 7831d56b0a35 ("tty: MAX3100")
Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
---
v2:
 - read s->rts into "rts" before s->rts_commit is consumed, as suggested by
   Maarten
 - mention s->rts_commit in the commit message
 - left the s->rts / s->rts_commit split alone; folding both into one byte
   would be a separate cleanup
---
 drivers/tty/serial/max3100.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -236,6 +236,7 @@ static void max3100_work(struct work_struct *w)
 	struct tty_port *tport = &s->port.state->port;
 	unsigned char ch;
 	int conf, cconf, cloopback, crts;
+	bool rts;
 	int rxchars;
 	u16 tx, rx;
 
@@ -249,6 +250,7 @@ static void max3100_work(struct work_struct *w)
 		s->conf_commit = 0;
 		cloopback = s->loopback_commit;
 		s->loopback_commit = 0;
+		rts = s->rts;
 		crts = s->rts_commit;
 		s->rts_commit = 0;
 		spin_unlock(&s->conf_lock);
@@ -258,7 +260,7 @@ static void max3100_work(struct work_struct *w)
 			max3100_sr(s, 0x4001, &rx);
 		if (crts) {
 			max3100_sr(s, MAX3100_WD | MAX3100_TE |
-				   (s->rts ? MAX3100_RTS : 0), &rx);
+				   (rts ? MAX3100_RTS : 0), &rx);
 			rxchars += max3100_handlerx(s, rx);
 		}
 
@@ -277,7 +279,7 @@ static void max3100_work(struct work_struct *w)
 			}
 			if (tx != 0xffff) {
 				max3100_calc_parity(s, &tx);
-				tx |= MAX3100_WD | (s->rts ? MAX3100_RTS : 0);
+				tx |= MAX3100_WD | (rts ? MAX3100_RTS : 0);
 				max3100_sr(s, tx, &rx);
 				rxchars += max3100_handlerx(s, rx);
 			}
-- 
2.43.0

Re: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()

[Ginger]

Hello Maarten,

Thanks for checking. I have created a new patch per your suggestions. Thanks.

Sincerely,
Junzhe

On Tue, Sep 22, 2026 at 5:25 PM Maarten Brock <Maarten.Brock@sttls.nl> wrote:
>
> Hello Ginger,
>
> I had missed the meaning of s->rts_commit, but it is this flag that indicates
> s->rts has changed. So your fix is correct after all, but the commit message
> can be improved by mentioning s->rts_commit.
>
> I suggest to read s->rts into rts before assigning crts under the spinlock,
> just like conf is read before cconf is assigned.
>
> Side note: personally I would not have constructed this difficult
> s->rts_commit flag but combined s->rts and s->rts_commit into one byte,
> making an assignment atomic in itself without lock.
>
> Kind regards,
> Maarten
>
> > -----Original Message-----
> > From: Ginger <ginger.jzllee@gmail.com>
> > Sent: Tuesday 22 September 2026 10:27
> > To: Maarten Brock <Maarten.Brock@sttls.nl>
> > Cc: jirislaby@kernel.org; gregkh@linuxfoundation.org; linux-serial@vger.kernel.org
> > Subject: Re: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()
> >
> > Hello Maarten Brock,
> >
> > Sorry that my previous reply did not use plain text, so I am sending
> > this email again.
> >
> > Thanks for your reply. Then, in that case, how about just directly
> > wrapping the reads of
> > s->rts with s->conf_lock in max3100_work()?
> >
> > Sincerely,
> > Ginger
> >
> > On Tue, Sep 22, 2026 at 4:17 PM Maarten Brock <Maarten.Brock@sttls.nl> wrote:
> > >
> > > Hello Ginger Li,
> > >
> > > > -----Original Message-----
> > > > From: Ginger Li <ginger.jzllee@gmail.com>
> > > > Sent: Tuesday 22 September 2026 7:32
> > > > To: jirislaby@kernel.org; gregkh@linuxfoundation.org
> > > > Cc: linux-serial@vger.kernel.org
> > > > Subject: [PATCH] serial: max3100: Fix a data race on s->rts in max3100_work()
> > > >
> > > > max3100_work() reads s->rts when it processes a pending RTS update and when
> > > > it transmits a character, both times without holding s->conf_lock.
> > > > max3100_set_mctrl() updates s->rts under that lock when the requested RTS
> > > > state changes, so the two accesses race and the work handler can program the
> > > > old RTS state in the hardware.
> > > >
> > > > Take the value of s->rts in the s->conf_lock protected snapshot at the top of
> > > > the loop, like the other configuration fields.
> > > >
> > > > Fixes: 7831d56b0a35 ("tty: MAX3100")
> > > > Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
> > > > ---
> > > >  drivers/tty/serial/max3100.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> > > > --- a/drivers/tty/serial/max3100.c
> > > > +++ b/drivers/tty/serial/max3100.c
> > > > @@ -236,6 +236,7 @@ static void max3100_work(struct work_struct *w)
> > > >       struct tty_port *tport = &s->port.state->port;
> > > >       unsigned char ch;
> > > >       int conf, cconf, cloopback, crts;
> > > > +     bool rts;
> > > >       int rxchars;
> > > >       u16 tx, rx;
> > > >
> > > > @@ -251,6 +252,7 @@ static void max3100_work(struct work_struct *w)
> > > >               s->loopback_commit = 0;
> > > >               crts = s->rts_commit;
> > > >               s->rts_commit = 0;
> > > > +             rts = s->rts;
> > > >               spin_unlock(&s->conf_lock);
> > >
> > > This is not going to help. Now s->rts is read _earlier_ under the lock and then
> > > the lock is released. So s->rts now has even more chance to be modified and
> > > the old value to be programmed.
> > >
> > > >               if (cconf)
> > > >                       max3100_sr(s, MAX3100_WC | conf, &rx);
> > > > @@ -258,7 +260,7 @@ static void max3100_work(struct work_struct *w)
> > > >                       max3100_sr(s, 0x4001, &rx);
> > > >               if (crts) {
> > > >                       max3100_sr(s, MAX3100_WD | MAX3100_TE |
> > > > -                                (s->rts ? MAX3100_RTS : 0), &rx);
> > > > +                                (rts ? MAX3100_RTS : 0), &rx);
> > > >                       rxchars += max3100_handlerx(s, rx);
> > > >               }
> > > >
> > > > @@ -277,7 +279,7 @@ static void max3100_work(struct work_struct *w)
> > > >                       }
> > > >                       if (tx != 0xffff) {
> > > >                               max3100_calc_parity(s, &tx);
> > > > -                             tx |= MAX3100_WD | (s->rts ? MAX3100_RTS : 0);
> > > > +                             tx |= MAX3100_WD | (rts ? MAX3100_RTS : 0);
> > > >                               max3100_sr(s, tx, &rx);
> > > >                               rxchars += max3100_handlerx(s, rx);
> > > >                       }
> > > > --
> > > > 2.43.0
> > > >
> > >
> > > Kind regards,
> > > Maarten
> > >

Re: [PATCH v2] serial: max3100: Fix a data race on s->rts in max3100_work()

[sashiko-bot]

> max3100_set_mctrl() stores the new RTS state and raises s->rts_commit to tell
> max3100_work() that it has to program it:
> 
> 	spin_lock(&s->conf_lock);
> 	if (s->rts != rts) {
> 		s->rts = rts;
> 		s->rts_commit = 1;
> 	}
> 	if (s->loopback_commit || s->rts_commit)
> 		max3100_dowork(s);
> 	spin_unlock(&s->conf_lock);
> 
> max3100_work() consumes s->rts_commit under s->conf_lock, but reads s->rts
> itself outside of the lock, both when it services that pending update and when
> it later transmits a character, so the state it programs into the hardware can
> be stale.
> 
> Read s->rts into a local variable in the s->conf_lock protected snapshot at
> the top of the loop, before s->rts_commit is consumed, in the same way as
> s->conf is read before s->conf_commit.
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922170239.33485-1-ginger.jzllee@gmail.com?part=1

Re: [PATCH v2] serial: max3100: Fix a data race on s->rts in max3100_work()

[Greg KH]

On Wed, Sep 23, 2026 at 01:02:39AM +0800, Ginger Li wrote:
> max3100_set_mctrl() stores the new RTS state and raises s->rts_commit to tell
> max3100_work() that it has to program it:
> 
> 	spin_lock(&s->conf_lock);
> 	if (s->rts != rts) {
> 		s->rts = rts;
> 		s->rts_commit = 1;
> 	}
> 	if (s->loopback_commit || s->rts_commit)
> 		max3100_dowork(s);
> 	spin_unlock(&s->conf_lock);
> 
> max3100_work() consumes s->rts_commit under s->conf_lock, but reads s->rts
> itself outside of the lock, both when it services that pending update and when
> it later transmits a character, so the state it programs into the hardware can
> be stale.
> 
> Read s->rts into a local variable in the s->conf_lock protected snapshot at
> the top of the loop, before s->rts_commit is consumed, in the same way as
> s->conf is read before s->conf_commit.
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
> ---
> v2:
>  - read s->rts into "rts" before s->rts_commit is consumed, as suggested by
>    Maarten
>  - mention s->rts_commit in the commit message
>  - left the s->rts / s->rts_commit split alone; folding both into one byte
>    would be a separate cleanup
> ---
>  drivers/tty/serial/max3100.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -236,6 +236,7 @@ static void max3100_work(struct work_struct *w)
>  	struct tty_port *tport = &s->port.state->port;
>  	unsigned char ch;
>  	int conf, cconf, cloopback, crts;
> +	bool rts;
>  	int rxchars;
>  	u16 tx, rx;
>  
> @@ -249,6 +250,7 @@ static void max3100_work(struct work_struct *w)
>  		s->conf_commit = 0;
>  		cloopback = s->loopback_commit;
>  		s->loopback_commit = 0;
> +		rts = s->rts;
>  		crts = s->rts_commit;
>  		s->rts_commit = 0;
>  		spin_unlock(&s->conf_lock);
> @@ -258,7 +260,7 @@ static void max3100_work(struct work_struct *w)
>  			max3100_sr(s, 0x4001, &rx);
>  		if (crts) {
>  			max3100_sr(s, MAX3100_WD | MAX3100_TE |
> -				   (s->rts ? MAX3100_RTS : 0), &rx);
> +				   (rts ? MAX3100_RTS : 0), &rx);
>  			rxchars += max3100_handlerx(s, rx);
>  		}
>  
> @@ -277,7 +279,7 @@ static void max3100_work(struct work_struct *w)
>  			}
>  			if (tx != 0xffff) {
>  				max3100_calc_parity(s, &tx);
> -				tx |= MAX3100_WD | (s->rts ? MAX3100_RTS : 0);
> +				tx |= MAX3100_WD | (rts ? MAX3100_RTS : 0);
>  				max3100_sr(s, tx, &rx);
>  				rxchars += max3100_handlerx(s, rx);
>  			}
> -- 
> 2.43.0
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

RE: [PATCH v2] serial: max3100: Fix a data race on s->rts in max3100_work()

[Maarten Brock]

> -----Original Message-----
> From: Ginger Li <ginger.jzllee@gmail.com>
> Sent: Tuesday 22 September 2026 19:03
> To: jirislaby@kernel.org; gregkh@linuxfoundation.org
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; Maarten Brock <Maarten.Brock@sttls.nl>
> Subject: [PATCH v2] serial: max3100: Fix a data race on s->rts in max3100_work()
> 
> max3100_set_mctrl() stores the new RTS state and raises s->rts_commit to tell
> max3100_work() that it has to program it:
> 
> 	spin_lock(&s->conf_lock);
> 	if (s->rts != rts) {
> 		s->rts = rts;
> 		s->rts_commit = 1;
> 	}
> 	if (s->loopback_commit || s->rts_commit)
> 		max3100_dowork(s);
> 	spin_unlock(&s->conf_lock);
> 
> max3100_work() consumes s->rts_commit under s->conf_lock, but reads s->rts
> itself outside of the lock, both when it services that pending update and when
> it later transmits a character, so the state it programs into the hardware can
> be stale.
> 
> Read s->rts into a local variable in the s->conf_lock protected snapshot at
> the top of the loop, before s->rts_commit is consumed, in the same way as
> s->conf is read before s->conf_commit.
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>

Reviewed-by: Maarten Brock <maarten.brock@sttls.nl>

> ---
> v2:
>  - read s->rts into "rts" before s->rts_commit is consumed, as suggested by
>    Maarten
>  - mention s->rts_commit in the commit message
>  - left the s->rts / s->rts_commit split alone; folding both into one byte
>    would be a separate cleanup
> ---
>  drivers/tty/serial/max3100.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -236,6 +236,7 @@ static void max3100_work(struct work_struct *w)
>  	struct tty_port *tport = &s->port.state->port;
>  	unsigned char ch;
>  	int conf, cconf, cloopback, crts;
> +	bool rts;
>  	int rxchars;
>  	u16 tx, rx;
> 
> @@ -249,6 +250,7 @@ static void max3100_work(struct work_struct *w)
>  		s->conf_commit = 0;
>  		cloopback = s->loopback_commit;
>  		s->loopback_commit = 0;
> +		rts = s->rts;
>  		crts = s->rts_commit;
>  		s->rts_commit = 0;
>  		spin_unlock(&s->conf_lock);
> @@ -258,7 +260,7 @@ static void max3100_work(struct work_struct *w)
>  			max3100_sr(s, 0x4001, &rx);
>  		if (crts) {
>  			max3100_sr(s, MAX3100_WD | MAX3100_TE |
> -				   (s->rts ? MAX3100_RTS : 0), &rx);
> +				   (rts ? MAX3100_RTS : 0), &rx);
>  			rxchars += max3100_handlerx(s, rx);
>  		}
> 
> @@ -277,7 +279,7 @@ static void max3100_work(struct work_struct *w)
>  			}
>  			if (tx != 0xffff) {
>  				max3100_calc_parity(s, &tx);
> -				tx |= MAX3100_WD | (s->rts ? MAX3100_RTS : 0);
> +				tx |= MAX3100_WD | (rts ? MAX3100_RTS : 0);
>  				max3100_sr(s, tx, &rx);
>  				rxchars += max3100_handlerx(s, rx);
>  			}
> --
> 2.43.0