All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch] IR: ene_ir: problems in unwinding on probe
@ 2010-08-12  7:46 ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2010-08-12  7:46 UTC (permalink / raw)
  To: Maxim Levitsky; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors

There were a couple issues here.  If the allocation failed for "dev"
then it would lead to a NULL dereference.  If request_irq() or
request_region() failed it would release the irq and the region even
though they were not successfully aquired.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/media/IR/ene_ir.c b/drivers/media/IR/ene_ir.c
index 5447750..8e5e964 100644
--- a/drivers/media/IR/ene_ir.c
+++ b/drivers/media/IR/ene_ir.c
@@ -781,21 +781,24 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 
 	/* allocate memory */
 	input_dev = input_allocate_device();
+	if (!input_dev)
+		goto err_out;
 	ir_props = kzalloc(sizeof(struct ir_dev_props), GFP_KERNEL);
+	if (!ir_props)
+		goto err_input_dev;
 	dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
-
-	if (!input_dev || !ir_props || !dev)
-		goto error;
+	if (!dev)
+		goto err_ir_props;
 
 	/* validate resources */
 	error = -ENODEV;
 
 	if (!pnp_port_valid(pnp_dev, 0) ||
 	    pnp_port_len(pnp_dev, 0) < ENE_MAX_IO)
-		goto error;
+		goto err_dev;
 
 	if (!pnp_irq_valid(pnp_dev, 0))
-		goto error;
+		goto err_dev;
 
 	dev->hw_io = pnp_port_start(pnp_dev, 0);
 	dev->irq = pnp_irq(pnp_dev, 0);
@@ -804,11 +807,11 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	/* claim the resources */
 	error = -EBUSY;
 	if (!request_region(dev->hw_io, ENE_MAX_IO, ENE_DRIVER_NAME))
-		goto error;
+		goto err_dev;
 
 	if (request_irq(dev->irq, ene_isr,
 			IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev))
-		goto error;
+		goto err_region;
 
 	pnp_set_drvdata(pnp_dev, dev);
 	dev->pnp_dev = pnp_dev;
@@ -816,7 +819,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	/* detect hardware version and features */
 	error = ene_hw_detect(dev);
 	if (error)
-		goto error;
+		goto err_irq;
 
 	ene_setup_settings(dev);
 
@@ -889,20 +892,22 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	error = -ENODEV;
 	if (ir_input_register(input_dev, RC_MAP_RC6_MCE, ir_props,
 							ENE_DRIVER_NAME))
-		goto error;
-
+		goto err_irq;
 
 	ene_printk(KERN_NOTICE, "driver has been succesfully loaded\n");
 	return 0;
-error:
-	if (dev->irq)
-		free_irq(dev->irq, dev);
-	if (dev->hw_io)
-		release_region(dev->hw_io, ENE_MAX_IO);
 
-	input_free_device(input_dev);
-	kfree(ir_props);
+err_irq:
+	free_irq(dev->irq, dev);
+err_region:
+	release_region(dev->hw_io, ENE_MAX_IO);
+err_dev:
 	kfree(dev);
+err_ir_props:
+	kfree(ir_props);
+err_input_dev:
+	input_free_device(input_dev);
+err_out:
 	return error;
 }
 

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

* [patch] IR: ene_ir: problems in unwinding on probe
@ 2010-08-12  7:46 ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2010-08-12  7:46 UTC (permalink / raw)
  To: Maxim Levitsky; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors

There were a couple issues here.  If the allocation failed for "dev"
then it would lead to a NULL dereference.  If request_irq() or
request_region() failed it would release the irq and the region even
though they were not successfully aquired.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/media/IR/ene_ir.c b/drivers/media/IR/ene_ir.c
index 5447750..8e5e964 100644
--- a/drivers/media/IR/ene_ir.c
+++ b/drivers/media/IR/ene_ir.c
@@ -781,21 +781,24 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 
 	/* allocate memory */
 	input_dev = input_allocate_device();
+	if (!input_dev)
+		goto err_out;
 	ir_props = kzalloc(sizeof(struct ir_dev_props), GFP_KERNEL);
+	if (!ir_props)
+		goto err_input_dev;
 	dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
