lkml.org 
[lkml]   [2019]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v4 3/5] misc: fastrpc: Add support for context Invoke method
From
Date
Thanks for the review,

I will fix them and send new version!

On 31/01/2019 15:34, Greg KH wrote:
> On Thu, Jan 24, 2019 at 03:24:10PM +0000, Srinivas Kandagatla wrote:
>> This patch adds support to compute context invoke method
>> on the remote processor (DSP).
>> This involves setting up the functions input and output arguments,
>> input and output handles and mapping the dmabuf fd for the
>> argument/handle buffers.
>>
>
> This says _what_ this code does, but not why. What about all of that
> explaination you had in the 0/5 patch, shouldn't that be here, or on
> patch 2/5?
>
Yes, I will add more details in to the log.

> Some nits below:
>
>> +static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
>> +{
>> + struct fastrpc_invoke_args *args = NULL;
>> + struct fastrpc_invoke inv;
>> + u32 nscalars;
>> + int err;
>> +
>> + if (copy_from_user(&inv, argp, sizeof(inv)))
>> + return -EFAULT;
>> +
>> + nscalars = REMOTE_SCALARS_LENGTH(inv.sc);
>> + if (nscalars) {
>> + args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL);
>
> Yeah, let's not bounds check the input variables and suck up all of the
> kernel memory!
>
> Remember:
> ALL INPUT IS EVIL

I will add more checks here and other such instances in next version....
>> +static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + struct fastrpc_user *fl = (struct fastrpc_user *)file->private_data;
>> + char __user *argp = (char __user *)arg;
>> + int err;
>> +
>> + switch (cmd) {
>> + case FASTRPC_IOCTL_INVOKE:
>> + err = fastrpc_invoke(fl, argp);
>> + break;
>> + default:
>> + err = -ENOTTY;
>> + dev_err(fl->sctx->dev, "bad ioctl: %d\n", cmd);
>
> Don't spam the syslog if someone sends you an invalid ioctl. That's a
> sure way to DoS the system.
will fix this in next version.
>
>> + break;
>> + }
>> +
>> + if (err)
>> + dev_dbg(fl->sctx->dev, "Error: IOCTL Failed with %d\n", err);
>> +
>> + return err;
>> +}
>> +
>> static const struct file_operations fastrpc_fops = {
>> .open = fastrpc_device_open,
>> .release = fastrpc_device_release,
>> + .unlocked_ioctl = fastrpc_device_ioctl,
>> + .compat_ioctl = fastrpc_device_ioctl,
>> };
>>
>> static int fastrpc_cb_probe(struct platform_device *pdev)
>> @@ -260,9 +932,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> return of_platform_populate(rdev->of_node, NULL, NULL, rdev);
>> }
>>
>> +static void fastrpc_notify_users(struct fastrpc_user *user)
>> +{
>> + struct fastrpc_invoke_ctx *ctx, *n;
>> +
>> + spin_lock(&user->lock);
>> + list_for_each_entry_safe(ctx, n, &user->pending, node)
>> + complete(&ctx->work);
>
> Why safe? You aren't deleting the list here.
>
Not sure why it ended up with safe here, does not make sense unless am
deleting it.. will fix this in next version.

...>> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
>> new file mode 100644
>> index 000000000000..a69ef33dc37e
>> --- /dev/null
>> +++ b/include/uapi/misc/fastrpc.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __QCOM_FASTRPC_H__
>> +#define __QCOM_FASTRPC_H__
>> +
>> +#include <linux/types.h>
>> +
>> +#define FASTRPC_IOCTL_INVOKE _IOWR('R', 3, struct fastrpc_invoke)
>> +
>> +struct fastrpc_invoke_args {
>> + __u64 ptr;
>> + __u64 length;
>> + __s32 fd;
>> + __u32 reserved;
> Are you checking that reserved is all 0 now?

No, I should add the checks!

>
>> +};
>> +
>> +struct fastrpc_invoke {
>> + __u32 handle;
>> + __u32 sc;
>> + __u64 args;
>> +};
>
> Do you need packed here? What about endian issues?
We do not need this packed here, as this is not the actual structure
that the passed to the DSP.

Thanks,
srini

>
> thanks,
>
> greg k-h
>

\
 
 \ /
  Last update: 2019-01-31 18:55    [W:0.069 / U:10.636 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site