[PATCH] binder: fixed coverity warning by moving pr_warn outside lock

[PATCH] binder: fixed coverity warning by moving pr_warn outside lock

[Ameer Hamza]

Coverity warns about using print operations within a lock due to
unlikely possible deadlock scenario, however, this warning can be
easily avoided here without having any effect on the program flow.

Addresses-Coverity: 1494148 ("Thread deadlock")

Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com>
---
 drivers/android/binder.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index cffbe57a8e08..8ee942eef51d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1507,14 +1507,14 @@ static void binder_free_transaction(struct binder_transaction *t)
 	if (target_proc) {
 		binder_inner_proc_lock(target_proc);
 		target_proc->outstanding_txns--;
-		if (target_proc->outstanding_txns < 0)
-			pr_warn("%s: Unexpected outstanding_txns %d\n",
-				__func__, target_proc->outstanding_txns);
 		if (!target_proc->outstanding_txns && target_proc->is_frozen)
 			wake_up_interruptible_all(&target_proc->freeze_wait);
 		if (t->buffer)
 			t->buffer->transaction = NULL;
 		binder_inner_proc_unlock(target_proc);
+		if (target_proc->outstanding_txns < 0)
+			pr_warn("%s: Unexpected outstanding_txns %d\n",
+				__func__, target_proc->outstanding_txns);
 	}
 	if (trace_binder_txn_latency_free_enabled())
 		binder_txn_latency_free(t);
-- 
2.25.1

Re: [PATCH] binder: fixed coverity warning by moving pr_warn outside lock

[Greg KH]

On Sat, Dec 04, 2021 at 01:50:41AM +0500, Ameer Hamza wrote:
> Coverity warns about using print operations within a lock due to
> unlikely possible deadlock scenario, however, this warning can be
> easily avoided here without having any effect on the program flow.
> 
> Addresses-Coverity: 1494148 ("Thread deadlock")

Sounds like coverity is wrong, you can print any time, locks do not
matter here.

Do you have a link to the exact coverity report for this?

> Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com>
> ---
>  drivers/android/binder.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index cffbe57a8e08..8ee942eef51d 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1507,14 +1507,14 @@ static void binder_free_transaction(struct binder_transaction *t)
>  	if (target_proc) {
>  		binder_inner_proc_lock(target_proc);
>  		target_proc->outstanding_txns--;
> -		if (target_proc->outstanding_txns < 0)
> -			pr_warn("%s: Unexpected outstanding_txns %d\n",
> -				__func__, target_proc->outstanding_txns);
>  		if (!target_proc->outstanding_txns && target_proc->is_frozen)
>  			wake_up_interruptible_all(&target_proc->freeze_wait);
>  		if (t->buffer)
>  			t->buffer->transaction = NULL;
>  		binder_inner_proc_unlock(target_proc);
> +		if (target_proc->outstanding_txns < 0)
> +			pr_warn("%s: Unexpected outstanding_txns %d\n",
> +				__func__, target_proc->outstanding_txns);

Have you seen problems with the location of the existing message?  If
not, this should be fine.

thanks,

greg k-h

Re: [PATCH] binder: fixed coverity warning by moving pr_warn outside lock

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Sat, Dec 04, 2021 at 01:50:44PM +0500, Ameer Hamza wrote:
> Thank you Greg for your response. The link to Coverity warning:
> https://scan5.coverity.com/reports.htm#v56991/p10063/fileInstanceId=204668511&defectInstanceId=52305699&mergedDefectId=1494148

That link does not seem to be public.  What project are you looking at?

> I have seen similar warnings if the print operation is used inside a lock,
> i.e., Coverity speculates a possible deadlock scenario, which might be a
> false positive because internal printk implementation uses a separate lock.

When dealing with Coverity, it is up to you to determine if what it says
is actually true before sending out patches for it, due to the HUGE
number of false-positives it spits out.

thanks,

greg k-h

Re: [PATCH] binder: fixed coverity warning by moving pr_warn outside lock

[Ameer Hamza]

On Sat, Dec 04, 2021 at 10:00:32AM +0100, Greg KH wrote:
> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
> 
> A: No.
> Q: Should I include quotations after my reply?
> 
> http://daringfireball.net/2007/07/on_top
Thank you so much sharing the useful post as I have just started my open source journey very recently

> 
> On Sat, Dec 04, 2021 at 01:50:44PM +0500, Ameer Hamza wrote:
> > Thank you Greg for your response. The link to Coverity warning:
> > https://scan5.coverity.com/reports.htm#v56991/p10063/fileInstanceId=204668511&defectInstanceId=52305699&mergedDefectId=1494148
> 
> That link does not seem to be public.  What project are you looking at?
Its Linux project under coverity scan and CID for this warning is 1494148

> 
> > I have seen similar warnings if the print operation is used inside a lock,
> > i.e., Coverity speculates a possible deadlock scenario, which might be a
> > false positive because internal printk implementation uses a separate lock.
> 
> When dealing with Coverity, it is up to you to determine if what it says
> is actually true before sending out patches for it, due to the HUGE
> number of false-positives it spits out.
I will keep keep this under consideration for now, thanks

Best Regards,
Hamza

Re: [PATCH] binder: fixed coverity warning by moving pr_warn outside lock

[Greg KH]

On Sat, Dec 04, 2021 at 03:34:19PM +0500, Ameer Hamza wrote:
> On Sat, Dec 04, 2021 at 10:00:32AM +0100, Greg KH wrote:
> > A: http://en.wikipedia.org/wiki/Top_post
> > Q: Were do I find info about this thing called top-posting?
> > A: Because it messes up the order in which people normally read text.
> > Q: Why is top-posting such a bad thing?
> > A: Top-posting.
> > Q: What is the most annoying thing in e-mail?
> > 
> > A: No.
> > Q: Should I include quotations after my reply?
> > 
> > http://daringfireball.net/2007/07/on_top
> Thank you so much sharing the useful post as I have just started my open source journey very recently
> 
> > 
> > On Sat, Dec 04, 2021 at 01:50:44PM +0500, Ameer Hamza wrote:
> > > Thank you Greg for your response. The link to Coverity warning:
> > > https://scan5.coverity.com/reports.htm#v56991/p10063/fileInstanceId=204668511&defectInstanceId=52305699&mergedDefectId=1494148
> > 
> > That link does not seem to be public.  What project are you looking at?
> Its Linux project under coverity scan and CID for this warning is 1494148

Ah, yeah, coverity is being crazy here.  Watch out for that, it is a
VERY difficult tool to use if you are not experienced with kernel
development.

good luck!

greg k-h