On Mon, Dec 03, 2018 at 11:50:48AM -0500, Doug Ledford wrote:
> On Sun, 2018-11-25 at 20:58 +0200, Leon Romanovsky wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> >
> > There is no reason for this, for response processing we simply need to
> > copy, truncate and zero fill the response into whatever output buffer
> > was provided. Add a function uverbs_response() that does this
> > consistently.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/core/uverbs_cmd.c | 126 +++++++++------------------
> > 1 file changed, 41 insertions(+), 85 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> > index 2eb19ddcdd77..6c9486f730fd 100644
> > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > @@ -47,6 +47,35 @@
> > #include "uverbs.h"
> > #include "core_priv.h"
> >
> > +/*
> > + * Copy a response to userspace. If the provided 'resp' is larger than the
> > + * user buffer it is silently truncated. If the user provided a larger buffer
> > + * then the trailing portion is zero filled.
> > + *
> > + * These semantics are intended to support future extension of the output
> > + * structures.
> > + */
> > +static int uverbs_response(struct uverbs_attr_bundle *attrs, const void *resp,
> > + size_t resp_len)
> > +{
> > + u8 __user *cur = attrs->ucore.outbuf + resp_len;
> > + u8 __user *end = attrs->ucore.outbuf + attrs->ucore.outlen;
> > + int ret;
> > +
> > + if (copy_to_user(attrs->ucore.outbuf, resp,
> > + min(attrs->ucore.outlen, resp_len)))
> > + return -EFAULT;
> > +
> > + /* Zero fill any extra memory that user space might have provided */
> > + for (; cur < end; cur++) {
> > + ret = put_user(0, cur);
> > + if (ret)
> > + return ret;
> > + }
>
> A per-char put_user() sounds pretty expensive since you are having to do
> a __uaccess_begin();__uaccess_end() around every put_user(). I took it
> as is, but it might be worth considering doing something like a 32
> byte
This patch requries the last series of write() handlers before it will
work properly, don't apply it just yet - will create a crazy security
problem..
> array on the stack that can be zero'ed and copied to user space instead
> of individual put_user calls. Maybe something like:
This code is just copied from some other udata based code we've had
for a long time, I hate it.
memset_user sure would be nice, I wonder if it exists?
> u8 zero_pad[32];
>
> if (cur < end) {
> memset(&zero_pad, 0, sizeof(zero_pad));
> while (cur < end) {
> if (copy_to_user(cur, &zero_pad, min(sizeof(zero_pad), end - cur)))
> return -EFAULT;
> cur += min(sizeof(zero_pad), end - cur);
> }
> }
We could fudge one like this sure, there are several places that need
this.. Would be a good patch for rdma
> Of course, this only matters if we have oversized outbufs on a regular
> basis, but it's worth considering. I suspect that even though the
> put_user way of doing things is easily readable and clear, the cost of
> uaccess begin/end might make it a bad idea..
I don't think put_user is as expensive as that, it relies on the mmu
and page fault handling, it doesn't actually do uaccess on x86
IIRC.. Although who knows now with PTI??
Jason
