Re: git: a4e4132fa3bf - main - swapoff(2): replace special device name argument with a structure

On 4 Dec 2021, at 22:21, Konstantin Belousov <kib@FreeBSD.org> wrote:
> 
> The branch main has been updated by kib:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=a4e4132fa3bfadb6047fc0fa5f399f4640460300
> 
> commit a4e4132fa3bfadb6047fc0fa5f399f4640460300
> Author:     Konstantin Belousov <kib@FreeBSD.org>
> AuthorDate: 2021-11-29 16:26:31 +0000
> Commit:     Konstantin Belousov <kib@FreeBSD.org>
> CommitDate: 2021-12-04 22:20:58 +0000
> 
>    swapoff(2): replace special device name argument with a structure
> 
>    For compatibility, add a placeholder pointer to the start of the
>    added struct swapoff_new_args, and use it to distinguish old vs. new
>    style of syscall invocation.
> 
>    Reviewed by:    markj
>    Discussed with: alc
>    Sponsored by:   The FreeBSD Foundation
>    MFC after:      1 week
>    Differential revision:  https://reviews.freebsd.org/D33165
> ---
> sys/vm/swap_pager.c | 27 +++++++++++++++++++++++++--
> sys/vm/swap_pager.h |  8 ++++++++
> 2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/sys/vm/swap_pager.c b/sys/vm/swap_pager.c
> index 165373d1b527..dc1df79f4fcd 100644
> --- a/sys/vm/swap_pager.c
> +++ b/sys/vm/swap_pager.c
> @@ -2491,15 +2491,38 @@ sys_swapoff(struct thread *td, struct swapoff_args *uap)
> 	struct vnode *vp;
> 	struct nameidata nd;
> 	struct swdevt *sp;
> -	int error;
> +	struct swapoff_new_args sa;
> +	int error, probe_byte;
> 
> 	error = priv_check(td, PRIV_SWAPOFF);
> 	if (error)
> 		return (error);
> 
> +	/*
> +	 * Detect old vs. new-style swapoff(2) syscall.  The first
> +	 * pointer in the memory pointed to by uap->name is NULL for
> +	 * the new variant.
> +	 */
> +	probe_byte = fubyte(uap->name);
> +	switch (probe_byte) {
> +	case -1:
> +		return (EFAULT);
> +	case 0:
> +		error = copyin(uap->name, &sa, sizeof(sa));
> +		if (error != 0)
> +			return (error);
> +		if (sa.flags != 0)
> +			return (EINVAL);
> +		break;
> +	default:
> +		bzero(&sa, sizeof(sa));
> +		sa.name = uap->name;
> +		break;
> +	}

Doesn’t this change the semantics of swapoff("")?

Previously it would fail deterministically, presumably with ENOENT or
something, but now it reinterprets whatever follows that string in
memory as the new argument structure. It probably doesn’t matter, but
this approach is ugly. Can we not just define a new syscall rather than
this kind of bodge?

Jess