| Message ID | 20190128101236.8257-1-shamir.rabinovitch@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
On Mon, Jan 28, 2019 at 12:12:23PM +0200, Shamir Rabinovitch wrote: > Helper function to get driver's context out of ib_udata wrapped > in uverbs_attr_bundle for uer objects or NULL for kernel objects. > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > include/rdma/ib_verbs.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index 94b6e1dd4dab..fb4d078d6db3 100644 > +++ b/include/rdma/ib_verbs.h > @@ -4264,4 +4264,18 @@ static inline struct ib_device *rdma_device_to_ibdev(struct device *device) > */ > #define rdma_device_to_drv_device(dev, drv_dev_struct, ibdev_member) \ > container_of(rdma_device_to_ibdev(dev), drv_dev_struct, ibdev_member) > + > +/** > + * rdma_udata_to_drv_context - Helper macro to get the driver's context out of > + * ib_udata which is embadded in uverbs_attr_bundle. > + * > + * NOTE: This macro return NULL for kernel objects and valid driver context for > + * user objects. > + */ > +#define rdma_udata_to_drv_context(udata, drv_dev_struct, member) \ > + ({ struct ib_ucontext *_ibctx = rdma_get_ucontext(udata); \ > + IS_ERR(_ibctx) ? NULL : \ > + container_of(_ibctx, drv_dev_struct, member); \ > + }) This macro body is the same as container_of_safe Jason
On Mon, Jan 28, 2019 at 12:12:23PM +0200, Shamir Rabinovitch wrote: > Helper function to get driver's context out of ib_udata wrapped > in uverbs_attr_bundle for uer objects or NULL for kernel objects. > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > include/rdma/ib_verbs.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index 94b6e1dd4dab..fb4d078d6db3 100644 > +++ b/include/rdma/ib_verbs.h > @@ -4264,4 +4264,18 @@ static inline struct ib_device *rdma_device_to_ibdev(struct device *device) > */ > #define rdma_device_to_drv_device(dev, drv_dev_struct, ibdev_member) \ > container_of(rdma_device_to_ibdev(dev), drv_dev_struct, ibdev_member) > + > +/** > + * rdma_udata_to_drv_context - Helper macro to get the driver's context out of > + * ib_udata which is embadded in uverbs_attr_bundle. > + * > + * NOTE: This macro return NULL for kernel objects and valid driver context for > + * user objects. Actually at this point I think this just can't fail if udata is set, so lets just say that: /* * If udata is not NULL this cannot fail. Otherwise a NULL udata will result * in a NULL ucontext pointer. Callers can rely on !NULL ucontext to show the * op is being called as part of a user system call. */ #define rdma_udata_to_drv_context(udata, drv_dev_struct, member) \ (udata ? container_of(container_of(udata, struct uverbs_attr_bundle, \ driver_udata) \ ->context, \ drv_dev_struct, member) : \ (drv_dev_struct *)NULL) And get rid of the rdma_get_ucontext helper function, umem can just inspect udata->context itself and fail with EIO if NULL. Jason
On Mon, Jan 28, 2019 at 12:12:23PM +0200, Shamir Rabinovitch wrote: > +/** > + * rdma_udata_to_drv_context - Helper macro to get the driver's context out of > + * ib_udata which is embadded in uverbs_attr_bundle. > + * > + * NOTE: This macro return NULL for kernel objects and valid driver context for > + * user objects. > + */ > +#define rdma_udata_to_drv_context(udata, drv_dev_struct, member) \ > + ({ struct ib_ucontext *_ibctx = rdma_get_ucontext(udata); \ > + IS_ERR(_ibctx) ? NULL : \ > + container_of(_ibctx, drv_dev_struct, member); \ > + }) Please don't hide that much magic in a helper. Opencoding this makes it much more obvious what is going on. I don't actually have rdma_get_ucontext in my tree, but an ERR_PTR return for a private data accessor also sounds like a bad idea - in general we use NULL returns for that. Last but not least your series does not even seem to add a user of this helper. Where do you want to use it?
On Mon, Jan 28, 2019 at 12:01:06PM -0700, Jason Gunthorpe wrote: > * If udata is not NULL this cannot fail. Otherwise a NULL udata will result > * in a NULL ucontext pointer. Callers can rely on !NULL ucontext to show the > * op is being called as part of a user system call. > */ > #define rdma_udata_to_drv_context(udata, drv_dev_struct, member) \ > (udata ? container_of(container_of(udata, struct uverbs_attr_bundle, \ > driver_udata) \ > ->context, \ > drv_dev_struct, member) : \ > (drv_dev_struct *)NULL) > > And get rid of the rdma_get_ucontext helper function, umem can just > inspect udata->context itself and fail with EIO if NULL. And what exactly is the benefit over opencoding it? Yes, it'll save about 2 lines of source per caller, but it will also make the code basically unreadable without looking at the defintion of rdma_udata_to_drv_context.
On Mon, Jan 28, 2019 at 11:45:35PM -0800, Christoph Hellwig wrote: > On Mon, Jan 28, 2019 at 12:01:06PM -0700, Jason Gunthorpe wrote: > > * If udata is not NULL this cannot fail. Otherwise a NULL udata will result > > * in a NULL ucontext pointer. Callers can rely on !NULL ucontext to show the > > * op is being called as part of a user system call. > > */ > > #define rdma_udata_to_drv_context(udata, drv_dev_struct, member) \ > > (udata ? container_of(container_of(udata, struct uverbs_attr_bundle, \ > > driver_udata) \ > > ->context, \ > > drv_dev_struct, member) : \ > > (drv_dev_struct *)NULL) > > > > And get rid of the rdma_get_ucontext helper function, umem can just > > inspect udata->context itself and fail with EIO if NULL. > > And what exactly is the benefit over opencoding it? Yes, it'll save > about 2 lines of source per caller, but it will also make the code > basically unreadable without looking at the defintion of > rdma_udata_to_drv_context. You are talking about the ternary expression, right? I think the benefit of hiding the ugly container_of stuff is obvious.. (and why the container_of is like that is a whole other email, I'm going to work on that too someday) The background here is that all driver callbacks can run in kernel mode, or user mode. Shamir has been cleaning things so that the drivers generally now inspect udata != NULL to determine if they are in user mode (instead of the crazy mis-match before). Only in user mode does a ucontext pointer exist. Without the ternary expression, calling the macro will oops if done unprotected in kernel mode. Shamir converted some of the users in all the drivers: https://patchwork.kernel.org/patch/10783539/ .. and if we drop the ternary then all those sites have to be much more carefully reworked to ensure that the rdma_udata_to_drv_context is only ever called under a 'if (udata)' block.. .. and that makes a big mess of a typical driver control flow pattern of: struct drv_uctx *uctx = rdma_udata_to_drv_context(udata, drv_uctx, ib_uctx); if (uctx) { uctx->blah } [..] if (udata) { if (copy_to_user(udata->outbuf)) goto err_copy; } err_copy: if (uctx) undo uctx->blah In other words adding the ternary is done to make reworking the drivers easier and the result less likely to be wrong. I think you are right though - the cases that do not have a complicated control flow should be remain-as/be-changed-to: if (udata) { struct drv_uctx *uctx = rdma_udata_to_drv_context(..); uctx->blah if (copy_to_user()) [..] } To make the relation clear, and keep udata at the center for determining user/kernel mode. Jason
On Mon, Jan 28, 2019 at 11:45:13AM -0700, Jason Gunthorpe wrote: > On Mon, Jan 28, 2019 at 12:12:23PM +0200, Shamir Rabinovitch wrote: > > Helper function to get driver's context out of ib_udata wrapped > > in uverbs_attr_bundle for uer objects or NULL for kernel objects. > > > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > > include/rdma/ib_verbs.h | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > index 94b6e1dd4dab..fb4d078d6db3 100644 > > +++ b/include/rdma/ib_verbs.h > > @@ -4264,4 +4264,18 @@ static inline struct ib_device *rdma_device_to_ibdev(struct device *device) > > */ > > #define rdma_device_to_drv_device(dev, drv_dev_struct, ibdev_member) \ > > container_of(rdma_device_to_ibdev(dev), drv_dev_struct, ibdev_member) > > + > > +/** > > + * rdma_udata_to_drv_context - Helper macro to get the driver's context out of > > + * ib_udata which is embadded in uverbs_attr_bundle. > > + * > > + * NOTE: This macro return NULL for kernel objects and valid driver context for > > + * user objects. > > + */ > > +#define rdma_udata_to_drv_context(udata, drv_dev_struct, member) \ > > + ({ struct ib_ucontext *_ibctx = rdma_get_ucontext(udata); \ > > + IS_ERR(_ibctx) ? NULL : \ > > + container_of(_ibctx, drv_dev_struct, member); \ > > + }) > > This macro body is the same as container_of_safe OK. Will use this but will arrange the helper to return NULL as stated and not ERR_PTR as rdma_get_ucontext return. > > Jason
On Mon, Jan 28, 2019 at 12:01:06PM -0700, Jason Gunthorpe wrote: > On Mon, Jan 28, 2019 at 12:12:23PM +0200, Shamir Rabinovitch wrote: > > Helper function to get driver's context out of ib_udata wrapped > > in uverbs_attr_bundle for uer objects or NULL for kernel objects. > > > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > > include/rdma/ib_verbs.h | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > index 94b6e1dd4dab..fb4d078d6db3 100644 > > +++ b/include/rdma/ib_verbs.h > > @@ -4264,4 +4264,18 @@ static inline struct ib_device *rdma_device_to_ibdev(struct device *device) > > */ > > #define rdma_device_to_drv_device(dev, drv_dev_struct, ibdev_member) \ > > container_of(rdma_device_to_ibdev(dev), drv_dev_struct, ibdev_member) > > + > > +/** > > + * rdma_udata_to_drv_context - Helper macro to get the driver's context out of > > + * ib_udata which is embadded in uverbs_attr_bundle. > > + * > > + * NOTE: This macro return NULL for kernel objects and valid driver context for > > + * user objects. > > Actually at this point I think this just can't fail if udata is set, > so lets just say that: > > /* > * If udata is not NULL this cannot fail. Otherwise a NULL udata will result > * in a NULL ucontext pointer. Callers can rely on !NULL ucontext to show the > * op is being called as part of a user system call. > */ > #define rdma_udata_to_drv_context(udata, drv_dev_struct, member) \ > (udata ? container_of(container_of(udata, struct uverbs_attr_bundle, \ > driver_udata) \ > ->context, \ > drv_dev_struct, member) : \ > (drv_dev_struct *)NULL) > > And get rid of the rdma_get_ucontext helper function, umem can just > inspect udata->context itself and fail with EIO if NULL. > > Jason OK. Change accordingly & skip previous mail.
On Wed, Feb 06, 2019 at 01:43:11PM +0200, Shamir Rabinovitch wrote: > On Mon, Jan 28, 2019 at 12:01:06PM -0700, Jason Gunthorpe wrote: > > On Mon, Jan 28, 2019 at 12:12:23PM +0200, Shamir Rabinovitch wrote: > > > Helper function to get driver's context out of ib_udata wrapped > > > in uverbs_attr_bundle for uer objects or NULL for kernel objects. > > > > > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > > > include/rdma/ib_verbs.h | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > index 94b6e1dd4dab..fb4d078d6db3 100644 > > > +++ b/include/rdma/ib_verbs.h > > > @@ -4264,4 +4264,18 @@ static inline struct ib_device *rdma_device_to_ibdev(struct device *device) > > > */ > > > #define rdma_device_to_drv_device(dev, drv_dev_struct, ibdev_member) \ > > > container_of(rdma_device_to_ibdev(dev), drv_dev_struct, ibdev_member) > > > + > > > +/** > > > + * rdma_udata_to_drv_context - Helper macro to get the driver's context out of > > > + * ib_udata which is embadded in uverbs_attr_bundle. > > > + * > > > + * NOTE: This macro return NULL for kernel objects and valid driver context for > > > + * user objects. > > > > Actually at this point I think this just can't fail if udata is set, > > so lets just say that: > > > > /* > > * If udata is not NULL this cannot fail. Otherwise a NULL udata will result > > * in a NULL ucontext pointer. Callers can rely on !NULL ucontext to show the > > * op is being called as part of a user system call. > > */ > > #define rdma_udata_to_drv_context(udata, drv_dev_struct, member) \ > > (udata ? container_of(container_of(udata, struct uverbs_attr_bundle, \ > > driver_udata) \ > > ->context, \ > > drv_dev_struct, member) : \ > > (drv_dev_struct *)NULL) > > > > And get rid of the rdma_get_ucontext helper function, umem can just > > inspect udata->context itself and fail with EIO if NULL. > > > > Jason > > OK. Change accordingly & skip previous mail. Yes, just don't miss the comment from CH to try as much as reasonable without major invasion to not call rdma_udata_to_drv_context if udata is NULL Jason
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 94b6e1dd4dab..fb4d078d6db3 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -4264,4 +4264,18 @@ static inline struct ib_device *rdma_device_to_ibdev(struct device *device) */ #define rdma_device_to_drv_device(dev, drv_dev_struct, ibdev_member) \ container_of(rdma_device_to_ibdev(dev), drv_dev_struct, ibdev_member) + +/** + * rdma_udata_to_drv_context - Helper macro to get the driver's context out of + * ib_udata which is embadded in uverbs_attr_bundle. + * + * NOTE: This macro return NULL for kernel objects and valid driver context for + * user objects. + */ +#define rdma_udata_to_drv_context(udata, drv_dev_struct, member) \ + ({ struct ib_ucontext *_ibctx = rdma_get_ucontext(udata); \ + IS_ERR(_ibctx) ? NULL : \ + container_of(_ibctx, drv_dev_struct, member); \ + }) + #endif /* IB_VERBS_H */
Helper function to get driver's context out of ib_udata wrapped in uverbs_attr_bundle for uer objects or NULL for kernel objects. Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> --- include/rdma/ib_verbs.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)