* [PATCH net] ipvlan: fix various issues in ipvlan_process_multicast()
@ 2016-12-21 21:30 Eric Dumazet
2016-12-21 22:21 ` Mahesh Bandewar (महेश बंडेवार)
2016-12-22 2:00 ` [PATCH v2 " Eric Dumazet
0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2016-12-21 21:30 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Mahesh Bandewar
From: Eric Dumazet <edumazet@google.com>
1) netif_rx() / dev_forward_skb() should not be called from process
context.
2) ipvlan_count_rx() should be called with preemption disabled.
3) We should check if ipvlan->dev is up before feeding packets
to netif_rx()
4) We need to prevent device from disappearing if some packets
are in the multicast backlog.
5) One kfree_skb() should be a consume_skb() eventually
Fixes: ba35f8588f47 ("ipvlan: Defer multicast / broadcast processing to a work-queue")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Mahesh Bandewar <maheshb@google.com>
---
drivers/net/ipvlan/ipvlan_core.c | 38 +++++++++++++++++++----------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
index b4e990743e1da0cb3a946f5473d02cce7447bd1a..ea6bc1e12cdf6827a69d8d54d96b4b59106ede96 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -207,6 +207,9 @@ void ipvlan_process_multicast(struct work_struct *work)
spin_unlock_bh(&port->backlog.lock);
while ((skb = __skb_dequeue(&list)) != NULL) {
+ struct net_device *dev = skb->dev;
+ bool consumed = false;
+
ethh = eth_hdr(skb);
hlocal = ether_addr_equal(ethh->h_source, port->dev->dev_addr);
mac_hash = ipvlan_mac_hash(ethh->h_dest);
@@ -219,27 +222,29 @@ void ipvlan_process_multicast(struct work_struct *work)
dlocal = false;
rcu_read_lock();
list_for_each_entry_rcu(ipvlan, &port->ipvlans, pnode) {
- if (hlocal && (ipvlan->dev == skb->dev)) {
+ if (hlocal && (ipvlan->dev == dev)) {
dlocal = true;
continue;
}
if (!test_bit(mac_hash, ipvlan->mac_filters))
continue;
-
+ if (!(ipvlan->dev->flags & IFF_UP))
+ continue;
ret = NET_RX_DROP;
len = skb->len + ETH_HLEN;
nskb = skb_clone(skb, GFP_ATOMIC);
- if (!nskb)
- goto acct;
-
- nskb->pkt_type = pkt_type;
- nskb->dev = ipvlan->dev;
- if (hlocal)
- ret = dev_forward_skb(ipvlan->dev, nskb);
- else
- ret = netif_rx(nskb);
-acct:
+ local_bh_disable();
+ if (nskb) {
+ consumed = true;
+ nskb->pkt_type = pkt_type;
+ nskb->dev = ipvlan->dev;
+ if (hlocal)
+ ret = dev_forward_skb(ipvlan->dev, nskb);
+ else
+ ret = netif_rx(nskb);
+ }
ipvlan_count_rx(ipvlan, len, ret == NET_RX_SUCCESS, true);
+ local_bh_enable();
}
rcu_read_unlock();
@@ -249,8 +254,13 @@ void ipvlan_process_multicast(struct work_struct *work)
skb->pkt_type = pkt_type;
dev_queue_xmit(skb);
} else {
- kfree_skb(skb);
+ if (consumed)
+ consume_skb(skb);
+ else
+ kfree_skb(skb);
}
+ if (dev)
+ dev_put(dev);
}
}
@@ -479,6 +489,8 @@ static void ipvlan_multicast_enqueue(struct ipvl_port *port,
spin_lock(&port->backlog.lock);
if (skb_queue_len(&port->backlog) < IPVLAN_QBACKLOG_LIMIT) {
+ if (skb->dev)
+ dev_hold(skb->dev);
__skb_queue_tail(&port->backlog, skb);
spin_unlock(&port->backlog.lock);
schedule_work(&port->wq);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] ipvlan: fix various issues in ipvlan_process_multicast()
2016-12-21 21:30 [PATCH net] ipvlan: fix various issues in ipvlan_process_multicast() Eric Dumazet
@ 2016-12-21 22:21 ` Mahesh Bandewar (महेश बंडेवार)
2016-12-22 1:58 ` Eric Dumazet
2016-12-22 2:00 ` [PATCH v2 " Eric Dumazet
1 sibling, 1 reply; 5+ messages in thread
From: Mahesh Bandewar (महेश बंडेवार) @ 2016-12-21 22:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Wed, Dec 21, 2016 at 1:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> 1) netif_rx() / dev_forward_skb() should not be called from process
> context.
>
> 2) ipvlan_count_rx() should be called with preemption disabled.
>
> 3) We should check if ipvlan->dev is up before feeding packets
> to netif_rx()
>
> 4) We need to prevent device from disappearing if some packets
> are in the multicast backlog.
>
> 5) One kfree_skb() should be a consume_skb() eventually
>
Thank you Eric for all these fixes.
> Fixes: ba35f8588f47 ("ipvlan: Defer multicast / broadcast processing to a work-queue")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Mahesh Bandewar <maheshb@google.com>
> Cc: Mahesh Bandewar <maheshb@google.com>
> ---
> drivers/net/ipvlan/ipvlan_core.c | 38 +++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
> index b4e990743e1da0cb3a946f5473d02cce7447bd1a..ea6bc1e12cdf6827a69d8d54d96b4b59106ede96 100644
> --- a/drivers/net/ipvlan/ipvlan_core.c
> +++ b/drivers/net/ipvlan/ipvlan_core.c
> @@ -207,6 +207,9 @@ void ipvlan_process_multicast(struct work_struct *work)
> spin_unlock_bh(&port->backlog.lock);
>
> while ((skb = __skb_dequeue(&list)) != NULL) {
> + struct net_device *dev = skb->dev;
> + bool consumed = false;
> +
> ethh = eth_hdr(skb);
> hlocal = ether_addr_equal(ethh->h_source, port->dev->dev_addr);
> mac_hash = ipvlan_mac_hash(ethh->h_dest);
> @@ -219,27 +222,29 @@ void ipvlan_process_multicast(struct work_struct *work)
> dlocal = false;
> rcu_read_lock();
> list_for_each_entry_rcu(ipvlan, &port->ipvlans, pnode) {
> - if (hlocal && (ipvlan->dev == skb->dev)) {
> + if (hlocal && (ipvlan->dev == dev)) {
> dlocal = true;
> continue;
> }
> if (!test_bit(mac_hash, ipvlan->mac_filters))
> continue;
> -
> + if (!(ipvlan->dev->flags & IFF_UP))
> + continue;
> ret = NET_RX_DROP;
> len = skb->len + ETH_HLEN;
> nskb = skb_clone(skb, GFP_ATOMIC);
> - if (!nskb)
> - goto acct;
> -
> - nskb->pkt_type = pkt_type;
> - nskb->dev = ipvlan->dev;
> - if (hlocal)
> - ret = dev_forward_skb(ipvlan->dev, nskb);
> - else
> - ret = netif_rx(nskb);
> -acct:
> + local_bh_disable();
> + if (nskb) {
> + consumed = true;
> + nskb->pkt_type = pkt_type;
> + nskb->dev = ipvlan->dev;
> + if (hlocal)
> + ret = dev_forward_skb(ipvlan->dev, nskb);
> + else
> + ret = netif_rx(nskb);
> + }
> ipvlan_count_rx(ipvlan, len, ret == NET_RX_SUCCESS, true);
> + local_bh_enable();
> }
> rcu_read_unlock();
>
> @@ -249,8 +254,13 @@ void ipvlan_process_multicast(struct work_struct *work)
> skb->pkt_type = pkt_type;
> dev_queue_xmit(skb);
> } else {
> - kfree_skb(skb);
> + if (consumed)
> + consume_skb(skb);
> + else
> + kfree_skb(skb);
> }
> + if (dev)
> + dev_put(dev);
> }
> }
>
> @@ -479,6 +489,8 @@ static void ipvlan_multicast_enqueue(struct ipvl_port *port,
>
> spin_lock(&port->backlog.lock);
> if (skb_queue_len(&port->backlog) < IPVLAN_QBACKLOG_LIMIT) {
> + if (skb->dev)
> + dev_hold(skb->dev);
> __skb_queue_tail(&port->backlog, skb);
> spin_unlock(&port->backlog.lock);
> schedule_work(&port->wq);
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] ipvlan: fix various issues in ipvlan_process_multicast()
2016-12-21 22:21 ` Mahesh Bandewar (महेश बंडेवार)
@ 2016-12-22 1:58 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2016-12-22 1:58 UTC (permalink / raw)
To: Mahesh Bandewar (महेश बंडेवार)
Cc: David Miller, netdev
On Wed, 2016-12-21 at 14:21 -0800, Mahesh Bandewar (महेश बंडेवार) wrote:
> >
> Thank you Eric for all these fixes.
Thanks Mahesh. I will send a V2 to include this part as well :
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 693ec5b6622233cd3a28c64c11d6abb97585318b..8b0f99300cbc97d8c8b93c3dfa99cd841914c086 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -135,6 +135,7 @@ static int ipvlan_port_create(struct net_device *dev)
static void ipvlan_port_destroy(struct net_device *dev)
{
struct ipvl_port *port = ipvlan_port_get_rtnl(dev);
+ struct sk_buff *skb;
dev->priv_flags &= ~IFF_IPVLAN_MASTER;
if (port->mode == IPVLAN_MODE_L3S) {
@@ -144,7 +145,11 @@ static void ipvlan_port_destroy(struct net_device *dev)
}
netdev_rx_handler_unregister(dev);
cancel_work_sync(&port->wq);
- __skb_queue_purge(&port->backlog);
+ while ((skb = __skb_dequeue(&port->backlog)) != NULL) {
+ if (skb->dev)
+ dev_put(skb->dev);
+ kfree_skb(skb);
+ }
kfree(port);
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 net] ipvlan: fix various issues in ipvlan_process_multicast()
2016-12-21 21:30 [PATCH net] ipvlan: fix various issues in ipvlan_process_multicast() Eric Dumazet
2016-12-21 22:21 ` Mahesh Bandewar (महेश बंडेवार)
@ 2016-12-22 2:00 ` Eric Dumazet
2016-12-22 16:20 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2016-12-22 2:00 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Mahesh Bandewar
From: Eric Dumazet <edumazet@google.com>
1) netif_rx() / dev_forward_skb() should not be called from process
context.
2) ipvlan_count_rx() should be called with preemption disabled.
3) We should check if ipvlan->dev is up before feeding packets
to netif_rx()
4) We need to prevent device from disappearing if some packets
are in the multicast backlog.
5) One kfree_skb() should be a consume_skb() eventually
Fixes: ba35f8588f47 ("ipvlan: Defer multicast / broadcast processing to
a work-queue")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Mahesh Bandewar <maheshb@google.com>
---
v2: did the correct purge in ipvlan_port_destroy()
drivers/net/ipvlan/ipvlan_core.c | 38 +++++++++++++++++++----------
drivers/net/ipvlan/ipvlan_main.c | 7 ++++-
2 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
index b4e990743e1da0cb3a946f5473d02cce7447bd1a..ea6bc1e12cdf6827a69d8d54d96b4b59106ede96 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -207,6 +207,9 @@ void ipvlan_process_multicast(struct work_struct *work)
spin_unlock_bh(&port->backlog.lock);
while ((skb = __skb_dequeue(&list)) != NULL) {
+ struct net_device *dev = skb->dev;
+ bool consumed = false;
+
ethh = eth_hdr(skb);
hlocal = ether_addr_equal(ethh->h_source, port->dev->dev_addr);
mac_hash = ipvlan_mac_hash(ethh->h_dest);
@@ -219,27 +222,29 @@ void ipvlan_process_multicast(struct work_struct *work)
dlocal = false;
rcu_read_lock();
list_for_each_entry_rcu(ipvlan, &port->ipvlans, pnode) {
- if (hlocal && (ipvlan->dev == skb->dev)) {
+ if (hlocal && (ipvlan->dev == dev)) {
dlocal = true;
continue;
}
if (!test_bit(mac_hash, ipvlan->mac_filters))
continue;
-
+ if (!(ipvlan->dev->flags & IFF_UP))
+ continue;
ret = NET_RX_DROP;
len = skb->len + ETH_HLEN;
nskb = skb_clone(skb, GFP_ATOMIC);
- if (!nskb)
- goto acct;
-
- nskb->pkt_type = pkt_type;
- nskb->dev = ipvlan->dev;
- if (hlocal)
- ret = dev_forward_skb(ipvlan->dev, nskb);
- else
- ret = netif_rx(nskb);
-acct:
+ local_bh_disable();
+ if (nskb) {
+ consumed = true;
+ nskb->pkt_type = pkt_type;
+ nskb->dev = ipvlan->dev;
+ if (hlocal)
+ ret = dev_forward_skb(ipvlan->dev, nskb);
+ else
+ ret = netif_rx(nskb);
+ }
ipvlan_count_rx(ipvlan, len, ret == NET_RX_SUCCESS, true);
+ local_bh_enable();
}
rcu_read_unlock();
@@ -249,8 +254,13 @@ void ipvlan_process_multicast(struct work_struct *work)
skb->pkt_type = pkt_type;
dev_queue_xmit(skb);
} else {
- kfree_skb(skb);
+ if (consumed)
+ consume_skb(skb);
+ else
+ kfree_skb(skb);
}
+ if (dev)
+ dev_put(dev);
}
}
@@ -479,6 +489,8 @@ static void ipvlan_multicast_enqueue(struct ipvl_port *port,
spin_lock(&port->backlog.lock);
if (skb_queue_len(&port->backlog) < IPVLAN_QBACKLOG_LIMIT) {
+ if (skb->dev)
+ dev_hold(skb->dev);
__skb_queue_tail(&port->backlog, skb);
spin_unlock(&port->backlog.lock);
schedule_work(&port->wq);
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 693ec5b6622233cd3a28c64c11d6abb97585318b..8b0f99300cbc97d8c8b93c3dfa99cd841914c086 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -135,6 +135,7 @@ static int ipvlan_port_create(struct net_device *dev)
static void ipvlan_port_destroy(struct net_device *dev)
{
struct ipvl_port *port = ipvlan_port_get_rtnl(dev);
+ struct sk_buff *skb;
dev->priv_flags &= ~IFF_IPVLAN_MASTER;
if (port->mode == IPVLAN_MODE_L3S) {
@@ -144,7 +145,11 @@ static void ipvlan_port_destroy(struct net_device *dev)
}
netdev_rx_handler_unregister(dev);
cancel_work_sync(&port->wq);
- __skb_queue_purge(&port->backlog);
+ while ((skb = __skb_dequeue(&port->backlog)) != NULL) {
+ if (skb->dev)
+ dev_put(skb->dev);
+ kfree_skb(skb);
+ }
kfree(port);
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net] ipvlan: fix various issues in ipvlan_process_multicast()
2016-12-22 2:00 ` [PATCH v2 " Eric Dumazet
@ 2016-12-22 16:20 ` David Miller
0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2016-12-22 16:20 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, maheshb
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 21 Dec 2016 18:00:24 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> 1) netif_rx() / dev_forward_skb() should not be called from process
> context.
>
> 2) ipvlan_count_rx() should be called with preemption disabled.
>
> 3) We should check if ipvlan->dev is up before feeding packets
> to netif_rx()
>
> 4) We need to prevent device from disappearing if some packets
> are in the multicast backlog.
>
> 5) One kfree_skb() should be a consume_skb() eventually
>
> Fixes: ba35f8588f47 ("ipvlan: Defer multicast / broadcast processing to
> a work-queue")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Mahesh Bandewar <maheshb@google.com>
Applied.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-12-22 16:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-21 21:30 [PATCH net] ipvlan: fix various issues in ipvlan_process_multicast() Eric Dumazet
2016-12-21 22:21 ` Mahesh Bandewar (महेश बंडेवार)
2016-12-22 1:58 ` Eric Dumazet
2016-12-22 2:00 ` [PATCH v2 " Eric Dumazet
2016-12-22 16:20 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).