Re: [PATCH v4 02/13] liveupdate: avoid mixing cleanup guards with goto in luo_session_retrieve_fd

[Pasha Tatashin]

On 05-30 22:19, Pasha Tatashin wrote:
> Refactoring luo_session_retrieve_fd() to avoid mixing automated
> cleanup-style guards with goto-based resource release, which is not
> recommended under the Linux kernel coding style.
> 
> Signed-off-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>
> ---
>  kernel/liveupdate/luo_session.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/liveupdate/luo_session.c b/kernel/liveupdate/luo_session.c
> index 146414933977..8d9201c25412 100644
> --- a/kernel/liveupdate/luo_session.c
> +++ b/kernel/liveupdate/luo_session.c
> @@ -291,25 +291,24 @@ static int luo_session_retrieve_fd(struct luo_session *session,
>  	if (argp->fd < 0)
>  		return argp->fd;
>  
> -	guard(mutex)(&session->mutex);
> -	err = luo_retrieve_file(&session->file_set, argp->token, &file);
> -	if (err < 0)
> -		goto  err_put_fd;
> +	scoped_guard(mutex, &session->mutex) {
> +		err = luo_retrieve_file(&session->file_set, argp->token, &file);
> +		if (err < 0) {
> +			put_unused_fd(argp->fd);
> +			return err;
> +		}
> +	}
>  
>  	err = luo_ucmd_respond(ucmd, sizeof(*argp));
> -	if (err)
> -		goto err_put_file;
> +	if (err) {
> +		fput(file);
> +		put_unused_fd(argp->fd);
> +		return err;
> +	}
>  
>  	fd_install(argp->fd, file);

>From Sashiko:
This is a pre-existing issue, but could this error path allow userspace
to hijack a recycled file descriptor?
...

Answer: No.

get_unused_fd_flags allocates an unused file descriptor (FD) that 
initially points to NULL. It is not associated with any "struct file"; 
this association occurs only during fd_install(), which is the final 
function called before returning to userspace.

>  
>  	return 0;
> -
> -err_put_file:
> -	fput(file);
> -err_put_fd:
> -	put_unused_fd(argp->fd);
> -
> -	return err;
>  }
>  
>  static int luo_session_finish(struct luo_session *session,
> -- 
> 2.53.0
>