Re: [PATCH rdma-next 4/4] RDMA/uverbs: Expose the new GID query API to user space

[Leon Romanovsky <leon@kernel.org>]

On Fri, Sep 11, 2020 at 04:59:18PM -0300, Jason Gunthorpe wrote:
> On Thu, Sep 10, 2020 at 05:22:04PM +0300, Leon Romanovsky wrote:
> > From: Avihai Horon <avihaih@nvidia.com>
> >
> > Expose the query GID table and entry API to user space by adding
> > two new methods and method handlers to the device object.
> >
> > This API provides a faster way to query a GID table using single call and
> > will be used in libibverbs to improve current approach that requires
> > multiple calls to open, close and read multiple sysfs files for a single
> > GID table entry.
> >
> > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> >  .../infiniband/core/uverbs_std_types_device.c | 180 +++++++++++++++++-
> >  include/rdma/ib_verbs.h                       |   6 +-
> >  include/uapi/rdma/ib_user_ioctl_cmds.h        |  16 ++
> >  include/uapi/rdma/ib_user_ioctl_verbs.h       |   6 +
> >  4 files changed, 204 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/uverbs_std_types_device.c b/drivers/infiniband/core/uverbs_std_types_device.c
> > index 7b03446b6936..beba1e284264 100644
> > +++ b/drivers/infiniband/core/uverbs_std_types_device.c
> > @@ -8,6 +8,7 @@
> >  #include "uverbs.h"
> >  #include <rdma/uverbs_ioctl.h>
> >  #include <rdma/opa_addr.h>
> > +#include <rdma/ib_cache.h>
> >
> >  /*
> >   * This ioctl method allows calling any defined write or write_ex
> > @@ -266,6 +267,157 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_CONTEXT)(
> >  	return ucontext->device->ops.query_ucontext(ucontext, attrs);
> >  }
> >
> > +static int copy_gid_entries_to_user(struct uverbs_attr_bundle *attrs,
> > +				    struct ib_uverbs_gid_entry *entries,
> > +				    size_t num_entries, size_t user_entry_size)
> > +{
> > +	const struct uverbs_attr *attr;
> > +	void __user *user_entries;
> > +	size_t copy_len;
> > +	int ret;
> > +	int i;
> > +
> > +	if (user_entry_size == sizeof(*entries)) {
> > +		ret = uverbs_copy_to(attrs,
> > +				     UVERBS_ATTR_QUERY_GID_TABLE_RESP_ENTRIES,
> > +				     entries, sizeof(*entries) * num_entries);
> > +		return ret;
> > +	}
> > +
> > +	copy_len = min_t(size_t, user_entry_size, sizeof(*entries));
> > +	attr = uverbs_attr_get(attrs, UVERBS_ATTR_QUERY_GID_TABLE_RESP_ENTRIES);
> > +	if (IS_ERR(attr))
> > +		return PTR_ERR(attr);
> > +
> > +	user_entries = u64_to_user_ptr(attr->ptr_attr.data);
> > +	for (i = 0; i < num_entries; i++) {
> > +		if (copy_to_user(user_entries, entries, copy_len))
> > +			return -EFAULT;
> > +
> > +		if (user_entry_size > sizeof(*entries)) {
> > +			if (clear_user(user_entries + sizeof(*entries),
> > +				       user_entry_size - sizeof(*entries)))
> > +				return -EFAULT;
> > +		}
> > +
> > +		entries++;
> > +		user_entries += user_entry_size;
> > +	}
> > +
> > +	return uverbs_output_written(attrs,
> > +				     UVERBS_ATTR_QUERY_GID_TABLE_RESP_ENTRIES);
> > +}
> > +
> > +static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_TABLE)(
> > +	struct uverbs_attr_bundle *attrs)
> > +{
> > +	struct ib_uverbs_gid_entry *entries;
> > +	struct ib_ucontext *ucontext;
> > +	struct ib_device *ib_dev;
> > +	size_t user_entry_size;
> > +	size_t max_entries;
> > +	size_t num_entries;
> > +	u32 flags;
> > +	int ret;
> > +
> > +	ret = uverbs_get_flags32(&flags, attrs,
> > +				 UVERBS_ATTR_QUERY_GID_TABLE_FLAGS, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = uverbs_get_const(&user_entry_size, attrs,
> > +			       UVERBS_ATTR_QUERY_GID_TABLE_ENTRY_SIZE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	max_entries = uverbs_attr_ptr_get_array_size(
> > +		attrs, UVERBS_ATTR_QUERY_GID_TABLE_RESP_ENTRIES,
> > +		user_entry_size);
> > +	if (max_entries <= 0)
> > +		return -EINVAL;
> > +
> > +	ucontext = ib_uverbs_get_ucontext(attrs);
> > +	if (IS_ERR(ucontext))
> > +		return PTR_ERR(ucontext);
> > +	ib_dev = ucontext->device;
> > +
> > +	entries = uverbs_zalloc(attrs, max_entries * sizeof(*entries));
>
> This multiplication could overflow

Right, I fixed it.

>
> > +
> > +static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_ENTRY)(
> > +	struct uverbs_attr_bundle *attrs)
> > +{
> > +	const struct ib_gid_attr *gid_attr;
> > +	struct ib_uverbs_gid_entry entry;
> > +	struct ib_ucontext *ucontext;
> > +	struct ib_device *ib_dev;
> > +	u32 gid_index;
> > +	u32 port_num;
> > +	u32 flags;
> > +	int ret;
> > +
> > +	ret = uverbs_get_flags32(&flags, attrs,
> > +				 UVERBS_ATTR_QUERY_GID_ENTRY_FLAGS, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = uverbs_get_const(&port_num, attrs,
> > +			       UVERBS_ATTR_QUERY_GID_ENTRY_PORT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = uverbs_get_const(&gid_index, attrs,
> > +			       UVERBS_ATTR_QUERY_GID_ENTRY_GID_INDEX);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ucontext = ib_uverbs_get_ucontext(attrs);
> > +	if (IS_ERR(ucontext))
> > +		return PTR_ERR(ucontext);
> > +	ib_dev = ucontext->device;
>
> > +	if (!rdma_is_port_valid(ib_dev, port_num))
> > +		return -EINVAL;
> > +
> > +	if (!rdma_ib_or_roce(ib_dev, port_num))
> > +		return -EINVAL;
>
> Why these two tests? I would expect rdma_get_gid_attr() to do them

First check is not needed, but the second check doesn't exist in
rdma_get_gid_attr(). We don't check that table returned from
rdma_gid_table() call exists.

>
> > +	gid_attr = rdma_get_gid_attr(ib_dev, port_num, gid_index);
> > +	if (IS_ERR(gid_attr))
> > +		return PTR_ERR(gid_attr);
> > +
> > +	memcpy(&entry.gid, &gid_attr->gid, sizeof(gid_attr->gid));
> > +	entry.gid_index = gid_attr->index;
> > +	entry.port_num = gid_attr->port_num;
> > +	entry.gid_type = gid_attr->gid_type;
> > +	ret = rdma_get_ndev_ifindex(gid_attr, &entry.netdev_ifindex);
>
> Use rdma_read_gid_attr_ndev_rcu()

I don't want to bring below logic to uverbs* file.

  1263         if (rdma_protocol_ib(gid_attr->device, gid_attr->port_num)) {
  1264                 *ndev_ifindex = 0;
  1265                 return 0;
  1266         }

>
> Jason