Re: [PATCH 1/2] fscontext: do not consume log entries for -EMSGSIZE case

[Al Viro <viro@zeniv.linux.org.uk>]

On Wed, Aug 06, 2025 at 03:31:10PM +1000, Aleksa Sarai wrote:
> Userspace generally expects APIs that return EMSGSIZE to allow for them
> to adjust their buffer size and retry the operation. However, the
> fscontext log would previously clear the message even in the EMSGSIZE
> case.
> 
> Given that it is very cheap for us to check whether the buffer is too
> small before we remove the message from the ring buffer, let's just do
> that instead.
> 
> Fixes: 007ec26cdc9f ("vfs: Implement logging through fs_context")
> Cc: David Howells <dhowells@redhat.com>
> Cc: <stable@vger.kernel.org> # v5.2+
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
>  fs/fsopen.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/fsopen.c b/fs/fsopen.c
> index 1aaf4cb2afb2..f5fdaa97965b 100644
> --- a/fs/fsopen.c
> +++ b/fs/fsopen.c
> @@ -36,23 +36,25 @@ static ssize_t fscontext_read(struct file *file,
>  	if (ret < 0)
>  		return ret;
>  
> -	if (log->head == log->tail) {
> -		mutex_unlock(&fc->uapi_mutex);
> -		return -ENODATA;
> -	}
> +	ret = -ENODATA;
> +	if (log->head == log->tail)
> +		goto err_unlock_nomsg;
>  
>  	index = log->tail & (logsize - 1);
>  	p = log->buffer[index];
> +	n = strlen(p);
> +
> +	ret = -EMSGSIZE;
> +	if (n > len)
> +		goto err_unlock_nomsg;
> +

FWIW, I would rather turn that into a helper taking log and len and
returning p or ERR_PTR(...); something like

static inline const char *fetch_message(struct fc_log *log, size_t len,
					bool *need_free)
{
	int index = log->tail & (ARRAY_SIZE(log->buffer) - 1);
	const char *p = log->buffer[index];

	if (unlikely(log->head == log->tail))
		return ERR_PTR(-ENODATA);

	n = strlen(p);
	if (unlikely(n > len))
		return ERR_PTR(-EMSGSIZE);

	log->buffer[index] = NULL;
	*need_free = log->need_free & (1 << index);
	log->need_free &= ~(1 << index);
	log->tail++;

	return p;
}

with caller being

        ret = mutex_lock_interruptible(&fc->uapi_mutex);
	if (ret < 0)
		return ret;
	p = fetch_message(log, len, &need_free);
	mutex_unlock(&fc->uapi_mutex);
	if (IS_ERR(p))
		return PTR_ERR(p);
	n = strlen(p);
	if (copy_to_user(_buf, p, n))
		n = -EFAULT;
	if (need_free)
		kfree(p);
	return n;

and that's it.