linux-kernel - Re: [PATCH] usb: core: add debugobjects support for urb object

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]

Date:	Tue, 24 May 2016 10:23:07 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	"Du, Changbin" <changbin.du@...el.com>
cc:	gregkh@...uxfoundation.org, <mathias.nyman@...ux.intel.com>,
	<stefan.koch10@...il.com>, <hkallweit1@...il.com>,
	<sergei.shtylyov@...entembedded.com>,
	<jonas.hesselmann@...mail.de>, <mingo@...nel.org>,
	<paulmck@...ux.vnet.ibm.com>, <akpm@...ux-foundation.org>,
	<tglx@...utronix.de>, <keescook@...omium.org>,
	<dan.j.williams@...el.com>, <aryabinin@...tuozzo.com>,
	<tj@...nel.org>, <dave@...olabs.net>,
	<nikolay@...ulusnetworks.com>, <dvyukov@...gle.com>,
	<adrien+dev@...ischi.me>, USB list <linux-usb@...r.kernel.org>,
	Kernel development list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: core: add debugobjects support for urb object

On Tue, 24 May 2016 changbin.du@...el.com wrote:

> From: "Du, Changbin" <changbin.du@...el.com>
> 
> Add debugobject support to track the life time of struct urb.
> This feature help us detect violation of urb operations by
> generating a warning message from debugobject core.

I'm pretty sure the USB core already does this tracking.  If it
doesn't, why not make the appropriate changes instead of adding a whole
new infrastructure?

>  And we fix
> the possible issues at runtime to avoid oops if we can.

This is questionable.  Under what conditions do you think you can fix
up a possible issue?

> I have done some tests with some class drivers, no violation
> found in them which is good. Expect this feature can be used
> for debugging future problems.
> 
> Signed-off-by: Du, Changbin <changbin.du@...el.com>
> ---
>  drivers/usb/core/hcd.c |   1 +
>  drivers/usb/core/urb.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/usb.h    |   8 ++++
>  lib/Kconfig.debug      |   8 ++++
>  4 files changed, 130 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 34b837a..a8ea128 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1750,6 +1750,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>  	/* pass ownership to the completion handler */
>  	urb->status = status;
>  
> +	debug_urb_deactivate(urb);
>  	/*
>  	 * We disable local IRQs here avoid possible deadlock because
>  	 * drivers may call spin_lock() to hold lock which might be

> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index c601e25..0d1eccb 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -10,6 +10,100 @@
>  
>  #define to_urb(d) container_of(d, struct urb, kref)
>  
> +#ifdef CONFIG_DEBUG_OBJECTS_URB
> +static struct debug_obj_descr urb_debug_descr;
> +
> +static void *urb_debug_hint(void *addr)
> +{
> +	return ((struct urb *) addr)->complete;
> +}
> +
> +/*
> + * fixup_init is called when:
> + * - an active object is initialized
> + */
> +static bool urb_fixup_init(void *addr, enum debug_obj_state state)
> +{
> +	struct urb *urb = addr;
> +
> +	switch (state) {
> +	case ODEBUG_STATE_ACTIVE:
> +		usb_kill_urb(urb);
> +		debug_object_init(urb, &urb_debug_descr);
> +		return true;
> +	default:
> +		return false;
> +	}
> +}

Is it guaranteed that this routine and the other new ones can be called
only in process context?  I don't think so -- but usb_kill_urb will 
oops if it is called with interrupts disabled.

> @@ -539,10 +635,23 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  		}
>  	}
>  
> -	return usb_hcd_submit_urb(urb, mem_flags);
> +	ret = debug_urb_activate(urb);
> +	if (ret)
> +		return ret;
> +	ret = usb_hcd_submit_urb(urb, mem_flags);
> +	if (ret)
> +		debug_urb_deactivate(urb);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(usb_submit_urb);
>  
> +static inline int __usb_unlink_urb(struct urb *urb, int status)
> +{
> +	debug_urb_deactivate(urb);
> +	return usb_hcd_unlink_urb(urb, status);
> +}

This is a mistake.  The URB is still active until it is given back
to the completion routine.

Alan Stern