On Thu, Feb 07, 2019 at 11:50:48AM -0700, Jason Gunthorpe wrote:
> On Thu, Feb 07, 2019 at 08:45:14PM +0200, Leon Romanovsky wrote:
> > On Thu, Feb 07, 2019 at 10:24:42AM -0700, Jason Gunthorpe wrote:
> > > On Thu, Feb 07, 2019 at 05:43:51PM +0200, Leon Romanovsky wrote:
> > > > On Tue, Feb 05, 2019 at 02:52:08PM -0700, Jason Gunthorpe wrote:
> > > > > On Sat, Feb 02, 2019 at 01:42:47PM +0200, Leon Romanovsky wrote:
> > > > > > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > >
> > > > > > Implement doit callbacks and ensure that users won't provide port values
> > > > > > on resource entry allocated in per-device mode needed for .doit callback.
> > > > > >
> > > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > > drivers/infiniband/core/nldev.c | 186 +++++++++++++++++++++-----------
> > > > > > include/rdma/restrack.h | 2 +-
> > > > > > 2 files changed, 127 insertions(+), 61 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
> > > > > > index 7bbc98080488..e5910bbbcec7 100644
> > > > > > +++ b/drivers/infiniband/core/nldev.c
> > > > > > @@ -375,7 +375,6 @@ static int fill_res_qp_entry(struct sk_buff *msg, bool has_cap_net_admin,
> > > > > > struct ib_qp *qp = container_of(res, struct ib_qp, res);
> > > > > > struct ib_device *dev = qp->device;
> > > > > > struct ib_qp_init_attr qp_init_attr;
> > > > > > - struct nlattr *entry_attr;
> > > > > > struct ib_qp_attr qp_attr;
> > > > > > int ret;
> > > > > >
> > > > > > @@ -386,10 +385,6 @@ static int fill_res_qp_entry(struct sk_buff *msg, bool has_cap_net_admin,
> > > > > > if (port && port != qp_attr.port_num)
> > > > > > return 0;
> > > > >
> > > > > Now that the caller is checking if the port is needed or not these
> > > > > 'if (port)' and !port should be removed from the fill functions
> > > >
> > > > I'll check
> > >
> > > In any event, the check for 0 port is wrong, so it need some kind of
> > > revision.
> >
> > See, my other email reply on that.
> >
> > >
> > > > >
> > > > > > + if ((port && fe->flags & NLDEV_PER_DEV) ||
> > > > > > + (!port && ~fe->flags & NLDEV_PER_DEV)) {
> > > > > > + ret = -EINVAL;
> > > > > > + goto err;
> > > > > > + }
> > > > >
> > > > > 0 is a valid port number, this should be something like
> > > >
> > > > Why? We are not supporting IB switches for now, and port provided
> > >
> > > The kernel code supports switches for some reason
> > >
> > > > by the user should pass rdma_is_port_valid() check, which requires
> > > > port to be >= 1.
> > >
> > > Not if rdma_cap_ib_switch is true.
> >
> > Long time ago, I posted patch to remove this switch, because it is
> > forwarded directly from HW to sysfs without any kernel need of it.
> >
> > Someone complained (I don't remember who) that it hurts their out-of-tree
> > kernel and he asked politely do not remove this bit together with check,
> > so we did this, but decided that rdmatool won't support switches for now.
> > This is why port == 0 is not valid. You probably can find that discussion
> > in web archives.
>
> That is a silly place to end up - there is no reason to use port == 0
> here as invalid, it should be fixed.
What exactly do you suggest to fix?
If user provides "port", it will need to pass rdma_is_port_valid() check.
We have exactly zero drivers which know how to handle is_switch correctly.
Let's try to narrow our focus on solving real life problems and don't
invent extra work for imaginary scenarios like port == 0.
Thanks
>
> Jason
Attachment:
signature.asc
Description: PGP signature
