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:   KY Srinivasan <kys@microsoft.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        "Michael Kelley (EOSG)" <Michael.H.Kelley@microsoft.com>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devel@linuxdriverproject.org" <devel@linuxdriverproject.org>,
        "olaf@aepfle.de" <olaf@aepfle.de>,
        "apw@canonical.com" <apw@canonical.com>,
        "vkuznets@redhat.com" <vkuznets@redhat.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        "marcelo.cerri@canonical.com" <marcelo.cerri@canonical.com>,
        Stephen Hemminger <sthemmin@microsoft.com>
Subject: RE: [PATCH char-misc 1/1] Drivers: hv: vmbus: Cleanup synic memory
 free path
Thread-Topic: [PATCH char-misc 1/1] Drivers: hv: vmbus: Cleanup synic memory
 free path
Thread-Index: AQHUKe6NBBh7WH660kaJPBPrLCap+6SsA9aAgACaWTA=
Date:   Thu, 2 Aug 2018 15:54:50 +0000
Message-ID: <MW2PR2101MB11135C6C11EF22882D9215F7A02C0@MW2PR2101MB1113.namprd21.prod.outlook.com>
References: <1533163513-10764-1-git-send-email-mikelley@microsoft.com>
 <20180802064133.jm5wo4e5ypn6p3xq@mwanda>
In-Reply-To: <20180802064133.jm5wo4e5ypn6p3xq@mwanda>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: microsoft.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: fd960bdb-1a6a-46bf-c671-08d5f89047fe
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Aug 2018 15:54:50.7932
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW2PR2101MB0906
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Wednesday, August 1, 2018 11:42 PM
> To: Michael Kelley (EOSG) <Michael.H.Kelley@microsoft.com>
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> vkuznets@redhat.com; jasowang@redhat.com;
> marcelo.cerri@canonical.com; Stephen Hemminger
> <sthemmin@microsoft.com>; KY Srinivasan <kys@microsoft.com>
> Subject: Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Cleanup synic
> memory free path
>=20
> On Wed, Aug 01, 2018 at 03:45:13PM -0700, mhkelley58@gmail.com wrote:
> > From: Michael Kelley <mikelley@microsoft.com>
> >
> > clk_evt memory is not being freed when the synic is shutdown
> > or when there is an allocation error.  Add the appropriate
> > kfree() call, along with a comment to clarify how the memory
> > gets freed after an allocation error.  Make the free path
> > consistent by removing checks for NULL since kfree() and
> > free_page() already do the check.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/hv/hv.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> > index 8d4fe0e..1fb9a6b 100644
> > --- a/drivers/hv/hv.c
> > +++ b/drivers/hv/hv.c
> > @@ -240,6 +240,10 @@ int hv_synic_alloc(void)
> >
> >  	return 0;
> >  err:
> > +	/*
> > +	 * Any memory allocations that succeeded will be freed when
> > +	 * the caller cleans up by calling hv_synic_free()
> > +	 */
> >  	return -ENOMEM;
> >  }
> >
> > @@ -252,12 +256,10 @@ void hv_synic_free(void)
> >         for_each_present_cpu(cpu) {
> >  		struct hv_per_cpu_context *hv_cpu
> >  			=3D per_cpu_ptr(hv_context.cpu_context, cpu);
> >
> > -		if (hv_cpu->synic_event_page)
> > -			free_page((unsigned long)hv_cpu-
> >synic_event_page);
> > -		if (hv_cpu->synic_message_page)
> > -			free_page((unsigned long)hv_cpu-
> >synic_message_page);
> > -		if (hv_cpu->post_msg_page)
> > -			free_page((unsigned long)hv_cpu-
> >post_msg_page);
> > +		kfree(hv_cpu->clk_evt);
> > +		free_page((unsigned long)hv_cpu->synic_event_page);
> > +		free_page((unsigned long)hv_cpu->synic_message_page);
> > +		free_page((unsigned long)hv_cpu->post_msg_page);
>=20
> This looks buggy.
>=20
> We can pass NULLs to free_page() so that's fine.  So the error handling
> assumes
> that hv_cpu->clk_evt is either NULL or allocated.  Here is how it is allo=
cated:
>=20
>    189  int hv_synic_alloc(void)
>    190  {
>    191          int cpu;
>    192
>    193          hv_context.hv_numa_map =3D kcalloc(nr_node_ids, sizeof(st=
ruct
> cpumask),
>    194                                           GFP_KERNEL);
>    195          if (hv_context.hv_numa_map =3D=3D NULL) {
>    196                  pr_err("Unable to allocate NUMA map\n");
>    197                  goto err;
>    198          }
>    199
>    200          for_each_present_cpu(cpu) {
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We loop over each CPU.
>=20
>    201                  struct hv_per_cpu_context *hv_cpu
>    202                          =3D per_cpu_ptr(hv_context.cpu_context, c=
pu);
>    203
>    204                  memset(hv_cpu, 0, sizeof(*hv_cpu));
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We set this cpu memory to NULL.
>=20
>    205                  tasklet_init(&hv_cpu->msg_dpc,
>    206                               vmbus_on_msg_dpc, (unsigned long) hv=
_cpu);
>    207
>    208                  hv_cpu->clk_evt =3D kzalloc(sizeof(struct clock_e=
vent_device),
>    209                                            GFP_KERNEL);
>    210                  if (hv_cpu->clk_evt =3D=3D NULL) {
>                             ^^^^^^^^^^^^^^^^^^^^^^^
> Let's assume this fails on the first iteration through the loop.  We
> haven't memset the next cpu to NULL or allocated it.  But we loop over
> all the cpus in the error handling.  Since we didn't set everything to NU=
LL in
> hv_synic_free() then it seems like this could be a double free.  It's pos=
sible I
> am misreading the code, but either it's buggy or the memset() can be
> removed.
>=20
> This is a very typical bug for this style of error handling where we free
> things which were never allocated.

Thanks Dan! We will fix this issue soon.

K. Y
>=20
>    211                          pr_err("Unable to allocate clock event de=
vice\n");
>    212                          goto err;
>    213                  }
>=20
> regards,
> dan carpenter