ib_core: Add API to support IBoE from userspace

Add ib_uverbs_get_eth_l2_addr() to allow ibv_create_ah() to resolve <sgid, dgid> to <vlan, dmac> for any gid type. Although user-space might bypass this call for link-local gids, it is better not to replicate the kernel resolution policy. Port link layer is also returned by ibv_query_port().

drivers/infiniband/core/uverbs_cmd.c +15 -0

+ssize_t ib_uverbs_get_eth_l2_addr(struct ib_uverbs_file *file, const char __user *buf,
+				  int in_len, int out_len)
+{
+	struct ib_uverbs_get_eth_l2_addr       cmd;
+	struct ib_uverbs_get_eth_l2_addr_resp  resp;
+	int              ret;
+	struct ib_pd    *pd;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
+	if (copy_from_user(&cmd, buf, sizeof cmd))
+		return -EFAULT;
+
+	pd = idr_read_pd(cmd.pd_handle, file->ucontext);

Quoted 4 lines

Add ib_uverbs_get_eth_l2_addr() to allow ibv_create_ah() to resolve <sgid, dgid> to <vlan, dmac> for any gid type. Although user-space might bypass this call for link-local gids, it is better not to replicate the kernel resolution policy. Port link layer is also returned by ibv_query_port().

A high-level comment/question, followed by some notes about the specific patch.

At the highest level, is having this very low-level command exposed as part of the kernel uverbs <-> userspace API the right place to split things? Making the Ethernet address resolution part of the low-level driver implies that it's not really a generic part of the verbs interface.

Maybe it is generic, and we should have a generic function instead of calling into the low-level driver. I see the argument that we should keep the policy in the kernel, although I'm not sure how strong that argument is -- once we start shipping a kernel with a certain policy (and I guess OFED has in effect already done that), how could we ever change that policy? We'll have interoperability issues anyway, so it seems having userspace and kernel use different policies doesn't cause much further problems anyway.

Or maybe it is device-specific, and we could wrap it up into the create AH uverbs call we already have?

Low-level comments:

Quoted 21 lines

drivers/infiniband/core/uverbs_cmd.c +21 -0

+ssize_t ib_uverbs_get_eth_l2_addr(struct ib_uverbs_file *file, const char __user *buf,
+				  int in_len, int out_len)
+{
+	struct ib_uverbs_get_eth_l2_addr       cmd;
+	struct ib_uverbs_get_eth_l2_addr_resp  resp;
+	int              ret;
+	struct ib_pd    *pd;
+
+	if (out_len < sizeof resp)
+		return -ENOSPC;
+
+	if (copy_from_user(&cmd, buf, sizeof cmd))
+		return -EFAULT;
+
+	pd = idr_read_pd(cmd.pd_handle, file->ucontext);
+	if (!pd)
+		return -EINVAL;
+
+	ret = ib_get_eth_l2_addr(pd->device, cmd.port, (union ib_gid *)cmd.gid,
+				 cmd.sgid_idx, resp.mac, &resp.vlan_id, &resp.tagged);
+	put_pd_read(pd);

drivers/infiniband/core/uverbs_cmd.c err: · line 1852

Quoted 4 lines

drivers/infiniband/core/uverbs_cmd.c +4 -0

+	if (!ret) {
+		if (copy_to_user((void __user *) (unsigned long) cmd.response,
+				 &resp, sizeof resp))
+			return -EFAULT;

This leaks kernel memory contents to userspace since the stack variable resp is never cleared. Also will cause problems if we ever need to use the reserved fields for anything.

Also I'm not sure I understand why you pass the PD into this method? It seems you just use it to get a pointer to the device, but you already have that from the uverbs_file structure that's passed into all commands.

drivers/infiniband/core/verbs.c int ib_detach_mcast(struct ib_qp *qp, union ib_gid *gid, u16 lid) · line 932

Quoted 10 lines

drivers/infiniband/core/verbs.c +9 -0

+int ib_get_eth_l2_addr(struct ib_device *device, u8 port, union ib_gid *gid,
+		       int sgid_idx, u8 *mac, __u16 *vlan_id, u8 *tagged)
+{
+	if (!device->get_eth_l2_addr)
+		return -ENOSYS;
+
+	return device->get_eth_l2_addr(device, port, gid, sgid_idx, mac, vlan_id, tagged);
+}
+EXPORT_SYMBOL(ib_get_eth_l2_addr);

I don't think we need this wrapper, since uverbs can just call the get_eth_l2_addr method directly (we already do that for quite a few other methods, eg alloc_ucontext is a uverbs-only method that has no in-kernel wrapper). Also the -ENOSYS test isn't necessary, since a device-driver shouldn't allow this method unless it actually implements it.

Address mapping policies depend on the address type. This patch only defines a policy for mapping link-local addresses, and we should indeed take care not to change it (if possible).
Later on, we can add more policies for other address types (e.g., normal IPv6 addresses, mapped IPv4 addresses, etc.)

Quoted 1 line

Quoted 2 lines

Or maybe it is device-specific, and we could wrap it up into the create AH uverbs call we already have?

Interesting idea. Not sure what is better here: a seperate ABI call or some additional 'u8 ctx[32]' field in struct ib_uverbs_create_ah_resp that will be interpreted by the hw-specific user-space driver.

Quoted 5 lines

Interesting idea. Not sure what is better here: a seperate ABI call or some additional 'u8 ctx[32]' field in struct ib_uverbs_create_ah_resp that will be interpreted by the hw-specific user-space driver.

I don't understand why mapping remote addresses should be driver specific.

In this case it will not be just mapping remote addresses but creating the required AH information which is unique to each device.