Messages in this thread | | | Date | Wed, 11 Jul 2012 12:33:13 +0530 | From | Venu Byravarasu <> | Subject | Re: [PATCH v1] usb: host: Fix possible kernel crash |
| |
On Tuesday 10 July 2012 10:05 PM, Scan Subscription wrote: > 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 for your detailed mail.
> Thanks > > Coverity Scan-admin scan-admin@coverity.com > Dakshesh Vyas | Technical Manager - SCAN > Coverity | 185 Berry Street | Suite 6500, Lobby 3 | San Francisco, CA 94107 > Office: 415.935.2957 | dvyas@coverity.com > > > ________________________________________ > From: linux-kernel-owner@vger.kernel.org [linux-kernel-owner@vger.kernel.org] On Behalf Of gregkh@linuxfoundation.org [gregkh@linuxfoundation.org] > Sent: Tuesday, July 10, 2012 7:45 AM > To: Venu Byravarasu > Cc: Alan Stern; linux-usb@vger.kernel.org; linux-kernel@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 majordomo@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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |