Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Nadav Amit <namit@vmware.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC:     Arnd Bergmann <arnd@arndb.de>,
        Xavier Deguillard <xdeguillard@vmware.com>,
        pv-drivers <pv-drivers@vmware.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Gil Kupfer <gilkup@gmail.com>,
        Oleksandr Natalenko <onatalen@redhat.com>,
        "ldu@redhat.com" <ldu@redhat.com>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH v3] vmw_balloon: fixing double free when batching mode is
 off
Thread-Topic: [PATCH v3] vmw_balloon: fixing double free when batching mode is
 off
Thread-Index: AQHT2Argu2pZ34klIkGamyuxGjdz+KQZodaAgC4XGICAAtrGAIAAuSeAgAB1toA=
Date:   Fri, 1 Jun 2018 15:00:48 +0000
Message-ID: <225A97FC-9215-4234-A181-3D7B45A46C70@vmware.com>
References: <20180419063856.GA7643@kroah.com>
 <20180419181722.12273-1-namit@vmware.com>
 <162DEB66-373B-4E01-8D64-3CE7BEF47920@vmware.com>
 <7B3793E8-8DD9-4566-9CCB-6D1B0C364754@vmware.com>
 <4D5BA2F0-377D-4959-9786-95E195EE2D22@vmware.com>
 <20180601075931.GB12809@kroah.com>
In-Reply-To: <20180601075931.GB12809@kroah.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: vmware.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-ID: <0011D3C9F0E5B94B921DA65BBB2228F2@namprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: aa6794e7-49d7-4f2e-7971-08d5c7d0759e
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Jun 2018 15:00:48.1316
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN2PR05MB2478
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, May 31, 2018 at 08:56:52PM +0000, Nadav Amit wrote:
>> Nadav Amit <namit@vmware.com> wrote:
>>=20
>>> Nadav Amit <namit@vmware.com> wrote:
>>>=20
>>>> Ping. Please consider it for inclusion for rc4.
>>>>=20
>>>> Nadav Amit <namit@vmware.com> wrote:
>>>>=20
>>>>> From: Gil Kupfer <gilkup@gmail.com>
>>>>>=20
>>>>> The balloon.page field is used for two different purposes if batching=
 is
>>>>> on or off. If batching is on, the field point to the page which is us=
ed
>>>>> to communicate with with the hypervisor. If it is off, balloon.page
>>>>> points to the page that is about to be (un)locked.
>>>>>=20
>>>>> Unfortunately, this dual-purpose of the field introduced a bug: when =
the
>>>>> balloon is popped (e.g., when the machine is reset or the balloon dri=
ver
>>>>> is explicitly removed), the balloon driver frees, unconditionally, th=
e
>>>>> page that is held in balloon.page.  As a result, if batching is
>>>>> disabled, this leads to double freeing the last page that is sent to =
the
>>>>> hypervisor.
>>>>>=20
>>>>> The following error occurs during rmmod when kernel checkers are on, =
and
>>>>> the balloon is not empty:
>>>>>=20
>>>>> [   42.307653] ------------[ cut here ]------------
>>>>> [   42.307657] Kernel BUG at ffffffffba1e4b28 [verbose debug info una=
vailable]
>>>>> [   42.307720] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
>>>>> [   42.312512] Modules linked in: vmw_vsock_vmci_transport vsock ppde=
v joydev vmw_balloon(-) input_leds serio_raw vmw_vmci parport_pc shpchp par=
port i2c_piix4 nfit mac_hid autofs4 vmwgfx drm_kms_helper hid_generic sysco=
pyarea sysfillrect usbhid sysimgblt fb_sys_fops hid ttm mptspi scsi_transpo=
rt_spi ahci mptscsih drm psmouse vmxnet3 libahci mptbase pata_acpi
>>>>> [   42.312766] CPU: 10 PID: 1527 Comm: rmmod Not tainted 4.12.0+ #5
>>>>> [   42.312803] Hardware name: VMware, Inc. VMware Virtual Platform/44=
0BX Desktop Reference Platform, BIOS 6.00 09/30/2016
>>>>> [   42.313042] task: ffff9bf9680f8000 task.stack: ffffbfefc1638000
>>>>> [   42.313290] RIP: 0010:__free_pages+0x38/0x40
>>>>> [   42.313510] RSP: 0018:ffffbfefc163be98 EFLAGS: 00010246
>>>>> [   42.313731] RAX: 000000000000003e RBX: ffffffffc02b9720 RCX: 00000=
00000000006
>>>>> [   42.313972] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff9=
bf97e08e0a0
>>>>> [   42.314201] RBP: ffffbfefc163be98 R08: 0000000000000000 R09: 00000=
00000000000
>>>>> [   42.314435] R10: 0000000000000000 R11: 0000000000000000 R12: fffff=
fffc02b97e4
>>>>> [   42.314505] R13: ffffffffc02b9748 R14: ffffffffc02b9728 R15: 00000=
00000000200
>>>>> [   42.314550] FS:  00007f3af5fec700(0000) GS:ffff9bf97e080000(0000) =
knlGS:0000000000000000
>>>>> [   42.314599] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> [   42.314635] CR2: 00007f44f6f4ab24 CR3: 00000003a7d12000 CR4: 00000=
000000006e0
>>>>> [   42.314864] Call Trace:
>>>>> [   42.315774]  vmballoon_pop+0x102/0x130 [vmw_balloon]
>>>>> [   42.315816]  vmballoon_exit+0x42/0xd64 [vmw_balloon]
>>>>> [   42.315853]  SyS_delete_module+0x1e2/0x250
>>>>> [   42.315891]  entry_SYSCALL_64_fastpath+0x23/0xc2
>>>>> [   42.315924] RIP: 0033:0x7f3af5b0e8e7
>>>>> [   42.315949] RSP: 002b:00007fffe6ce0148 EFLAGS: 00000206 ORIG_RAX: =
00000000000000b0
>>>>> [   42.315996] RAX: ffffffffffffffda RBX: 000055be676401e0 RCX: 00007=
f3af5b0e8e7
>>>>> [   42.316951] RDX: 000000000000000a RSI: 0000000000000800 RDI: 00005=
5be67640248
>>>>> [   42.317887] RBP: 0000000000000003 R08: 0000000000000000 R09: 19999=
99999999999
>>>>> [   42.318845] R10: 0000000000000883 R11: 0000000000000206 R12: 00007=
fffe6cdf130
>>>>> [   42.319755] R13: 0000000000000000 R14: 0000000000000000 R15: 00005=
5be676401e0
>>>>> [   42.320606] Code: c0 74 1c f0 ff 4f 1c 74 02 5d c3 85 f6 74 07 e8 =
0f d8 ff ff 5d c3 31 f6 e8 c6 fb ff ff 5d c3 48 c7 c6 c8 0f c5 ba e8 58 be =
02 00 <0f> 0b 66 0f 1f 44 00 00 66 66 66 66 90 48 85 ff 75 01 c3 55 48
>>>>> [   42.323462] RIP: __free_pages+0x38/0x40 RSP: ffffbfefc163be98
>>>>> [   42.325735] ---[ end trace 872e008e33f81508 ]---
>>>>>=20
>>>>> To solve the bug, we eliminate the dual purpose of balloon.page.
>>>>>=20
>>>>> Fixes: f220a80f0c2e ("VMware balloon: add batching to the vmw_balloon=
.")
>>>>> Cc: stable@vger.kernel.org
>>>>> Reported-by: Oleksandr Natalenko <onatalen@redhat.com>
>>>>> Signed-off-by: Gil Kupfer <gilkup@gmail.com>
>>>>> Signed-off-by: Nadav Amit <namit@vmware.com>
>>>>> Reviewed-by: Xavier Deguillard <xdeguillard@vmware.com>
>>>>> ---
>>>>> drivers/misc/vmw_balloon.c | 23 +++++++----------------
>>>>> 1 file changed, 7 insertions(+), 16 deletions(-)
>>>>>=20
>>>>> diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c
>>>>> index 9047c0a529b2..efd733472a35 100644
>>>>> --- a/drivers/misc/vmw_balloon.c
>>>>> +++ b/drivers/misc/vmw_balloon.c
>>>>> @@ -576,15 +576,9 @@ static void vmballoon_pop(struct vmballoon *b)
>>>>> 		}
>>>>> 	}
>>>>>=20
>>>>> -	if (b->batch_page) {
>>>>> -		vunmap(b->batch_page);
>>>>> -		b->batch_page =3D NULL;
>>>>> -	}
>>>>> -
>>>>> -	if (b->page) {
>>>>> -		__free_page(b->page);
>>>>> -		b->page =3D NULL;
>>>>> -	}
>>>>> +	/* Clearing the batch_page unconditionally has no adverse effect */
>>>>> +	free_page((unsigned long)b->batch_page);
>>>>> +	b->batch_page =3D NULL;
>>>>> }
>>>>>=20
>>>>> /*
>>>>> @@ -991,16 +985,13 @@ static const struct vmballoon_ops vmballoon_bat=
ched_ops =3D {
>>>>>=20
>>>>> static bool vmballoon_init_batching(struct vmballoon *b)
>>>>> {
>>>>> -	b->page =3D alloc_page(VMW_PAGE_ALLOC_NOSLEEP);
>>>>> -	if (!b->page)
>>>>> -		return false;
>>>>> +	struct page *page;
>>>>>=20
>>>>> -	b->batch_page =3D vmap(&b->page, 1, VM_MAP, PAGE_KERNEL);
>>>>> -	if (!b->batch_page) {
>>>>> -		__free_page(b->page);
>>>>> +	page =3D alloc_page(GFP_KERNEL | __GFP_ZERO);
>>>>> +	if (!page)
>>>>> 		return false;
>>>>> -	}
>>>>>=20
>>>>> +	b->batch_page =3D page_address(page);
>>>>> 	return true;
>>>>> }
>>>>>=20
>>>>> --=20
>>>>> 2.14.1
>>>=20
>>> Greg, can you please include this patch?? 4.17 is almost out of the doo=
r,
>>> and the last version was sent a month ago.
>>>=20
>>> If you have any reservations, please let me know immediately.
>>=20
>> Arnd,
>>=20
>> Perhaps you can - the very least - respond (or just include the patch)?
>>=20
>> The last version of this patch was sent over a month ago.
>=20
> It's too late for 4.17 now, and as this touches core code, I'll wait for
> the next merge window cycle.  It's also not even in my patch review
> queue anymore, I guess the long "does this solve the problem" delays
> that this went through pushed it out.
>=20
> So I would need it resent no matter what.

There was no delay from our side. This whole interaction regarding a simple
fix is really frustrating. I will resend the patch.