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
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ 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
array on the stack that can be zero'ed and copied to user space instead
of individual put_user calls. Maybe something like:
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);
}
}
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..
--
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
