2020-01-14 13:43:19

by Martin Liu

[permalink] [raw]
Subject: [PATCH] dma-buf: support 32bit DMA_BUF_SET_NAME ioctl

This commit adds SET_NAME ioctl coversion to
support 32 bit ioctl.

Signed-off-by: Martin Liu <[email protected]>
---
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


2020-02-24 03:41:26

by Martin Liu

[permalink] [raw]
Subject: Re: [PATCH] dma-buf: support 32bit DMA_BUF_SET_NAME ioctl

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 <[email protected]>
> ---
> 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
>

2020-04-07 13:29:14

by Sumit Semwal

[permalink] [raw]
Subject: Re: [PATCH] dma-buf: support 32bit DMA_BUF_SET_NAME ioctl

Hello,

Daniel, your comments here, please?

On Mon, 24 Feb 2020 at 09:09, Martin Liu <[email protected]> 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 <[email protected]>
> > ---
> > 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

2020-04-07 14:09:57

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH] dma-buf: support 32bit DMA_BUF_SET_NAME ioctl

On Tue, Apr 7, 2020 at 3:26 PM Sumit Semwal <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch