linux-kernel - Re: [PATCH v15 1/5] dma-buf: Add dma-buf heaps framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALAqxLW1vgLCik5WfDN7qkRsO=K9U4otNBp72aOH5UNN1jUgMQ@mail.gmail.com>
Date:   Wed, 6 Nov 2019 09:03:10 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     "Andrew F. Davis" <afd@...com>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Laura Abbott <labbott@...hat.com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Liam Mark <lmark@...eaurora.org>,
        Pratik Patel <pratikp@...eaurora.org>,
        Brian Starkey <Brian.Starkey@....com>,
        Vincent Donnefort <Vincent.Donnefort@....com>,
        Sudipto Paul <Sudipto.Paul@....com>,
        Christoph Hellwig <hch@...radead.org>,
        Chenbo Feng <fengc@...gle.com>,
        Alistair Strachan <astrachan@...gle.com>,
        Hridya Valsaraju <hridya@...gle.com>,
        Sandeep Patil <sspatil@...gle.com>,
        Hillf Danton <hdanton@...a.com>,
        Dave Airlie <airlied@...il.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v15 1/5] dma-buf: Add dma-buf heaps framework

On Wed, Nov 6, 2019 at 5:52 AM Andrew F. Davis <afd@...com> wrote:
>
> On 11/5/19 11:22 PM, John Stultz wrote:
> > +unsigned int dma_heap_ioctl_cmds[] = {
> > +     DMA_HEAP_IOC_ALLOC,
> > +};
> > +
> > +static long dma_heap_ioctl(struct file *file, unsigned int ucmd,
> > +                        unsigned long arg)
> > +{
> > +     char stack_kdata[128];
> > +     char *kdata = stack_kdata;
> > +     unsigned int kcmd;
> > +     unsigned int in_size, out_size, drv_size, ksize;
> > +     int nr = _IOC_NR(ucmd);
> > +     int ret = 0;
> > +
> > +     if (nr >= ARRAY_SIZE(dma_heap_ioctl_cmds))
> > +             return -EINVAL;
> > +
> > +     /* Get the kernel ioctl cmd that matches */
> > +     kcmd = dma_heap_ioctl_cmds[nr];
>
>
> Why do we need this indirection here and all the complexity below? I
> know DRM ioctl does something like this but it has a massive table,
> legacy ioctls, driver defined ioctls, etc..
>
> I don't expect we will ever need complex handling like this, could we
> switch back to the more simple handler from v13?

I agree it does add complexity, but I'm not sure I see how to avoid
some of this. The logic trying to handle that the user may pass a cmd
that has the same _IOC_NR() as DMA_HEAP_IOC_ALLOC but not the same
size. So the simple "switch(cmd) { case DMA_HEAP_IOC_ALLOC:" we had
before won't work (as the cmd will be a different value).

Thus why I thought the cleanest approach would be to use the
dma_heap_ioctl_cmds array to convert from whatever the user cmd is to
the matching kernel cmd value.

Do you have an alternative suggestion that I'm overlooking?

thanks
-john