-
-	if (!input_dev || !ir_props || !dev)
-		goto error;
+	if (!dev)
+		goto err_ir_props;
 
 	/* validate resources */
 	error = -ENODEV;
 
 	if (!pnp_port_valid(pnp_dev, 0) ||
 	    pnp_port_len(pnp_dev, 0) < ENE_MAX_IO)
-		goto error;
+		goto err_dev;
 
 	if (!pnp_irq_valid(pnp_dev, 0))
-		goto error;
+		goto err_dev;
 
 	dev->hw_io = pnp_port_start(pnp_dev, 0);
 	dev->irq = pnp_irq(pnp_dev, 0);
@@ -804,11 +807,11 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	/* claim the resources */
 	error = -EBUSY;
 	if (!request_region(dev->hw_io, ENE_MAX_IO, ENE_DRIVER_NAME))
-		goto error;
+		goto err_dev;
 
 	if (request_irq(dev->irq, ene_isr,
 			IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev))
-		goto error;
+		goto err_region;
 
 	pnp_set_drvdata(pnp_dev, dev);
 	dev->pnp_dev = pnp_dev;
@@ -816,7 +819,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	/* detect hardware version and features */
 	error = ene_hw_detect(dev);
 	if (error)
-		goto error;
+		goto err_irq;
 
 	ene_setup_settings(dev);
 
@@ -889,20 +892,22 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	error = -ENODEV;
 	if (ir_input_register(input_dev, RC_MAP_RC6_MCE, ir_props,
 							ENE_DRIVER_NAME))
-		goto error;
-
+		goto err_irq;
 
 	ene_printk(KERN_NOTICE, "driver has been succesfully loaded\n");
 	return 0;
-error:
-	if (dev->irq)
-		free_irq(dev->irq, dev);
-	if (dev->hw_io)
-		release_region(dev->hw_io, ENE_MAX_IO);
 
-	input_free_device(input_dev);
-	kfree(ir_props);
+err_irq:
+	free_irq(dev->irq, dev);
+err_region:
+	release_region(dev->hw_io, ENE_MAX_IO);
+err_dev:
 	kfree(dev);
+err_ir_props:
+	kfree(ir_props);
+err_input_dev:
+	input_free_device(input_dev);
+err_out:
 	return error;
 }
 

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

* Re: [patch] IR: ene_ir: problems in unwinding on probe
  2010-08-12  7:46 ` Dan Carpenter
@ 2010-08-12 14:35   ` Maxim Levitsky
  -1 siblings, 0 replies; 8+ messages in thread
From: Maxim Levitsky @ 2010-08-12 14:35 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors

On Thu, 2010-08-12 at 09:46 +0200, Dan Carpenter wrote: 
> There were a couple issues here.  If the allocation failed for "dev"
> then it would lead to a NULL dereference.  If request_irq() or
> request_region() failed it would release the irq and the region even
> though they were not successfully aquired.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

I don't think this is needed.
I just alloc all the stuff, and if one of allocations fail, I free them
all. {k}free on NULL pointer is perfectly legal.

Same about IO and IRQ.
IRQ0 and IO 0 isn't valid, and I do test that in error path.


Best regards,
Maxim Levitsky


