kfree() is incorrectly used to release the pages allocated by
__get_free_page() and __get_free_pages(). Use the matching deallocators
i.e., free_page() and free_pages(), respectively.
Signed-off-by: Pan Bian <[email protected]>
---
drivers/xen/pvcalls-front.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
index 2f11ca7..77224d8 100644
--- a/drivers/xen/pvcalls-front.c
+++ b/drivers/xen/pvcalls-front.c
@@ -385,8 +385,8 @@ static int create_active(struct sock_mapping *map, int *evtchn)
out_error:
if (*evtchn >= 0)
xenbus_free_evtchn(pvcalls_front_dev, *evtchn);
- kfree(map->active.data.in);
- kfree(map->active.ring);
+ free_pages((unsigned long)map->active.data.in, PVCALLS_RING_ORDER);
+ free_page((unsigned long)map->active.ring);
return ret;
}
--
2.7.4
On 11/21/18 9:07 PM, Pan Bian wrote:
> kfree() is incorrectly used to release the pages allocated by
> __get_free_page() and __get_free_pages(). Use the matching deallocators
> i.e., free_page() and free_pages(), respectively.
>
> Signed-off-by: Pan Bian <[email protected]>
> ---
> drivers/xen/pvcalls-front.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> index 2f11ca7..77224d8 100644
> --- a/drivers/xen/pvcalls-front.c
> +++ b/drivers/xen/pvcalls-front.c
> @@ -385,8 +385,8 @@ static int create_active(struct sock_mapping *map, int *evtchn)
> out_error:
> if (*evtchn >= 0)
> xenbus_free_evtchn(pvcalls_front_dev, *evtchn);
> - kfree(map->active.data.in);
> - kfree(map->active.ring);
> + free_pages((unsigned long)map->active.data.in, PVCALLS_RING_ORDER);
Is map->active.data.in guaranteed to be NULL when entering this routine?
-boris
> + free_page((unsigned long)map->active.ring);
> return ret;
> }
>
On Mon, Nov 26, 2018 at 03:31:39PM -0500, Boris Ostrovsky wrote:
> On 11/21/18 9:07 PM, Pan Bian wrote:
> > kfree() is incorrectly used to release the pages allocated by
> > __get_free_page() and __get_free_pages(). Use the matching deallocators
> > i.e., free_page() and free_pages(), respectively.
> >
> > Signed-off-by: Pan Bian <[email protected]>
> > ---
> > drivers/xen/pvcalls-front.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> > index 2f11ca7..77224d8 100644
> > --- a/drivers/xen/pvcalls-front.c
> > +++ b/drivers/xen/pvcalls-front.c
> > @@ -385,8 +385,8 @@ static int create_active(struct sock_mapping *map, int *evtchn)
> > out_error:
> > if (*evtchn >= 0)
> > xenbus_free_evtchn(pvcalls_front_dev, *evtchn);
> > - kfree(map->active.data.in);
> > - kfree(map->active.ring);
> > + free_pages((unsigned long)map->active.data.in, PVCALLS_RING_ORDER);
>
> Is map->active.data.in guaranteed to be NULL when entering this routine?
I am not sure yet. Sorry for that. I observed the mismatches between
__get_free_page and kfree, and submitted the patch.
But I think your consideration is reasonable. A better solution is to
directly free bytes, a local variable that holds __get_free_pages return
value. If you agree, I will rewrite the patch.
Thanks,
Pan
>
> -boris
>
> > + free_page((unsigned long)map->active.ring);
> > return ret;
> > }
> >
>
On Tue, 27 Nov 2018, PanBian wrote:
> On Mon, Nov 26, 2018 at 03:31:39PM -0500, Boris Ostrovsky wrote:
> > On 11/21/18 9:07 PM, Pan Bian wrote:
> > > kfree() is incorrectly used to release the pages allocated by
> > > __get_free_page() and __get_free_pages(). Use the matching deallocators
> > > i.e., free_page() and free_pages(), respectively.
> > >
> > > Signed-off-by: Pan Bian <[email protected]>
> > > ---
> > > drivers/xen/pvcalls-front.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> > > index 2f11ca7..77224d8 100644
> > > --- a/drivers/xen/pvcalls-front.c
> > > +++ b/drivers/xen/pvcalls-front.c
> > > @@ -385,8 +385,8 @@ static int create_active(struct sock_mapping *map, int *evtchn)
> > > out_error:
> > > if (*evtchn >= 0)
> > > xenbus_free_evtchn(pvcalls_front_dev, *evtchn);
> > > - kfree(map->active.data.in);
> > > - kfree(map->active.ring);
> > > + free_pages((unsigned long)map->active.data.in, PVCALLS_RING_ORDER);
> >
> > Is map->active.data.in guaranteed to be NULL when entering this routine?
>
> I am not sure yet. Sorry for that. I observed the mismatches between
> __get_free_page and kfree, and submitted the patch.
>
> But I think your consideration is reasonable. A better solution is to
> directly free bytes, a local variable that holds __get_free_pages return
> value. If you agree, I will rewrite the patch.
Like Boris said, map->active.ring and map->active.data.in are not
guaranteed to be NULL or != NULL here. For instance,map->active.ring can
be != NULL and map->active.data.in can be NULL. However, free_pages and
free_page should be able to cope with it, the same way that kfree is
able to cope with it?
> > -boris
> >
> > > + free_page((unsigned long)map->active.ring);
> > > return ret;
> > > }
> > >
> >
>
On 11/27/18 3:37 PM, Stefano Stabellini wrote:
> On Tue, 27 Nov 2018, PanBian wrote:
>> On Mon, Nov 26, 2018 at 03:31:39PM -0500, Boris Ostrovsky wrote:
>>> On 11/21/18 9:07 PM, Pan Bian wrote:
>>>> kfree() is incorrectly used to release the pages allocated by
>>>> __get_free_page() and __get_free_pages(). Use the matching deallocators
>>>> i.e., free_page() and free_pages(), respectively.
>>>>
>>>> Signed-off-by: Pan Bian <[email protected]>
>>>> ---
>>>> drivers/xen/pvcalls-front.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
>>>> index 2f11ca7..77224d8 100644
>>>> --- a/drivers/xen/pvcalls-front.c
>>>> +++ b/drivers/xen/pvcalls-front.c
>>>> @@ -385,8 +385,8 @@ static int create_active(struct sock_mapping *map, int *evtchn)
>>>> out_error:
>>>> if (*evtchn >= 0)
>>>> xenbus_free_evtchn(pvcalls_front_dev, *evtchn);
>>>> - kfree(map->active.data.in);
>>>> - kfree(map->active.ring);
>>>> + free_pages((unsigned long)map->active.data.in, PVCALLS_RING_ORDER);
>>> Is map->active.data.in guaranteed to be NULL when entering this routine?
>> I am not sure yet. Sorry for that. I observed the mismatches between
>> __get_free_page and kfree, and submitted the patch.
>>
>> But I think your consideration is reasonable. A better solution is to
>> directly free bytes, a local variable that holds __get_free_pages return
>> value. If you agree, I will rewrite the patch.
> Like Boris said, map->active.ring and map->active.data.in are not
> guaranteed to be NULL or != NULL here. For instance,map->active.ring can
> be != NULL and map->active.data.in can be NULL. However, free_pages and
> free_page should be able to cope with it, the same way that kfree is
> able to cope with it?
If map->active.data.in can be non-NULL on entry to this routine then I
think this has been a problem all along. Pan's suggestion to use bytes
for freeing is going to solve this (assuming bytes will be initialized
to NULL).
-boris
>
>>> -boris
>>>
>>>> + free_page((unsigned long)map->active.ring);
>>>> return ret;
>>>> }
>>>>
On Tue, 27 Nov 2018, Boris Ostrovsky wrote:
> On 11/27/18 3:37 PM, Stefano Stabellini wrote:
> > On Tue, 27 Nov 2018, PanBian wrote:
> >> On Mon, Nov 26, 2018 at 03:31:39PM -0500, Boris Ostrovsky wrote:
> >>> On 11/21/18 9:07 PM, Pan Bian wrote:
> >>>> kfree() is incorrectly used to release the pages allocated by
> >>>> __get_free_page() and __get_free_pages(). Use the matching deallocators
> >>>> i.e., free_page() and free_pages(), respectively.
> >>>>
> >>>> Signed-off-by: Pan Bian <[email protected]>
> >>>> ---
> >>>> drivers/xen/pvcalls-front.c | 4 ++--
> >>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> >>>> index 2f11ca7..77224d8 100644
> >>>> --- a/drivers/xen/pvcalls-front.c
> >>>> +++ b/drivers/xen/pvcalls-front.c
> >>>> @@ -385,8 +385,8 @@ static int create_active(struct sock_mapping *map, int *evtchn)
> >>>> out_error:
> >>>> if (*evtchn >= 0)
> >>>> xenbus_free_evtchn(pvcalls_front_dev, *evtchn);
> >>>> - kfree(map->active.data.in);
> >>>> - kfree(map->active.ring);
> >>>> + free_pages((unsigned long)map->active.data.in, PVCALLS_RING_ORDER);
> >>> Is map->active.data.in guaranteed to be NULL when entering this routine?
> >> I am not sure yet. Sorry for that. I observed the mismatches between
> >> __get_free_page and kfree, and submitted the patch.
> >>
> >> But I think your consideration is reasonable. A better solution is to
> >> directly free bytes, a local variable that holds __get_free_pages return
> >> value. If you agree, I will rewrite the patch.
> > Like Boris said, map->active.ring and map->active.data.in are not
> > guaranteed to be NULL or != NULL here. For instance,map->active.ring can
> > be != NULL and map->active.data.in can be NULL. However, free_pages and
> > free_page should be able to cope with it, the same way that kfree is
> > able to cope with it?
>
> If map->active.data.in can be non-NULL on entry to this routine then I
> think this has been a problem all along. Pan's suggestion to use bytes
> for freeing is going to solve this (assuming bytes will be initialized
> to NULL).
Why is it a problem? map->active.data.in and map->active.ring are only
!= NULL if they need to be freed. Otherwise, they are NULL. All structs
are always initialized to zero. I don't think there are any issues.
On 11/27/18 4:08 PM, Stefano Stabellini wrote:
> On Tue, 27 Nov 2018, Boris Ostrovsky wrote:
>> On 11/27/18 3:37 PM, Stefano Stabellini wrote:
>>> On Tue, 27 Nov 2018, PanBian wrote:
>>>> On Mon, Nov 26, 2018 at 03:31:39PM -0500, Boris Ostrovsky wrote:
>>>>> On 11/21/18 9:07 PM, Pan Bian wrote:
>>>>>> kfree() is incorrectly used to release the pages allocated by
>>>>>> __get_free_page() and __get_free_pages(). Use the matching deallocators
>>>>>> i.e., free_page() and free_pages(), respectively.
>>>>>>
>>>>>> Signed-off-by: Pan Bian <[email protected]>
>>>>>> ---
>>>>>> drivers/xen/pvcalls-front.c | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
>>>>>> index 2f11ca7..77224d8 100644
>>>>>> --- a/drivers/xen/pvcalls-front.c
>>>>>> +++ b/drivers/xen/pvcalls-front.c
>>>>>> @@ -385,8 +385,8 @@ static int create_active(struct sock_mapping *map, int *evtchn)
>>>>>> out_error:
>>>>>> if (*evtchn >= 0)
>>>>>> xenbus_free_evtchn(pvcalls_front_dev, *evtchn);
>>>>>> - kfree(map->active.data.in);
>>>>>> - kfree(map->active.ring);
>>>>>> + free_pages((unsigned long)map->active.data.in, PVCALLS_RING_ORDER);
>>>>> Is map->active.data.in guaranteed to be NULL when entering this routine?
>>>> I am not sure yet. Sorry for that. I observed the mismatches between
>>>> __get_free_page and kfree, and submitted the patch.
>>>>
>>>> But I think your consideration is reasonable. A better solution is to
>>>> directly free bytes, a local variable that holds __get_free_pages return
>>>> value. If you agree, I will rewrite the patch.
>>> Like Boris said, map->active.ring and map->active.data.in are not
>>> guaranteed to be NULL or != NULL here. For instance,map->active.ring can
>>> be != NULL and map->active.data.in can be NULL. However, free_pages and
>>> free_page should be able to cope with it, the same way that kfree is
>>> able to cope with it?
>> If map->active.data.in can be non-NULL on entry to this routine then I
>> think this has been a problem all along. Pan's suggestion to use bytes
>> for freeing is going to solve this (assuming bytes will be initialized
>> to NULL).
> Why is it a problem? map->active.data.in and map->active.ring are only
> != NULL if they need to be freed. Otherwise, they are NULL.
That was my question --- I wasn't sure about it, and I read your
previous message as if it was possible to be calling create_active()
with map->active.data.in pointing somewhere non-NULL.
If it is NULL *upon entry* to calling_create() then Pan's original patch
is fine.
-boris
> All structs
> are always initialized to zero. I don't think there are any issues.
On Tue, 27 Nov 2018, Boris Ostrovsky wrote:
> On 11/27/18 4:08 PM, Stefano Stabellini wrote:
> > On Tue, 27 Nov 2018, Boris Ostrovsky wrote:
> >> On 11/27/18 3:37 PM, Stefano Stabellini wrote:
> >>> On Tue, 27 Nov 2018, PanBian wrote:
> >>>> On Mon, Nov 26, 2018 at 03:31:39PM -0500, Boris Ostrovsky wrote:
> >>>>> On 11/21/18 9:07 PM, Pan Bian wrote:
> >>>>>> kfree() is incorrectly used to release the pages allocated by
> >>>>>> __get_free_page() and __get_free_pages(). Use the matching deallocators
> >>>>>> i.e., free_page() and free_pages(), respectively.
> >>>>>>
> >>>>>> Signed-off-by: Pan Bian <[email protected]>
> >>>>>> ---
> >>>>>> drivers/xen/pvcalls-front.c | 4 ++--
> >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> >>>>>> index 2f11ca7..77224d8 100644
> >>>>>> --- a/drivers/xen/pvcalls-front.c
> >>>>>> +++ b/drivers/xen/pvcalls-front.c
> >>>>>> @@ -385,8 +385,8 @@ static int create_active(struct sock_mapping *map, int *evtchn)
> >>>>>> out_error:
> >>>>>> if (*evtchn >= 0)
> >>>>>> xenbus_free_evtchn(pvcalls_front_dev, *evtchn);
> >>>>>> - kfree(map->active.data.in);
> >>>>>> - kfree(map->active.ring);
> >>>>>> + free_pages((unsigned long)map->active.data.in, PVCALLS_RING_ORDER);
> >>>>> Is map->active.data.in guaranteed to be NULL when entering this routine?
> >>>> I am not sure yet. Sorry for that. I observed the mismatches between
> >>>> __get_free_page and kfree, and submitted the patch.
> >>>>
> >>>> But I think your consideration is reasonable. A better solution is to
> >>>> directly free bytes, a local variable that holds __get_free_pages return
> >>>> value. If you agree, I will rewrite the patch.
> >>> Like Boris said, map->active.ring and map->active.data.in are not
> >>> guaranteed to be NULL or != NULL here. For instance,map->active.ring can
> >>> be != NULL and map->active.data.in can be NULL. However, free_pages and
> >>> free_page should be able to cope with it, the same way that kfree is
> >>> able to cope with it?
> >> If map->active.data.in can be non-NULL on entry to this routine then I
> >> think this has been a problem all along. Pan's suggestion to use bytes
> >> for freeing is going to solve this (assuming bytes will be initialized
> >> to NULL).
> > Why is it a problem? map->active.data.in and map->active.ring are only
> > != NULL if they need to be freed. Otherwise, they are NULL.
>
> That was my question --- I wasn't sure about it, and I read your
> previous message as if it was possible to be calling create_active()
> with map->active.data.in pointing somewhere non-NULL.
>
> If it is NULL *upon entry* to calling_create() then Pan's original patch
> is fine.
Right, I think it is fine too.
Reviewed-by: Stefano Stabellini <[email protected]>
> > All structs
> > are always initialized to zero. I don't think there are any issues.
>
On 22/11/2018 03:07, Pan Bian wrote:
> kfree() is incorrectly used to release the pages allocated by
> __get_free_page() and __get_free_pages(). Use the matching deallocators
> i.e., free_page() and free_pages(), respectively.
>
> Signed-off-by: Pan Bian <[email protected]>
Pushed to xen/tip.git for-linus-4.20a
Juergen