Re: [PATCH] tty: n_tty: order lockless input availability checks

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, May 04, 2026 at 03:23:21PM +0800, Cen Zhang wrote:
> The N_TTY read buffer uses release/acquire ordering for its
> lockless ring indices. Input producers release-publish canon_head and
> commit_head after updating the buffer and delimiter flags, and readers
> acquire those heads before copying data. Readers also release-publish
> read_tail before producers use it to calculate room.
> 
> chars_in_buffer() and input_available_p() sample the same indices
> for availability and flow-control decisions, but use plain loads. That
> can miss the ordering used by the data-copy paths and can also let
> poll() observe termios-synthesized availability with weaker ordering
> than normal receive-side publication.
> 
> Use acquire loads for the lockless head/tail samples in those
> helpers. When n_tty_set_termios() updates canonical/noncanonical
> availability, publish the updated heads with release stores as well.
> Keep the cached icanon bit as an intentionally lockless mode snapshot
> and annotate that access.
> 
> Fixes: 70aca71f92ca ("n_tty: Fix unordered accesses to lockless read buffer")
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
>  drivers/tty/n_tty.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index e6a0f5b40d0a..56b0cd96a453 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -213,9 +213,17 @@ static void n_tty_kick_worker(const struct tty_struct *tty)
>  static ssize_t chars_in_buffer(const struct tty_struct *tty)
>  {
>  	const struct n_tty_data *ldata = tty->disc_data;
> -	size_t head = ldata->icanon ? ldata->canon_head : ldata->commit_head;
> +	bool icanon = data_race((int)ldata->icanon); /* lockless snapshot */
> +	size_t head;
> +	size_t tail;
>  
> -	return head - ldata->read_tail;
> +	if (icanon)
> +		head = smp_load_acquire(&ldata->canon_head); /* producer publish */
> +	else
> +		head = smp_load_acquire(&ldata->commit_head); /* producer publish */
> +	tail = smp_load_acquire(&ldata->read_tail); /* consumer publish */

I still don't understand the need for this change, nor do I understand
the changelog text, sorry.

These reads can not "tear", so there shouldn't be an issue with the
read, so why is smp_load_acquire() suddenly now required?  What changed
to need this?


> +
> +	return head - tail;
>  }
>  
>  /**
> @@ -1779,14 +1787,14 @@ static void n_tty_set_termios(struct tty_struct *tty, const struct ktermios *old
>  		bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE);
>  		ldata->line_start = ldata->read_tail;
>  		if (!L_ICANON(tty) || !read_cnt(ldata)) {
> -			ldata->canon_head = ldata->read_tail;
> +			smp_store_release(&ldata->canon_head, ldata->read_tail); /* publish */

So everything is a "publish"?  That doesn't make sense to me, what am I
missing?

>  			ldata->push = 0;
>  		} else {
>  			set_bit(MASK(ldata->read_head - 1), ldata->read_flags);
> -			ldata->canon_head = ldata->read_head;
> +			smp_store_release(&ldata->canon_head, ldata->read_head); /* publish */
>  			ldata->push = 1;
>  		}
> -		ldata->commit_head = ldata->read_head;
> +		smp_store_release(&ldata->commit_head, ldata->read_head); /* publish */
>  		ldata->erasing = 0;
>  		ldata->lnext = 0;
>  	}
> @@ -1908,11 +1916,17 @@ static inline int input_available_p(const struct tty_struct *tty, int poll)
>  {
>  	const struct n_tty_data *ldata = tty->disc_data;
>  	int amt = poll && !TIME_CHAR(tty) && MIN_CHAR(tty) ? MIN_CHAR(tty) : 1;
> +	bool icanon = data_race((int)ldata->icanon); /* lockless snapshot */

What is racing?  How?


> +	size_t tail = smp_load_acquire(&ldata->read_tail); /* consumer publish */
> +	size_t head;
>  
> -	if (ldata->icanon && !L_EXTPROC(tty))
> -		return ldata->canon_head != ldata->read_tail;
> -	else
> -		return ldata->commit_head - ldata->read_tail >= amt;
> +	if (icanon && !L_EXTPROC(tty)) {
> +		head = smp_load_acquire(&ldata->canon_head); /* producer publish */
> +		return head != tail;
> +	}
> +
> +	head = smp_load_acquire(&ldata->commit_head); /* producer publish */
> +	return head - tail >= amt;

Again, this is some very old and battle-tested code, and I am loath to
touch it and add any additional latencies here unless there is a real
reported bug that can be found, and not just some theoretical one.

thanks,

greg k-h