[RFC PATCH] ioctl: decode VFIO_IOMMU_MAP_DMA

[Dmitry V. Levin]

On Thu, May 06, 2021 at 05:07:43PM +0000, Alyssa Ross wrote:
[...]
> But the main reason I'm submitting this as an RFC is that I'm not sure
> how to handle the 'request' argument of the ioctl() call
> (i.e. VFIO_IOMMU_MAP_DMA).  Some of the VFIO ioctl request numbers are
> reused.  VFIO_IOMMU_MAP_DMA == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO.  To
> tell them apart, we have to check the device type of the file the
> ioctl was sent to.  It's easy enough to do that in vfio_ioctl(), as
> I've done here, but that means that the request argument will still
> show up in strace as "VFIO_DEVICE_PCI_HOT_RESET or VFIO_IOMMU_MAP_DMA".
> Ideally, we'd do this check earlier, before printing the request
> argument, so we could figure out which of the two candidate names to
> print.  But this doesn't seem to be a problem strace has solved before,
> and it's not at all obvious to me how ioctl.c could be modified to
> accomodate this, in a way where the disambiguated name could be passed
> through ioctl_decode() to vfio_ioctl().  I'd appreciate any direction I
> could be offered on this.

There is a way to override "VFIO_DEVICE_PCI_HOT_RESET or VFIO_IOMMU_MAP_DMA"
output: when ioctl_decode_command_number() return value contains
IOCTL_NUMBER_STOP_LOOKUP, ioctl_lookup() is not called.

The function called by ioctl_decode_command_number() can use e.g.
set_tcb_priv_data() to pass data to the function called by ioctl_decode().

> Another, easier question: should I use a bundled header for this?  I
> did that in my initial implementation, just because it felt easiest to
> me at the time.  (I've ommitted the content of the header in this
> patch, since that should make reviewing easier, and I don't expect
> this patch to be applied as-is anyway.)  I don't really understand
> when it's preferable to let xlat detect values, when fallbacks should
> be given, and when bundled headers should be used.

Bundled headers were introduced recently to avoid portability issues.
I suggest to use bundled headers in the new code.

[...]
> @@ -684,6 +683,14 @@ printfd_pid_tracee_ns(struct tcb *tcp, pid_t pid, int fd)
>  	printfd_pid(tcp, strace_pid, fd);
>  }
>  
> +int
> +stat_fd_in_proc(pid_t pid, int fd, strace_stat_t *statbuf)

This has to be "struct tcb *" instead of "pid_t" as in getfdproto(), so
that get_proc_pid() could be used to obtain the pid suitable for /proc/.

> +{
> +	char path[33];
> +	xsprintf(path, "/proc/%d/fd/%d", pid, fd);

Wouldn't it be better to use the expression behind this magic value,
e.g. sizeof("/proc/" STRINGIFY(INT_MIN) "/fd/" STRINGIFY(INT_MIN))?

[...]
> +int
> +main(void)
> +{
> +	int fd = open("/dev/vfio/vfio", O_RDONLY);
> +	if (fd == -1)
> +		return 77;

Consider using perror_msg_and_skip("/dev/vfio/vfio").

> +	

Consider enabling .git/hooks/pre-commit to avoid trailing whitespace.


-- 
ldv