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

On Sun, Dec 05, 2021 at 03:03:26AM +0000, Jessica Clarke wrote:
> 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?

Having two swapoff() syscalls is worse, and having them only differ in
semantic by single flag is kind of crime.

I do not see swapoff("") as problematic, we are changing a minor semantic of
the management syscall.  I only wanted to avoid flag day for swapoff binaries.

BTW, I considered requiring proper alignment for uap->name, and then checking
the whole uap->name_old_syscall for NULL, but then decided that this is
overkill.  If you think that swapoff("") that important, I can add that
additional verification.