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
