From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 44EDE3016F9 for ; Thu, 27 Nov 2025 12:43:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764247382; cv=none; b=EphB/9Sz+PDGBkmMKknqiZmCLV16jIV+nbCNyW2Ww+RXIceEOErdFDcop0Gm2JWCWQG+0flohFPv1vPUR4tc9xFqUdU6egAPvRKUbwBAKpYloUCyHVYEnrZNFWlG2U7ABXnXraKJqlEZig3rQGNpHnTsKEBGDeVKbbeYUjRstRQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764247382; c=relaxed/simple; bh=srq9yjmsWq4oHDKhQ8yO2Ow+5hsVzA/3vIlFU9ARz+k=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=lE25bm2cUHVuPe4GsH2xxJDBrjOJhVyjGYHSGG8q1yWRyhDFrG1doNBJ4vFFAwokKHkAs9BlPXqYFDopE+eYO5GKGvHMY6AyFhhbIqr1UXbmVLfHsJAnQverzrYADaUoI+fXMmyKHU1/hWu1cB0dKxqwu0qJ8QlsS8o38RsAcy8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linuxfoundation.org header.i=@linuxfoundation.org header.b=ZHYPZaPb; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linuxfoundation.org header.i=@linuxfoundation.org header.b="ZHYPZaPb" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 72550C4CEF8; Thu, 27 Nov 2025 12:43:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1764247382; bh=srq9yjmsWq4oHDKhQ8yO2Ow+5hsVzA/3vIlFU9ARz+k=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ZHYPZaPbRtaaw/ReWjwspaBm/BdbISxJD4M9TbfBrwrmeb0dAFUaUJ2BVpQhecFR3 JCJMxNGg2hJUcCftS/b44iyo3N0oPnh4zT5SAcSa9Rr9kq9B8OuAii67Eb4JLMpEZ3 CEb6z7hvulQ/k1JI5anyIATVgm0wVAsQKAeI0A+I= Date: Thu, 27 Nov 2025 13:42:59 +0100 From: Greg KH To: Zhengqiao Xia Cc: treapking@chromium.org, rafael@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] device_core: check null pointer Message-ID: <2025112744-unpaired-spruce-854b@gregkh> References: <20251114141821.416835-1-xiazhengqiao@huaqin.corp-partner.google.com> <2025111523-preface-scoreless-b478@gregkh> <2025111720-sensitize-schnapps-3189@gregkh> <2025112725-comrade-willfully-67f1@gregkh> <2025112721-savanna-overlabor-1269@gregkh> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Thu, Nov 27, 2025 at 08:34:45PM +0800, Zhengqiao Xia wrote: > Greg KH 于2025年11月27日周四 19:51写道： > > > > On Thu, Nov 27, 2025 at 07:37:34PM +0800, Zhengqiao Xia wrote: > > > Greg KH 于2025年11月27日周四 17:54写道： > > > > > > > > On Tue, Nov 18, 2025 at 03:31:03PM +0800, Zhengqiao Xia wrote: > > > > > Greg KH 于2025年11月17日周一 20:37写道： > > > > > > > > > > > > > > > > > On Mon, Nov 17, 2025 at 10:55:05AM +0800, Zhengqiao Xia wrote: > > > > > > > Hi Greg, > > > > > > > > > > > > > > Based on my analysis, > > > > > > > > > > > > Please do not top-post :( > > > > > > > > > > got it > > > > > > > > > > > > > > > > > > > > > > > > > usb_new_device -> device_add -> device_private_init > > > > > > > -> > > > > > > > bus_probe_device -> usb_generic_driver_probe -> > > > > > > > usb_set_configuration -> "add each interface" -> > > > > > > > device_add(&intf->dev); > > > > > > > > > > > > > > > > > > > > > --> if of_node status is disabled > > > > > > > -> of_device_is_available is false -> not running device_add -> > > > > > > > > > > > > Why is of_node related to USB interfaces? > > > > > > > > > > you can see usb/core/message.c > > > > > ``` > > > > > for (i = 0; i < nintf; ++i) { > > > > > struct usb_interface *intf = cp->interface[i]; > > > > > > > > > > dump_usb_intf_info(intf); > > > > > > > > > > if (intf->dev.of_node && > > > > > !of_device_is_available(intf->dev.of_node)) { > > > > > dev_info(&dev->dev, "skipping disabled interface %d\n", > > > > > intf->cur_altsetting->desc.bInterfaceNumber); > > > > > continue; > > > > > } > > > > > ... > > > > > ret = device_add(&intf->dev); > > > > > > > > Yes, device_add() is skipped, so that means we should not ever be seeing > > > > this device in the device tree anywhere, so that the driver core > > > > shouldn't be seeing it anywhere else. > > > > > > > > This device_add() call is not made, so what is attempting to register > > > > this "disabled" interface? > > > > > > > > And the more and more I see this "tacked on" ability to disable > > > > interfaces based on firmware node, the more I dislike it (we are having > > > > other discussions on the linux-usb list about it.) > > > > > > > > So perhaps we just missed a path where the interface is not really known > > > > to be disabled and we try to do something with it later on? > > > > > > > > > ... > > > > > } > > > > > ``` > > > > > > > > > > > > > > > > > > interface dev->p is null > > > > > > > usbdev_ioctl -> proc_ioctl -> device_attach -> __device_attach -> > > > > > > > device_attach -> if (dev->p->dead) > > > > > > > > > > > > > > Since dev->p has not been created at this time, dev->p is NULL, So I > > > > > > > think we should add a check here. > > > > > > > > > > > > > > Here is my current analysis, but there may be some misunderstandings > > > > > > > or mistakes in it. Could you please help clarify my confusion? > > > > > > > > > > > > As this code is very old, USB was one of the first subsystems to get > > > > > > driver core support decades ago, what has changed recently to require > > > > > > this check? Have you seen crashes here? If so, what is the traceback > > > > > > for them and with what hardware/drivers? > > > > > > > > > > ``` > > > > > [ 93.530302] usb 4-1: USB disconnect, device number 2 > > > > > [ 93.535637] cdc_mbim 4-1:1.0 wwan0: unregister 'cdc_mbim' > > > > > usb-11260000.usb-1, CDC MBIM > > > > > [ 93.564041] option1 ttyUSB0: GSM modem (1-port) converter now > > > > > disconnected from ttyUSB0 > > > > > [ 93.574619] option 4-1:1.2: device disconnected > > > > > [ 93.948062] usb 4-1: new high-speed USB device number 3 using xhci-mtk > > > > > [ 94.084037] usb 4-1: New USB device found, idVendor=05c6, > > > > > idProduct=9008, bcdDevice= 0.00 > > > > > [ 94.092275] usb 4-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 > > > > > [ 94.099455] usb 4-1: Product: QUSB__BULK > > > > > [ 94.103407] usb 4-1: Manufacturer: Qualcomm CDMA Technologies MSM > > > > > [ 94.114020] [USB-ATTACH] dev=4-1 dev->p=ffffff811faec600 > > > > > [ 94.122394] ===== USB Interface Dump ===== > > > > > [ 94.122394] dev name: 4-1:1.0 > > > > > [ 94.122394] dev of_node: hub > > > > > [ 94.122394] dev->p: 0000000000000000 (NULL) > > > > > [ 94.122394] parent name: 4-1 > > > > > [ 94.122394] parent of_node: hub > > > > > [ 94.122394] ============================== > > > > > [ 94.148881] usb 4-1: skipping disabled interface 0 > > > > > > > > > > --> 4-1:1.0 doesn't run device_add > > > > > > > > Good. > > > > > > > > > [ 94.161987] [USB-ATTACH] dev=4-1:1.0 dev->p=0000000000000000 > > > > > > > > > > --> usbdev_ioctl -> __device_attach --> > > > > > > > > Wait, why are you attempting to talk to this interface through usbfs? > > > > > > When LTE enters QUSB__BULK mode, it indicates that the > > > userspace(modemfwd) needs to update the firmware via usbfs. > > > Therefore, this device cannot be disabled. > > > > Then this should not be disabled in the firmware, please fix your dt. > > > > But again, let's fix the real problem here. > As this（https://lore.kernel.org/all/CAEXTbpeN0Mk+Y-UeV79JzE547UCa_Fhh7T75L+2mhoSq3ark8g@mail.gmail.com/） > says： > > We have device A that has an on-board USB hub, so we describe it in the DTS. > Then, another device B is similar to device A but does not have the USB hub. > So, we inherit device A's DTS and disable the hub nodes. > > LTE device is connected to device B' port and no hub, so we disabled > the usb hub' node. > I think this is reasonable. > > > > > > > Maybe that's the path here, we need to disable that "for real" and > > > > prevent usbfs from accessing the interface as well. > > > > > > > > What userspace tool attempts to call this "by default" or are you just > > > > running fuzzing tools? > > > > > > > > Again, I think this needs to be fixed properly, not papered over in the > > > > driver core. So let's either rip the ability to disable interfaces out > > > > of the usb core, or fix up where we missed that a device was not > > > > registered. > > > > > > this is why I submit a new patch > > > https://lore.kernel.org/all/CAEXTbpeN0Mk+Y-UeV79JzE547UCa_Fhh7T75L+2mhoSq3ark8g@mail.gmail.com/ > > > > Does that really solve this problem? How? > > yes, this prevents the LTE device's of_node from pointing to the USB hub. > > @@ -31,6 +31,9 @@ struct device_node *usb_of_get_device_node(struct > usb_device *hub, int port1) > if (of_property_read_u32(node, "reg", ®)) > continue; > > + if (!of_device_is_available(node)) > + continue; > + > if (reg == port1) > return node; > } > > if (intf->dev.of_node && > !of_device_is_available(intf->dev.of_node)) { > dev_info(&dev->dev, "skipping disabled interface %d\n", > intf->cur_altsetting->desc.bInterfaceNumber); > continue; > } > so usb interface can continue to register. But where does this change usbfs's access to this interface? thanks, greg k-h