linux-kernel - Re: [PATCH v3 8/9] xen/gntdev: Implement dma-buf export functionality

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: <9f006537-e57f-67b3-e9a1-7897fa3beee2@epam.com>
Date:   Thu, 14 Jun 2018 08:41:09 +0300
From:   Oleksandr Andrushchenko <Oleksandr_Andrushchenko@...m.com>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Oleksandr Andrushchenko <andr2000@...il.com>,
        xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-media@...r.kernel.org,
        jgross@...e.com, konrad.wilk@...cle.com
Cc:     daniel.vetter@...el.com, dongwon.kim@...el.com,
        matthew.d.roper@...el.com
Subject: Re: [PATCH v3 8/9] xen/gntdev: Implement dma-buf export functionality

On 06/14/2018 01:19 AM, Boris Ostrovsky wrote:
> On 06/13/2018 07:57 AM, Oleksandr Andrushchenko wrote:
>> On 06/13/2018 05:58 AM, Boris Ostrovsky wrote:
>>>
>>> On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:
>>>> +
>>>> +static struct gntdev_dmabuf *
>>>> +dmabuf_exp_wait_obj_get_by_fd(struct gntdev_dmabuf_priv *priv, int fd)
>>>
>>> The name of this routine implies (to me) that we are getting a wait
>>> object but IIUIC we are getting a gntdev_dmabuf that we are going to
>>> later associate with a wait object.
>>>
>> How about dmabuf_exp_wait_obj_get_dmabuf_by_fd?
>> I would like to keep function prefixes, e.g. dmabuf_exp_wait_obj_
>> just to show to which functionality a routine belongs.
>
> That's too long IMO. If you really want to keep the prefix then let's
> keep this the way it is. Maybe it's just me who read it that way.
I'll rename it to dmabuf_exp_wait_obj_get_dmabuf.
As it already wants fd it seems to be clear that
the lookup will be based on it.
>
>>>>    +#ifdef CONFIG_XEN_GNTDEV_DMABUF
>>>> +void gntdev_remove_map(struct gntdev_priv *priv, struct
>>>> gntdev_grant_map *map)
>>>> +{
>>>> +    mutex_lock(&priv->lock);
>>>> +    list_del(&map->next);
>>>> +    gntdev_put_map(NULL /* already removed */, map);
>>>
>>> Why not pass call gntdev_put_map(priv, map) and then not have this
>>> routine at all?
>>>
>> Well, I wish I could, but the main difference when calling
>> gntdev_put_map(priv, map)
>> with priv != NULL and my code is that:
>>
>> void gntdev_put_map(struct gntdev_priv *priv, struct gntdev_grant_map
>> *map)
>> {
>>      [...]
>>      if (populate_freeable_maps && priv) {
>>      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>          mutex_lock(&priv->lock);
>>          list_del(&map->next);
>>          mutex_unlock(&priv->lock);
>>      }
>>      [...]
>> }
>>
>> and
>>
>> #define populate_freeable_maps use_ptemod
>>                                 ^^^^^^^^^^
>> which means the map will never be removed from the list in my case
>> because use_ptemod == false for dma-buf.
>> This is why I do that by hand, e.g. remove the item from the list
>> and then pass NULL for priv.
>>
>> Also, I will remove gntdev_remove_map as I can now access
>> priv->lock and gntdev_put_map directly form gntdev-dmabuf.c
>
> Yes, that's a good idea.
>
>>> I really dislike the fact that we are taking a lock here that
>>> gntdev_put_map() takes as well, although not with NULL argument. (And
>>> yes, I see that gntdev_release() does it too.)
>>>
>> This can be re-factored later I guess?
> OK.
>
> -boris
>