linux-kernel - Re: [[PATCH staging] 4/7] staging: wfx: annotate nested gc

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

Message-ID: <263570919.qXAG0u9DAH@pc-42>
Date:   Tue, 11 Feb 2020 10:34:30 +0000
From:   Jérôme Pouiller <Jerome.Pouiller@...abs.com>
To:     Michał Mirosław <mirq-linux@...e.qmqm.pl>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [[PATCH staging] 4/7] staging: wfx: annotate nested gc_list vs tx
 queue locking

On Tuesday 11 February 2020 09:46:55 CET Michał Mirosław wrote:
> Lockdep is complaining about recursive locking, because it can't make
> a difference between locked skb_queues. Annotate nested locks and avoid
> double bh_disable/enable.
> 
> [...]
> insmod/815 is trying to acquire lock:
> cb7d6418 (&(&list->lock)->rlock){+...}, at: wfx_tx_queues_clear+0xfc/0x198 [wfx]
> 
> but task is already holding lock:
> cb7d61f4 (&(&list->lock)->rlock){+...}, at: wfx_tx_queues_clear+0xa0/0x198 [wfx]
> 
> [...]
> Possible unsafe locking scenario:
> 
>       CPU0
>       ----
>  lock(&(&list->lock)->rlock);
>  lock(&(&list->lock)->rlock);
> 
> Cc: stable@...r.kernel.org
> Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> ---
>  drivers/staging/wfx/queue.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
> index 0bcc61feee1d..51d6c55ae91f 100644
> --- a/drivers/staging/wfx/queue.c
> +++ b/drivers/staging/wfx/queue.c
> @@ -130,12 +130,12 @@ static void wfx_tx_queue_clear(struct wfx_dev *wdev, struct wfx_queue *queue,
>         spin_lock_bh(&queue->queue.lock);
>         while ((item = __skb_dequeue(&queue->queue)) != NULL)
>                 skb_queue_head(gc_list, item);
> -       spin_lock_bh(&stats->pending.lock);
> +       spin_lock_nested(&stats->pending.lock, 1);
>         for (i = 0; i < ARRAY_SIZE(stats->link_map_cache); ++i) {
>                 stats->link_map_cache[i] -= queue->link_map_cache[i];
>                 queue->link_map_cache[i] = 0;
>         }
> -       spin_unlock_bh(&stats->pending.lock);
> +       spin_unlock(&stats->pending.lock);
>         spin_unlock_bh(&queue->queue.lock);
>  }
> 
> @@ -207,9 +207,9 @@ void wfx_tx_queue_put(struct wfx_dev *wdev, struct wfx_queue *queue,
> 
>         ++queue->link_map_cache[tx_priv->link_id];
> 
> -       spin_lock_bh(&stats->pending.lock);
> +       spin_lock_nested(&stats->pending.lock, 1);
>         ++stats->link_map_cache[tx_priv->link_id];
> -       spin_unlock_bh(&stats->pending.lock);
> +       spin_unlock(&stats->pending.lock);
>         spin_unlock_bh(&queue->queue.lock);
>  }
> 
> @@ -237,11 +237,11 @@ static struct sk_buff *wfx_tx_queue_get(struct wfx_dev *wdev,
>                 __skb_unlink(skb, &queue->queue);
>                 --queue->link_map_cache[tx_priv->link_id];
> 
> -               spin_lock_bh(&stats->pending.lock);
> +               spin_lock_nested(&stats->pending.lock, 1);
>                 __skb_queue_tail(&stats->pending, skb);
>                 if (!--stats->link_map_cache[tx_priv->link_id])
>                         wakeup_stats = true;
> -               spin_unlock_bh(&stats->pending.lock);
> +               spin_unlock(&stats->pending.lock);
>         }
>         spin_unlock_bh(&queue->queue.lock);
>         if (wakeup_stats)
> @@ -259,10 +259,10 @@ int wfx_pending_requeue(struct wfx_dev *wdev, struct sk_buff *skb)
>         spin_lock_bh(&queue->queue.lock);
>         ++queue->link_map_cache[tx_priv->link_id];
> 
> -       spin_lock_bh(&stats->pending.lock);
> +       spin_lock_nested(&stats->pending.lock, 1);
>         ++stats->link_map_cache[tx_priv->link_id];
>         __skb_unlink(skb, &stats->pending);
> -       spin_unlock_bh(&stats->pending.lock);
> +       spin_unlock(&stats->pending.lock);
>         __skb_queue_tail(&queue->queue, skb);
>         spin_unlock_bh(&queue->queue.lock);
>         return 0;
> --
> 2.20.1
> 

Reviewed-by: Jérôme Pouiller <jerome.pouiller@...abs.com>

-- 
Jérôme Pouiller