On Tue, Sep 15, 2020 at 04:06:14PM -0300, Jason Gunthorpe wrote:
> On Tue, Sep 15, 2020 at 02:47:04PM +0300, Leon Romanovsky wrote:
> > 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.
>
> I usually put it before the uverbs_get_XX.. but doesn't matter
>
> > @@ -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)) {
>
> Not sure this is needed? non-roce still has gid table entries, and the
> attr->ndev should be null so it will do the ENODEV naturally.
It depends on how you want to treat errors from rdma_read_gid_attr_ndev_rcu().
Current check allows us to ensure that any error returned by this call is
handled.
Otherwise we will find ourselves with something like this:
ndev = rdma_read_gid_attr_ndev_rcu(gid_attr);
if (IS_ERR(ndev)) {
if (rdma_protocol_roce())
goto error;
if (ERR_PTR(ndev) != -ENODEV)
goto error;
}
>
> > + 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();
> > + }
>
> This is better than what is in rdma_get_ndev_ifindex()
>
> Jason
