On Mon, 2018-12-03 at 17:16 +0000, Jason Gunthorpe wrote:
> 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..
Yes, I thought I saw you say you applied it already? And I started from
your wip/jgg-for-next that's currently pushed. Was I incorrect in that?
> > 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?
I didn't see anything like that in uaccess.h
> > 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??
It's worth checking. It used to show up crazy high in perf output, but
you're right, it might be better today.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