> 
> diff --git a/drivers/media/IR/ene_ir.c b/drivers/media/IR/ene_ir.c
> index 5447750..8e5e964 100644
> --- a/drivers/media/IR/ene_ir.c
> +++ b/drivers/media/IR/ene_ir.c
> @@ -781,21 +781,24 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
>  
>  	/* allocate memory */
>  	input_dev = input_allocate_device();
> +	if (!input_dev)
> +		goto err_out;
>  	ir_props = kzalloc(sizeof(struct ir_dev_props), GFP_KERNEL);
> +	if (!ir_props)
> +		goto err_input_dev;
>  	dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
> -
> -	if (!input_dev || !ir_props || !dev)
> -		goto error;
> +	if (!dev)
> +		goto err_ir_props;
>  
>  	/* validate resources */
>  	error = -ENODEV;
>  
>  	if (!pnp_port_valid(pnp_dev, 0) ||
>  	    pnp_port_len(pnp_dev, 0) < ENE_MAX_IO)
> -		goto error;
> +		goto err_dev;
>  
>  	if (!pnp_irq_valid(pnp_dev, 0))
> -		goto error;
> +		goto err_dev;
>  
>  	dev->hw_io = pnp_port_start(pnp_dev, 0);
>  	dev->irq = pnp_irq(pnp_dev, 0);
> @@ -804,11 +807,11 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
>  	/* claim the resources */
>  	error = -EBUSY;
>  	if (!request_region(dev->hw_io, ENE_MAX_IO, ENE_DRIVER_NAME))
> -		goto error;
> +		goto err_dev;
>  
>  	if (request_irq(dev->irq, ene_isr,
>  			IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev))
> -		goto error;
> +		goto err_region;
>  
>  	pnp_set_drvdata(pnp_dev, dev);
>  	dev->pnp_dev = pnp_dev;
> @@ -816,7 +819,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
>  	/* detect hardware version and features */
>  	error = ene_hw_detect(dev);
>  	if (error)
> -		goto error;
> +		goto err_irq;
>  
>  	ene_setup_settings(dev);
>  
> @@ -889,20 +892,22 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
>  	error = -ENODEV;
>  	if (ir_input_register(input_dev, RC_MAP_RC6_MCE, ir_props,
>  							ENE_DRIVER_NAME))
> -		goto error;
> -
> +		goto err_irq;
>  
>  	ene_printk(KERN_NOTICE, "driver has been succesfully loaded\n");
>  	return 0;
> -error:
> -	if (dev->irq)
> -		free_irq(dev->irq, dev);
> -	if (dev->hw_io)
> -		release_region(dev->hw_io, ENE_MAX_IO);
>  
> -	input_free_device(input_dev);
> -	kfree(ir_props);
> +err_irq:
> +	free_irq(dev->irq, dev);
> +err_region:
> +	release_region(dev->hw_io, ENE_MAX_IO);
> +err_dev:
>  	kfree(dev);
> +err_ir_props:
> +	kfree(ir_props);
> +err_input_dev:
> +	input_free_device(input_dev);
> +err_out:
>  	return error;
>  }
>  



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

* Re: [patch] IR: ene_ir: problems in unwinding on probe
@ 2010-08-12 14:35   ` Maxim Levitsky
  0 siblings, 0 replies; 8+ messages in thread
From: Maxim Levitsky @ 2010-08-12 14:35 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors

On Thu, 2010-08-12 at 09:46 +0200, Dan Carpenter wrote: 
> There were a couple issues here.  If the allocation failed for "dev"
> then it would lead to a NULL dereference.  If request_irq() or
> request_region() failed it would release the irq and the region even
> though they were not successfully aquired.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

I don't think this is needed.
I just alloc all the stuff, and if one of allocations fail, I free them
all. {k}free on NULL pointer is perfectly legal.

Same about IO and IRQ.
IRQ0 and IO 0 isn't valid, and I do test that in error path.


Best regards,
Maxim Levitsky


> 
> diff --git a/drivers/media/IR/ene_ir.c b/drivers/media/IR/ene_ir.c
> index 5447750..8e5e964 100644
> --- a/drivers/media/IR/ene_ir.c
> +++ b/drivers/media/IR/ene_ir.c
> @@ -781,21 +781,24 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
>  
>  	/* allocate memory */
>  	input_dev = input_allocate_device();
> +	if (!input_dev)
> +		goto err_out;
>  	ir_props = kzalloc(sizeof(struct ir_dev_props), GFP_KERNEL);
> +	if (!ir_props)
> +		goto err_input_dev;
>  	dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
> -
> -	if (!input_dev || !ir_props || !dev)
> -		goto error;
> +	if (!dev)
> +		goto err_ir_props;
>  
>  	/* validate resources */
>  	error = -ENODEV;
>  
>  	if (!pnp_port_valid(pnp_dev, 0) ||
>  	    pnp_port_len(pnp_dev, 0) < ENE_MAX_IO)
> -		goto error;
> +		goto err_dev;
>  
>  	if (!pnp_irq_valid(pnp_dev, 0))
> -		goto error;
> +		goto err_dev;
>  
>  	dev->hw_io = pnp_port_start(pnp_dev, 0);
>  	dev->irq = pnp_irq(pnp_dev, 0);
> @@ -804,11 +807,11 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
>  	/* claim the resources */
>  	error = -EBUSY;
>  	if (!request_region(dev->hw_io, ENE_MAX_IO, ENE_DRIVER_NAME))
> -		goto error;
> +		goto err_dev;
>  
>  	if (request_irq(dev->irq, ene_isr,
>  			IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev))
> -		goto error;
> +		goto err_region;
>  
>  	pnp_set_drvdata(pnp_dev, dev);
>  	dev->pnp_dev = pnp_dev;
> @@ -816,7 +819,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
>  	/* detect hardware version and features */
>  	error = ene_hw_detect(dev);
>  	if (error)
> -		goto error;
> +		goto err_irq;
>  
>  	ene_setup_settings(dev);
>  
> @@ -889,20 +892,22 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
>  	error = -ENODEV;
>  	if (ir_input_register(input_dev, RC_MAP_RC6_MCE, ir_props,
>  							ENE_DRIVER_NAME))
> -		goto error;
> -
> +		goto err_irq;
>  
>  	ene_printk(KERN_NOTICE, "driver has been succesfully loaded\n");
>  	return 0;
> -error:
> -	if (dev->irq)
> -		free_irq(dev->irq, dev);
> -	if (dev->hw_io)
> -		release_region(dev->hw_io, ENE_MAX_IO);
>  
> -	input_free_device(input_dev);
> -	kfree(ir_props);
> +err_irq:
> +	free_irq(dev->irq, dev);
> +err_region:
> +	release_region(dev->hw_io, ENE_MAX_IO);
> +err_dev:
>  	kfree(dev);
> +err_ir_props:
> +	kfree(ir_props);
> +err_input_dev:
> +	input_free_device(input_dev);
> +err_out:
>  	return error;
>  }
>  



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

* Re: [patch] IR: ene_ir: problems in unwinding on probe
  2010-08-12 14:35   ` Maxim Levitsky
