Re: [PATCH] usb: hub: report failure to enumerate uevent to userspace

[Eugeniu Rosca <erosca@de.adit-jv.com>]

Hi Greg,

We really appreciate your feedback.

On Wed, Jun 05, 2019 at 12:03:37PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 05, 2019 at 11:05:56AM +0200, Eugeniu Rosca wrote:
> > From: Spyridon Papageorgiou <spapageorgiou@de.adit-jv.com>
> > 
> > When a USB device fails to enumerate, only a kernel message is printed.
> > With this patch, a uevent is also generated to notify userspace.
> > Services can monitor for the event through udev and handle failures
> > accordingly.
> > 
> > The "port_enumerate_fail_notify()" function name follows the syntax of
> > "port_over_current_notify()" used in v4.20-rc1
> > commit 201af55da8a398 ("usb: core: added uevent for over-current").
> > 
> > Signed-off-by: Spyridon Papageorgiou <spapageorgiou@de.adit-jv.com>
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> All we need is one special notifier!  ...
> 
> {grumble}
> 
> This can end up causing loads of new kobject change events to be added,
> overloading what uevents were supposed to be in the first place
> (add/remove of sysfs objects).

I guess that's the case for every other kobject_uevent*(*, KOBJ_CHANGE)
call in the USB subsystem (in case of either HW or code misbehavior).
JFTR, there are around 120 such calls in the entire v5.2-rc3 kernel.

> 
> I just talked with David Howells, and this type of thing really should
> be tied into the new "notifier" interface/api.  That way you can
> register for any specific type of event and just get notified of them
> when they happen.  No need to mess with uevents.
> 
> See his posts on linux-api starting with:
> 	Subject: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
> for the proposal.
> 
> If we added USB (or really any hardware events) to that interface, would
> it solve the issue you are trying to solve here?

I checked this patch series in linux-fs.git [3], as well as shared my
thoughts with our security and RFS experts, and we came up with the
following questions/remarks:

 - Looking at commit [4], it seems that the new "notifier" interface/api
   forces userspace applications to link against -lkeyutils [5].
   Assuming the latter is designed for ("Kernel key management") [6],
   it may look like the keyutils library is being abused to handle
   the "USB (or really any hardware events)". Do you really plan to
   extend the scope of the library to handle these new tasks?

 - Currently, to be able to get kobject uevent notifications, our
   applications must include "libudev.h" and must link against -ludev.
   By using the feature implemented in [3], we would significantly
   increase the complexity of those applications, particularly because
   they would need to arbitrate between two different categories of
   events received via two different APIs.

 - It is also my assumption that the existing KOBJ_CHANGE events cannot
   be easily converted to the new API, since this would hurt a dozen of
   userland applications relying on them.

Overall, I am quite clueless how to proceed with this patch, except to
keep it in our internal tree, most likely forever. Any
comments/recommendations would be appreciated.

> 
> thanks,
> 
> greg k-h

[1] linux (v5.2-rc3) git grep KOBJ_CHANGE -- drivers/usb/
drivers/usb/core/hub.c:	kobject_uevent_env(&hub_dev->kobj, KOBJ_CHANGE, envp);
drivers/usb/gadget/udc/core.c:	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
drivers/usb/gadget/udc/core.c:	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
drivers/usb/phy/phy.c:	kobject_uevent_env(&usb_phy->dev->kobj, KOBJ_CHANGE, envp);
drivers/usb/typec/class.c:	kobject_uevent(&adev->dev.kobj, KOBJ_CHANGE);
drivers/usb/typec/class.c:	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
drivers/usb/typec/class.c:	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
drivers/usb/typec/class.c:	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
drivers/usb/typec/class.c:	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);

[2] git grep -w KOBJ_CHANGE -- ":\!Documentation" ":\!include" | wc -l
122

[3] https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=notifications
[4] https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=16a8aad951990
[5] https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git
[6] https://lwn.net/Articles/210502/ ("Kernel key management")

-- 
Best Regards,
Eugeniu.