linux-kernel - Re: [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers

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]

Date:   Thu, 21 Mar 2019 13:01:12 -0700
From:   John Stultz <john.stultz@...aro.org>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Laura Abbott <labbott@...hat.com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Liam Mark <lmark@...eaurora.org>,
        Brian Starkey <Brian.Starkey@....com>,
        "Andrew F . Davis" <afd@...com>, Chenbo Feng <fengc@...gle.com>,
        Alistair Strachan <astrachan@...gle.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers

On Fri, Mar 15, 2019 at 2:06 AM Christoph Hellwig <hch@...radead.org> wrote:
> > +     if (buffer->kmap_cnt) {
> > +             buffer->kmap_cnt++;
> > +             return buffer->vaddr;
> > +     }
> > +     vaddr = dma_heap_map_kernel(buffer);
> > +     if (WARN_ONCE(!vaddr,
> > +                   "heap->ops->map_kernel should return ERR_PTR on error"))
> > +             return ERR_PTR(-EINVAL);
> > +     if (IS_ERR(vaddr))
> > +             return vaddr;
> > +     buffer->vaddr = vaddr;
> > +     buffer->kmap_cnt++;
>
> The cnt manioulation is odd.  The normal way to make this readable
> is to use a postfix op on the check, as that makes it clear to everyone,
> e.g.:
>
>         if (buffer->kmap_cnt++)
>                 return buffer->vaddr;
>         ..

Thanks again on the feedback, I have had some other distractions
recently, so just getting around to these details again now.

The trouble w/ the suggestion here, if we increment in the check, then
if we trip on any of the other error paths, the cnt value will be
wrong when we return (and doing the extra decrementing in the error
paths feels as ugly as just doing the increment at the end of the
success paths)

> > +     buffer->kmap_cnt--;
> > +     if (!buffer->kmap_cnt) {
> > +             vunmap(buffer->vaddr);
> > +             buffer->vaddr = NULL;
> > +     }
>
> Same here, just with an infix.
>
> > +static inline void INIT_HEAP_HELPER_BUFFER(struct heap_helper_buffer *buffer,
> > +                              void (*free)(struct heap_helper_buffer *))
> > +{
> > +     buffer->private_flags = 0;
> > +     buffer->priv_virt = NULL;
> > +     mutex_init(&buffer->lock);
> > +     buffer->kmap_cnt = 0;
> > +     buffer->vaddr = NULL;
> > +     buffer->sg_table = NULL;
> > +     INIT_LIST_HEAD(&buffer->attachments);
> > +     buffer->free = free;
> > +}
>
> There is absolutely no reason to inlines this as far as I can tell.

Yea, I think I was mimicing some of the helpers like INIT_LIST_HEAD()
But sounds good. I can uninline it.

> Also it would seem much simpler to simply let the caller assign the
> free callback.

Yea, its a bit ugly but I worry the caller might forget?

Thanks again for the feedback!
-john