[openib-general] [RFC] [PATCH 1/3] RDMA CM: add rdma_get/set_optioncalls to get/set path records

[Michael S. Tsirkin]

Quoting r. Sean Hefty <mshefty at ichips.intel.com>:
> Subject: Re: [openib-general] [RFC] [PATCH 1/3] RDMA CM: add?rdma_get/set_optioncalls to get/set path records
> 
> Michael S. Tsirkin wrote:
> >>+int rdma_set_option(struct rdma_cm_id *id, int level, int optname,
> >>+		    void *optval, size_t optlen);
> >>+
> >
> >It seems optval is a user pointer. Should it be parked as such
> >void __user *.
> 
> The getsockopt / setsockopt calls both use char *optval in their 
> interfaces. Internally, they do get_user(), put_user(), copy_to_user(), etc.
> 
> It's my understanding, which could be way off, that both getsockopt and 
> setsockopt are also callable from kernel modules.  I have not had a chance 
> to try these calls with kernel memory to see what copy_to_user() would do.

I think this works on typical intel, but not so sure about other architectures.
You might need to be in process context so that current pointer is valid.

> >>+static int cma_set_ib_paths(struct rdma_id_private *id_priv,
> >>+			    void *optval, size_t optlen)
> >>+{
> >>+	struct rdma_route *route = &id_priv->id.route;
> >>+	struct ib_user_path_rec user_path;
> >>+	int ret, i;
> >>+
> >>+	if (!cma_comp_exch(id_priv, CMA_ADDR_RESOLVED, CMA_ROUTE_RESOLVED))
> >>+		return -EINVAL;
> >>+
> >>+	if (optlen == sizeof(user_path))
> >>+		route->num_paths = 1;
> >>+	else if (optlen == (sizeof(user_path) << 1))
> >>+		route->num_paths = 2;
> >>+	else {
> >>+		ret = -EINVAL;
> >>+		goto err1;
> >>+	}
> >>+
> >>+	route->path_rec = kmalloc(sizeof *route->path_rec * route->num_paths,
> >>+				  GFP_KERNEL);
> >>+	if (!route->path_rec) {
> >>+		ret = -ENOMEM;
> >>+		goto err2;
> >>+	}
> >>+
> >>+	for (i = 0; i < route->num_paths; i++, optval += sizeof(user_path)) {
> >>+		if (copy_from_user(&user_path, (void __user *) optval,
> >>+				   sizeof(user_path))) {
> >
> >
> >Apparently you assume userspace pointer here: so the interface is not 
> >intended
> >for kernel users? So why is it not in ucma?
> 
> The call needs to be handled in the cma, as opposed to the ucma for a 
> couple of reasons.  For the get_option, I need to protect against device 
> removal while accessing the list of available path records.  For the 
> set_option routine, the call changes the state of the rdma_cm_id.

How about doing copy_from_user in ucma, and implementing
rdma_set_path/rdma_get_path in cma?

-- 
MST