@ 2010-08-12 16:19     ` Dan Carpenter
  -1 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2010-08-12 16:19 UTC (permalink / raw)
  To: Maxim Levitsky; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors

On Thu, Aug 12, 2010 at 05:35:04PM +0300, Maxim Levitsky wrote:
> On Thu, 2010-08-12 at 09:46 +0200, Dan Carpenter wrote: 
> > There were a couple issues here.  If the allocation failed for "dev"
> > then it would lead to a NULL dereference.  If request_irq() or
> > request_region() failed it would release the irq and the region even
> > though they were not successfully aquired.
> > 
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> I don't think this is needed.
> I just alloc all the stuff, and if one of allocations fail, I free them
> all. {k}free on NULL pointer is perfectly legal.
> 
> Same about IO and IRQ.
> IRQ0 and IO 0 isn't valid, and I do test that in error path.
> 
>

Here is the original code:

Here is where we set "dev".

   785          dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
   786  
   787          if (!input_dev || !ir_props || !dev)
   788                  goto error;

	[snip]

Here is where we set the IO and IRQ:

   800          dev->hw_io = pnp_port_start(pnp_dev, 0);
   801          dev->irq = pnp_irq(pnp_dev, 0);

	[snip]

Here is where the request_region() and request_irq() are.

   806          if (!request_region(dev->hw_io, ENE_MAX_IO, ENE_DRIVER_NAME))
   807                  goto error;
   808  
   809          if (request_irq(dev->irq, ene_isr, 
   810                          IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev))
   811                  goto error;

	[snip]

Here is the error label:

   897  error:
   898          if (dev->irq)
		    ^^^^^^^^

	Oops!  The allocation of dev failed and this is a NULL
	dereference.

   899                  free_irq(dev->irq, dev);

	Oops!  Request region failed and dev->irq is non-zero but
	request_irq() hasn't been called.

   900          if (dev->hw_io)
   901                  release_region(dev->hw_io, ENE_MAX_IO);

	Oops! dev->hw_io is non-zero but request_region() failed and so
	we just released someone else's region.


Hehe.  :P

regards,
dan carpenter


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

* Re: [patch] IR: ene_ir: problems in unwinding on probe
@ 2010-08-12 16:19     ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2010-08-12 16:19 UTC (permalink / raw)
  To: Maxim Levitsky; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors

On Thu, Aug 12, 2010 at 05:35:04PM +0300, Maxim Levitsky wrote:
> On Thu, 2010-08-12 at 09:46 +0200, Dan Carpenter wrote: 
> > There were a couple issues here.  If the allocation failed for "dev"
> > then it would lead to a NULL dereference.  If request_irq() or
> > request_region() failed it would release the irq and the region even
> > though they were not successfully aquired.
> > 
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> I don't think this is needed.
> I just alloc all the stuff, and if one of allocations fail, I free them
> all. {k}free on NULL pointer is perfectly legal.
> 
> Same about IO and IRQ.
> IRQ0 and IO 0 isn't valid, and I do test that in error path.
> 
>

Here is the original code:

Here is where we set "dev".

   785          dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
   786  
   787          if (!input_dev || !ir_props || !dev)
   788                  goto error;

	[snip]

Here is where we set the IO and IRQ:

   800          dev->hw_io = pnp_port_start(pnp_dev, 0);
   801          dev->irq = pnp_irq(pnp_dev, 0);

	[snip]

Here is where the request_region() and request_irq() are.

   806          if (!request_region(dev->hw_io, ENE_MAX_IO, ENE_DRIVER_NAME))
   807                  goto error;
   808  
   809          if (request_irq(dev->irq, ene_isr, 
   810                          IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev))
   811                  goto error;

	[snip]

Here is the error label:

   897  error:
   898          if (dev->irq)
		    ^^^^^^^^

	Oops!  The allocation of dev failed and this is a NULL
	dereference.

   899                  free_irq(dev->irq, dev);

	Oops!  Request region failed and dev->irq is non-zero but
	request_irq() hasn't been called.

   900          if (dev->hw_io)
   901                  release_region(dev->hw_io, ENE_MAX_IO);

	Oops! dev->hw_io is non-zero but request_region() failed and so
	we just released someone else's region.


Hehe.  :P

regards,
dan carpenter


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

* Re: [patch] IR: ene_ir: problems in unwinding on probe
  2010-08-12 16:19     ` Dan Carpenter
