linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] genirq:Dereference desc after null pointer check
@ 2018-07-16 22:27 RAGHU Halharvi
  2018-07-16 23:03 ` RAGHU Halharvi
  2018-07-17 12:25 ` Dan Carpenter
  0 siblings, 2 replies; 5+ messages in thread
From: RAGHU Halharvi @ 2018-07-16 22:27 UTC (permalink / raw)
  To: linux-kernel; +Cc: RAGHU Halharvi, tglx

* Remove unchecked dereferencing of *desc & assigning it local irq
variable

* Move the assignement after proper check of *desc for NULL pointer.

Signed-off-by: RAGHU Halharvi <raghuhack78@gmail.com>
---
 kernel/irq/manage.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index facfecfc543c..064d98e5ae32 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1557,7 +1557,7 @@ EXPORT_SYMBOL_GPL(setup_irq);
  */
 static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
 {
-	unsigned irq = desc->irq_data.irq;
+	unsigned int irq;
 	struct irqaction *action, **action_ptr;
 	unsigned long flags;
 
@@ -1566,6 +1566,7 @@ static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
 	if (!desc)
 		return NULL;
 
+	irq = desc->irq_data.irq;
 	mutex_lock(&desc->request_mutex);
 	chip_bus_lock(desc);
 	raw_spin_lock_irqsave(&desc->lock, flags);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH] genirq:Dereference desc after null pointer check
  2018-07-16 22:27 [PATCH] genirq:Dereference desc after null pointer check RAGHU Halharvi
@ 2018-07-16 23:03 ` RAGHU Halharvi
  2018-07-17  7:51   ` Thomas Gleixner
  2018-07-17 12:25 ` Dan Carpenter
  1 sibling, 1 reply; 5+ messages in thread
From: RAGHU Halharvi @ 2018-07-16 23:03 UTC (permalink / raw)
  To: linux-kernel; +Cc: RAGHU Halharvi, tglx

* Remove unchecked dereferencing of *desc & assigning it local irq
	variable
* Move the assignement after proper check of *desc for NULL pointer.

Signed-off-by: RAGHU Halharvi <raghuhack78@gmail.com>
---
 kernel/irq/manage.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index facfecfc543c..33c77eb4fdb6 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1557,15 +1557,16 @@ EXPORT_SYMBOL_GPL(setup_irq);
  */
 static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
 {
-	unsigned irq = desc->irq_data.irq;
+	unsigned int irq;
 	struct irqaction *action, **action_ptr;
 	unsigned long flags;
 
-	WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);
 
 	if (!desc)
 		return NULL;
 
+	irq = desc->irq_data.irq;
+	WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);
 	mutex_lock(&desc->request_mutex);
 	chip_bus_lock(desc);
 	raw_spin_lock_irqsave(&desc->lock, flags);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] genirq:Dereference desc after null pointer check
  2018-07-16 23:03 ` RAGHU Halharvi
@ 2018-07-17  7:51   ` Thomas Gleixner
  0 siblings, 0 replies; 5+ messages in thread
From: Thomas Gleixner @ 2018-07-17  7:51 UTC (permalink / raw)
  To: RAGHU Halharvi; +Cc: linux-kernel

On Tue, 17 Jul 2018, RAGHU Halharvi wrote:

> * Remove unchecked dereferencing of *desc & assigning it local irq
> 	variable
> * Move the assignement after proper check of *desc for NULL pointer.

Sorry no. The NULL pointer check is pointless because the call sites
already do it before calling that function.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] genirq:Dereference desc after null pointer check
  2018-07-16 22:27 [PATCH] genirq:Dereference desc after null pointer check RAGHU Halharvi
  2018-07-16 23:03 ` RAGHU Halharvi
@ 2018-07-17 12:25 ` Dan Carpenter
  2018-07-17 12:38   ` RAGHU H
  1 sibling, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2018-07-17 12:25 UTC (permalink / raw)
  To: kbuild, RAGHU Halharvi; +Cc: kbuild-all, linux-kernel, RAGHU Halharvi, tglx

Hi RAGHU,

Thank you for the patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/RAGHU-Halharvi/genirq-Dereference-desc-after-null-pointer-check/20180717-150842

