ACK/Cmnt: [PATCH] irda: Fix lockdep annotations in hashbin_delete().

[Stefan Bader]

On 20.07.2018 18:49, Colin King wrote:
> From: "David S. Miller" <davem at davemloft.net>
> 
> CVE-2017-6348
> 
> A nested lock depth was added to the hasbin_delete() code but it
> doesn't actually work some well and results in tons of lockdep splats.
> 
> Fix the code instead to properly drop the lock around the operation
> and just keep peeking the head of the hashbin queue.
> 
> Reported-by: Dmitry Vyukov <dvyukov at google.com>
> Tested-by: Dmitry Vyukov <dvyukov at google.com>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (clean cherry pick of upstream commit 4c03b862b12f980456f9de92db6d508a4999b788)
> Acked-by: Colin Ian King <colin.king at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---

Again, as I am not sure how much creativity the bot can handle, the pick line
should be replaced by:
 (cherry picked from commit 4c03b862b12f980456f9de92db6d508a4999b788)

upstream is implied by not saying anything. when a source is given, I believe it
should be after the SHA1.

-Stefan

>  net/irda/irqueue.c | 34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/net/irda/irqueue.c b/net/irda/irqueue.c
> index acbe61c..160dc89 100644
> --- a/net/irda/irqueue.c
> +++ b/net/irda/irqueue.c
> @@ -383,9 +383,6 @@ hashbin_t *hashbin_new(int type)
>   *    for deallocating this structure if it's complex. If not the user can
>   *    just supply kfree, which should take care of the job.
>   */
> -#ifdef CONFIG_LOCKDEP
> -static int hashbin_lock_depth = 0;
> -#endif
>  int hashbin_delete( hashbin_t* hashbin, FREE_FUNC free_func)
>  {
>  	irda_queue_t* queue;
> @@ -396,22 +393,27 @@ int hashbin_delete( hashbin_t* hashbin, FREE_FUNC free_func)
>  	IRDA_ASSERT(hashbin->magic == HB_MAGIC, return -1;);
>  
>  	/* Synchronize */
> -	if ( hashbin->hb_type & HB_LOCK ) {
> -		spin_lock_irqsave_nested(&hashbin->hb_spinlock, flags,
> -					 hashbin_lock_depth++);
> -	}
> +	if (hashbin->hb_type & HB_LOCK)
> +		spin_lock_irqsave(&hashbin->hb_spinlock, flags);
>  
>  	/*
>  	 *  Free the entries in the hashbin, TODO: use hashbin_clear when
>  	 *  it has been shown to work
>  	 */
>  	for (i = 0; i < HASHBIN_SIZE; i ++ ) {
> -		queue = dequeue_first((irda_queue_t**) &hashbin->hb_queue[i]);
> -		while (queue ) {
> -			if (free_func)
> -				(*free_func)(queue);
> -			queue = dequeue_first(
> -				(irda_queue_t**) &hashbin->hb_queue[i]);
> +		while (1) {
> +			queue = dequeue_first((irda_queue_t**) &hashbin->hb_queue[i]);
> +
> +			if (!queue)
> +				break;
> +
> +			if (free_func) {
> +				if (hashbin->hb_type & HB_LOCK)
> +					spin_unlock_irqrestore(&hashbin->hb_spinlock, flags);
> +				free_func(queue);
> +				if (hashbin->hb_type & HB_LOCK)
> +					spin_lock_irqsave(&hashbin->hb_spinlock, flags);
> +			}
>  		}
>  	}
>  
> @@ -420,12 +422,8 @@ int hashbin_delete( hashbin_t* hashbin, FREE_FUNC free_func)
>  	hashbin->magic = ~HB_MAGIC;
>  
>  	/* Release lock */
> -	if ( hashbin->hb_type & HB_LOCK) {
> +	if (hashbin->hb_type & HB_LOCK)
>  		spin_unlock_irqrestore(&hashbin->hb_spinlock, flags);
> -#ifdef CONFIG_LOCKDEP
> -		hashbin_lock_depth--;
> -#endif
> -	}
>  
>  	/*
>  	 *  Free the hashbin structure
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180723/731c93a3/attachment.sig>