From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752374AbbCXIkv (ORCPT ); Tue, 24 Mar 2015 04:40:51 -0400 Received: from mail-bn1bon0142.outbound.protection.outlook.com ([157.56.111.142]:20160 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751917AbbCXIks (ORCPT ); Tue, 24 Mar 2015 04:40:48 -0400 Date: Tue, 24 Mar 2015 16:39:29 +0800 From: Peter Chen To: Badhri Jagan Sridharan CC: Felipe Balbi , Greg Kroah-Hartman , , Subject: Re: [PATCH] usb: gadget: Check for NULL pointer in disconnect Message-ID: <20150324083927.GB2293@shlinux2> References: <1426894852-4256-1-git-send-email-Badhri@google.com> <20150322074353.GA25278@shlinux2> <55109B15.9080800@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <55109B15.9080800@google.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-EOPAttributedMessage: 0 Authentication-Results: spf=fail (sender IP is 192.88.158.2) smtp.mailfrom=peter.chen@freescale.com; vger.kernel.org; dkim=none (message not signed) header.d=none; X-Forefront-Antispam-Report: CIP:192.88.158.2;CTRY:US;IPV:NLI;EFV:NLI;BMV:1;SFV:NSPM;SFS:(10019020)(6009001)(339900001)(189002)(24454002)(479174004)(51704005)(377454003)(199003)(23726002)(110136001)(86362001)(77156002)(104016003)(50466002)(33716001)(33656002)(46406003)(54356999)(105606002)(62966003)(47776003)(19580405001)(76176999)(87936001)(85426001)(92566002)(6806004)(50986999)(15975445007)(106466001)(97756001)(46102003)(77096005)(83506001)(19580395003)(2950100001)(61793002);DIR:OUT;SFP:1102;SCL:1;SRVR:BN1PR0301MB0625;H:az84smr01.freescale.net;FPR:;SPF:Fail;MLV:sfv;MX:1;A:1;LANG:en; X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN1PR0301MB0625; X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(5005006)(5002010);SRVR:BN1PR0301MB0625;BCL:0;PCL:0;RULEID:;SRVR:BN1PR0301MB0625; X-Forefront-PRVS: 0525BB0ADF X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Mar 2015 08:40:44.9998 (UTC) X-MS-Exchange-CrossTenant-Id: 710a03f5-10f6-4d38-9ff4-a80b81da590d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=710a03f5-10f6-4d38-9ff4-a80b81da590d;Ip=[192.88.158.2];Helo=[az84smr01.freescale.net] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN1PR0301MB0625 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 23, 2015 at 04:00:37PM -0700, Badhri Jagan Sridharan wrote: > > Do you really see some udc drivers call it after composite_unbind is > > called? If it is, you may add dump_stack() to track that error. > > > > Besides this, function suspended_show is needed to add cdev NULL pointer > > checking. > > We see this happening occasionally in *not yet* upstreamed UDC > code of some vendors (Yes, disconnect being called after unbind) Does it a spurious interrupt? I guess if you add struct usb_gadget_driver NULL pointer check check, it will not occur: if (you_udc->driver) you_udc->driver->disconnect(&you_udc->gadget); > After reviewing the entire composite.c file, I did notice that > none of the functions check for NULL pointer when cdev is > obtained from get_gadget_data. > Is crashing/bringing down the whole kernel intentionally > left to happen ? Hmm, I don't think, the design may think it never occurs. > Isn't printing a WARN/ERROR msg and > returning not the preferable approach ? I think BUG_ON is more suitable, let's see Felipe's comment. > > On 03/22/2015 12:43 AM, Peter Chen wrote: > >On Fri, Mar 20, 2015 at 04:40:52PM -0700, Badhri Jagan Sridharan wrote: > >>Added a safety net to make sure that > >>composite_disconnect does not end up disconneting > >>a NULL device. Prevents NULL pointer crash. > >> > >>Signed-off-by: Badhri Jagan Sridharan > >>--- > >> drivers/usb/gadget/composite.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >>diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > >>index 13adfd1..90b37bd 100644 > >>--- a/drivers/usb/gadget/composite.c > >>+++ b/drivers/usb/gadget/composite.c > >>@@ -1823,6 +1823,11 @@ void composite_disconnect(struct usb_gadget *gadget) > >> struct usb_composite_dev *cdev = get_gadget_data(gadget); > >> unsigned long flags; > >> > >>+ if (!cdev) { > >>+ WARN(1, "Trying to disconnect a NULL composite device\n"); > >>+ return; > >>+ } > >>+ > > > >Do you really see some udc drivers call it after composite_unbind is > >called? If it is, you may add dump_stack() to track that error. > > > >Besides this, function suspended_show is needed to add cdev NULL pointer > >checking. > > > >> /* REVISIT: should we have config and device level > >> * disconnect callbacks? > >> */ > >>-- > >>2.2.0.rc0.207.ga3a616c > >> > >>-- > >>To unsubscribe from this list: send the line "unsubscribe linux-usb" in > >>the body of a message to majordomo@vger.kernel.org > >>More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- Best Regards, Peter Chen