Re: [PATCH 1/2] errseq: don't let errseq_check_and_advance() hide later errors

[Jan Kara]

On Sun 27-09-26 10:47:25, Shashank Mohan Jain wrote:
> errseq_check_and_advance() reads the errseq_t, sets ERRSEQ_SEEN with a
> cmpxchg() and then advances *since to the value it computed, ignoring
> whether the cmpxchg() succeeded. When it fails because errseq_set()
> recorded a different error in the meantime, *since ends up holding a
> value that was never stored in the errseq_t.
> 
> errseq_set() only bumps the counter when ERRSEQ_SEEN is set. So as long
> as nobody has seen the new error, recording the original errno again
> recreates exactly the old value, and once another subscriber marks it
> seen, it is equal to the stale *since. The next check through that
> cursor then reports nothing, although -ENOSPC was recorded after the
> value that check reported, and -EIO was recorded again after it
> returned:
> 
>   cursor f                writeback              cursor g
>   --------                ---------              --------
>                           set(-EIO)
>   check_and_advance(&f)
>     old = [c, EIO]
>                           set(-ENOSPC)
>                           -> [c, ENOSPC]
>     cmpxchg() fails
>     f = [c, EIO, SEEN]
>     returns -EIO
>                           set(-EIO)
>                           -> [c, EIO]
>                           (no bump: unseen)
>                                                  check_and_advance(&g)
>                                                  -> [c, EIO, SEEN]
>                                                  returns -EIO
>   check_and_advance(&f)
>     [c, EIO, SEEN] == f
>     returns 0             <- -ENOSPC and the second -EIO are lost
> 
> For file->f_wb_err this means that fsync() on one descriptor can return
> 0 although writeback failed after the previous fsync() on that
> descriptor returned, when writeback errors race with fsync() on another
> descriptor of the same file. The same applies to syncfs() through
> sb->s_wb_err, which every file on the filesystem shares, and to the
> ext4 and jbd2 cursors on the block device mapping. The kernel-doc
> promises "Negative errno if one has been stored, or 0 if no new error
> has occurred".
> 
> Retry with the value found when the cmpxchg() fails, so that *since is
> only ever advanced to a value that was actually stored with ERRSEQ_SEEN
> set. Every later errseq_set() then has to bump the counter, and the
> error is reported. The retry loop terminates because each iteration
> needs a concurrent update, like the loop in errseq_set().
> 
> A TLA+ model of errseq_set() and errseq_check_and_advance(), checked
> with the TLC model checker, finds the lost error with the current code;
> with this change TLC checks that an error recorded after a check
> returned is always reported by the next check (exhaustively for two
> subscribers with three checks each, and one writer recording four
> errors or two writers recording two each). The KUnit race test added
> in the next patch misses the error in 1-6% of 2 million rounds on
> 4-CPU UML before this change (depending on host load), and in none
> after it.
> 
> Fixes: 84cbadadc6ea ("lib: add errseq_t type and infrastructure for handling it")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM TLC
> Signed-off-by: Shashank Mohan Jain <jain.sm@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> Found with a TLA+ model of lib/errseq.c checked with TLC; the fix, the
> test and the changelogs were drafted with an LLM assistant (Claude
> Code, Claude Opus 5.5).
> 
> Tested: the KUnit case in patch 2 on UML x86_64 with 4 CPUs (8 runs)
> and 2 CPUs, and on UML i386 with 4 CPUs (--kernel_args seccomp=on
> --kernel_args ncpus=4): 22k-116k of 2M rounds lose the error before,
> 0 after; a userspace replay of the unmodified lib/errseq.c with a hook
> before the cmpxchg(); TLC (exhaustive for 2 files x 3 checks, 1 writer
> x 4 errors or 2 writers x 2 errors); W=1 builds for UML x86_64 and
> i386.
> Not tested: fsync()/syncfs() on a real failing device, and weakly
> ordered hardware (the model is sequentially consistent; the fix adds
> no ordering requirements beyond cmpxchg()).
> 
> Applies to mainline on its own; see the cover letter for patch 2.
> 
>  lib/errseq.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/errseq.c b/lib/errseq.c
> index 13a2581c5a87..ea4ff9cc2166 100644
> --- a/lib/errseq.c
> +++ b/lib/errseq.c
> @@ -162,7 +162,8 @@ EXPORT_SYMBOL(errseq_check);
>   * points to. If it does, then just return 0.
>   *
>   * If it doesn't, then the value has changed. Set the "seen" flag, and try to
> - * swap it into place as the new eseq value. Then, set that value as the new
> + * swap it into place as the new eseq value. If the swap fails because the
> + * value changed, retry with the new value. Then, set that value as the new
>   * "since" value, and return whatever the error portion is set to.
>   *
>   * Note that no locking is provided here for concurrent updates to the "since"
> @@ -184,24 +185,30 @@ int errseq_check_and_advance(errseq_t *eseq, errseq_t *since)
>  	 * to take the lock that protects the "since" value.
>  	 */
>  	old = READ_ONCE(*eseq);
> -	if (old != *since) {
> +	while (old != *since) {
>  		/*
>  		 * Set the flag and try to swap it into place if it has
>  		 * changed.
>  		 *
> -		 * We don't care about the outcome of the swap here. If the
> -		 * swap doesn't occur, then it has either been updated by a
> -		 * writer who is altering the value in some way (updating
> -		 * counter or resetting the error), or another reader who is
> -		 * just setting the "seen" flag. Either outcome is OK, and we
> -		 * can advance "since" and return an error based on what we
> -		 * have.
> +		 * If the swap fails, a writer or another reader changed the
> +		 * value under us, so retry with the new value.  "since" must
> +		 * only ever be set to a value that was stored with the SEEN
> +		 * flag: errseq_set() does not bump the counter while the
> +		 * flag is clear, so an unstored value could come back later
> +		 * and hide the errors that were recorded in the meantime.
>  		 */
>  		new = old | ERRSEQ_SEEN;
> -		if (new != old)
> -			cmpxchg(eseq, old, new);
> +		if (new != old) {
> +			errseq_t cur = cmpxchg(eseq, old, new);
> +
> +			if (cur != old) {
> +				old = cur;
> +				continue;
> +			}
> +		}
>  		*since = new;
>  		err = -(new & ERRNO_MASK);
> +		break;
>  	}
>  	return err;
>  }
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR