linuxkpi ioctl handler: restore the user data pointer

[kib]

Summary

instead of trying to hack around it with LINUX_IOCTL_MIN_PTR.  Since
linux file ioctl methods expect the user address in the data argument,
this should work for all ioctls, including the variable-length cases
like ibcore.

Only do it for the FreeBSD ABI, where we know how to reliably access the
original syscall arguments.

[zishun.yi.dev_gmail.com]

Hi, I think this is a good change overall. I actually tried to solve the same problem before in D55328, but I ended up abandoning it because my use case no longer required it.

However, I see a couple of issues with this revision:

• For IOC_IN, sys_ioctl does one copyin, but your patch bypasses it, so there is the cost of two copyin operations. (It's not a performance bottleneck, so I think maybe it's ok.)
• For IOC_OUT: sys_ioctl always performs a final copyout after the driver returns. If you give the driver the raw user-space pointer, then whatever the driver writes directly to userspace will be overwritten by that final copyout from the kernel buffer.

[kib]

In D57612#1320914, @zishun.yi.dev_gmail.com wrote:

Hi, I think this is a good change overall. I actually tried to solve the same problem before in D55328, but I ended up abandoning it because my use case no longer required it.

However, I see a couple of issues with this revision:

• For IOC_IN, sys_ioctl does one copyin, but your patch bypasses it, so there is the cost of two copyin operations. (It's not a performance bottleneck, so I think maybe it's ok.)

Yes it does not matter.

• For IOC_OUT: sys_ioctl always performs a final copyout after the driver returns. If you give the driver the raw user-space pointer, then whatever the driver writes directly to userspace will be overwritten by that final copyout from the kernel buffer.

Right, we discussed this internally and then decided that it does not matter for RDMA_VERBS_IOCTL, but it must be worked around for the generic case.

[kib]

Copy-in the user data to make sys_generic.c ioctl code copy-out identical content, for OUT ioctls.

[zishun.yi.dev_gmail.com]

sys/compat/linuxkpi/common/src/linux_compat.c

1002

Did you mean error1 == 0 here?

[kib]

Fix thinko.

[aehrenberg_nvidia.com]

I verified the changes by running these test applications:
ibv_rc_pingpong; ibv_uc_pingpong; ibv_ud_pingpong; ibv_srq_pingpong; ibv_xsrq_pingpong
All are passing.

[markj]

sys/compat/linuxkpi/common/src/linux_compat.c

956

Should we make sure that this is a userspace thread before accessing td->td_sa? I am wondering if it's possible for a kernel thread to invoke fo_ioctl without coming from userspace.

1003

Can't you just copyin() directly to task->bsd_ioctl_data?

error1 is not returned to the caller.

[kib]

sys/compat/linuxkpi/common/src/linux_compat.c

1003

I wanted to make the copying to bsd_ioctl_data more 'atomic'. If copyin failed, which could happen in theory for variadic ioctls, I do not want to touch bsd_ioctl_data. Doing copyin directly to it causes partial coping.

Yes, error1 is intentionally not propagated anywhere.

[kib]

Check for !TDP_KTHREAD.

Additionally we could check that the vmspace is usermode, e.g. by checking the flag, but I believe that TDP_KTHREAD is good enough.

[markj]

sys/compat/linuxkpi/common/src/linux_compat.c

939–951

This comment is confusing now. I'd write something more verbose:

/*
 * Background: Linux kernel code expects to operate on userspace addresses, but FreeBSD's kern_ioctl() will generally provide a kernel address. For the native process ABI, where we know how to find the original address, we reach directly into the system call args to get it. Then, if the Linux driver copied out to that address, we copy the whole block back into the kernel buffer allocated by kern_ioctl() so that kern_ioctl() itself doesn't clobber the driver's data.
*
* Otherwise, fall back to the LINUX_IOCTL_MIN_PTR hack.
*/

1003

I think the comment should explain this.

[kib]

Rewrite comments.

[markj]

sys/compat/linuxkpi/common/src/linux_compat.c

1000

1001