netdev - Re: [PATCH -next] net: neigh: use dev_kfree_skb_irq instead of kfree

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]

Message-ID: <79784952-0d15-8a4a-aa8d-590bc243ab5e@virtuozzo.com>
Date:   Thu, 18 Aug 2022 11:00:13 +0200
From:   "Denis V. Lunev" <den@...tuozzo.com>
To:     Yang Yingliang <yangyingliang@...wei.com>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org
Subject: Re: [PATCH -next] net: neigh: use dev_kfree_skb_irq instead of
 kfree_skb()

On 18.08.2022 06:37, 'Yang Yingliang' via den wrote:
> It is not allowed to call kfree_skb() from hardware interrupt
> context or with interrupts being disabled. So replace kfree_skb()
> with dev_kfree_skb_irq() under spin_lock_irqsave().
>
> Fixes: 66ba215cb513 ("neigh: fix possible DoS due to net iface start/stop loop")
> Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> ---
>   net/core/neighbour.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 5b669eb80270..167826200f3e 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -328,7 +328,7 @@ static void pneigh_queue_purge(struct sk_buff_head *list, struct net *net)
>   			__skb_unlink(skb, list);
>   
>   			dev_put(dev);
> -			kfree_skb(skb);
> +			dev_kfree_skb_irq(skb);
>   		}
>   		skb = skb_next;
>   	}

Technically this is pretty much correct, but would it be better to
move all skb to purge into the new list and after that purge
them at once?

what about something like this?

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index aa16a8017c5e..f7e30daa46ae 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -311,6 +311,9 @@ static void pneigh_queue_purge(struct sk_buff_head 
*list, struct net *net)
  {
         unsigned long flags;
         struct sk_buff *skb;
+       struct sk_buff_head tmp;
+
+       skb_queue_head_init(&tmp);

         spin_lock_irqsave(&list->lock, flags);
         skb = skb_peek(list);
@@ -318,12 +321,16 @@ static void pneigh_queue_purge(struct sk_buff_head 
*list, struct net *net)
                 struct sk_buff *skb_next = skb_peek_next(skb, list);
                 if (net == NULL || net == dev_net(skb->dev)) {
                         __skb_unlink(skb, list);
-                       dev_put(skb->dev);
-                       kfree_skb(skb);
+                       __skb_queue_tail(&tmp, skb);
                 }
                 skb = skb_next;
         } while (skb != NULL);
         spin_unlock_irqrestore(&list->lock, flags);
+
+       while ((skb = __skb_dequeue(&tmp)) != NULL) {
+               dev_put(skb->dev);
+               kfree_skb(skb);
+       }
  }

iris ~/src/linux-2.6 $