linux-kernel - Re: [PATCH V2 1/1] staging: usbip: remove an unnecessary lock in usbip_event

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] [day] [month] [year] [list]

Date:	Mon, 5 Nov 2012 11:04:08 +0100
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	"harvey.yang" <harvey.huawei.yang@...il.com>
Cc:	Matt Mooney <mfm@...eddisk.com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 1/1] staging: usbip: remove an unnecessary lock in
 usbip_event_happened.

On Mon, Nov 05, 2012 at 05:51:11PM +0800, harvey.yang wrote:
> From: Harvey.Yang <harvey.huawei.yang@...il.com>
> 
> 
> Signed-off-by: Harvey.Yang <harvey.huawei.yang@...il.com>
> ---
>  drivers/staging/usbip/usbip_common.h |    2 +-
>  drivers/staging/usbip/usbip_event.c  |   11 ++---------
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/usbip/usbip_common.h b/drivers/staging/usbip/usbip_common.h
> index 5d89c0f..b6d7add 100644
> --- a/drivers/staging/usbip/usbip_common.h
> +++ b/drivers/staging/usbip/usbip_common.h
> @@ -330,7 +330,7 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb);
>  int usbip_start_eh(struct usbip_device *ud);
>  void usbip_stop_eh(struct usbip_device *ud);
>  void usbip_event_add(struct usbip_device *ud, unsigned long event);
> -int usbip_event_happened(struct usbip_device *ud);
> +inline int usbip_event_happened(struct usbip_device *ud);
>  
>  static inline int interface_to_busnum(struct usb_interface *interface)
>  {
> diff --git a/drivers/staging/usbip/usbip_event.c b/drivers/staging/usbip/usbip_event.c
> index d332a34..15871f0 100644
> --- a/drivers/staging/usbip/usbip_event.c
> +++ b/drivers/staging/usbip/usbip_event.c
> @@ -112,15 +112,8 @@ void usbip_event_add(struct usbip_device *ud, unsigned long event)
>  }
>  EXPORT_SYMBOL_GPL(usbip_event_add);
>  
> -int usbip_event_happened(struct usbip_device *ud)
> +inline int usbip_event_happened(struct usbip_device *ud)
>  {
> -	int happened = 0;
> -
> -	spin_lock(&ud->lock);
> -	if (ud->event != 0)
> -		happened = 1;
> -	spin_unlock(&ud->lock);
> -
> -	return happened;
> +	return ud->event ? 1 : 0;

No, you aren't locking to see if something changed with ud->event here.

Actually, that's pretty broken anyway, what is to say that event doesn't
change right after this call?  What exactly is this call supposed to be
doing?  Who is calling it?

>  }
>  EXPORT_SYMBOL_GPL(usbip_event_happened);

You can't inline an exported symbol, how would that work?

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/