linux-kernel - Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

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:   Mon, 2 Sep 2019 07:28:52 +0200
From:   Gerd Hoffmann <kraxel@...hat.com>
To:     David Riley <davidriley@...omium.org>
Cc:     dri-devel@...ts.freedesktop.org,
        virtualization@...ts.linux-foundation.org,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Stéphane Marchesin <marcheu@...omium.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

On Fri, Aug 30, 2019 at 10:49:25AM -0700, David Riley wrote:
> Hi Gerd,
> 
> On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@...hat.com> wrote:
> >
> >   Hi,
> >
> > > > > -     kfree(vbuf->data_buf);
> > > > > +     kvfree(vbuf->data_buf);
> > > >
> > > > if (is_vmalloc_addr(vbuf->data_buf)) ...
> > > >
> > > > needed here I gues?
> > > >
> > >
> > > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.
> >
> > Ok.
> >
> > > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> > > assumes contiguous array of scatterlist and that the buffer being converted
> > > is page aligned
> >
> > Well, vmalloc memory _is_ page aligned.
> 
> True, but this function gets called for all potential enqueuings (eg
> resource_create_3d, resource_attach_backing) and I was concerned that
> some other usage in the future might not have that guarantee.

The vmalloc_to_sg call is wrapped into "if (is_vmalloc())", so this
should not be a problem.

> > sg_alloc_table_from_pages() does alot of what you need, you just need a
> > small loop around vmalloc_to_page() create a struct page array
> > beforehand.
> 
> That feels like an extra allocation when under memory pressure and
> more work, to not gain much -- there still needs to be a function that
> iterates through all the pages.  But I don't feel super strongly about
> it and can change it if you think that it will be less maintenance
> overhead.

Lets see how vmalloc_to_sg looks like when it assumes page-aligned
memory.  It's probably noticeable shorter then.

cheers,
  Gerd