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

[Leon Romanovsky <leon@kernel.org>]

On Mon, Sep 14, 2020 at 12:55:50PM -0300, Jason Gunthorpe wrote:
> On Sun, Sep 13, 2020 at 12:13:02PM +0300, Leon Romanovsky wrote:
> > > > +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.
>
> Oh that is a bit exciting, maybe it should be checked...
>
> Ideally we should also block this uapi entirely if the device doesn't
> have a gid table, so this should be -EOPNOTSUP and moved up to the
> top so it can be moved once we figure it out.

It is already in earliest possible stage, right after we get ib_device.

>
> > > > +	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         }
>
> Shouldn't be needed, rdma_read_gid_attr_ndev_rcu() already returns -1
> for IB

It will be something like this:

diff --git a/drivers/infiniband/core/uverbs_std_types_device.c b/drivers/infiniband/core/uverbs_std_types_device.c
index 8e957aa38531..071698af4b8e 100644
--- a/drivers/infiniband/core/uverbs_std_types_device.c
+++ b/drivers/infiniband/core/uverbs_std_types_device.c
@@ -368,10 +368,11 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_TABLE)(
 static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_ENTRY)(
 	struct uverbs_attr_bundle *attrs)
 {
+	struct ib_uverbs_gid_entry entry = {};
 	const struct ib_gid_attr *gid_attr;
-	struct ib_uverbs_gid_entry entry;
 	struct ib_ucontext *ucontext;
 	struct ib_device *ib_dev;
+	struct net_device *ndev;
 	u32 gid_index;
 	u32 port_num;
 	u32 flags;
@@ -408,9 +409,17 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_ENTRY)(
 	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);
-	if (ret)
-		goto out;
+
+	if (rdma_protocol_roce(ib_dev, port_num)) {
+		rcu_read_lock();
+		ndev = rdma_read_gid_attr_ndev_rcu(gid_attr);
+		if (IS_ERR(ndev)) {
+		       rcu_read_unlock();
+		       goto out;
+		}
+		entry.netdev_ifindex = ndev->ifindex;
+		rcu_read_unlock();
+	}

 	ret = uverbs_copy_to_struct_or_zero(
 		attrs, UVERBS_ATTR_QUERY_GID_ENTRY_RESP_ENTRY, &entry,