| 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
| ||
|
Message-ID: <4FF4BD68.1090304@xdin.com>
Date: Wed, 4 Jul 2012 22:02:16 +0000
From: Arvid Brodin <Arvid.Brodin@...n.com>
To: Joe Perches <joe@...ches.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Stephen Hemminger <shemminger@...tta.com>,
Alexey Kuznetsov <kuznet@....inr.ac.ru>,
Javier Boticario <jboticario@...il.com>,
Bruno Ferreira <balferreira@...glemail.com>
Subject: Re: [RFC v2 1/2] net/hsr: Add support for IEC 62439-3
High-availability Seamless Redundancy
On 2012-07-04 02:30, Joe Perches wrote:
> On Wed, 2012-07-04 at 00:12 +0000, Arvid Brodin wrote:
>> The kernel patch.
>
> []
What does this mean (the "[]")?
>
>> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
>
>> @@ -0,0 +1,531 @@
>
>> +static int is_admin_up(struct net_device *dev)
>> +{
>> + return (dev->flags & IFF_UP);
>> +}
>> +
>> +static int is_operstate_up(struct net_device *dev)
>> +{
>> + return (dev->operstate == IF_OPER_UP);
>> +}
>
> bool?
Yep, didn't know the bool type existed.
>
>> +static void __hsr_set_operstate(struct net_device *dev, int transition)
>> +{
>> + if (dev->operstate != transition) {
>> +/*
>> + switch (transition) {
>> + case IF_OPER_UP:
>> + printk(KERN_INFO "%s: new operstate is IF_OPER_UP\n", dev->name);
>
> netdev_info(dev, "new operstate is IF_OPER_UP\n");
>
>> + break;
>> + default:
>> + printk(KERN_INFO "%s: new operstate is !IF_OPER_UP (%d)\n", dev->name, transition);
>
> etc.
>
>> +void hsr_set_operstate(struct net_device *hsr_dev, struct net_device *slave1,
>> + struct net_device *slave2)
>> +{
>> + if (!is_admin_up(hsr_dev)) {
>> + __hsr_set_operstate(hsr_dev, IF_OPER_DOWN);
>> + return;
>> + }
>> +/*
>> + printk(KERN_INFO "Slave1/2 operstate: %d/%d\n",
>> + slave1->operstate, slave2->operstate);
>> +*/
>
> Please remove commented out code.
I intended to do so when I send the patch of course. I didn't know it would be so frowned
upon in a RFC, I thought it would be enough to note the existence of the commented out
code under known problems, as I did in RFC part 0. Lesson learned!
>
>> +static void restore_slaves(struct net_device *hsr_dev)
>> +{
>> + struct hsr_priv *hsr_priv;
>> + struct net_device *slave[2];
>> + int i;
>> + int res;
>> +
>> + hsr_priv = netdev_priv(hsr_dev);
>> + for (i = 0; i < 2; i++)
>> + slave[i] = hsr_priv->slave_data[i].dev;
>> +
>> + rtnl_lock();
>> +
>> + /* Restore promiscuity */
>> + for (i = 0; i < 2; i++) {
>> + if (!hsr_priv->slave_data[i].promisc)
>> + continue;
>> + res = dev_set_promiscuity(slave[i],
>> + -hsr_priv->slave_data[i].promisc);
>> + if (res)
>> + pr_info("HSR: Cannot restore promiscuity (%s, %d)\n",
>> + slave[i]->name,
>> + res);
>
> shouldn't this just be:
>
> netdev_info(slave[i], "cannot restore promiscuity: %d\n",
> res);
>
> If you must use pr_<level> please add
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> before any include and let the printk subsystem
> add MODNAME as a prefix.
>
>
>> +static int check_slave_ok(struct net_device *dev)
>> +{
>> + /* Don't allow HSR on non-ethernet like devices */
>> + if ((dev->flags & IFF_LOOPBACK) || (dev->type != ARPHRD_ETHER) ||
>> + (dev->addr_len != ETH_ALEN)) {
>> + pr_info("%s: Cannot enslave loopback or non-ethernet device\n",
>> + dev->name);
>
> netdev_info(dev, "Cannot enslave...");
>
>> + return -EINVAL;
>> + }
>> +
>> + /* Don't allow enslaving hsr devices */
>> + if (is_hsr_master(dev)) {
>> + pr_info("%s: Don't try to create trees of hsr devices!\n",
>> + dev->name);
>
>
> netdev_err(dev, "Cannot create trees of hsr devices\n");
>
>
>> +int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2])
>> +{
>
>> + /* Set hsr_dev's MAC address to that of mac_slave1 */
>> + memcpy(hsr_dev->dev_addr, hsr_priv->slave_data[0].dev->dev_addr,
>> + hsr_dev->addr_len);
>
> ETH_ALEN?
>
>> diff --git a/net/hsr/hsr_device.h b/net/hsr/hsr_device.h
> []
>> @@ -0,0 +1,27 @@
>
>> +void hsr_dev_setup(struct net_device *dev);
>> +int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2]);
>> +void hsr_set_operstate(struct net_device *hsr_dev, struct net_device *slave1,
>> + struct net_device *slave2);
>
> please align arguments immediately after the open parenthesis.
>
>> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> []
>> +static struct node_entry *find_node_by_AddrA(struct list_head *node_db,
>> + unsigned char addr[ETH_ALEN])
>
> static struct node_entry *find_node_by_AddrA(struct list_head *node_db,
> unsigned char addr[ETH_ALEN])
> []
>> +static struct node_entry *find_node_by_AddrB(struct list_head *node_db,
>> + unsigned char addr[ETH_ALEN])
>
> Alignment...
>
>> +int framereg_merge_node(struct hsr_priv *hsr_priv, enum hsr_dev_idx dev_idx,
>> + struct sk_buff *skb)
>> +{
> []
>> + node = find_node_by_AddrA(&hsr_priv->node_db, hsr_stag->MacAddressA);
>> + if (!node) {
>> + rcu_read_unlock();
>> + found = 0;
>> + node = kmalloc(sizeof(*node), GFP_ATOMIC);
>
> why GFP_ATOMIC?
This function is (indirectly) called by the receive callback for packet type ETH_P_HSR
(hsr_rcv() in hsr_main.c). If I recall correctly, I tried GFP_KERNEL first but the kernel
complained over sleeping in atomic context. I'll check it out again.
>
>> + if (!node)
>> + return -ENOMEM;
>> +
>> + memcpy(node->MacAddressA, hsr_stag->MacAddressA, ETH_ALEN);
>> + memcpy(node->MacAddressB, ethhdr->h_source, ETH_ALEN);
>> +
>> + for (i = 0; i < HSR_MAX_SLAVE; i++)
>> + node->time_in[i] = 0;
>> + for (i = 0; i < HSR_MAX_DEV; i++)
>> + node->seq_out[i] = 0;
>> +/*
>> + printk(KERN_INFO "HSR: Added node %pM / %pM\n",
>> + node->MacAddressA,
>> + node->MacAddressB);
>> +*/
>
> Please remove commented out code here and everywhere else...
>
> [too long, stopped reading]
>
Thank you for your time. I will take care of these issues when I get back from my vacation. :)
--
Arvid Brodin | Consultant (Linux)
XDIN AB | Jan Stenbecks Torg 17 | SE-164 40 Kista | Sweden | xdin.com
Powered by blists - more mailing lists