linux-kernel - Re: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE

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: <5A0ECC95.7070300@intel.com>
Date:   Fri, 17 Nov 2017 19:48:37 +0800
From:   Wei Wang <wei.w.wang@...el.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
CC:     aarcange@...hat.com, virtio-dev@...ts.oasis-open.org,
        kvm@...r.kernel.org, mawilcox@...rosoft.com, qemu-devel@...gnu.org,
        amit.shah@...hat.com, penguin-kernel@...ove.SAKURA.ne.jp,
        linux-kernel@...r.kernel.org, willy@...radead.org,
        virtualization@...ts.linux-foundation.org, linux-mm@...ck.org,
        yang.zhang.wz@...il.com, quan.xu@...yun.com,
        cornelia.huck@...ibm.com, pbonzini@...hat.com,
        akpm@...ux-foundation.org, mhocko@...nel.org,
        mgorman@...hsingularity.net, liliang.opensource@...il.com
Subject: Re: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ

On 11/17/2017 07:35 PM, Wei Wang wrote:
> On 11/16/2017 09:27 PM, Wei Wang wrote:
>> On 11/16/2017 04:32 AM, Michael S. Tsirkin wrote:
>>> On Fri, Nov 03, 2017 at 04:13:06PM +0800, Wei Wang wrote:
>>>> Negotiation of the VIRTIO_BALLOON_F_FREE_PAGE_VQ feature indicates the
>>>> support of reporting hints of guest free pages to the host via
>>>> virtio-balloon. The host requests the guest to report the free 
>>>> pages by
>>>> sending commands via the virtio-balloon configuration registers.
>>>>
>>>> When the guest starts to report, the first element added to the 
>>>> free page
>>>> vq is a sequence id of the start reporting command. The id is given by
>>>> the host, and it indicates whether the following free pages correspond
>>>> to the command. For example, the host may stop the report and start 
>>>> again
>>>> with a new command id. The obsolete pages for the previous start 
>>>> command
>>>> can be detected by the id dismatching on the host. The id is added 
>>>> to the
>>>> vq using an output buffer, and the free pages are added to the vq 
>>>> using
>>>> input buffer.
>>>>
>>>> Here are some explainations about the added configuration registers:
>>>> - host2guest_cmd: a register used by the host to send commands to the
>>>> guest.
>>>> - guest2host_cmd: written by the guest to ACK to the host about the
>>>> commands that have been received. The host will clear the 
>>>> corresponding
>>>> bits on the host2guest_cmd register. The guest also uses this register
>>>> to send commands to the host (e.g. when finish free page reporting).
>>>> - free_page_cmd_id: the sequence id of the free page report command
>>>> given by the host.
>>>>
>>>> Signed-off-by: Wei Wang <wei.w.wang@...el.com>
>>>> Signed-off-by: Liang Li <liang.z.li@...el.com>
>>>> Cc: Michael S. Tsirkin <mst@...hat.com>
>>>> Cc: Michal Hocko <mhocko@...nel.org>
>>>> ---
>>>>
>>>> +
>>>> +static void report_free_page(struct work_struct *work)
>>>> +{
>>>> +    struct virtio_balloon *vb;
>>>> +
>>>> +    vb = container_of(work, struct virtio_balloon, 
>>>> report_free_page_work);
>>>> +    report_free_page_cmd_id(vb);
>>>> +    walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
>>>> +    /*
>>>> +     * The last few free page blocks that were added may not reach 
>>>> the
>>>> +     * batch size, but need a kick to notify the device to handle 
>>>> them.
>>>> +     */
>>>> +    virtqueue_kick(vb->free_page_vq);
>>>> +    report_free_page_end(vb);
>>>> +}
>>>> +
>>> I think there's an issue here: if pages are poisoned and hypervisor
>>> subsequently drops them, testing them after allocation will
>>> trigger a false positive.
>>>
>>> The specific configuration:
>>>
>>> PAGE_POISONING on
>>> PAGE_POISONING_NO_SANITY off
>>> PAGE_POISONING_ZERO off
>>>
>>>
>>> Solutions:
>>> 1. disable the feature in that configuration
>>>     suggested as an initial step
>>
>> Thanks for the finding.
>> Similar to this option: I'm thinking could we make 
>> walk_free_mem_block() simply return if that option is on?
>> That is, at the beginning of the function:
>>     if (!page_poisoning_enabled())
>>                 return;
>>
>
>
> Thought about it more, I think it would be better to put this logic to 
> virtio_balloon:
>
>         send_free_page_cmd_id(vb, &vb->start_cmd_id);
>         if (page_poisoning_enabled() &&
>             !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY))
>                 walk_free_mem_block(vb, 0, 
> &virtio_balloon_send_free_pages);

logic should be inverse:
     if (!(page_poisoning_enabled() &&
             !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY)))

Best,
Wei