[ofa-general] [PATCH] uverbs: return ENOSYS for unimplemented commands (not EINVAL)

[Jack Morgenstein]

uverbs: return ENOSYS for unimplemented commands (not EINVAL)

In the original commit (883a99c7024c5763d6d4f22d9239c133893e8d74)
(Add a mask of device methods allowed for userspace), 
the driver returned EINVAL for unimplemented commands.

This creates a problem that there is no way to differentiate between
an unimplemented command and an implemented one which is incorrectly
invoked (which also returns EINVAL).

The fix is to have unimplemented commands return ENOSYS.

Signed-off-by: Jack Morgenstein <jackm at dev.mellanox.co.il>

---

Roland,
We've got a bit of a salad here (d--ned if we do, d--ned if we don't).

In userspace, we have low-level libraries put NULL in the virtual function
table for unimplemented verbs. libibverbs then returns NULL for those
unimplemented verbs which expect a pointer return (e.g., ibv_create_srq)
(also a problem since this does not differentiate between a missing verb and
an incorrectly-invoked one),
and ENOSYS for verbs which expect an int returned (e.g., resize_cq).

This is not consistent with what was done in the kernel for unimplemented verbs
(where EINVAL is returned).

Additionally, what was done in the kernel (returning EINVAL) is problematic
in that it makes it impossible to differentiate an unimplemented verb from one
which simply had errors in the calling parameters.

IMHO, the correct fix is to have unimplemented kernel verbs return ENOSYS.
**(MPI already checks for ENOSYS when deciding whether or not to use resize-cq)**.

I'm not sure, though, how to handle returns from older kernels -- should all user
apps check for either ENOSYS or EINVAL?  What about cases where the verb actually
is implemented, but incorrectly called -- in older kernels this is already a problem.
What about apps which checked for ENOSYS (consistent with userspace usage), and
then run with a new low-level library over an older kernel, and suddenly start getting
EINVAL returns (for resize_cq, there is no "activation" bit anywhere which gets passed
from kernel to the user-level)?

Ouch!

Any ideas?

Index: infiniband/drivers/infiniband/core/uverbs_main.c
===================================================================
--- infiniband.orig/drivers/infiniband/core/uverbs_main.c
+++ infiniband/drivers/infiniband/core/uverbs_main.c
@@ -584,10 +584,12 @@ static ssize_t ib_uverbs_write(struct fi
 
 	if (hdr.command < 0				||
 	    hdr.command >= ARRAY_SIZE(uverbs_cmd_table) ||
-	    !uverbs_cmd_table[hdr.command]		||
-	    !(file->device->ib_dev->uverbs_cmd_mask & (1ull << hdr.command)))
+	    !uverbs_cmd_table[hdr.command])
 		return -EINVAL;
 
+	if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << hdr.command)))
+		return -ENOSYS;
+
 	if (!file->ucontext &&
 	    hdr.command != IB_USER_VERBS_CMD_GET_CONTEXT)
 		return -EINVAL;