[PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling paths

[PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling paths

[Christophe JAILLET]

If 'vmbus_establish_gpadl()' fails, the (recv|send)_gpadl will not be
updated and 'hv_uio_cleanup()' in the error handling path will not be
able to free the corresponding buffer.

In such a case, we need to free the buffer explicitly.

Fixes: cdfa835c6e5e ("uio_hv_generic: defer opening vmbus until first use")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Before commit cdfa835c6e5e, the 'vfree' were done unconditionally
in 'hv_uio_cleanup()'.
So, another way for fixing the potential leak is to modify
'hv_uio_cleanup()' and revert to the previous behavior.

I don't know the underlying reason for this change so I don't know which is
the best way to fix this error handling path. Unless there is a specific
reason, changing 'hv_uio_cleanup()' could be better because it would keep
the error handling path of the probe cleaner, IMHO.
---
 drivers/uio/uio_hv_generic.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
index 0330ba99730e..eebc399f2cc7 100644
--- a/drivers/uio/uio_hv_generic.c
+++ b/drivers/uio/uio_hv_generic.c
@@ -296,8 +296,10 @@ hv_uio_probe(struct hv_device *dev,
 
 	ret = vmbus_establish_gpadl(channel, pdata->recv_buf,
 				    RECV_BUFFER_SIZE, &pdata->recv_gpadl);
-	if (ret)
+	if (ret) {
+		vfree(pdata->recv_buf);
 		goto fail_close;
+	}
 
 	/* put Global Physical Address Label in name */
 	snprintf(pdata->recv_name, sizeof(pdata->recv_name),
@@ -316,8 +318,10 @@ hv_uio_probe(struct hv_device *dev,
 
 	ret = vmbus_establish_gpadl(channel, pdata->send_buf,
 				    SEND_BUFFER_SIZE, &pdata->send_gpadl);
-	if (ret)
+	if (ret) {
+		vfree(pdata->send_buf);
 		goto fail_close;
+	}
 
 	snprintf(pdata->send_name, sizeof(pdata->send_name),
 		 "send:%u", pdata->send_gpadl);
-- 
2.30.2

[PATCH 2/2] uio_hv_generic: Fix another memory leak in error handling paths

[Christophe JAILLET]

Memory allocated by 'vmbus_alloc_ring()' at the beginning of the probe
function is never freed in the error handling path.

Add the missing 'vmbus_free_ring()' call.

Note that it is already freed in the .remove function.

Fixes: cdfa835c6e5e ("uio_hv_generic: defer opening vmbus until first use")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/uio/uio_hv_generic.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
index eebc399f2cc7..652fe2547587 100644
--- a/drivers/uio/uio_hv_generic.c
+++ b/drivers/uio/uio_hv_generic.c
@@ -291,7 +291,7 @@ hv_uio_probe(struct hv_device *dev,
 	pdata->recv_buf = vzalloc(RECV_BUFFER_SIZE);
 	if (pdata->recv_buf == NULL) {
 		ret = -ENOMEM;
-		goto fail_close;
+		goto fail_free_ring;
 	}
 
 	ret = vmbus_establish_gpadl(channel, pdata->recv_buf,
@@ -351,6 +351,8 @@ hv_uio_probe(struct hv_device *dev,
 
 fail_close:
 	hv_uio_cleanup(dev, pdata);
+fail_free_ring:
+	vmbus_free_ring(dev->channel);
 
 	return ret;
 }
-- 
2.30.2

Re: [PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling paths

[Wei Liu]

On Sun, May 09, 2021 at 09:13:03AM +0200, Christophe JAILLET wrote:
> If 'vmbus_establish_gpadl()' fails, the (recv|send)_gpadl will not be
> updated and 'hv_uio_cleanup()' in the error handling path will not be
> able to free the corresponding buffer.
> 
> In such a case, we need to free the buffer explicitly.
> 
> Fixes: cdfa835c6e5e ("uio_hv_generic: defer opening vmbus until first use")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Before commit cdfa835c6e5e, the 'vfree' were done unconditionally
> in 'hv_uio_cleanup()'.
> So, another way for fixing the potential leak is to modify
> 'hv_uio_cleanup()' and revert to the previous behavior.
> 

I think this is cleaner.

Stephen, you authored cdfa835c6e5e. What do you think?

Christophe, OOI how did you discover these issues?

> I don't know the underlying reason for this change so I don't know which is
> the best way to fix this error handling path. Unless there is a specific
> reason, changing 'hv_uio_cleanup()' could be better because it would keep
> the error handling path of the probe cleaner, IMHO.
> ---
>  drivers/uio/uio_hv_generic.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index 0330ba99730e..eebc399f2cc7 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -296,8 +296,10 @@ hv_uio_probe(struct hv_device *dev,
>  
>  	ret = vmbus_establish_gpadl(channel, pdata->recv_buf,
>  				    RECV_BUFFER_SIZE, &pdata->recv_gpadl);
> -	if (ret)
> +	if (ret) {
> +		vfree(pdata->recv_buf);
>  		goto fail_close;
> +	}
>  
>  	/* put Global Physical Address Label in name */
>  	snprintf(pdata->recv_name, sizeof(pdata->recv_name),
> @@ -316,8 +318,10 @@ hv_uio_probe(struct hv_device *dev,
>  
>  	ret = vmbus_establish_gpadl(channel, pdata->send_buf,
>  				    SEND_BUFFER_SIZE, &pdata->send_gpadl);
> -	if (ret)
> +	if (ret) {
> +		vfree(pdata->send_buf);
>  		goto fail_close;
> +	}
>  
>  	snprintf(pdata->send_name, sizeof(pdata->send_name),
>  		 "send:%u", pdata->send_gpadl);
> -- 
> 2.30.2
> 

Re: [PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling paths

[Christophe JAILLET]

Le 11/05/2021 à 11:52, Wei Liu a écrit :
>> Before commit cdfa835c6e5e, the 'vfree' were done unconditionally
>> in 'hv_uio_cleanup()'.
>> So, another way for fixing the potential leak is to modify
>> 'hv_uio_cleanup()' and revert to the previous behavior.
>>
> 
> I think this is cleaner.

Agreed

> 
> Stephen, you authored cdfa835c6e5e. What do you think?
> 
> Christophe, OOI how did you discover these issues?

I use an ugly coccinelle script which tries to spot functions called in 
the .remove function of a driver, but which is not in the error handling 
path of the .probe

It is way to verbose and gives too much false positive, but I manage to 
spot a few things with it.
Here it is, should it be useful for someone, or if anyone want to 
improve it.

The idea is:
    - find the probe and remove function
    - find a function (f1) which is not the first of the remove function 
(the first is likely the last in the probe that doesn't need to be 
undone in the probe error handling path)
    - try to filter with likely false positive pattern
    - search in the probe if this function is also called
    - if not, then it is a candidate.

CJ

---------------------------------

// Find the probe and remove functions
@platform@
identifier type, p, probefn, removefn;
@@
struct type p = {
   .probe = probefn,
   .remove = removefn,
};


// In the remove function, find a function that is called
@rem depends on platform@
identifier platform.removefn, firstFct, lastFct;
identifier f1 !~ 
"(pr_.*|dev_.*|cancel_work.*|cancel_delayed_work.*|tasklet_kill|list_del|.*unregister.*|.*deregister.*|spin_.*|flush_.*|pm_runtime_.*)";
@@
removefn(...) {
(
   <+...
   firstFct(...);
   f1(...);
   ...+>
|
   <+...
   fXXX1(...);
   lastFct(...);
   ...+>
)
}


// Check if this function is also called in the probe error path
@prb depends on rem@
identifier platform.probefn, platform.removefn, rem.f1;
@@
probefn(...) {
(
   <+...
     f1(...);
   ...+>
|
   <+...
     removefn(...);
   ...+>
)
}


// If not, this function is likely missing from the probe error path
@prb3 depends on !prb@
identifier platform.removefn, rem.f1;
@@
*removefn(...) {
   <+...
*  f1(...);
   ...+>
}

Re: [PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling paths

[Wei Liu]

On Tue, May 11, 2021 at 08:18:23PM +0200, Christophe JAILLET wrote:
> Le 11/05/2021 à 11:52, Wei Liu a écrit :
> > > Before commit cdfa835c6e5e, the 'vfree' were done unconditionally
> > > in 'hv_uio_cleanup()'.
> > > So, another way for fixing the potential leak is to modify
> > > 'hv_uio_cleanup()' and revert to the previous behavior.
> > > 
> > 
> > I think this is cleaner.
> 
> Agreed

Stephen, ping?

If I don't hear back from you, I think Christophe should move ahead with
modifying hv_uio_cleanup.

Wei.

Re: [PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling paths

[Wei Liu]

Cc Long Li.

Long, Stephen suggested I check with you. Do you have any opinion?

Wei.

On Tue, May 11, 2021 at 09:52:27AM +0000, Wei Liu wrote:
> On Sun, May 09, 2021 at 09:13:03AM +0200, Christophe JAILLET wrote:
> > If 'vmbus_establish_gpadl()' fails, the (recv|send)_gpadl will not be
> > updated and 'hv_uio_cleanup()' in the error handling path will not be
> > able to free the corresponding buffer.
> > 
> > In such a case, we need to free the buffer explicitly.
> > 
> > Fixes: cdfa835c6e5e ("uio_hv_generic: defer opening vmbus until first use")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > Before commit cdfa835c6e5e, the 'vfree' were done unconditionally
> > in 'hv_uio_cleanup()'.
> > So, another way for fixing the potential leak is to modify
> > 'hv_uio_cleanup()' and revert to the previous behavior.
> > 
> 
> I think this is cleaner.
> 
> Stephen, you authored cdfa835c6e5e. What do you think?
> 
> Christophe, OOI how did you discover these issues?
> 
> > I don't know the underlying reason for this change so I don't know which is
> > the best way to fix this error handling path. Unless there is a specific
> > reason, changing 'hv_uio_cleanup()' could be better because it would keep
> > the error handling path of the probe cleaner, IMHO.
> > ---
> >  drivers/uio/uio_hv_generic.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> > index 0330ba99730e..eebc399f2cc7 100644
> > --- a/drivers/uio/uio_hv_generic.c
> > +++ b/drivers/uio/uio_hv_generic.c
> > @@ -296,8 +296,10 @@ hv_uio_probe(struct hv_device *dev,
> >  
> >  	ret = vmbus_establish_gpadl(channel, pdata->recv_buf,
> >  				    RECV_BUFFER_SIZE, &pdata->recv_gpadl);
> > -	if (ret)
> > +	if (ret) {
> > +		vfree(pdata->recv_buf);
> >  		goto fail_close;
> > +	}
> >  
> >  	/* put Global Physical Address Label in name */
> >  	snprintf(pdata->recv_name, sizeof(pdata->recv_name),
> > @@ -316,8 +318,10 @@ hv_uio_probe(struct hv_device *dev,
> >  
> >  	ret = vmbus_establish_gpadl(channel, pdata->send_buf,
> >  				    SEND_BUFFER_SIZE, &pdata->send_gpadl);
> > -	if (ret)
> > +	if (ret) {
> > +		vfree(pdata->send_buf);
> >  		goto fail_close;
> > +	}
> >  
> >  	snprintf(pdata->send_name, sizeof(pdata->send_name),
> >  		 "send:%u", pdata->send_gpadl);
> > -- 
> > 2.30.2
> > 

Re: [PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling paths

[Christophe JAILLET]

Le 15/05/2021 à 18:09, Wei Liu a écrit :
> On Tue, May 11, 2021 at 08:18:23PM +0200, Christophe JAILLET wrote:
>> Le 11/05/2021 à 11:52, Wei Liu a écrit :
>>>> Before commit cdfa835c6e5e, the 'vfree' were done unconditionally
>>>> in 'hv_uio_cleanup()'.
>>>> So, another way for fixing the potential leak is to modify
>>>> 'hv_uio_cleanup()' and revert to the previous behavior.
>>>>
>>>
>>> I think this is cleaner.
>>
>> Agreed
> 
> Stephen, ping?
> 
> If I don't hear back from you, I think Christophe should move ahead with
> modifying hv_uio_cleanup.
> 
> Wei.
> 

Hi,
just for your information, it has already been picked by Greg KH (see [1]).
If the cleaner solution is preferred, it will be built on top of this patch.

CJ

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/uio/uio_hv_generic.c?id=3ee098f96b8b6c1a98f7f97915f8873164e6af9d

RE: [PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling paths

[Long Li]

> Subject: Re: [PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling
> paths
> 
> Cc Long Li.
> 
> Long, Stephen suggested I check with you. Do you have any opinion?
> 
> Wei.
> 
> On Tue, May 11, 2021 at 09:52:27AM +0000, Wei Liu wrote:
> > On Sun, May 09, 2021 at 09:13:03AM +0200, Christophe JAILLET wrote:
> > > If 'vmbus_establish_gpadl()' fails, the (recv|send)_gpadl will not
> > > be updated and 'hv_uio_cleanup()' in the error handling path will
> > > not be able to free the corresponding buffer.
> > >
> > > In such a case, we need to free the buffer explicitly.
> > >
> > > Fixes: cdfa835c6e5e ("uio_hv_generic: defer opening vmbus until
> > > first use")
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > Before commit cdfa835c6e5e, the 'vfree' were done unconditionally in
> > > 'hv_uio_cleanup()'.
> > > So, another way for fixing the potential leak is to modify
> > > 'hv_uio_cleanup()' and revert to the previous behavior.
> > >
> >
> > I think this is cleaner.

Christophe has mentioned that the patch is already picked up by Greg KH.

I think both approaches are correct. We can go with this one.

> >
> > Stephen, you authored cdfa835c6e5e. What do you think?
> >
> > Christophe, OOI how did you discover these issues?
> >
> > > I don't know the underlying reason for this change so I don't know
> > > which is the best way to fix this error handling path. Unless there
> > > is a specific reason, changing 'hv_uio_cleanup()' could be better
> > > because it would keep the error handling path of the probe cleaner,
> IMHO.
> > > ---
> > >  drivers/uio/uio_hv_generic.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/uio/uio_hv_generic.c
> > > b/drivers/uio/uio_hv_generic.c index 0330ba99730e..eebc399f2cc7
> > > 100644
> > > --- a/drivers/uio/uio_hv_generic.c
> > > +++ b/drivers/uio/uio_hv_generic.c
> > > @@ -296,8 +296,10 @@ hv_uio_probe(struct hv_device *dev,
> > >
> > >  	ret = vmbus_establish_gpadl(channel, pdata->recv_buf,
> > >  				    RECV_BUFFER_SIZE, &pdata->recv_gpadl);
> > > -	if (ret)
> > > +	if (ret) {
> > > +		vfree(pdata->recv_buf);
> > >  		goto fail_close;
> > > +	}
> > >
> > >  	/* put Global Physical Address Label in name */
> > >  	snprintf(pdata->recv_name, sizeof(pdata->recv_name), @@ -316,8
> > > +318,10 @@ hv_uio_probe(struct hv_device *dev,
> > >
> > >  	ret = vmbus_establish_gpadl(channel, pdata->send_buf,
> > >  				    SEND_BUFFER_SIZE, &pdata->send_gpadl);
> > > -	if (ret)
> > > +	if (ret) {
> > > +		vfree(pdata->send_buf);
> > >  		goto fail_close;
> > > +	}
> > >
> > >  	snprintf(pdata->send_name, sizeof(pdata->send_name),
> > >  		 "send:%u", pdata->send_gpadl);
> > > --
> > > 2.30.2
> > >

Re: [PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling paths

[Wei Liu]

On Tue, May 18, 2021 at 09:50:17PM +0000, Long Li wrote:
> > Subject: Re: [PATCH 1/2] uio_hv_generic: Fix a memory leak in error handling
> > paths
> > 
> > Cc Long Li.
> > 
> > Long, Stephen suggested I check with you. Do you have any opinion?
> > 
> > Wei.
> > 
> > On Tue, May 11, 2021 at 09:52:27AM +0000, Wei Liu wrote:
> > > On Sun, May 09, 2021 at 09:13:03AM +0200, Christophe JAILLET wrote:
> > > > If 'vmbus_establish_gpadl()' fails, the (recv|send)_gpadl will not
> > > > be updated and 'hv_uio_cleanup()' in the error handling path will
> > > > not be able to free the corresponding buffer.
> > > >
> > > > In such a case, we need to free the buffer explicitly.
> > > >
> > > > Fixes: cdfa835c6e5e ("uio_hv_generic: defer opening vmbus until
> > > > first use")
> > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > > ---
> > > > Before commit cdfa835c6e5e, the 'vfree' were done unconditionally in
> > > > 'hv_uio_cleanup()'.
> > > > So, another way for fixing the potential leak is to modify
> > > > 'hv_uio_cleanup()' and revert to the previous behavior.
> > > >
> > >
> > > I think this is cleaner.
> 
> Christophe has mentioned that the patch is already picked up by Greg KH.
> 
> I think both approaches are correct. We can go with this one.

OK. That's fine.

Wei.