Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934234AbcKVSJm (ORCPT ); Tue, 22 Nov 2016 13:09:42 -0500 Received: from canardo.mork.no ([148.122.252.1]:60335 "EHLO canardo.mork.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755328AbcKVSJZ (ORCPT ); Tue, 22 Nov 2016 13:09:25 -0500 From: =?utf-8?Q?Bj=C3=B8rn_Mork?= To: Wim Osterholt Cc: Oliver Neukum , poma , linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Subject: Re: crash by cdc_acm driver in kernels 4.8-rc1/5 Organization: m References: <1476793123.2637.3.camel@suse.com> <20161115001619.GA21030@djo.tudelft.nl> <20161115132930.GA20918@djo.tudelft.nl> <1479299670.2000.13.camel@suse.com> <20161116150757.GA15605@djo.tudelft.nl> <20161117015732.GA17637@djo.tudelft.nl> <20161117091434.GA6107@djo.tudelft.nl> <20161117161134.GA14413@djo.tudelft.nl> <1479734372.2332.1.camel@suse.com> <20161122153852.GA32591@djo.tudelft.nl> Date: Tue, 22 Nov 2016 19:08:30 +0100 In-Reply-To: <20161122153852.GA32591@djo.tudelft.nl> (Wim Osterholt's message of "Tue, 22 Nov 2016 16:38:52 +0100") Message-ID: <87oa17o09t.fsf@miraculix.mork.no> User-Agent: Gnus/5.130015 (Ma Gnus v0.15) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAMIA8u9031533 Content-Length: 2229 Lines: 60 Wim Osterholt writes: > On Mon, Nov 21, 2016 at 02:19:32PM +0100, Oliver Neukum wrote: > >> I don't understand it, bit please test the attached patch >> with dynamic debugging for cdc-acm and the kernel log level >> at maximum. > >> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c >> index 6895f9e..f03b5db 100644 >> --- a/drivers/usb/class/cdc-acm.c >> +++ b/drivers/usb/class/cdc-acm.c >> @@ -1188,6 +1188,12 @@ static int acm_probe(struct usb_interface *intf, >> >> cdc_parse_cdc_header(&h, intf, buffer, buflen); >> union_header = h.usb_cdc_union_desc; >> + >> + dev_dbg(&intf->dev, "Parsed device header\n"); >> + dev_dbg(&intf->dev, "Union descriptor %p\n", h.usb_cdc_union_desc); >> + dev_dbg(&intf->dev, "ACM descriptor %p\n", h.usb_cdc_acm_descriptor); >> + dev_dbg(&intf->dev, "Country descriptor %p\n", h.usb_cdc_country_functional_desc); >> + >> cmgmd = h.usb_cdc_call_mgmt_descriptor; >> if (cmgmd) >> call_intf_num = cmgmd->bDataInterface; > > > On kernel 4.8.8 this crashes hard and produces over a serial link: Huh? That device shouldn't ever enter that code path AFAICS. Unless.... you wouldn't happen to add a dynamic entry for this device, would you? What's the output of cat /sys/bus/usb/drivers/cdc_acm/new_id ? We should probably survive it, but I think the current acm_probe() is going to barf hard on the last data interface, if probed without the default NO_UNION_NORMAL quirk. cdc_parse_cdc_header() will happily parse all the functional descriptors, including the union pointing to interfaces 0 and 1. I might be missing it, but I cannot see any sanity check verifying that the currently probed interface is actually part of the set of interfaces pointed to by the union. There is a sanity check for the availability of the data interface, but there is none for the control interface (the assumption of course that that's the interface we're probing). I think we need a bit more sanity checking of the union. This is likely a generic problem for any CDC driver, so it is worth considering adding a shared function for that. And all this fails to explain anything if you didn't add the device dynamically, of course... Bjørn