* [Patch net 1/3] hsr: fix a memory leak in hsr_del_port()
2019-07-04 0:21 [Patch net 0/3] hsr: a few bug fixes Cong Wang
@ 2019-07-04 0:21 ` Cong Wang
2019-07-04 0:21 ` [Patch net 2/3] hsr: implement dellink to clean up resources Cong Wang
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Cong Wang @ 2019-07-04 0:21 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Arvid Brodin
hsr_del_port() should release all the resources allocated
in hsr_add_port().
As a consequence of this change, hsr_for_each_port() is no
longer safe to work with hsr_del_port(), switch to
list_for_each_entry_safe() as we always hold RTNL lock.
Cc: Arvid Brodin <arvid.brodin@alten.se>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/hsr/hsr_device.c | 6 ++++--
net/hsr/hsr_slave.c | 1 +
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 15c72065df79..f48b6a275cf0 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -351,13 +351,14 @@ static void hsr_dev_destroy(struct net_device *hsr_dev)
{
struct hsr_priv *hsr;
struct hsr_port *port;
+ struct hsr_port *tmp;
hsr = netdev_priv(hsr_dev);
hsr_debugfs_term(hsr);
rtnl_lock();
- hsr_for_each_port(hsr, port)
+ list_for_each_entry_safe(port, tmp, &hsr->ports, port_list)
hsr_del_port(port);
rtnl_unlock();
@@ -428,6 +429,7 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
{
struct hsr_priv *hsr;
struct hsr_port *port;
+ struct hsr_port *tmp;
int res;
hsr = netdev_priv(hsr_dev);
@@ -492,7 +494,7 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
return 0;
fail:
- hsr_for_each_port(hsr, port)
+ list_for_each_entry_safe(port, tmp, &hsr->ports, port_list)
hsr_del_port(port);
err_add_port:
hsr_del_node(&hsr->self_node_db);
diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index 88b6705ded83..ee561297d8a7 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -193,4 +193,5 @@ void hsr_del_port(struct hsr_port *port)
if (port != master)
dev_put(port->dev);
+ kfree(port);
}
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [Patch net 2/3] hsr: implement dellink to clean up resources
2019-07-04 0:21 [Patch net 0/3] hsr: a few bug fixes Cong Wang
2019-07-04 0:21 ` [Patch net 1/3] hsr: fix a memory leak in hsr_del_port() Cong Wang
@ 2019-07-04 0:21 ` Cong Wang
2019-07-04 0:21 ` [Patch net 3/3] hsr: fix a NULL pointer deref in hsr_dev_xmit() Cong Wang
2019-07-05 22:22 ` [Patch net 0/3] hsr: a few bug fixes David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Cong Wang @ 2019-07-04 0:21 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, syzbot+c6167ec3de7def23d1e8, Arvid Brodin
hsr_link_ops implements ->newlink() but not ->dellink(),
which leads that resources not released after removing the device,
particularly the entries in self_node_db and node_db.
So add ->dellink() implementation to replace the priv_destructor.
This also makes the code slightly easier to understand.
Reported-by: syzbot+c6167ec3de7def23d1e8@syzkaller.appspotmail.com
Cc: Arvid Brodin <arvid.brodin@alten.se>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/hsr/hsr_device.c | 13 +++++--------
net/hsr/hsr_device.h | 1 +
net/hsr/hsr_framereg.c | 11 ++++++++++-
net/hsr/hsr_framereg.h | 3 ++-
net/hsr/hsr_netlink.c | 7 +++++++
5 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index f48b6a275cf0..4ea7d54a8262 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -344,10 +344,7 @@ static void hsr_announce(struct timer_list *t)
rcu_read_unlock();
}
-/* According to comments in the declaration of struct net_device, this function
- * is "Called from unregister, can be used to call free_netdev". Ok then...
- */
-static void hsr_dev_destroy(struct net_device *hsr_dev)
+void hsr_dev_destroy(struct net_device *hsr_dev)
{
struct hsr_priv *hsr;
struct hsr_port *port;
@@ -357,15 +354,16 @@ static void hsr_dev_destroy(struct net_device *hsr_dev)
hsr_debugfs_term(hsr);
- rtnl_lock();
list_for_each_entry_safe(port, tmp, &hsr->ports, port_list)
hsr_del_port(port);
- rtnl_unlock();
del_timer_sync(&hsr->prune_timer);
del_timer_sync(&hsr->announce_timer);
synchronize_rcu();
+
+ hsr_del_self_node(&hsr->self_node_db);
+ hsr_del_nodes(&hsr->node_db);
}
static const struct net_device_ops hsr_device_ops = {
@@ -392,7 +390,6 @@ void hsr_dev_setup(struct net_device *dev)
dev->priv_flags |= IFF_NO_QUEUE;
dev->needs_free_netdev = true;
- dev->priv_destructor = hsr_dev_destroy;
dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
NETIF_F_GSO_MASK | NETIF_F_HW_CSUM |
@@ -497,7 +494,7 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
list_for_each_entry_safe(port, tmp, &hsr->ports, port_list)
hsr_del_port(port);
err_add_port:
- hsr_del_node(&hsr->self_node_db);
+ hsr_del_self_node(&hsr->self_node_db);
return res;
}
diff --git a/net/hsr/hsr_device.h b/net/hsr/hsr_device.h
index 6d7759c4f5f9..d0fa6b0696d2 100644
--- a/net/hsr/hsr_device.h
+++ b/net/hsr/hsr_device.h
@@ -14,6 +14,7 @@
void hsr_dev_setup(struct net_device *dev);
int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
unsigned char multicast_spec, u8 protocol_version);
+void hsr_dev_destroy(struct net_device *hsr_dev);
void hsr_check_carrier_and_operstate(struct hsr_priv *hsr);
bool is_hsr_master(struct net_device *dev);
int hsr_get_max_mtu(struct hsr_priv *hsr);
diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index 2d7a19750436..292be446007b 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -104,7 +104,7 @@ int hsr_create_self_node(struct list_head *self_node_db,
return 0;
}
-void hsr_del_node(struct list_head *self_node_db)
+void hsr_del_self_node(struct list_head *self_node_db)
{
struct hsr_node *node;
@@ -117,6 +117,15 @@ void hsr_del_node(struct list_head *self_node_db)
}
}
+void hsr_del_nodes(struct list_head *node_db)
+{
+ struct hsr_node *node;
+ struct hsr_node *tmp;
+
+ list_for_each_entry_safe(node, tmp, node_db, mac_list)
+ kfree(node);
+}
+
/* Allocate an hsr_node and add it to node_db. 'addr' is the node's address_A;
* seq_out is used to initialize filtering of outgoing duplicate frames
* originating from the newly added node.
diff --git a/net/hsr/hsr_framereg.h b/net/hsr/hsr_framereg.h
index a3bdcdab469d..89a3ce38151d 100644
--- a/net/hsr/hsr_framereg.h
+++ b/net/hsr/hsr_framereg.h
@@ -12,7 +12,8 @@
struct hsr_node;
-void hsr_del_node(struct list_head *self_node_db);
+void hsr_del_self_node(struct list_head *self_node_db);
+void hsr_del_nodes(struct list_head *node_db);
struct hsr_node *hsr_add_node(struct list_head *node_db, unsigned char addr[],
u16 seq_out);
struct hsr_node *hsr_get_node(struct hsr_port *port, struct sk_buff *skb,
diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c
index 8f8337f893ba..160edd24de4e 100644
--- a/net/hsr/hsr_netlink.c
+++ b/net/hsr/hsr_netlink.c
@@ -69,6 +69,12 @@ static int hsr_newlink(struct net *src_net, struct net_device *dev,
return hsr_dev_finalize(dev, link, multicast_spec, hsr_version);
}
+static void hsr_dellink(struct net_device *hsr_dev, struct list_head *head)
+{
+ hsr_dev_destroy(hsr_dev);
+ unregister_netdevice_queue(hsr_dev, head);
+}
+
static int hsr_fill_info(struct sk_buff *skb, const struct net_device *dev)
{
struct hsr_priv *hsr;
@@ -113,6 +119,7 @@ static struct rtnl_link_ops hsr_link_ops __read_mostly = {
.priv_size = sizeof(struct hsr_priv),
.setup = hsr_dev_setup,
.newlink = hsr_newlink,
+ .dellink = hsr_dellink,
.fill_info = hsr_fill_info,
};
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [Patch net 3/3] hsr: fix a NULL pointer deref in hsr_dev_xmit()
2019-07-04 0:21 [Patch net 0/3] hsr: a few bug fixes Cong Wang
2019-07-04 0:21 ` [Patch net 1/3] hsr: fix a memory leak in hsr_del_port() Cong Wang
2019-07-04 0:21 ` [Patch net 2/3] hsr: implement dellink to clean up resources Cong Wang
@ 2019-07-04 0:21 ` Cong Wang
2019-07-05 22:22 ` [Patch net 0/3] hsr: a few bug fixes David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Cong Wang @ 2019-07-04 0:21 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Arvid Brodin
hsr_port_get_hsr() could return NULL and kernel
could crash:
BUG: kernel NULL pointer dereference, address: 0000000000000010
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 8000000074b84067 P4D 8000000074b84067 PUD 7057d067 PMD 0
Oops: 0000 [#1] SMP PTI
CPU: 0 PID: 754 Comm: a.out Not tainted 5.2.0-rc6+ #718
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-2.fc30 04/01/2014
RIP: 0010:hsr_dev_xmit+0x20/0x31
Code: 48 8b 1b eb e0 5b 5d 41 5c c3 66 66 66 66 90 55 48 89 fd 48 8d be 40 0b 00 00 be 04 00 00 00 e8 ee f2 ff ff 48 89 ef 48 89 c6 <48> 8b 40 10 48 89 45 10 e8 6c 1b 00 00 31 c0 5d c3 66 66 66 66 90
RSP: 0018:ffffb5b400003c48 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff9821b4509a88 RCX: 0000000000000000
RDX: ffff9821b4509a88 RSI: 0000000000000000 RDI: ffff9821bc3fc7c0
RBP: ffff9821bc3fc7c0 R08: 0000000000000000 R09: 00000000000c2019
R10: 0000000000000000 R11: 0000000000000002 R12: ffff9821bc3fc7c0
R13: ffff9821b4509a88 R14: 0000000000000000 R15: 000000000000006e
FS: 00007fee112a1800(0000) GS:ffff9821bd800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000010 CR3: 000000006e9ce000 CR4: 00000000000406f0
Call Trace:
<IRQ>
netdev_start_xmit+0x1b/0x38
dev_hard_start_xmit+0x121/0x21e
? validate_xmit_skb.isra.0+0x19/0x1e3
__dev_queue_xmit+0x74c/0x823
? lockdep_hardirqs_on+0x12b/0x17d
ip6_finish_output2+0x3d3/0x42c
? ip6_mtu+0x55/0x5c
? mld_sendpack+0x191/0x229
mld_sendpack+0x191/0x229
mld_ifc_timer_expire+0x1f7/0x230
? mld_dad_timer_expire+0x58/0x58
call_timer_fn+0x12e/0x273
__run_timers.part.0+0x174/0x1b5
? mld_dad_timer_expire+0x58/0x58
? sched_clock_cpu+0x10/0xad
? mark_lock+0x26/0x1f2
? __lock_is_held+0x40/0x71
run_timer_softirq+0x26/0x48
__do_softirq+0x1af/0x392
irq_exit+0x53/0xa2
smp_apic_timer_interrupt+0x1c4/0x1d9
apic_timer_interrupt+0xf/0x20
</IRQ>
Cc: Arvid Brodin <arvid.brodin@alten.se>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/hsr/hsr_device.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 4ea7d54a8262..f0f9b493c47b 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -227,9 +227,13 @@ static int hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
struct hsr_port *master;
master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
- skb->dev = master->dev;
- hsr_forward_skb(skb, master);
-
+ if (master) {
+ skb->dev = master->dev;
+ hsr_forward_skb(skb, master);
+ } else {
+ atomic_long_inc(&dev->tx_dropped);
+ dev_kfree_skb_any(skb);
+ }
return NETDEV_TX_OK;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread