RFC userfaultfd ioctl decode

[Dr. David Alan Gilbert]

* Gabriel Laskar (gabriel at lse.epita.fr) wrote:
> On Thu, 21 Apr 2016 14:04:33 +0100
> "Dr. David Alan Gilbert" <dgilbert at redhat.com> wrote:
> 
> > Hi,
> >    Please find below a decoder for the UFFDIO_COPY ioctl
> > on the userfaultfd fd;  but there are a few other ioctls
> > in the set to decode and I thought I'd ask before doing those:
> > 
> >   1) Is it basically on the right lines;  I'm not that confident
> >      I understand the return flags on the ioctl decoder function.
> 
> The return flags are the same as for the syscalls in general, it is
> written in defs.h around line 378. It allows to type the return value
> if it is not an int.
> 
> For the ioctl decoding function, on entering there is:
> 
> * 0 if nothing has been decoded, (and it will be probably done on
>   exiting())
> * 1 parameter have been parsed, and printed
> * RVAL_DECODED if there is nothing else to do.
> * 1 | RVAL_DECODED : parameter have been parsed, but there is some kind
>   of error/nothing has been printed, so this allow the ioctl decoder to
>   write a default value.
> 
> and on exiting, there is only 0 or 1.

Thanks;  it would be great if one of these explanations went in ioctl.c
somewhere obvious.

> >   2) Are there any hooks for decoding data read off an fd?  The
> > userfaultfd returns structures giving address of the fault etc.
> 
> For the moment, there is no way to hook the read/write decoders in
> order to decode the buffers.

Pity, but OK.

> > Dave
> > 
> > 
> > >From a0f7d0e63a6d33a98d336b588830cd2dada649fc Mon Sep 17 00:00:00
> > >2001  
> > From: "Dr. David Alan Gilbert" <dgilbert at redhat.com>
> > Date: Thu, 21 Apr 2016 13:49:40 +0100
> > Subject: [PATCH] Decode UFFDIO_COPY ioctl
> > 
> > UFFDIO_COPY is one of the ioctls on the fd returned by userfaultfd.
> > Note that it reads from and also returns a result in it's data
> > structure.
> > 
> > * configure.ac: Add test for userfaultfd.h.
> > * userfaultfd.c: Add ioctl decoder.
> > * defs.h: declare that decoder.
> > * ioctl.c: Wire in the new decoder.
> > ---
> >  configure.ac  |  3 +++
> >  defs.h        |  1 +
> >  ioctl.c       |  2 ++
> >  userfaultfd.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 47 insertions(+)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 2b29c94..cb9b494 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -420,6 +420,9 @@ AC_CHECK_HEADERS([linux/input.h], [
> >  	AC_CHECK_MEMBERS([struct input_absinfo.resolution],,,
> > [#include <linux/input.h>]) ])
> >  
> > +AC_CHECK_HEADERS([linux/userfaultfd.h], [
> > +])
> 
> If there is no custom behavior to have for AC_CHECK_HEADERS, you can
> add the header to the global list.

Thanks, yes, done.

> > +
> >  AC_CHECK_HEADERS([linux/bpf.h], [
> >  	AC_CACHE_CHECK([whether union bpf_attr.log_buf
> > initialization works], [st_cv_have_union_bpf_attr_log_buf],
> > diff --git a/defs.h b/defs.h
> > index ac59349..4894fdb 100644
> > --- a/defs.h
> > +++ b/defs.h
> > @@ -649,6 +649,7 @@ extern int scsi_ioctl(struct tcb *, const
> > unsigned int, long); extern int sock_ioctl(struct tcb *, const
> > unsigned int, long); extern int term_ioctl(struct tcb *, const
> > unsigned int, long); extern int ubi_ioctl(struct tcb *, const
> > unsigned int, long); +extern int uffdio_ioctl(struct tcb *, const
> > unsigned int, long); extern int v4l2_ioctl(struct tcb *, const
> > unsigned int, long); 
> >  extern int tv_nz(const struct timeval *);
> > diff --git a/ioctl.c b/ioctl.c
> > index f70dc44..464329b 100644
> > --- a/ioctl.c
> > +++ b/ioctl.c
> > @@ -263,6 +263,8 @@ ioctl_decode(struct tcb *tcp)
> >  	case 'E':
> >  		return evdev_ioctl(tcp, code, arg);
> >  #endif
> > +	case 0xaa:
> > +		return uffdio_ioctl(tcp, code, arg);
> 
> for most of the ioctl code, we prefer to have the #ifdef HEADER around
> the case instead of inside the function, but this is mostly cosmetic
> and for consistence.

Yes, done.

> 
> >  	default:
> >  		break;
> >  	}
> > diff --git a/userfaultfd.c b/userfaultfd.c
> > index 15f825a..2a06e1f 100644
> > --- a/userfaultfd.c
> > +++ b/userfaultfd.c
> > @@ -27,9 +27,50 @@
> >  
> >  #include "defs.h"
> >  #include <fcntl.h>
> > +#include <linux/ioctl.h>
> > +#ifdef HAVE_LINUX_USERFAULTFD_H
> > +#include <linux/userfaultfd.h>
> > +#endif
> >  
> >  #include "xlat/uffd_flags.h"
> >  
> > +
> > +int
> > +uffdio_ioctl(struct tcb *tcp, const unsigned int code, const long
> > arg) +{
> > +#ifdef HAVE_LINUX_USERFAULTFD_H
> > +	static char auxbuf[64];
> > +#endif
> > +
> > +	switch (code) {
> > +#ifdef HAVE_LINUX_USERFAULTFD_H
> > +	case UFFDIO_COPY: {
> 
> before that, you need to check 
> 
> > +		struct uffdio_copy uc;
> > +		if (!umove_or_printaddr(tcp, arg, &uc)) {
> > +			if (entering(tcp)) {
> > +				tprintf("{dst=%" PRIx64 ", src=%"
> > PRIx64
> > +				", len=%" PRIu64 ", mode=%" PRIu64
> > "}",
> > +				(uint64_t)uc.dst, (uint64_t)uc.src,
> > +				(uint64_t)uc.len, (uint64_t)uc.mode);
> > +				return 1;
> > +			} else {
> > +				/* copy also returns the number of
> > bytes
> > +				 * copied */
> > +				sprintf(auxbuf, "copy=%" PRId64,
> > +				(int64_t)uc.copy);
> > +				tcp->auxstr = auxbuf;
> > +				return RVAL_STR | 1;
> 
> There is no need to do anything on exit. The return value as an int is
> fine.

Thanks, yes, similar to Dmitry's comments.

Thanks again,

Dave

> > +			}
> > +		}
> > +		break;
> > +	}
> > +#endif
> > +
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> >  SYS_FUNC(userfaultfd)
> >  {
> >  	printflags(uffd_flags, tcp->u_arg[0], "UFFD_???");
> 
> 
> 
> -- 
> Gabriel Laskar
--
Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK