Re: [PATCH] autofs: ignore notification errors from a replaced pipe

[Ian Kent]

On 8/10/26 01:35, tjdqudcks0424@xxxxxxxxx wrote:
> From: Sung Byeongchan <tjdqudcks0424@xxxxxxxxx>
>
> autofs_notify_daemon() takes a reference to the current notification
> pipe under wq_mutex, then drops the mutex before writing the request. The
> write can block while the daemon enters catatonic mode and installs a
> replacement pipe.
>
> If the old pipe's reader is then closed, the delayed write returns -EPIPE
> and the generic error path enters catatonic mode again. That transition
> acts on the current superblock state, closes the replacement pipe, and
> releases the new daemon generation's wait queues.
>
> Only enter catatonic mode when the pipe which failed is still the current
> pipe. Factor the already-locked transition so the identity check and state
> change are atomic with respect to another replacement.
>
> This was reproduced on v7.3-rc5-337-gff47652a4b66c in a local QEMU guest.
> An unprivileged UID 65534 lookup filled the old packet pipe before a
> normal CATATONIC/SETPIPEFD restart. On the unmodified kernel, the new pipe
> received HUP and both a pending victim lookup and a later lookup failed
> with ENOENT in two out of two fresh-mount runs.
>
> With this change, the new pipe remained active, received an intact
> 304-byte request, and both victim lookups completed in two out of two
> runs. Normal lookup and restart-without-a-blocked-writer controls passed.
> No KASAN, oops, refcount, or lock diagnostic was observed. A source
> reproducer and complete local logs are available privately on request.
>
> The demonstrated impact is limited to denial of a shared autofs service
> during a legitimate daemon restart. No memory corruption, information leak,
> privilege escalation, or code execution primitive was observed.

The description looks sound and a after a quick look over the patch it

looks fine. So I'll add my acked-by and also look more closely at the

change later, mostly, because of the problem I mention below.


This sounds like a problem I have been struggling with for ages and had

stopped working on it because I ended up starting an quite ugly refactor.

Looking at this I think that was misguided.


Acked-by: Ian Kent <raven@xxxxxxxxxx>


Thanks for this Sung, much appreciated.

Christian, it would be great if you could pick this up as you usually

do, ;)


Thanks

Ian

> Fixes: 8d7b48e0bc5fa ("autofs4: add miscellaneous device for ioctls")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Sung Byeongchan <tjdqudcks0424@xxxxxxxxx>
> ---
>   fs/autofs/waitq.c | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/fs/autofs/waitq.c b/fs/autofs/waitq.c
> index d46241342dfc9..fd78d96e105d 100644
> --- a/fs/autofs/waitq.c
> +++ b/fs/autofs/waitq.c
> @@ -12,15 +12,13 @@
>    */
>   static autofs_wqt_t autofs_next_wait_queue = 1;
>   
> -void autofs_catatonic_mode(struct autofs_sb_info *sbi)
> +static void autofs_catatonic_mode_locked(struct autofs_sb_info *sbi)
>   {
>   	struct autofs_wait_queue *wq, *nwq;
>   
> -	mutex_lock(&sbi->wq_mutex);
> -	if (sbi->flags & AUTOFS_SBI_CATATONIC) {
> -		mutex_unlock(&sbi->wq_mutex);
> +	lockdep_assert_held(&sbi->wq_mutex);
> +	if (sbi->flags & AUTOFS_SBI_CATATONIC)
>   		return;
> -	}
>   
>   	pr_debug("entering catatonic mode\n");
>   
> @@ -40,6 +38,21 @@ void autofs_catatonic_mode(struct autofs_sb_info *sbi)
>   	fput(sbi->pipe);	/* Close the pipe */
>   	sbi->pipe = NULL;
>   	sbi->pipefd = -1;
> +}
> +
> +void autofs_catatonic_mode(struct autofs_sb_info *sbi)
> +{
> +	mutex_lock(&sbi->wq_mutex);
> +	autofs_catatonic_mode_locked(sbi);
> +	mutex_unlock(&sbi->wq_mutex);
> +}
> +
> +static void autofs_catatonic_mode_for_pipe(struct autofs_sb_info *sbi,
> +					   struct file *pipe)
> +{
> +	mutex_lock(&sbi->wq_mutex);
> +	if (sbi->pipe == pipe)
> +		autofs_catatonic_mode_locked(sbi);
>   	mutex_unlock(&sbi->wq_mutex);
>   }
>   
> @@ -170,7 +183,7 @@ static void autofs_notify_daemon(struct autofs_sb_info *sbi,
>   		autofs_wait_release(sbi, wq->wait_queue_token, ret);
>   		break;
>   	default:
> -		autofs_catatonic_mode(sbi);
> +		autofs_catatonic_mode_for_pipe(sbi, pipe);
>   		break;
>   	}
>   	fput(pipe);