* [PATCH -next] net: neigh: use dev_kfree_skb_irq instead of kfree_skb()
@ 2022-08-18 4:37 Yang Yingliang
2022-08-18 9:00 ` Denis V. Lunev
0 siblings, 1 reply; 6+ messages in thread
From: Yang Yingliang @ 2022-08-18 4:37 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: den, davem, edumazet, kuba
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@huawei.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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -next] net: neigh: use dev_kfree_skb_irq instead of kfree_skb()
2022-08-18 4:37 [PATCH -next] net: neigh: use dev_kfree_skb_irq instead of kfree_skb() Yang Yingliang
@ 2022-08-18 9:00 ` Denis V. Lunev
2022-08-18 10:47 ` Yang Yingliang
2022-08-18 16:32 ` Jakub Kicinski
0 siblings, 2 replies; 6+ messages in thread
From: Denis V. Lunev @ 2022-08-18 9:00 UTC (permalink / raw)
To: Yang Yingliang, linux-kernel, netdev; +Cc: davem, edumazet, kuba
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@huawei.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 $
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -next] net: neigh: use dev_kfree_skb_irq instead of kfree_skb()
2022-08-18 9:00 ` Denis V. Lunev
@ 2022-08-18 10:47 ` Yang Yingliang
2022-08-18 16:32 ` Jakub Kicinski
1 sibling, 0 replies; 6+ messages in thread
From: Yang Yingliang @ 2022-08-18 10:47 UTC (permalink / raw)
To: Denis V. Lunev, linux-kernel, netdev; +Cc: davem, edumazet, kuba
Hi,
On 2022/8/18 17:00, Denis V. Lunev wrote:
> 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@huawei.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);
> + }
> }
It's better, I can send a v2 later.
Thanks,
Yang
>
> iris ~/src/linux-2.6 $
>
> .
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] net: neigh: use dev_kfree_skb_irq instead of kfree_skb()
2022-08-18 9:00 ` Denis V. Lunev
2022-08-18 10:47 ` Yang Yingliang
@ 2022-08-18 16:32 ` Jakub Kicinski
2022-08-19 1:44 ` Yang Yingliang
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2022-08-18 16:32 UTC (permalink / raw)
To: Yang Yingliang; +Cc: Denis V. Lunev, linux-kernel, netdev, davem, edumazet
Please put [PATCH net] as the tag for v2, this is a fix, not -next
material.
On Thu, 18 Aug 2022 11:00:13 +0200 Denis V. Lunev wrote:
> unsigned long flags;
> struct sk_buff *skb;
> + struct sk_buff_head tmp;
reverse xmas tree, so tmp should be declared before the shorter lines
> + 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);
while at it let's add an empty line here
> 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) {
No need to compare pointers to NULL
> + dev_put(skb->dev);
> + kfree_skb(skb);
> + }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] net: neigh: use dev_kfree_skb_irq instead of kfree_skb()
2022-08-18 16:32 ` Jakub Kicinski
@ 2022-08-19 1:44 ` Yang Yingliang
2022-08-19 2:39 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Yang Yingliang @ 2022-08-19 1:44 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Denis V. Lunev, linux-kernel, netdev, davem, edumazet
On 2022/8/19 0:32, Jakub Kicinski wrote:
> Please put [PATCH net] as the tag for v2, this is a fix, not -next
> material.
OK.
I don't find the commit 66ba215cb513 ("neigh: fix possible DoS due to
net iface start/stop loop")
on linux/master, so made the patch based on linux-next/master, and add
-next.
Later I will send a v2 based on net/master.
Thanks,
Yang
>
> On Thu, 18 Aug 2022 11:00:13 +0200 Denis V. Lunev wrote:
>> unsigned long flags;
>> struct sk_buff *skb;
>> + struct sk_buff_head tmp;
> reverse xmas tree, so tmp should be declared before the shorter lines
>
>> + 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);
> while at it let's add an empty line here
>
>> 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) {
> No need to compare pointers to NULL
>
>> + dev_put(skb->dev);
>> + kfree_skb(skb);
>> + }
> .
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] net: neigh: use dev_kfree_skb_irq instead of kfree_skb()
2022-08-19 1:44 ` Yang Yingliang
@ 2022-08-19 2:39 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2022-08-19 2:39 UTC (permalink / raw)
To: Yang Yingliang; +Cc: Denis V. Lunev, linux-kernel, netdev, davem, edumazet
On Fri, 19 Aug 2022 09:44:29 +0800 Yang Yingliang wrote:
> On 2022/8/19 0:32, Jakub Kicinski wrote:
> > Please put [PATCH net] as the tag for v2, this is a fix, not -next
> > material.
> OK.
> I don't find the commit 66ba215cb513 ("neigh: fix possible DoS due to
> net iface start/stop loop")
I see where the confusion is coming from. It's too fresh to have made
it to Linus. It's part of this pull request:
https://lore.kernel.org/all/20220818195549.1805709-1-kuba@kernel.org/
but Linus has not pulled yet.
I believe if something is in the "pending-fixes" branch of linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?h=pending-fixes
you can consider it to be an immediate fix, rather than -next material.
Not sure if that makes sense but that's the best I can do explaining
it...
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-19 2:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-18 4:37 [PATCH -next] net: neigh: use dev_kfree_skb_irq instead of kfree_skb() Yang Yingliang
2022-08-18 9:00 ` Denis V. Lunev
2022-08-18 10:47 ` Yang Yingliang
2022-08-18 16:32 ` Jakub Kicinski
2022-08-19 1:44 ` Yang Yingliang
2022-08-19 2:39 ` Jakub Kicinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.