RFC userfaultfd ioctl decode

[Dr. David Alan Gilbert]

* Dmitry V. Levin (ldv at altlinux.org) wrote:
> On Thu, Apr 21, 2016 at 02:04:33PM +0100, Dr. David Alan Gilbert wrote:
> [...]
> > --- 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], [
> > +])
> 
> This should rather go to the first (big) AC_CHECK_HEADERS list
> in the file.

Done.

> > --- 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);
> 
> The tradition approach is to wrap this into
> #ifdef HAVE_LINUX_USERFAULTFD_H / #endif
> so that ...

Done

> 
> > --- 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
> 
> ... the whole parser with all headers it needs could be wrapped
> with a single ifdef.

OK, that is easier.

> > +	case UFFDIO_COPY: {
> > +		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);
> 
> I suppose you can use recently added PRI__*64 instead of PRI*64,
> this way you won't need explicit casts.

Done.

> > +				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;
> > +			}
> 
> The preferred method is to print uc.copy as a part of structure:
> 
> 	if (entering(tcp)) {
> 		if (umove_or_printaddr(tcp, arg, &uc))
> 			return RVAL_DECODED | 1;
> 		tprintf("{dst=%" PRI__x64 ", src=%" PRI__x64
> 			", len=%" PRI__u64 ", mode=%" PRI__u64
> 			", copy=",
> 			uc.dst, uc.src, uc.len, uc.mode);
> 			return 1;
> 	} else {
> 		if (!umove_or_printaddr(tcp, arg, &uc.copy))
> 			tprintf("%" PRI__d64, uc.copy);
> 		tprints("}");
> 		return 1;
> 	}

OK, thanks, taken that.
strace seems to have PRI__s64 rather than PRId64 that would have matched
the C library; I've also changed those to %# for the hex., and that
second umove is still &uc - it's still part of the full structure.

> 
> > +		}
> > +		break;
> 
> As (your edition of) this parser has already printed something at this
> point, it has to return 1 + RVAL_something to let the caller know,
> otherwise the address will be printed twice.  You can easily reproduce
> this by invoking UFFDIO_COPY with NULL address.
> 
> > +	}
> > +#endif
> > +
> > +	default:
> > +		return 0;
> 
> As this parser in default case is not going to do anything on exiting,
> it report this to the caller by returning RVAL_DECODED.

OK, I think.

and thanks for the pointer to that explanatory email.

I'll post an updated version.

Thanks,

Dave

> 
> 
> -- 
> ldv



> ------------------------------------------------------------------------------
> Find and fix application performance issues faster with Applications Manager
> Applications Manager provides deep performance insights into multiple tiers of
> your business applications. It resolves application problems quickly and
> reduces your MTTR. Get your free trial!
> https://ad.doubleclick.net/ddm/clk/302982198;130105516;z

> _______________________________________________
> Strace-devel mailing list
> Strace-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/strace-devel

--
Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK