| Message ID | 20181207163513.16412-7-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | misc: Add support to Qualcomm FastRPC driver | expand |
On Fri, Dec 07, 2018 at 04:35:13PM +0000, Srinivas Kandagatla wrote: > This patch adds support for compat ioctl from 32 bits userland to > Qualcomm fastrpc driver. Ick, why? Why not just fix up your ioctl structures to not need that at all? For new code being added to the kernel, there's no excuse to have to have compat structures anymore, just make your api sane and all should be fine. What prevents you from doing that and requiring compat support? thanks, greg k-h
Thanks for the comments, On 12/12/18 10:59, Greg KH wrote: >> This patch adds support for compat ioctl from 32 bits userland to >> Qualcomm fastrpc driver. > Ick, why? > > Why not just fix up your ioctl structures to not need that at all? For > new code being added to the kernel, there's no excuse to have to have > compat structures anymore, just make your api sane and all should be > fine. Yes, I did try that after comments from Arnd, > > What prevents you from doing that and requiring compat support? > I removed most of the compat IOCTLS except this one. The reason is that this ioctl takes arguments which can vary in number for each call. So args are passed as pointer to structure, rather than fixed size. I could not find better way to rearrange this to give a fixed size data structure. In theory number of arguments can vary from 0-255 for both in & out. current data structure looks like this: struct fastrpc_invoke_args { __s32 fd; size_t length; void *ptr; }; struct fastrpc_invoke { __u32 handle; __u32 sc; struct fastrpc_invoke_args *args; }; Other option is to split this IOCTL into 3 parts SET_INVOKE_METHOD, SET_INVOKE_ARGS and INVOKE with some kinda handle to bind these three. with below structures: struct fastrpc_invoke { __u32 handle; __u32 sc; }; struct fastrpc_invoke_arg { __s32 fd; __u64 length; __u64 ptr; }; I can try this and see how this works before I send next version of patch! --srini > thanks, > > greg k-h
On Wed, Dec 12, 2018 at 11:44:05AM +0000, Srinivas Kandagatla wrote: > Thanks for the comments, > > On 12/12/18 10:59, Greg KH wrote: > > > This patch adds support for compat ioctl from 32 bits userland to > > > Qualcomm fastrpc driver. > > Ick, why? > > > > Why not just fix up your ioctl structures to not need that at all? For > > new code being added to the kernel, there's no excuse to have to have > > compat structures anymore, just make your api sane and all should be > > fine. > > Yes, I did try that after comments from Arnd, > > > > > > What prevents you from doing that and requiring compat support? > > > I removed most of the compat IOCTLS except this one. > The reason is that this ioctl takes arguments which can vary in number for > each call. Then do not do that :) Remember, you get to design the api, fix the structure size to work properly everywhere. > So args are passed as pointer to structure, rather than fixed > size. I could not find better way to rearrange this to give a fixed size > data structure. In theory number of arguments can vary from 0-255 for both > in & out. > > current data structure looks like this: > > struct fastrpc_invoke_args { > __s32 fd; > size_t length; > void *ptr; > }; Make length and ptr both __u64 and you should be fine, right? If you do that, might as well make fd __u64 as well to align things better. > struct fastrpc_invoke { > __u32 handle; > __u32 sc; > struct fastrpc_invoke_args *args; > }; > > Other option is to split this IOCTL into 3 parts > SET_INVOKE_METHOD, SET_INVOKE_ARGS and INVOKE > with some kinda handle to bind these three. > > with below structures: > struct fastrpc_invoke { > __u32 handle; > __u32 sc; > }; > > struct fastrpc_invoke_arg { > __s32 fd; > __u64 length; > __u64 ptr; > }; > I can try this and see how this works before I send next version of patch! No need to have 3 calls, just change your one structure and you should be fine. thanks, greg k-h
>>> >>> What prevents you from doing that and requiring compat support? >>> >> I removed most of the compat IOCTLS except this one. >> The reason is that this ioctl takes arguments which can vary in number for >> each call. > > Then do not do that :) > > Remember, you get to design the api, fix the structure size to work > properly everywhere. > >> So args are passed as pointer to structure, rather than fixed >> size. I could not find better way to rearrange this to give a fixed size >> data structure. In theory number of arguments can vary from 0-255 for both >> in & out. >> >> current data structure looks like this: >> >> struct fastrpc_invoke_args { >> __s32 fd; >> size_t length; >> void *ptr; >> }; > > Make length and ptr both __u64 and you should be fine, right? If you do > that, might as well make fd __u64 as well to align things better. > That is fine for the args structure, but below "args" pointer in "struct fastrpc_invoke" is still not fixed size, unless we change that to __u64 pointing to array of struct fastrpc_invoke_args. I have seen such usages in i915_drm.h. Is that the preferred? >> struct fastrpc_invoke { >> __u32 handle; >> __u32 sc; >> struct fastrpc_invoke_args *args; >> }; >>--srini
On Wed, Dec 12, 2018 at 12:26:26PM +0000, Srinivas Kandagatla wrote: > > > > > > > > > > What prevents you from doing that and requiring compat support? > > > > > > > I removed most of the compat IOCTLS except this one. > > > The reason is that this ioctl takes arguments which can vary in number for > > > each call. > > > > Then do not do that :) > > > > Remember, you get to design the api, fix the structure size to work > > properly everywhere. > > > > > So args are passed as pointer to structure, rather than fixed > > > size. I could not find better way to rearrange this to give a fixed size > > > data structure. In theory number of arguments can vary from 0-255 for both > > > in & out. > > > > > > current data structure looks like this: > > > > > > struct fastrpc_invoke_args { > > > __s32 fd; > > > size_t length; > > > void *ptr; > > > }; > > > > Make length and ptr both __u64 and you should be fine, right? If you do > > that, might as well make fd __u64 as well to align things better. > > > > That is fine for the args structure, but below "args" pointer in "struct > fastrpc_invoke" is still not fixed size, unless we change that to __u64 > pointing to array of struct fastrpc_invoke_args. I have seen such usages in > i915_drm.h. > Is that the preferred? Yes, force all pointers to be __u64 and you should be fine. thanks, greg k-h
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 5ea2bba1e4bd..f36657263183 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -2,6 +2,7 @@ // Copyright (c) 2011-2018, The Linux Foundation. All rights reserved. // Copyright (c) 2018, Linaro Limited +#include <linux/compat.h> #include <linux/completion.h> #include <linux/device.h> #include <linux/dma-buf.h> @@ -194,6 +195,22 @@ struct fastrpc_user { struct mutex mutex; }; +#ifdef CONFIG_COMPAT + +#define FASTRPC_IOCTL_INVOKE32 _IOWR('R', 3, struct fastrpc_invoke32) +struct fastrpc_invoke_args32 { + __s32 fd; + compat_size_t length; + compat_caddr_t ptr; +}; + +struct fastrpc_invoke32 { + __u32 handle; + __u32 sc; + compat_uptr_t args; +}; +#endif + static void fastrpc_free_map(struct kref *ref) { struct fastrpc_map *map; @@ -1092,6 +1109,52 @@ static long fastrpc_invoke(struct fastrpc_user *fl, char __user *argp) return err; } +#ifdef CONFIG_COMPAT +static long fastrpc_invoke32(struct fastrpc_user *fl, compat_uptr_t arg) +{ + struct fastrpc_invoke_args32 *args32 = NULL; + struct fastrpc_invoke_args *args = NULL; + struct fastrpc_invoke32 inv32; + struct fastrpc_invoke inv; + int i, ret, nscalars; + + if (copy_from_user(&inv32, compat_ptr(arg), sizeof(inv32))) + return -EFAULT; + + inv.handle = inv32.handle; + inv.sc = inv32.sc; + inv.args = NULL; + nscalars = REMOTE_SCALARS_LENGTH(inv.sc); + + if (nscalars) { + args32 = kcalloc(nscalars, sizeof(*args32), GFP_KERNEL); + if (!args32) + return -ENOMEM; + + if (copy_from_user(args32, compat_ptr(inv32.args), + nscalars * sizeof(*args32))) + return -EFAULT; + + args = kcalloc(nscalars, sizeof(*args), GFP_KERNEL); + if (!args) + return -ENOMEM; + + for (i = 0; i < nscalars; i++) { + args[i].length = args32[i].length; + args[i].ptr = (void *)(unsigned long)args32[i].ptr; + args[i].fd = args32[i].fd; + } + inv.args = &args[0]; + } + + ret = fastrpc_internal_invoke(fl, 0, &inv); + kfree(args32); + kfree(args); + + return ret; +} +#endif + static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { @@ -1103,6 +1166,11 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, case FASTRPC_IOCTL_INVOKE: err = fastrpc_invoke(fl, argp); break; +#ifdef CONFIG_COMPAT + case FASTRPC_IOCTL_INVOKE32: + err = fastrpc_invoke32(fl, ptr_to_compat(argp)); + break; +#endif case FASTRPC_IOCTL_INIT_ATTACH: err = fastrpc_init_attach(fl); break; @@ -1131,6 +1199,9 @@ static const struct file_operations fastrpc_fops = { .open = fastrpc_device_open, .release = fastrpc_device_release, .unlocked_ioctl = fastrpc_device_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = fastrpc_device_ioctl, +#endif }; static int fastrpc_cb_probe(struct platform_device *pdev)