smatch warnings:
kernel/irq/manage.c:1571 __free_irq() error: uninitialized symbol 'irq'.

# https://github.com/0day-ci/linux/commit/3ebb02d34bcb1c8111a79856326bca85fb3321eb
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 3ebb02d34bcb1c8111a79856326bca85fb3321eb
vim +/irq +1571 kernel/irq/manage.c

d3c60047 Thomas Gleixner                2008-10-16  1560  
cbf94f06 Magnus Damm                    2009-03-12  1561  /*
cbf94f06 Magnus Damm                    2009-03-12  1562   * Internal function to unregister an irqaction - used to free
cbf94f06 Magnus Damm                    2009-03-12  1563   * regular and special interrupts that are part of the architecture.
^1da177e Linus Torvalds                 2005-04-16  1564   */
83ac4ca9 Uwe Kleine König               2018-03-19  1565  static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
^1da177e Linus Torvalds                 2005-04-16  1566  {
3ebb02d3 RAGHU Halharvi                 2018-07-17  1567  	unsigned int irq;
f17c7545 Ingo Molnar                    2009-02-17  1568  	struct irqaction *action, **action_ptr;
^1da177e Linus Torvalds                 2005-04-16  1569  	unsigned long flags;
^1da177e Linus Torvalds                 2005-04-16  1570  
ae88a23b Ingo Molnar                    2009-02-15 @1571  	WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);
                                                                                                                                  ^^^
Not initialized.

7d94f7ca Yinghai Lu                     2008-08-19  1572  
7d94f7ca Yinghai Lu                     2008-08-19  1573  	if (!desc)
f21cfb25 Magnus Damm                    2009-03-12  1574  		return NULL;
^1da177e Linus Torvalds                 2005-04-16  1575  
3ebb02d3 RAGHU Halharvi                 2018-07-17  1576  	irq = desc->irq_data.irq;
9114014c Thomas Gleixner                2017-06-29  1577  	mutex_lock(&desc->request_mutex);
abc7e40c Thomas Gleixner                2015-12-13  1578  	chip_bus_lock(desc);
239007b8 Thomas Gleixner                2009-11-17  1579  	raw_spin_lock_irqsave(&desc->lock, flags);
ae88a23b Ingo Molnar                    2009-02-15  1580  
ae88a23b Ingo Molnar                    2009-02-15  1581  	/*
ae88a23b Ingo Molnar                    2009-02-15  1582  	 * There can be multiple actions per IRQ descriptor, find the right
ae88a23b Ingo Molnar                    2009-02-15  1583  	 * one based on the dev_id:
ae88a23b Ingo Molnar                    2009-02-15  1584  	 */
f17c7545 Ingo Molnar                    2009-02-17  1585  	action_ptr = &desc->action;
^1da177e Linus Torvalds                 2005-04-16  1586  	for (;;) {
f17c7545 Ingo Molnar                    2009-02-17  1587  		action = *action_ptr;
^1da177e Linus Torvalds                 2005-04-16  1588  
ae88a23b Ingo Molnar                    2009-02-15  1589  		if (!action) {
ae88a23b Ingo Molnar                    2009-02-15  1590  			WARN(1, "Trying to free already-free IRQ %d\n", irq);
239007b8 Thomas Gleixner                2009-11-17  1591  			raw_spin_unlock_irqrestore(&desc->lock, flags);
abc7e40c Thomas Gleixner                2015-12-13  1592  			chip_bus_sync_unlock(desc);
19d39a38 Thomas Gleixner                2017-07-11  1593  			mutex_unlock(&desc->request_mutex);
f21cfb25 Magnus Damm                    2009-03-12  1594  			return NULL;
ae88a23b Ingo Molnar                    2009-02-15  1595  		}
^1da177e Linus Torvalds                 2005-04-16  1596  
8316e381 Ingo Molnar                    2009-02-17  1597  		if (action->dev_id == dev_id)
ae88a23b Ingo Molnar                    2009-02-15  1598  			break;
f17c7545 Ingo Molnar                    2009-02-17  1599  		action_ptr = &action->next;
ae88a23b Ingo Molnar                    2009-02-15  1600  	}
^1da177e Linus Torvalds                 2005-04-16  1601  
ae88a23b Ingo Molnar                    2009-02-15  1602  	/* Found it - now remove it from the list of entries: */
f17c7545 Ingo Molnar                    2009-02-17  1603  	*action_ptr = action->next;
dbce706e Paolo 'Blaisorblade' Giarrusso 2005-06-21  1604  
cab303be Thomas Gleixner                2014-08-28  1605  	irq_pm_remove_action(desc, action);
cab303be Thomas Gleixner                2014-08-28  1606  
ae88a23b Ingo Molnar                    2009-02-15  1607  	/* If this was the last handler, shut down the IRQ line: */
c1bacbae Thomas Gleixner                2014-03-08  1608  	if (!desc->action) {
e9849777 Thomas Gleixner                2015-10-09  1609  		irq_settings_clr_disable_unlazy(desc);
46999238 Thomas Gleixner                2011-02-02  1610  		irq_shutdown(desc);
c1bacbae Thomas Gleixner                2014-03-08  1611  	}
3aa551c9 Thomas Gleixner                2009-03-23  1612  

