linux-kernel - Re: [PATCH 5/9] drm/xen-front: Implement handling of shared display buffers

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:   Fri, 23 Feb 2018 09:36:16 -0500
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Oleksandr Andrushchenko <andr2000@...il.com>,
        xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, airlied@...ux.ie,
        daniel.vetter@...el.com, seanpaul@...omium.org,
        gustavo@...ovan.org, jgross@...e.com, konrad.wilk@...cle.com
Cc:     Oleksandr Andrushchenko <oleksandr_andrushchenko@...m.com>
Subject: Re: [PATCH 5/9] drm/xen-front: Implement handling of shared display
 buffers

On 02/23/2018 02:53 AM, Oleksandr Andrushchenko wrote:
> On 02/23/2018 02:25 AM, Boris Ostrovsky wrote:
>> On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
>>>     static int __init xen_drv_init(void)
>>>   {
>>> +    /* At the moment we only support case with XEN_PAGE_SIZE ==
>>> PAGE_SIZE */
>>> +    BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
>>
>> Why BUILD_BUG_ON? This should simply not load if page sizes are
>> different.
>>
>>   
> This is a compile time check, so if kernel/Xen is configured
> to use page size combination which is not supported by the
> driver it will fail during compilation. This seems correct to me,
> because you shouldn't even try to load the driver which
> cannot handle different page sizes to not make any harm.


This will prevent whole kernel from building. So, for example,
randconfig builds will fail.


>>
>>
>>
>>> +    ret = gnttab_map_refs(map_ops, NULL, buf->pages, buf->num_pages);
>>> +    BUG_ON(ret);
>>
>> We should try not to BUG*(). There are a few in this patch (and possibly
>> others) that I think can be avoided.
>>
> I will rework BUG_* for map/unmap code to handle errors,
> but will still leave
>     /* either pages or sgt, not both */
>     BUG_ON(cfg->pages && cfg->sgt);
> which is a real driver bug and must not happen

Why not return an error?

In fact, AFAICS you only call it in patch 9 where both of these can be
tested, in which case something like -EINVAL would look reasonable.

-boris