@ 2010-08-12 16:42       ` Maxim Levitsky
  -1 siblings, 0 replies; 8+ messages in thread
From: Maxim Levitsky @ 2010-08-12 16:42 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors

On Thu, 2010-08-12 at 18:19 +0200, Dan Carpenter wrote: 
> On Thu, Aug 12, 2010 at 05:35:04PM +0300, Maxim Levitsky wrote:
> > On Thu, 2010-08-12 at 09:46 +0200, Dan Carpenter wrote: 
> > > There were a couple issues here.  If the allocation failed for "dev"
> > > then it would lead to a NULL dereference.  If request_irq() or
> > > request_region() failed it would release the irq and the region even
> > > though they were not successfully aquired.
> > > 
> > > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > 
> > I don't think this is needed.
> > I just alloc all the stuff, and if one of allocations fail, I free them
> > all. {k}free on NULL pointer is perfectly legal.
> > 
> > Same about IO and IRQ.
> > IRQ0 and IO 0 isn't valid, and I do test that in error path.
> > 
> >
> 
> Here is the original code:
> 
> Here is where we set "dev".
> 
>    785          dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
>    786  
>    787          if (!input_dev || !ir_props || !dev)
>    788                  goto error;
> 
> 	[snip]
> 
> Here is where we set the IO and IRQ:
> 
>    800          dev->hw_io = pnp_port_start(pnp_dev, 0);
>    801          dev->irq = pnp_irq(pnp_dev, 0);
> 
> 	[snip]
> 
> Here is where the request_region() and request_irq() are.
> 
>    806          if (!request_region(dev->hw_io, ENE_MAX_IO, ENE_DRIVER_NAME))
>    807                  goto error;
>    808  
>    809          if (request_irq(dev->irq, ene_isr, 
>    810                          IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev))
>    811                  goto error;
> 
> 	[snip]
> 
> Here is the error label:
> 
>    897  error:
>    898          if (dev->irq)
> 		    ^^^^^^^^
> 
> 	Oops!  The allocation of dev failed and this is a NULL
> 	dereference.
> 
>    899                  free_irq(dev->irq, dev);
> 
> 	Oops!  Request region failed and dev->irq is non-zero but
> 	request_irq() hasn't been called.
> 
>    900          if (dev->hw_io)
>    901                  release_region(dev->hw_io, ENE_MAX_IO);
> 
> 	Oops! dev->hw_io is non-zero but request_region() failed and so
> 	we just released someone else's region.


Ok, this is something different.
To be honest I was in hurry when I prepared the patch, so I didn't look
at this.
The intent was correct, and untill you pointed that out I somehow
assumed that error patch does what I supposed it to do... well...
In few days I will switch back to this driver and fix this problem.
I also have quite a lot of work to do in this driver now that I have
some hardware documentation (register renames are the fun part...).

So thanks for catching this.

Best regards,
Maxim Levitsky


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

* Re: [patch] IR: ene_ir: problems in unwinding on probe
@ 2010-08-12 16:42       ` Maxim Levitsky
  0 siblings, 0 replies; 8+ messages in thread