:::::: The code at line 1571 was first introduced by commit
:::::: ae88a23b32fa7e0dc9fa7ce735966e68eb41b0bc irq: refactor and clean up the free_irq() code flow

:::::: TO: Ingo Molnar <mingo@elte.hu>
:::::: CC: Ingo Molnar <mingo@elte.hu>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] genirq:Dereference desc after null pointer check
  2018-07-17 12:25 ` Dan Carpenter
@ 2018-07-17 12:38   ` RAGHU H
  0 siblings, 0 replies; 5+ messages in thread
From: RAGHU H @ 2018-07-17 12:38 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: kbuild, kbuild-all, linux-kernel, tglx

Yes Dan, I should have done better.

Also Tom suggested to get rid of NULL pointer check as its redundant
because callee does that before calling __free_irq.


On Tue, Jul 17, 2018 at 03:25:57PM +0300, Dan Carpenter wrote:
> Hi RAGHU,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> url:    https://github.com/0day-ci/linux/commits/RAGHU-Halharvi/genirq-Dereference-desc-after-null-pointer-check/20180717-150842
> 
> smatch warnings:
> kernel/irq/manage.c:1571 __free_irq() error: uninitialized symbol 'irq'.
> 
> # https://github.com/0day-ci/linux/commit/3ebb02d34bcb1c8111a79856326bca85fb3321eb
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 3ebb02d34bcb1c8111a79856326bca85fb3321eb
> vim +/irq +1571 kernel/irq/manage.c
> 
> d3c60047 Thomas Gleixner                2008-10-16  1560  
> cbf94f06 Magnus Damm                    2009-03-12  1561  /*
> cbf94f06 Magnus Damm                    2009-03-12  1562   * Internal function to unregister an irqaction - used to free
> cbf94f06 Magnus Damm                    2009-03-12  1563   * regular and special interrupts that are part of the architecture.
> ^1da177e Linus Torvalds                 2005-04-16  1564   */
> 83ac4ca9 Uwe Kleine König               2018-03-19  1565  static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
> ^1da177e Linus Torvalds                 2005-04-16  1566  {
> 3ebb02d3 RAGHU Halharvi                 2018-07-17  1567  	unsigned int irq;
> f17c7545 Ingo Molnar                    2009-02-17  1568  	struct irqaction *action, **action_ptr;
> ^1da177e Linus Torvalds                 2005-04-16  1569  	unsigned long flags;
> ^1da177e Linus Torvalds                 2005-04-16  1570  
> ae88a23b Ingo Molnar                    2009-02-15 @1571  	WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);
>                                                                                                                                   ^^^
> Not initialized.
> 
> 7d94f7ca Yinghai Lu                     2008-08-19  1572  
> 7d94f7ca Yinghai Lu                     2008-08-19  1573  	if (!desc)
> f21cfb25 Magnus Damm                    2009-03-12  1574  		return NULL;
> ^1da177e Linus Torvalds                 2005-04-16  1575  
> 3ebb02d3 RAGHU Halharvi                 2018-07-17  1576  	irq = desc->irq_data.irq;
> 9114014c Thomas Gleixner                2017-06-29  1577  	mutex_lock(&desc->request_mutex);
> abc7e40c Thomas Gleixner                2015-12-13  1578  	chip_bus_lock(desc);
> 239007b8 Thomas Gleixner                2009-11-17  1579  	raw_spin_lock_irqsave(&desc->lock, flags);
> ae88a23b Ingo Molnar                    2009-02-15  1580  
> ae88a23b Ingo Molnar                    2009-02-15  1581  	/*
> ae88a23b Ingo Molnar                    2009-02-15  1582  	 * There can be multiple actions per IRQ descriptor, find the right
> ae88a23b Ingo Molnar                    2009-02-15  1583  	 * one based on the dev_id:
> ae88a23b Ingo Molnar                    2009-02-15  1584  	 */
> f17c7545 Ingo Molnar                    2009-02-17  1585  	action_ptr = &desc->action;
> ^1da177e Linus Torvalds                 2005-04-16  1586  	for (;;) {
> f17c7545 Ingo Molnar                    2009-02-17  1587  		action = *action_ptr;
> ^1da177e Linus Torvalds                 2005-04-16  1588  
> ae88a23b Ingo Molnar                    2009-02-15  1589  		if (!action) {
> ae88a23b Ingo Molnar                    2009-02-15  1590  			WARN(1, "Trying to free already-free IRQ %d\n", irq);
> 239007b8 Thomas Gleixner                2009-11-17  1591  			raw_spin_unlock_irqrestore(&desc->lock, flags);
> abc7e40c Thomas Gleixner                2015-12-13  1592  			chip_bus_sync_unlock(desc);
> 19d39a38 Thomas Gleixner                2017-07-11  1593  			mutex_unlock(&desc->request_mutex);
> f21cfb25 Magnus Damm                    2009-03-12  1594  			return NULL;
> ae88a23b Ingo Molnar                    2009-02-15  1595  		}
> ^1da177e Linus Torvalds                 2005-04-16  1596  
> 8316e381 Ingo Molnar                    2009-02-17  1597  		if (action->dev_id == dev_id)
> ae88a23b Ingo Molnar                    2009-02-15  1598  			break;
> f17c7545 Ingo Molnar                    2009-02-17  1599  		action_ptr = &action->next;
> ae88a23b Ingo Molnar                    2009-02-15  1600  	}
> ^1da177e Linus Torvalds                 2005-04-16  1601  
> ae88a23b Ingo Molnar                    2009-02-15  1602  	/* Found it - now remove it from the list of entries: */
> f17c7545 Ingo Molnar                    2009-02-17  1603  	*action_ptr = action->next;
> dbce706e Paolo 'Blaisorblade' Giarrusso 2005-06-21  1604  
> cab303be Thomas Gleixner                2014-08-28  1605  	irq_pm_remove_action(desc, action);
> cab303be Thomas Gleixner                2014-08-28  1606  
> ae88a23b Ingo Molnar                    2009-02-15  1607  	/* If this was the last handler, shut down the IRQ line: */
> c1bacbae Thomas Gleixner                2014-03-08  1608  	if (!desc->action) {
> e9849777 Thomas Gleixner                2015-10-09  1609  		irq_settings_clr_disable_unlazy(desc);
> 46999238 Thomas Gleixner                2011-02-02  1610  		irq_shutdown(desc);
> c1bacbae Thomas Gleixner                2014-03-08  1611  	}
> 3aa551c9 Thomas Gleixner                2009-03-23  1612  
> 
> :::::: The code at line 1571 was first introduced by commit
> :::::: ae88a23b32fa7e0dc9fa7ce735966e68eb41b0bc irq: refactor and clean up the free_irq() code flow
> 
> :::::: TO: Ingo Molnar <mingo@elte.hu>
> :::::: CC: Ingo Molnar <mingo@elte.hu>
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-17 13:24 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-16 22:27 [PATCH] genirq:Dereference desc after null pointer check RAGHU Halharvi
2018-07-16 23:03 ` RAGHU Halharvi
2018-07-17  7:51   ` Thomas Gleixner
2018-07-17 12:25 ` Dan Carpenter
2018-07-17 12:38   ` RAGHU H

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).