LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] pvcalls-front: fixes incorrect error handling
@ 2018-11-22  2:07 Pan Bian
  2018-11-26 20:31 ` [Xen-devel] " Boris Ostrovsky
  2018-11-29 16:54 ` Juergen Gross
  0 siblings, 2 replies; 9+ messages in thread
From: Pan Bian @ 2018-11-22  2:07 UTC (permalink / raw)
  To: Boris Ostrovsky, Juergen Gross, Stefano Stabellini, xen-devel,
	linux-kernel

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 <bianpan2016@163.com>
---
 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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] pvcalls-front: fixes incorrect error handling
  2018-11-22  2:07 [PATCH] pvcalls-front: fixes incorrect error handling Pan Bian
@ 2018-11-26 20:31 ` Boris Ostrovsky
  2018-11-27  0:58   ` PanBian
  2018-11-29 16:54 ` Juergen Gross
  1 sibling, 1 reply; 9+ messages in thread
From: Boris Ostrovsky @ 2018-11-26 20:31 UTC (permalink / raw)
  To: Pan Bian, Juergen Gross, Stefano Stabellini, xen-devel, linux-kernel

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 <bianpan2016@163.com>
> ---
>  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;
>  }
>  



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] pvcalls-front: fixes incorrect error handling
  2018-11-26 20:31 ` [Xen-devel] " Boris Ostrovsky
@ 2018-11-27  0:58   ` PanBian
  2018-11-27 20:37     ` Stefano Stabellini
  0 siblings, 1 reply; 9+ messages in thread
From: PanBian @ 2018-11-27  0:58 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Juergen Gross, Stefano Stabellini, xen-devel, linux-kernel

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 <bianpan2016@163.com>
> > ---
> >  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;
> >  }
> >  
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] pvcalls-front: fixes incorrect error handling
  2018-11-27  0:58   ` PanBian
@ 2018-11-27 20:37     ` Stefano Stabellini
  2018-11-27 20:57       ` Boris Ostrovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Stefano Stabellini @ 2018-11-27 20:37 UTC (permalink / raw)
  To: PanBian
  Cc: Boris Ostrovsky, Juergen Gross, Stefano Stabellini, xen-devel,
	linux-kernel

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 <bianpan2016@163.com>
> > > ---
> > >  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;
> > >  }
> > >  
> > 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] pvcalls-front: fixes incorrect error handling
  2018-11-27 20:37     ` Stefano Stabellini
@ 2018-11-27 20:57       ` Boris Ostrovsky
  2018-11-27 21:08         ` Stefano Stabellini
  0 siblings, 1 reply; 9+ messages in thread
From: Boris Ostrovsky @ 2018-11-27 20:57 UTC (permalink / raw)
  To: Stefano Stabellini, PanBian; +Cc: Juergen Gross, xen-devel, linux-kernel

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 <bianpan2016@163.com>
>>>> ---
>>>>  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;
>>>>  }
>>>>  


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] pvcalls-front: fixes incorrect error handling
  2018-11-27 20:57       ` Boris Ostrovsky
@ 2018-11-27 21:08         ` Stefano Stabellini
  2018-11-27 22:47           ` Boris Ostrovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Stefano Stabellini @ 2018-11-27 21:08 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Stefano Stabellini, PanBian, Juergen Gross, xen-devel, linux-kernel

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 <bianpan2016@163.com>
> >>>> ---
> >>>>  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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] pvcalls-front: fixes incorrect error handling
  2018-11-27 21:08         ` Stefano Stabellini
@ 2018-11-27 22:47           ` Boris Ostrovsky
  2018-11-27 22:51             ` Stefano Stabellini
  0 siblings, 1 reply; 9+ messages in thread
From: Boris Ostrovsky @ 2018-11-27 22:47 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: PanBian, Juergen Gross, xen-devel, linux-kernel

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 <bianpan2016@163.com>
>>>>>> ---
>>>>>>  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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] pvcalls-front: fixes incorrect error handling
  2018-11-27 22:47           ` Boris Ostrovsky
@ 2018-11-27 22:51             ` Stefano Stabellini
  0 siblings, 0 replies; 9+ messages in thread
From: Stefano Stabellini @ 2018-11-27 22:51 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Stefano Stabellini, PanBian, Juergen Gross, xen-devel, linux-kernel

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 <bianpan2016@163.com>
> >>>>>> ---
> >>>>>>  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 <sstabellini@kernel.org>

 
> > All structs
> > are always initialized to zero. I don't think there are any issues.
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] pvcalls-front: fixes incorrect error handling
  2018-11-22  2:07 [PATCH] pvcalls-front: fixes incorrect error handling Pan Bian
  2018-11-26 20:31 ` [Xen-devel] " Boris Ostrovsky
@ 2018-11-29 16:54 ` Juergen Gross
  1 sibling, 0 replies; 9+ messages in thread
From: Juergen Gross @ 2018-11-29 16:54 UTC (permalink / raw)
  To: Pan Bian, Boris Ostrovsky, Stefano Stabellini, xen-devel, linux-kernel

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 <bianpan2016@163.com>

Pushed to xen/tip.git for-linus-4.20a


Juergen

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-11-29 16:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-22  2:07 [PATCH] pvcalls-front: fixes incorrect error handling Pan Bian
2018-11-26 20:31 ` [Xen-devel] " Boris Ostrovsky
2018-11-27  0:58   ` PanBian
2018-11-27 20:37     ` Stefano Stabellini
2018-11-27 20:57       ` Boris Ostrovsky
2018-11-27 21:08         ` Stefano Stabellini
2018-11-27 22:47           ` Boris Ostrovsky
2018-11-27 22:51             ` Stefano Stabellini
2018-11-29 16:54 ` Juergen Gross

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).