lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CADRPvQubTEjKeJc=+LQ2jb0L=N4mxY8n21Bf8U-tS1stpB_eGw@mail.gmail.com>
Date:   Wed, 3 Mar 2021 17:03:25 +0800
From:   Chien Kun Niu <rickyniu@...gle.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     stern@...land.harvard.edu, erosca@...adit-jv.com,
        gustavoars@...nel.org, a.darwish@...utronix.de, oneukum@...e.com,
        Kyle Tso <kyletso@...gle.com>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, James Wei <jameswei@...gle.com>
Subject: Re: [PATCH] ANDROID: usb: core: Send uevent when USB TOPO layer over 6

Hi , Greg

What tool will "catch" this?  Where is that code located at?
=> I prepare merge the code to Android phone , so I used Android HLOS
to catch this uevent.

uevents are not for stuff like this, you are trying to send "error
conditions" to userspace, please use the "proper" interfaces like this
and not abuse existing ones.
=> Sorry , I am not sure what is the "proper" interfaces your mean.
     Could you please give me more description?

You just created a whole new sysfs class with no Documentation/ABI/
update?
=> Yes, I will add it.

Wait, how did you even test this code?  This will not work if you have
more than one hub in the system at a single time, right?
=> I build the test build which flash in Android phone and connect
several hubs to try it.
     When the new hub which met MAX_TOPO_LEVEL connected , it sent
notify to user space.

Phone ------↓
                 hub ------↓
                             hub ------↓
                                           ...------↓
                                                    hub

     if (hdev->level == MAX_TOPO_LEVEL) {
                dev_err(&intf->dev,
                        "Unsupported bus topology: hub nested too deep\n");
                hub_over_tier();
                return -E2BIG;
     }


So, proof that this works?  How did you test this?
=> I use the Pixel phone to verify the code , the framework received
the uevent when the last connected hub over "MAX_TOPO_LEVEL".

Also, you have a memory leak in this submission :(
=> Do you mean I should add device_destroy here ?

hub_device =
device_create(hub_class, NULL, MKDEV(0, 0), NULL, "usb_hub");
+if (IS_ERR(hub_device))
+               return PTR_ERR(hub_device);

void usb_hub_cleanup(void)
{
+if (!IS_ERR(hub_device))
+device_destroy(hub_class, hub_device->devt);

if (!IS_ERR(hub_class))
class_destroy(hub_class);

Best Regards,

Chien Kun Niu
rickyniu@...gle.com


Chien Kun Niu
SSD USB
rickyniu@...gle.com
02 8729 0651


Greg KH <gregkh@...uxfoundation.org> 於 2021年2月26日 週五 下午5:31寫道:
>
> On Fri, Feb 26, 2021 at 05:16:12PM +0800, Ricky Niu wrote:
> > When the topology of the nested hubs are over 6 layers
> > Send uevent to user space when USB TOPO layer over 6.
> > Let end user more understand what happened.
> >
> > Signed-off-by: Ricky Niu <rickyniu@...gle.com>
> > ---
> >  drivers/usb/core/hub.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 7f71218cc1e5..e5e924526822 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -55,6 +55,10 @@ static DEFINE_SPINLOCK(device_state_lock);
> >  static struct workqueue_struct *hub_wq;
> >  static void hub_event(struct work_struct *work);
> >
> > +/* struct to notify userspace of hub events */
> > +static struct class *hub_class;
> > +static struct device *hub_device;
>
> Wait, how did you even test this code?  This will not work if you have
> more than one hub in the system at a single time, right?
>
> That's going to be really rough, given here's the output of just my
> desktop system, count the number of hubs in it:rdevmgmt.msc
>
> $ lsusb -t
> /:  Bus 10.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 10000M
> /:  Bus 09.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 480M
>     |__ Port 5: Dev 2, If 0, Class=Wireless, Driver=btusb, 12M
>     |__ Port 5: Dev 2, If 1, Class=Wireless, Driver=btusb, 12M
>     |__ Port 6: Dev 3, If 0, Class=Hub, Driver=hub/4p, 480M
> /:  Bus 08.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 10000M
>     |__ Port 2: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M
>     |__ Port 3: Dev 3, If 0, Class=Mass Storage, Driver=uas, 10000M
> /:  Bus 07.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 480M
>     |__ Port 2: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>         |__ Port 2: Dev 4, If 0, Class=Hub, Driver=hub/2p, 12M
>             |__ Port 2: Dev 5, If 0, Class=Human Interface Device, Driver=usbhid, 12M
>             |__ Port 2: Dev 5, If 1, Class=Human Interface Device, Driver=usbhid, 12M
>             |__ Port 2: Dev 5, If 2, Class=Human Interface Device, Driver=usbhid, 12M
>     |__ Port 5: Dev 3, If 3, Class=Audio, Driver=snd-usb-audio, 480M
>     |__ Port 5: Dev 3, If 1, Class=Audio, Driver=snd-usb-audio, 480M
>     |__ Port 5: Dev 3, If 6, Class=Audio, Driver=snd-usb-audio, 480M
>     |__ Port 5: Dev 3, If 4, Class=Audio, Driver=snd-usb-audio, 480M
>     |__ Port 5: Dev 3, If 2, Class=Audio, Driver=snd-usb-audio, 480M
>     |__ Port 5: Dev 3, If 0, Class=Audio, Driver=snd-usb-audio, 480M
>     |__ Port 5: Dev 3, If 7, Class=Human Interface Device, Driver=usbhid, 480M
>     |__ Port 5: Dev 3, If 5, Class=Audio, Driver=snd-usb-audio, 480M
>     |__ Port 6: Dev 6, If 0, Class=Human Interface Device, Driver=usbhid, 12M
> /:  Bus 06.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/1p, 10000M/x2
> /:  Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/1p, 480M
> /:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 10000M
>     |__ Port 1: Dev 11, If 0, Class=Hub, Driver=hub/3p, 5000M
>         |__ Port 3: Dev 12, If 0, Class=Hub, Driver=hub/3p, 5000M
>             |__ Port 1: Dev 13, If 0, Class=Mass Storage, Driver=usb-storage, 5000M
> /:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 480M
>     |__ Port 1: Dev 14, If 0, Class=Hub, Driver=hub/3p, 480M
>         |__ Port 3: Dev 15, If 0, Class=Hub, Driver=hub/3p, 480M
>             |__ Port 2: Dev 17, If 0, Class=Human Interface Device, Driver=usbhid, 12M
>             |__ Port 3: Dev 18, If 3, Class=Human Interface Device, Driver=usbhid, 12M
>             |__ Port 3: Dev 18, If 1, Class=Audio, Driver=snd-usb-audio, 12M
>             |__ Port 3: Dev 18, If 2, Class=Audio, Driver=snd-usb-audio, 12M
>             |__ Port 3: Dev 18, If 0, Class=Audio, Driver=snd-usb-audio, 12M
> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 10000M
> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 480M
>
>
> So, proof that this works?  How did you test this?
>
> Also, you have a memory leak in this submission :(
>
> greg k-h

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