From: Maxim Levitsky @ 2010-08-12 16:42 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors

On Thu, 2010-08-12 at 18:19 +0200, Dan Carpenter wrote: 
> On Thu, Aug 12, 2010 at 05:35:04PM +0300, Maxim Levitsky wrote:
> > On Thu, 2010-08-12 at 09:46 +0200, Dan Carpenter wrote: 
> > > There were a couple issues here.  If the allocation failed for "dev"
> > > then it would lead to a NULL dereference.  If request_irq() or
> > > request_region() failed it would release the irq and the region even
> > > though they were not successfully aquired.
> > > 
> > > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > 
> > I don't think this is needed.
> > I just alloc all the stuff, and if one of allocations fail, I free them
> > all. {k}free on NULL pointer is perfectly legal.
> > 
> > Same about IO and IRQ.
> > IRQ0 and IO 0 isn't valid, and I do test that in error path.
> > 
> >
> 
> Here is the original code:
> 
> Here is where we set "dev".
> 
>    785          dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
>    786  
>    787          if (!input_dev || !ir_props || !dev)
>    788                  goto error;
> 
> 	[snip]
> 
> Here is where we set the IO and IRQ:
> 
>    800          dev->hw_io = pnp_port_start(pnp_dev, 0);
>    801          dev->irq = pnp_irq(pnp_dev, 0);
> 
> 	[snip]
> 
> Here is where the request_region() and request_irq() are.
> 
>    806          if (!request_region(dev->hw_io, ENE_MAX_IO, ENE_DRIVER_NAME))
>    807                  goto error;
>    808  
>    809          if (request_irq(dev->irq, ene_isr, 
>    810                          IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev))
>    811                  goto error;
> 
> 	[snip]
> 
> Here is the error label:
> 
>    897  error:
>    898          if (dev->irq)
> 		    ^^^^^^^^
> 
> 	Oops!  The allocation of dev failed and this is a NULL
> 	dereference.
> 
>    899                  free_irq(dev->irq, dev);
> 
> 	Oops!  Request region failed and dev->irq is non-zero but
> 	request_irq() hasn't been called.
> 
>    900          if (dev->hw_io)
>    901                  release_region(dev->hw_io, ENE_MAX_IO);
> 
> 	Oops! dev->hw_io is non-zero but request_region() failed and so
> 	we just released someone else's region.


Ok, this is something different.
To be honest I was in hurry when I prepared the patch, so I didn't look
at this.
The intent was correct, and untill you pointed that out I somehow
assumed that error patch does what I supposed it to do... well...
In few days I will switch back to this driver and fix this problem.
I also have quite a lot of work to do in this driver now that I have
some hardware documentation (register renames are the fun part...).

So thanks for catching this.

Best regards,
Maxim Levitsky


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

end of thread, other threads:[~2010-08-12 16:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-12  7:46 [patch] IR: ene_ir: problems in unwinding on probe Dan Carpenter
2010-08-12  7:46 ` Dan Carpenter
2010-08-12 14:35 ` Maxim Levitsky
2010-08-12 14:35   ` Maxim Levitsky
2010-08-12 16:19   ` Dan Carpenter
2010-08-12 16:19     ` Dan Carpenter
2010-08-12 16:42     ` Maxim Levitsky
2010-08-12 16:42       ` Maxim Levitsky

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.