Re: [PATCH for-next 2/5] IB/core: Add support for extended query device caps

[Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

On 03/11/2014 10:02, Eli Cohen wrote:
> +int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,
> +			      struct ib_udata *ucore,
> +			      struct ib_udata *uhw)
> +{
> +	struct ib_uverbs_ex_query_device_resp resp;
> +	struct ib_uverbs_ex_query_device  cmd;
> +	struct ib_device_attr attr;
> +	struct ib_device *device;
> +	int err;
> +
> +	device = file->device->ib_dev;
> +	if (ucore->inlen < sizeof(cmd))
> +		return -EINVAL;
> +
> +	if (ucore->outlen < sizeof(resp))
> +		return -ENOSPC;

This check may cause compatibility problems when running a newer kernel
with old userspace. The userspace code will have a smaller
ib_uverbs_ex_query_device_resp struct, so the verb will always fail. A
possible solution is to drop this check, and modify ib_copy_to_udata so
that it only copies up to ucore->outlen bytes.

> +
> +	err = ib_copy_from_udata(&cmd, ucore, sizeof(cmd));
> +	if (err)
> +		return err;
> +
> +	if (cmd.comp_mask)
> +		return -EINVAL;

This check may make it difficult for userspace to use this verb. If
running an older kernel with a newer userspace, the userspace will need
to run the verb multiple times to find out which combination of
comp_mask bits is actually supported. I think a better way would be to
drop this check, and let userspace rely on the returned comp_mask in the
ib_uverbs_ex_query_device_resp struct to determine which features are
supported by the current kernel.

> +
> +	err = device->query_device(device, &attr);
> +	if (err)
> +		return err;
> +
> +	memset(&resp, 0, sizeof(resp));
> +	copy_query_dev_fields(file, &resp.base, &attr);
> +	resp.comp_mask = 0;
> +
> +	err = ib_copy_to_udata(ucore, &resp, sizeof(resp));
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}

Regards,
Haggai
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html