* [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 related [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).