> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Friday, July 27, 2018 10:19 AM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford
> <dledford@xxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>;
> Yuval Shaia <yuval.shaia@xxxxxxxxxx>; Leon Romanovsky
> <leonro@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next v1] RDMA: Check namespace for user space
> supplied GID index
>
> On Fri, Jul 27, 2018 at 04:53:18AM +0000, Parav Pandit wrote:
> >
> >
> > > From: Jason Gunthorpe <jgg@xxxxxxxx>
> > > Sent: Thursday, July 26, 2018 5:41 PM
> > > To: Leon Romanovsky <leon@xxxxxxxxxx>
> > > Cc: Doug Ledford <dledford@xxxxxxxxxx>; Parav Pandit
> > > <parav@xxxxxxxxxxxx>; RDMA mailing list
> > > <linux-rdma@xxxxxxxxxxxxxxx>; Yuval Shaia <yuval.shaia@xxxxxxxxxx>;
> > > Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > Subject: Re: [PATCH rdma-next v1] RDMA: Check namespace for user
> > > space supplied GID index
> > >
> > > On Tue, Jul 24, 2018 at 10:09:27AM +0300, Leon Romanovsky wrote:
> > > > diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h
> > > > index 1108d4220276..511ff6be503a 100644
> > > > +++ b/include/rdma/ib_cache.h
> > > > @@ -128,8 +128,23 @@ int ib_get_cached_port_state(struct ib_device
> > > *device,
> > > > enum ib_port_state *port_active);
> > > >
> > > > bool rdma_is_zero_gid(const union ib_gid *gid); -const struct
> > > > ib_gid_attr *rdma_get_gid_attr(struct ib_device *device,
> > > > - u8 port_num, int index);
> > > > +
> > > > +const struct ib_gid_attr *_rdma_get_gid_attr(struct ib_device *device,
> > > > + u8 port_num, int index,
> > > > + struct net *net);
> > > > +static inline const struct ib_gid_attr * rdma_get_gid_attr(struct
> > > > +ib_device *device, u8 port_num, int index) {
> > > > + return _rdma_get_gid_attr(device, port_num, index, NULL); }
> > >
> > > I don't think supporting NULL makes much sense here. ie the ability
> > > for NULL to search all namespaces is just ugly/potential security hole upon
> misuse.
> > >
> > > It should be init_net and this wrapper shouldn't exist - all callers
> > > of the API are broken for namespaces and should be fixed. Prefer to
> > > push the init_net out to them.
> >
> > For ib_init_ah_attr_from_wc() for IB link layer, it doesn't make
> > sense, but in current code flow context it seems fine.
>
> The ib_init_ah_attr_from_wc() API is only for generating response packets for
> MAD's (right?), and it must simply reverse the incoming headers. It shouldn't
Yes. GID attribute are used till the life of cm_id.
> really care about namespaces or other things, and if it needs a private interface
> to the gid cache to do so, then so be it.
>
We would end up creating two APIs which does same thing, for example in future you can demand rdma_find_gid_by_filter(), rdma_find_gid_by_port() to have variant with and without net ns in it depending on from where it is called.
And to avoid creating two APIs, null was being used to differentiate whether to honor given net_ns or not.
rdma_get_user_gid_attr() with _user embedded in API name itself identifies that all user space facing calls need to pass the right ns.
But I am fine to drop rdma_get_user_gid_attr() and extend rdma_get_gid_attr() in current case as call site are rxe, smc and ib_init_ah_attr_from_wc().
I guess I need to wait for Stephen Rothwell to finish smc merge conflict merge and once the merged code appears in rdma next tree, my patch would make sense to avoid another merge conflict?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
