viotape: Fix memory and semaphore leak

Submitted by Michael Buesch on July 18, 2009, 1:06 p.m.

Details

Message ID 200907181506.33499.mb@bu3sch.de
State Rejected
Delegated to: Benjamin Herrenschmidt
Headers show

Commit Message

Michael Buesch July 18, 2009, 1:06 p.m.
This patch fixes a memory and semaphore leak in the viotape driver's
char device write op. It leaks the DMA memory and the semaphore lock
in case the device was opened with O_NONBLOCK.

This patch is only compile tested, because I do not have the hardware.

Signed-off-by: Michael Buesch <mb@bu3sch.de>

---
 drivers/char/viotape.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Comments

Benjamin Herrenschmidt Aug. 13, 2009, 5 a.m.
On Sat, 2009-07-18 at 15:06 +0200, Michael Buesch wrote:
> This patch fixes a memory and semaphore leak in the viotape driver's
> char device write op. It leaks the DMA memory and the semaphore lock
> in case the device was opened with O_NONBLOCK.
> 
> This patch is only compile tested, because I do not have the hardware.
> 
> Signed-off-by: Michael Buesch <mb@bu3sch.de>

(going trough my backlog ...)

Thanks Michael, but I don't think that's right...

IE. We aren't waiting for the write to complete, which means that it can
be happening asynchronously, thus we must not free the DMA memory until
it has actually complete.

Now, if you look at vioHandleTapeEvent(), it does appear that when the
completion happens, the DMA memory will eventually be released and the
mutex up'ed. 

Or am I missing something ?

Cheers,
Ben.

> ---
>  drivers/char/viotape.c |   19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> --- linux-2.6.orig/drivers/char/viotape.c
> +++ linux-2.6/drivers/char/viotape.c
> @@ -401,30 +401,31 @@ static ssize_t viotap_write(struct file 
>  			viopath_targetinst(viopath_hostLp),
>  			(u64)(unsigned long)op, VIOVERSION << 16,
>  			((u64)devi.devno << 48) | op->dmaaddr, count, 0, 0);
>  	if (hvrc != HvLpEvent_Rc_Good) {
>  		printk(VIOTAPE_KERN_WARN "hv error on op %d\n",
>  				(int)hvrc);
>  		ret = -EIO;
>  		goto free_dma;
>  	}
>  
> -	if (noblock)
> -		return count;
> -
> -	wait_for_completion(&op->com);
> +	if (noblock) {
> +		ret = count;
> +	} else {
> +		wait_for_completion(&op->com);
>  
> -	if (op->rc)
> -		ret = tape_rc_to_errno(op->rc, "write", devi.devno);
> -	else {
> -		chg_state(devi.devno, VIOT_WRITING, file);
> -		ret = op->count;
> +		if (op->rc)
> +			ret = tape_rc_to_errno(op->rc, "write", devi.devno);
> +		else {
> +			chg_state(devi.devno, VIOT_WRITING, file);
> +			ret = op->count;
> +		}
>  	}
>  
>  free_dma:
>  	dma_free_coherent(op->dev, count, op->buffer, op->dmaaddr);
>  up_sem:
>  	up(&reqSem);
>  free_op:
>  	free_op_struct(op);
>  	return ret;
>  }
>
Michael Buesch Aug. 13, 2009, 1:18 p.m.
On Thursday 13 August 2009 07:00:03 Benjamin Herrenschmidt wrote:
> On Sat, 2009-07-18 at 15:06 +0200, Michael Buesch wrote:
> > This patch fixes a memory and semaphore leak in the viotape driver's
> > char device write op. It leaks the DMA memory and the semaphore lock
> > in case the device was opened with O_NONBLOCK.
> > 
> > This patch is only compile tested, because I do not have the hardware.
> > 
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> 
> (going trough my backlog ...)
> 
> Thanks Michael, but I don't think that's right...
> 
> IE. We aren't waiting for the write to complete, which means that it can
> be happening asynchronously, thus we must not free the DMA memory until
> it has actually complete.
> 
> Now, if you look at vioHandleTapeEvent(), it does appear that when the
> completion happens, the DMA memory will eventually be released and the
> mutex up'ed. 
> 
> Or am I missing something ?

I think you are right.

Patch hide | download patch | download mbox

--- linux-2.6.orig/drivers/char/viotape.c
+++ linux-2.6/drivers/char/viotape.c
@@ -401,30 +401,31 @@  static ssize_t viotap_write(struct file 
 			viopath_targetinst(viopath_hostLp),
 			(u64)(unsigned long)op, VIOVERSION << 16,
 			((u64)devi.devno << 48) | op->dmaaddr, count, 0, 0);
 	if (hvrc != HvLpEvent_Rc_Good) {
 		printk(VIOTAPE_KERN_WARN "hv error on op %d\n",
 				(int)hvrc);
 		ret = -EIO;
 		goto free_dma;
 	}
 
-	if (noblock)
-		return count;
-
-	wait_for_completion(&op->com);
+	if (noblock) {
+		ret = count;
+	} else {
+		wait_for_completion(&op->com);
 
-	if (op->rc)
-		ret = tape_rc_to_errno(op->rc, "write", devi.devno);
-	else {
-		chg_state(devi.devno, VIOT_WRITING, file);
-		ret = op->count;
+		if (op->rc)
+			ret = tape_rc_to_errno(op->rc, "write", devi.devno);
+		else {
+			chg_state(devi.devno, VIOT_WRITING, file);
+			ret = op->count;
+		}
 	}
 
 free_dma:
 	dma_free_coherent(op->dev, count, op->buffer, op->dmaaddr);
 up_sem:
 	up(&reqSem);
 free_op:
 	free_op_struct(op);
 	return ret;
 }