Skip to content

L
linux
Commit 095e90e0 authored by David S. Miller's avatar David S. Miller
Browse files
Options

Merge branch 'hsr-fix-several-bugs-in-hsr-module' 

Taehee Yoo says:

====================
hsr: fix several bugs in hsr module

1. The first patch fixes debugfs warning when it's opened when hsr module
is being removed. debugfs file is opened, it tries to hold .owner module,
but it would print warning messages if it couldn't hold .owner module.
In order to avoid the warning message, this patch makes hsr module does
not set .owner. Unsetting .owner is safe because these are protected by
inode_lock().

2. The second patch fixes wrong error handling of hsr_dev_finalize()
a) hsr_dev_finalize() calls debugfs_create_{dir/file} to create debugfs.
it checks NULL pointer but debugfs don't return NULL so it's wrong code.
b) hsr_dev_finalize() calls register_netdevice(). so if it fails after
register_netdevice(), it should call unregister_netdevice().
But it doesn't.
c) debugfs doesn't affect any actual logic of hsr module.
So, the failure of creating of debugfs could be ignored.

3. The third patch adds hsr root debugfs directory.
When hsr interface is created, it creates debugfs directory in
/sys/kernel/debug/<interface name>.
It's a little bit faulty path because if an interface is the same with
another directory name in the same path, it will fail. If hsr root
directory is existing, the possibility of failure of creating debugfs
file will be reduced.

4. The fourth patch adds debugfs rename routine.
debugfs directory name is the same with hsr interface name.
So hsr interface name is changed, debugfs directory name should be
changed too.

5. The fifth patch fixes a race condition in node list add and del.
hsr nodes are protected by RCU and there is no write side lock.
But node insertions and deletions could be being operated concurrently.
So write side locking is needed.

6. The Sixth patch resets network header
Tap routine is enabled, below message will be printed.

[  175.852292][    C3] protocol 88fb is buggy, dev veth0

