Hi Greg/Venu/Alan and others,
The defect discussed in this thread was found in 2006, and was marked in
Coverity Scan as false positive - intentional ( by linux developer or coverity
admin that we don't know)...
As a general rule,
1. what was discussed with some of the Linux folks, Focus on NEW defects...
2. Do NOT fix anything that is already marked as Intentional /False Positive
For any defects found by Covierty Scan there could be potential 3 actions
1. Review and Fix the defect
2. Mark it as Intentional in Coverity Scan, and it will not appear as Defect in
the future
3. Contact Coverity Scan-admin, if you would like to understand it more why it
was flagged as defect.
• As we all know, inherent in the technology foundation, static analysis will
report some false positives. We put a lot of effort into our product to drive
this rate to a very low, acceptable limit (commonly between 5% and 25%)
• In order to address FPs, the SCAN part of our product offers a triage process
that utilizes a persistent database based on defect hashing. This gives
developers the option to declare a defect as FP and then never have to look at
it again.
For instance, 3 weeks ago, Coverity reported 7 new defects introduced based on
recent code changes, and as we can see in the various threads almost
everything was fixed and committed by maintainer.
But a week after that, out of 6 new defects reported based on latest code
change, 1 was ignored stating False positive, Intentional...
I hope this clarifies the issue that was discussed here.
Thanks
Coverity Scan-admin scan-ad...@coverity.com
Dakshesh Vyas | Technical Manager - SCAN
Coverity | 185 Berry Street | Suite 6500, Lobby 3 | San Francisco, CA 94107
Office: 415.935.2957 | dv...@coverity.com
________________________________________
From: linux-kernel-ow...@vger.kernel.org [linux-kernel-ow...@vger.kernel.org]
On Behalf Of gre...@linuxfoundation.org [gre...@linuxfoundation.org]
Sent: Tuesday, July 10, 2012 7:45 AM
To: Venu Byravarasu
Cc: Alan Stern; linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH v1] usb: host: Fix possible kernel crash
On Tue, Jul 10, 2012 at 09:56:39AM +0530, Venu Byravarasu wrote:
> Thanks Alan for your comments.
>
> On Monday 09 July 2012 08:04 PM, Alan Stern wrote:
> >On Mon, 9 Jul 2012, Venu Byravarasu wrote:
> >
> >>In functions itd_complete & sitd_complete, a pointer
> >>by name stream may get dereferenced after freeing it, when
> >>iso_stream_put is called with stream->refcount = 2.
> >I don't understand the problem. Did you actually see this happen or is
> >it only theoretical?
>
> Yes it is a theoretical problem, as complained by Coverity.
>
> > /* for each uframe with a packet */
> > for (uframe = 0; uframe < 8; uframe++) {
> >@@ -1783,7 +1784,8 @@ itd_complete (
> > dev->devpath, stream->bEndpointAddress & 0x0f,
> > (stream->bEndpointAddress & USB_DIR_IN) ? "in" : "out");
> > }
> >- iso_stream_put (ehci, stream);
> >+ stream_ref_count = stream->refcount;
> >+ iso_stream_put(ehci, stream);
> >This iso_stream_put removes the reference held by the URB. Before it
> >is called, stream->refcount must be >= 3:
> >
> > refcount is set to 1 when the stream is created;
> >
> > each active URB holds a reference;
> >
> > each itd holds a reference.
> >
> >So after the call, the refcount value must be >= 2 and the stream could
> >not have been deallocated.
> >
> >> done:
> >> itd->urb = NULL;
> >>@@ -1797,7 +1799,7 @@ done:
> >> * Move it to a safe place until a new frame starts.
> >> */
> >> list_move(&itd->itd_list, &ehci->cached_itd_list);
> >>- if (stream->refcount == 2) {
> >>+ if (stream_ref_count == 3) {
> >Therefore this seems unnecessary.
>
> As per the logic you explained above, this change is not needed.
> However coverity was complaining as below:
>
> /kernel/drivers/usb/host/ehci-sched.c 1777 USE_AFTER_FREE
> Dereferencing freed pointer "stream"
>
> Hence to pacify coverity, this change is done.
Why are you trying to "pacify" coverity, when the tool is wrong in this
case? Go poke the owners of that tool to get it to stop emitting this
false warning. Don't paper over it in the kernel. Especially for a
tool that none of us can run on our own.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
