| Message ID | 20200114134101.159194-1-liumartin@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: support 32bit DMA_BUF_SET_NAME ioctl | expand |
On Tue, Jan 14, 2020 at 09:41:01PM +0800, Martin Liu wrote: CC more MLs for winder review. > This commit adds SET_NAME ioctl coversion to > support 32 bit ioctl. > > Signed-off-by: Martin Liu <liumartin@google.com> > --- > drivers/dma-buf/dma-buf.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index ce41cd9b758a..a73048b34843 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -25,6 +25,7 @@ > #include <linux/mm.h> > #include <linux/mount.h> > #include <linux/pseudo_fs.h> > +#include <linux/compat.h> > > #include <uapi/linux/dma-buf.h> > #include <uapi/linux/magic.h> > @@ -409,13 +410,32 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) > dma_resv_unlock(dmabuf->resv); > } > > +#ifdef CONFIG_COMPAT > +static long dma_buf_ioctl_compat(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + switch (_IOC_NR(cmd)) { > + case _IOC_NR(DMA_BUF_SET_NAME): > + /* Fix up pointer size*/ > + if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) { > + cmd &= ~IOCSIZE_MASK; > + cmd |= sizeof(void *) << IOCSIZE_SHIFT; > + } > + break; > + } > + return dma_buf_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); > +} > +#endif > + > static const struct file_operations dma_buf_fops = { > .release = dma_buf_release, > .mmap = dma_buf_mmap_internal, > .llseek = dma_buf_llseek, > .poll = dma_buf_poll, > .unlocked_ioctl = dma_buf_ioctl, > - .compat_ioctl = compat_ptr_ioctl, > +#ifdef CONFIG_COMPAT > + .compat_ioctl = dma_buf_ioctl_compat, > +#endif > .show_fdinfo = dma_buf_show_fdinfo, > }; > > -- > 2.25.0.rc1.283.g88dfdc4193-goog >
Hello, Daniel, your comments here, please? On Mon, 24 Feb 2020 at 09:09, Martin Liu <liumartin@google.com> wrote: > > On Tue, Jan 14, 2020 at 09:41:01PM +0800, Martin Liu wrote: > > CC more MLs for winder review. > > > This commit adds SET_NAME ioctl coversion to > > support 32 bit ioctl. > > > > Signed-off-by: Martin Liu <liumartin@google.com> > > --- > > drivers/dma-buf/dma-buf.c | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > index ce41cd9b758a..a73048b34843 100644 > > --- a/drivers/dma-buf/dma-buf.c > > +++ b/drivers/dma-buf/dma-buf.c > > @@ -25,6 +25,7 @@ > > #include <linux/mm.h> > > #include <linux/mount.h> > > #include <linux/pseudo_fs.h> > > +#include <linux/compat.h> > > > > #include <uapi/linux/dma-buf.h> > > #include <uapi/linux/magic.h> > > @@ -409,13 +410,32 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) > > dma_resv_unlock(dmabuf->resv); > > } > > > > +#ifdef CONFIG_COMPAT > > +static long dma_buf_ioctl_compat(struct file *file, unsigned int cmd, > > + unsigned long arg) > > +{ > > + switch (_IOC_NR(cmd)) { > > + case _IOC_NR(DMA_BUF_SET_NAME): > > + /* Fix up pointer size*/ > > + if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) { > > + cmd &= ~IOCSIZE_MASK; > > + cmd |= sizeof(void *) << IOCSIZE_SHIFT; > > + } > > + break; > > + } > > + return dma_buf_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); > > +} > > +#endif > > + > > static const struct file_operations dma_buf_fops = { > > .release = dma_buf_release, > > .mmap = dma_buf_mmap_internal, > > .llseek = dma_buf_llseek, > > .poll = dma_buf_poll, > > .unlocked_ioctl = dma_buf_ioctl, > > - .compat_ioctl = compat_ptr_ioctl, > > +#ifdef CONFIG_COMPAT > > + .compat_ioctl = dma_buf_ioctl_compat, > > +#endif > > .show_fdinfo = dma_buf_show_fdinfo, > > }; > > > > -- > > 2.25.0.rc1.283.g88dfdc4193-goog > >
On Tue, Apr 7, 2020 at 3:26 PM Sumit Semwal <sumit.semwal@linaro.org> wrote: > > Hello, > > Daniel, your comments here, please? Don't :-) I mean rule of thumb you should never need a compat_ioctl for a new ioctl, that's just failure to read https://www.kernel.org/doc/html/v5.4-preprc-cpu/ioctl/botching-up-ioctls.html Specifically the char * pointer in the ioctl definition is the problem. Now we're somewhat lucky here, since the actual layout of the data isn't different between 32 and 64 bit, it's only the ioctl. Which is the 2nd issue, the type in there should be the type of the data in userspace, not the type of the _pointer_ to the stuff in userspace. So here actually variable-sized char[] array, which also doesn't work really. Anyway I've created a quick patch to have distinct ioctl number defines and handle both in the main handler, that should work. Cc'ed everyone from this thread, please test. -Daniel > > On Mon, 24 Feb 2020 at 09:09, Martin Liu <liumartin@google.com> wrote: > > > > On Tue, Jan 14, 2020 at 09:41:01PM +0800, Martin Liu wrote: > > > > CC more MLs for winder review. > > > > > This commit adds SET_NAME ioctl coversion to > > > support 32 bit ioctl. > > > > > > Signed-off-by: Martin Liu <liumartin@google.com> > > > --- > > > drivers/dma-buf/dma-buf.c | 22 +++++++++++++++++++++- > > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > > index ce41cd9b758a..a73048b34843 100644 > > > --- a/drivers/dma-buf/dma-buf.c > > > +++ b/drivers/dma-buf/dma-buf.c > > > @@ -25,6 +25,7 @@ > > > #include <linux/mm.h> > > > #include <linux/mount.h> > > > #include <linux/pseudo_fs.h> > > > +#include <linux/compat.h> > > > > > > #include <uapi/linux/dma-buf.h> > > > #include <uapi/linux/magic.h> > > > @@ -409,13 +410,32 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) > > > dma_resv_unlock(dmabuf->resv); > > > } > > > > > > +#ifdef CONFIG_COMPAT > > > +static long dma_buf_ioctl_compat(struct file *file, unsigned int cmd, > > > + unsigned long arg) > > > +{ > > > + switch (_IOC_NR(cmd)) { > > > + case _IOC_NR(DMA_BUF_SET_NAME): > > > + /* Fix up pointer size*/ > > > + if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) { > > > + cmd &= ~IOCSIZE_MASK; > > > + cmd |= sizeof(void *) << IOCSIZE_SHIFT; > > > + } > > > + break; > > > + } > > > + return dma_buf_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); > > > +} > > > +#endif > > > + > > > static const struct file_operations dma_buf_fops = { > > > .release = dma_buf_release, > > > .mmap = dma_buf_mmap_internal, > > > .llseek = dma_buf_llseek, > > > .poll = dma_buf_poll, > > > .unlocked_ioctl = dma_buf_ioctl, > > > - .compat_ioctl = compat_ptr_ioctl, > > > +#ifdef CONFIG_COMPAT > > > + .compat_ioctl = dma_buf_ioctl_compat, > > > +#endif > > > .show_fdinfo = dma_buf_show_fdinfo, > > > }; > > > > > > -- > > > 2.25.0.rc1.283.g88dfdc4193-goog > > > > > > > -- > Thanks and regards, > > Sumit Semwal > Linaro Consumer Group - Kernel Team Lead > Linaro.org │ Open source software for ARM SoCs
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index ce41cd9b758a..a73048b34843 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -25,6 +25,7 @@ #include <linux/mm.h> #include <linux/mount.h> #include <linux/pseudo_fs.h> +#include <linux/compat.h> #include <uapi/linux/dma-buf.h> #include <uapi/linux/magic.h> @@ -409,13 +410,32 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) dma_resv_unlock(dmabuf->resv); } +#ifdef CONFIG_COMPAT +static long dma_buf_ioctl_compat(struct file *file, unsigned int cmd, + unsigned long arg) +{ + switch (_IOC_NR(cmd)) { + case _IOC_NR(DMA_BUF_SET_NAME): + /* Fix up pointer size*/ + if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) { + cmd &= ~IOCSIZE_MASK; + cmd |= sizeof(void *) << IOCSIZE_SHIFT; + } + break; + } + return dma_buf_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); +} +#endif + static const struct file_operations dma_buf_fops = { .release = dma_buf_release, .mmap = dma_buf_mmap_internal, .llseek = dma_buf_llseek, .poll = dma_buf_poll, .unlocked_ioctl = dma_buf_ioctl, - .compat_ioctl = compat_ptr_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = dma_buf_ioctl_compat, +#endif .show_fdinfo = dma_buf_show_fdinfo, };
This commit adds SET_NAME ioctl coversion to support 32 bit ioctl. Signed-off-by: Martin Liu <liumartin@google.com> --- drivers/dma-buf/dma-buf.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)