hsr module doesn't set network header for supervision frame.
But tap routine validates network header.
If network header wasn't set, it resets and warns about it.
====================
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parents 57b948e2 3ed0a1d5
Hide whitespace changes
Inline Side-by-side
Showing with 125 additions and 63 deletions
net/hsr/hsr_debugfs.c
View file @ 095e90e0
......@@ -20,6 +20,8 @@
#include "hsr_main.h"
#include "hsr_framereg.h"
static struct dentry *hsr_debugfs_root_dir;
static void print_mac_address(struct seq_file *sfp, unsigned char *mac)
{
seq_printf(sfp, "%02x:%02x:%02x:%02x:%02x:%02x:",
......@@ -63,8 +65,20 @@ hsr_node_table_open(struct inode *inode, struct file *filp)
return single_open(filp, hsr_node_table_show, inode->i_private);
}
void hsr_debugfs_rename(struct net_device *dev)
{
struct hsr_priv *priv = netdev_priv(dev);
struct dentry *d;
d = debugfs_rename(hsr_debugfs_root_dir, priv->node_tbl_root,
hsr_debugfs_root_dir, dev->name);
if (IS_ERR(d))
netdev_warn(dev, "failed to rename\n");
else
priv->node_tbl_root = d;
}
static const struct file_operations hsr_fops = {
.owner = THIS_MODULE,
.open = hsr_node_table_open,
.read = seq_read,
.llseek = seq_lseek,
......@@ -78,15 +92,14 @@ static const struct file_operations hsr_fops = {
* When debugfs is configured this routine sets up the node_table file per
* hsr device for dumping the node_table entries
*/
int hsr_debugfs_init(struct hsr_priv *priv, struct net_device *hsr_dev)
void hsr_debugfs_init(struct hsr_priv *priv, struct net_device *hsr_dev)
{
int rc = -1;
struct dentry *de = NULL;
de = debugfs_create_dir(hsr_dev->name, NULL);
if (!de) {
pr_err("Cannot create hsr debugfs root\n");
return rc;
de = debugfs_create_dir(hsr_dev->name, hsr_debugfs_root_dir);
if (IS_ERR(de)) {
pr_err("Cannot create hsr debugfs directory\n");
return;
}
priv->node_tbl_root = de;
......@@ -94,13 +107,13 @@ int hsr_debugfs_init(struct hsr_priv *priv, struct net_device *hsr_dev)
de = debugfs_create_file("node_table", S_IFREG | 0444,
priv->node_tbl_root, priv,
&hsr_fops);
if (!de) {
pr_err("Cannot create hsr node_table directory\n");
return rc;
if (IS_ERR(de)) {
pr_err("Cannot create hsr node_table file\n");
debugfs_remove(priv->node_tbl_root);
priv->node_tbl_root = NULL;
return;
}
priv->node_tbl_file = de;
return 0;
}
/* hsr_debugfs_term - Tear down debugfs intrastructure
......@@ -117,3 +130,18 @@ hsr_debugfs_term(struct hsr_priv *priv)
debugfs_remove(priv->node_tbl_root);
priv->node_tbl_root = NULL;
}
void hsr_debugfs_create_root(void)
{
hsr_debugfs_root_dir = debugfs_create_dir("hsr", NULL);
if (IS_ERR(hsr_debugfs_root_dir)) {
pr_err("Cannot create hsr debugfs root directory\n");
hsr_debugfs_root_dir = NULL;
}
}
void hsr_debugfs_remove_root(void)
{
/* debugfs_remove() internally checks NULL and ERROR */
debugfs_remove(hsr_debugfs_root_dir);
}
net/hsr/hsr_device.c
View file @ 095e90e0
......@@ -272,6 +272,8 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
skb->dev->dev_addr, skb->len) <= 0)
goto out;
skb_reset_mac_header(skb);
skb_reset_network_header(skb);
skb_reset_transport_header(skb);
if (hsr_ver > 0) {
hsr_tag = skb_put(skb, sizeof(struct hsr_tag));
......@@ -368,7 +370,7 @@ static void hsr_dev_destroy(struct net_device *hsr_dev)
del_timer_sync(&hsr->prune_timer);
del_timer_sync(&hsr->announce_timer);
hsr_del_self_node(&hsr->self_node_db);
hsr_del_self_node(hsr);
hsr_del_nodes(&hsr->node_db);
}
......@@ -440,11 +442,12 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
INIT_LIST_HEAD(&hsr->ports);
INIT_LIST_HEAD(&hsr->node_db);
INIT_LIST_HEAD(&hsr->self_node_db);
spin_lock_init(&hsr->list_lock);
ether_addr_copy(hsr_dev->dev_addr, slave[0]->dev_addr);
/* Make sure we recognize frames from ourselves in hsr_rcv() */
res = hsr_create_self_node(&hsr->self_node_db, hsr_dev->dev_addr,
res = hsr_create_self_node(hsr, hsr_dev->dev_addr,
slave[1]->dev_addr);
if (res < 0)
return res;
......@@ -477,31 +480,32 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
res = hsr_add_port(hsr, hsr_dev, HSR_PT_MASTER);
if (res)
goto err_add_port;
goto err_add_master;
res = register_netdevice(hsr_dev);
if (res)
goto fail;
goto err_unregister;
res = hsr_add_port(hsr, slave[0], HSR_PT_SLAVE_A);
if (res)
goto fail;
goto err_add_slaves;
res = hsr_add_port(hsr, slave[1], HSR_PT_SLAVE_B);
if (res)
goto fail;
goto err_add_slaves;
hsr_debugfs_init(hsr, hsr_dev);
mod_timer(&hsr->prune_timer, jiffies + msecs_to_jiffies(PRUNE_PERIOD));
res = hsr_debugfs_init(hsr, hsr_dev);
if (res)
goto fail;
return 0;
fail:
err_add_slaves:
unregister_netdevice(hsr_dev);
err_unregister:
list_for_each_entry_safe(port, tmp, &hsr->ports, port_list)
hsr_del_port(port);
err_add_port:
hsr_del_self_node(&hsr->self_node_db);
err_add_master:
hsr_del_self_node(hsr);
return res;
}
net/hsr/hsr_framereg.c
View file @ 095e90e0
......@@ -75,10 +75,11 @@ static struct hsr_node *find_node_by_addr_A(struct list_head *node_db,
/* Helper for device init; the self_node_db is used in hsr_rcv() to recognize
* frames from self that's been looped over the HSR ring.
*/
int hsr_create_self_node(struct list_head *self_node_db,
int hsr_create_self_node(struct hsr_priv *hsr,
unsigned char addr_a[ETH_ALEN],
unsigned char addr_b[ETH_ALEN])
{
struct list_head *self_node_db = &hsr->self_node_db;
struct hsr_node *node, *oldnode;
node = kmalloc(sizeof(*node), GFP_KERNEL);
......@@ -88,33 +89,33 @@ int hsr_create_self_node(struct list_head *self_node_db,
ether_addr_copy(node->macaddress_A, addr_a);
ether_addr_copy(node->macaddress_B, addr_b);
rcu_read_lock();
spin_lock_bh(&hsr->list_lock);
oldnode = list_first_or_null_rcu(self_node_db,
struct hsr_node, mac_list);
if (oldnode) {
list_replace_rcu(&oldnode->mac_list, &node->mac_list);
rcu_read_unlock();
synchronize_rcu();
kfree(oldnode);
spin_unlock_bh(&hsr->list_lock);
kfree_rcu(oldnode, rcu_head);
} else {
rcu_read_unlock();
list_add_tail_rcu(&node->mac_list, self_node_db);
spin_unlock_bh(&hsr->list_lock);
}
return 0;
}
void hsr_del_self_node(struct list_head *self_node_db)
void hsr_del_self_node(struct hsr_priv *hsr)
{
struct list_head *self_node_db = &hsr->self_node_db;
struct hsr_node *node;
rcu_read_lock();
spin_lock_bh(&hsr->list_lock);
node = list_first_or_null_rcu(self_node_db, struct hsr_node, mac_list);
rcu_read_unlock();
if (node) {
list_del_rcu(&node->mac_list);
kfree(node);
kfree_rcu(node, rcu_head);
}
spin_unlock_bh(&hsr->list_lock);
}
void hsr_del_nodes(struct list_head *node_db)
......@@ -130,30 +131,43 @@ void hsr_del_nodes(struct list_head *node_db)
* seq_out is used to initialize filtering of outgoing duplicate frames
* originating from the newly added node.
*/
struct hsr_node *hsr_add_node(struct list_head *node_db, unsigned char addr[],
u16 seq_out)
static struct hsr_node *hsr_add_node(struct hsr_priv *hsr,
struct list_head *node_db,
unsigned char addr[],
u16 seq_out)
{
struct hsr_node *node;
struct hsr_node *new_node, *node;
unsigned long now;
int i;
node = kzalloc(sizeof(*node), GFP_ATOMIC);
if (!node)
new_node = kzalloc(sizeof(*new_node), GFP_ATOMIC);
if (!new_node)
return NULL;
ether_addr_copy(node->macaddress_A, addr);
ether_addr_copy(new_node->macaddress_A, addr);
/* We are only interested in time diffs here, so use current jiffies
* as initialization. (0 could trigger an spurious ring error warning).
*/
now = jiffies;
for (i = 0; i < HSR_PT_PORTS; i++)
node->time_in[i] = now;
new_node->time_in[i] = now;
for (i = 0; i < HSR_PT_PORTS; i++)
node->seq_out[i] = seq_out;
list_add_tail_rcu(&node->mac_list, node_db);
new_node->seq_out[i] = seq_out;
spin_lock_bh(&hsr->list_lock);
list_for_each_entry_rcu(node, node_db, mac_list) {
if (ether_addr_equal(node->macaddress_A, addr))
goto out;
if (ether_addr_equal(node->macaddress_B, addr))
goto out;
}
list_add_tail_rcu(&new_node->mac_list, node_db);
spin_unlock_bh(&hsr->list_lock);
return new_node;
out:
spin_unlock_bh(&hsr->list_lock);
kfree(new_node);
return node;
}
......@@ -163,6 +177,7 @@ struct hsr_node *hsr_get_node(struct hsr_port *port, struct sk_buff *skb,
bool is_sup)
{
struct list_head *node_db = &port->hsr->node_db;
struct hsr_priv *hsr = port->hsr;
struct hsr_node *node;
struct ethhdr *ethhdr;
u16 seq_out;
......@@ -196,7 +211,7 @@ struct hsr_node *hsr_get_node(struct hsr_port *port, struct sk_buff *skb,
seq_out = HSR_SEQNR_START;
}
return hsr_add_node(node_db, ethhdr->h_source, seq_out);
return hsr_add_node(hsr, node_db, ethhdr->h_source, seq_out);
}
/* Use the Supervision frame's info about an eventual macaddress_B for merging
......@@ -206,10 +221,11 @@ struct hsr_node *hsr_get_node(struct hsr_port *port, struct sk_buff *skb,
void hsr_handle_sup_frame(struct sk_buff *skb, struct hsr_node *node_curr,
struct hsr_port *port_rcv)
{
struct ethhdr *ethhdr;
struct hsr_node *node_real;
struct hsr_priv *hsr = port_rcv->hsr;
struct hsr_sup_payload *hsr_sp;
struct hsr_node *node_real;
struct list_head *node_db;
struct ethhdr *ethhdr;
int i;
ethhdr = (struct ethhdr *)skb_mac_header(skb);
......@@ -231,7 +247,7 @@ void hsr_handle_sup_frame(struct sk_buff *skb, struct hsr_node *node_curr,
node_real = find_node_by_addr_A(node_db, hsr_sp->macaddress_A);
if (!node_real)
/* No frame received from AddrA of this node yet */
node_real = hsr_add_node(node_db, hsr_sp->macaddress_A,
node_real = hsr_add_node(hsr, node_db, hsr_sp->macaddress_A,
HSR_SEQNR_START - 1);
if (!node_real)
goto done; /* No mem */
......@@ -252,7 +268,9 @@ void hsr_handle_sup_frame(struct sk_buff *skb, struct hsr_node *node_curr,
}
node_real->addr_B_port = port_rcv->type;
spin_lock_bh(&hsr->list_lock);
list_del_rcu(&node_curr->mac_list);
spin_unlock_bh(&hsr->list_lock);
kfree_rcu(node_curr, rcu_head);
done:
......@@ -368,12 +386,13 @@ void hsr_prune_nodes(struct timer_list *t)
{
struct hsr_priv *hsr = from_timer(hsr, t, prune_timer);
struct hsr_node *node;
struct hsr_node *tmp;
struct hsr_port *port;
unsigned long timestamp;
unsigned long time_a, time_b;
rcu_read_lock();
list_for_each_entry_rcu(node, &hsr->node_db, mac_list) {
spin_lock_bh(&hsr->list_lock);
list_for_each_entry_safe(node, tmp, &hsr->node_db, mac_list) {
/* Don't prune own node. Neither time_in[HSR_PT_SLAVE_A]
* nor time_in[HSR_PT_SLAVE_B], will ever be updated for
* the master port. Thus the master node will be repeatedly
......@@ -421,7 +440,7 @@ void hsr_prune_nodes(struct timer_list *t)
kfree_rcu(node, rcu_head);
}
}
rcu_read_unlock();
spin_unlock_bh(&hsr->list_lock);
/* Restart timer */
mod_timer(&hsr->prune_timer,
......
net/hsr/hsr_framereg.h
View file @ 095e90e0
......@@ -12,10 +12,8 @@
struct hsr_node;
void hsr_del_self_node(struct list_head *self_node_db);
void hsr_del_self_node(struct hsr_priv *hsr);
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,
bool is_sup);
void hsr_handle_sup_frame(struct sk_buff *skb, struct hsr_node *node_curr,
......@@ -33,7 +31,7 @@ int hsr_register_frame_out(struct hsr_port *port, struct hsr_node *node,
void hsr_prune_nodes(struct timer_list *t);
int hsr_create_self_node(struct list_head *self_node_db,
int hsr_create_self_node(struct hsr_priv *hsr,
unsigned char addr_a[ETH_ALEN],
unsigned char addr_b[ETH_ALEN]);
......
net/hsr/hsr_main.c
View file @ 095e90e0
......@@ -45,6 +45,9 @@ static int hsr_netdev_notify(struct notifier_block *nb, unsigned long event,
case NETDEV_CHANGE: /* Link (carrier) state changes */
hsr_check_carrier_and_operstate(hsr);
break;
case NETDEV_CHANGENAME:
hsr_debugfs_rename(dev);
break;
case NETDEV_CHANGEADDR:
if (port->type == HSR_PT_MASTER) {
/* This should not happen since there's no
......@@ -64,7 +67,7 @@ static int hsr_netdev_notify(struct notifier_block *nb, unsigned long event,
/* Make sure we recognize frames from ourselves in hsr_rcv() */
port = hsr_port_get_hsr(hsr, HSR_PT_SLAVE_B);
res = hsr_create_self_node(&hsr->self_node_db,
res = hsr_create_self_node(hsr,
master->dev->dev_addr,
port ?
port->dev->dev_addr :
......@@ -123,6 +126,7 @@ static void __exit hsr_exit(void)
{
unregister_netdevice_notifier(&hsr_nb);
hsr_netlink_exit();
hsr_debugfs_remove_root();
}
module_init(hsr_init);
......
net/hsr/hsr_main.h
View file @ 095e90e0
......@@ -160,8 +160,9 @@ struct hsr_priv {
int announce_count;
u16 sequence_nr;
u16 sup_sequence_nr; /* For HSRv1 separate seq_nr for supervision */
u8 prot_version; /* Indicate if HSRv0 or HSRv1. */
spinlock_t seqnr_lock; /* locking for sequence_nr */
u8 prot_version; /* Indicate if HSRv0 or HSRv1. */
spinlock_t seqnr_lock; /* locking for sequence_nr */
spinlock_t list_lock; /* locking for node list */
unsigned char sup_multicast_addr[ETH_ALEN];
#ifdef CONFIG_DEBUG_FS
struct dentry *node_tbl_root;
......@@ -184,17 +185,24 @@ static inline u16 hsr_get_skb_sequence_nr(struct sk_buff *skb)
}
#if IS_ENABLED(CONFIG_DEBUG_FS)
int hsr_debugfs_init(struct hsr_priv *priv, struct net_device *hsr_dev);
void hsr_debugfs_rename(struct net_device *dev);
void hsr_debugfs_init(struct hsr_priv *priv, struct net_device *hsr_dev);
void hsr_debugfs_term(struct hsr_priv *priv);
void hsr_debugfs_create_root(void);
void hsr_debugfs_remove_root(void);
#else
static inline int hsr_debugfs_init(struct hsr_priv *priv,
struct net_device *hsr_dev)
static inline void void hsr_debugfs_rename(struct net_device *dev)
{
return 0;
}
static inline void hsr_debugfs_init(struct hsr_priv *priv,
struct net_device *hsr_dev)
{}
static inline void hsr_debugfs_term(struct hsr_priv *priv)
{}
static inline void hsr_debugfs_create_root(void)
{}
static inline void hsr_debugfs_remove_root(void)
{}
#endif
#endif /* __HSR_PRIVATE_H */
net/hsr/hsr_netlink.c
View file @ 095e90e0
......@@ -476,6 +476,7 @@ int __init hsr_netlink_init(void)
if (rc)
goto fail_genl_register_family;
hsr_debugfs_create_root();
return 0;
fail_genl_register_family:
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7