| 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
| ||
|
Date: Thu, 16 Aug 2012 19:12:50 +0000
From: Arvid Brodin <Arvid.Brodin@...n.com>
To: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC: Joe Perches <joe@...ches.com>,
Stephen Hemminger <shemminger@...tta.com>,
Alexey Kuznetsov <kuznet@....inr.ac.ru>,
Javier Boticario <jboticario@...il.com>,
Bruno Ferreira <balferreira@...glemail.com>
Subject: Re: [RFC v3 0/1] net/hsr: Add support for IEC 62439-3
High-availability Seamless Redundancy
Hello! This is version 3 of my RFC for HSR support. The complete diffstat of this version
of the kernel patch:
Documentation/networking/hsr/hsr_genl.c | 221 ++++++++++++++
include/linux/if_ether.h | 1 +
include/linux/if_link.h | 11 +
net/Kconfig | 1 +
net/Makefile | 1 +
net/hsr/Kconfig | 33 ++
net/hsr/Makefile | 7 +
net/hsr/hsr_device.c | 506 +++++++++++++++++++++++++++++++
net/hsr/hsr_device.h | 27 ++
net/hsr/hsr_framereg.c | 303 ++++++++++++++++++
net/hsr/hsr_framereg.h | 52 ++++
net/hsr/hsr_main.c | 402 ++++++++++++++++++++++++
net/hsr/hsr_netlink.c | 293 ++++++++++++++++++
net/hsr/hsr_netlink.h | 64 ++++
net/hsr/hsr_private.h | 162 ++++++++++
15 files changed, 2084 insertions(+), 0 deletions(-)
Changes since v2:
On 2012-07-04 02:30, Joe Perches wrote:
> On Wed, 2012-07-04 at 00:12 +0000, Arvid Brodin wrote:
>> +static int is_operstate_up(struct net_device *dev)
>> +{
>> + return (dev->operstate == IF_OPER_UP);
>> +}
>
> bool?
Check. (Changed to bool here and in other places where a boolean was returned.)
>> +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");
Check. Changed all printouts to use netdev_info() etc instead of printk()/pr_info().
>> +/*
>> + printk(KERN_INFO "Slave1/2 operstate: %d/%d\n",
>> + slave1->operstate, slave2->operstate);
>> +*/
>
> Please remove commented out code.
Check. (Removed debug code; still need to keep the FIXMEs so I don't forget
them.)
>> +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.
Check, fixed.
>> +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 is in the frame receive path; we're not allowed to sleep here (?).
On 2012-07-04 06:20, Stephen Hemminger wrote:
> On Wed, 4 Jul 2012 00:12:13 +0000, Arvid Brodin wrote:
>> diff --git a/Documentation/networking/hsr/hsr_genl.c b/Documentation/networking/hsr/hsr_genl.c
> 1. I like documentation, I like examples, but examples in the Documentation
> directory get stale and end up not working.
I see your point, but I'd really like to supply this example code, because
frankly, the documentation on Generic Netlink with libnl isn't very good if you
aren't already "in the know". It took some time to figure this out.
I added "Current as of <date>" in the header comment so that people at least see
when it is out of date. Is this an OK compromise?
> 2. I am not a fan of the non-standard unaligned access optimization.
> Stinks too much like the old BSD IFF_TRAILERS thing that still persists
> to this day.
This has now been removed. Architectures without HAVE_EFFICIENT_UNALIGNED_ACCESS
will always memmove() the payload.
> 3. The code seems to go to a lot of locking effort to get atomic state
> update, if it used the bits in dev_state (ie netif_carrier etc) instead of
> operstate and if_flags I think it would be simpler and safer. I.e
>
> +static int is_admin_up(struct net_device *dev)
> +{
> + return (dev->flags & IFF_UP);
> +}
> is redundant with netif_running()
Not sure what you mean by neither "a lot of locking effort" nor "atomic state
update" here? (Please elaborate.)
netif_running() checks __LINK_STATE_START. This has got to do with carrier state
rather than administrative state, right? So it's not the same as checking
IFF_UP? Actually I'm pretty confused by how the kernel implements state tracking
of network devices...
It doesn't seem to be normal practice to change operstate like I do, so I'm
probably not doing it right. Linkwatch sets IF_OPER_LOWERLAYERDOWN in
default_operstate() (net/core/link_watch.c) if !netif_carrier_ok(dev) and
(dev->ifindex != dev->iflink). Can I use this for my virtual hsr device driver
somehow? What is dev->iflink?
The HSR interface should be IF_OPER_LOWERLAYERDOWN if it's admin UP but both
slaves are inoperable.
(The linkwatch reference to IF_OPER_LOWERLAYERDOWN is the only one that I can
find in the kernel code.)
> 4. Don't use mixed case as in:
> + HSR_Ver = 0;
The HSR_Ver (and others like MacAddressA, HSR_TLV_Length etc) are the names as
written in the HSR IEC standard. Is it still desirable that I change them to
lower case?
I've changed the code a bit so the HSR_Ver local variable and some others are
no longer present, but the struct hsr_tag fields is still as written in the
IEC spec. I also added a comment about this. Ok?
> 5. The header create code has to handle error cases where:
> 1. skb must be copied to add header
> 2. no space left for header in skb
These checks does not seem to be done anywhere else where eth_header() is used,
but I added
if (skb_headroom(skb) < HSR_TAGLEN + ETH_HLEN)
return -ENOBUFS;
It is not possible to copy the skb to make room for the header, since there is
no way to return the new pointer from hsr_header_create().
> 6. Don't comment out code and then submit it. I don't like reading and
> reviewing leftover debug stuff.
Sorry about that. All debug leftovers should now have been removed.
> 7. Any operations type structure should be declared 'const'. This is safer
> from a security point of view and keeps dirty and immutable variables in separate
> cache areas.
In general, this results in "warning: passing argument <n> of '<some-func>' discards
qualifiers from pointer target type", where <some-func> is genl_register_family,
genl_register_mc_group, dev_add_pack etc. It does work (and is now fixed) for struct
net_device_ops and struct nla_policy though.
> 8. If possible use standard netdev macros for printing info messages:
> see netdev_info() etc.
Done.
> 9. Be careful about variable names: "seqlock" implies you are using
> seq_lock() but you aren't doing that. It is just a spinlock.
Check (changed variable name to seqnr_lock - it's the lock for the sequence_nr variable
in the same struct).
> 10. above() seems like it could be done by signed math in one operation
> without conditional ?: operation.
> See timer_before/timer_after for example.
Thanks. I've now changed the function to something more akin to the time_after()
function. However, there's an inconsistency in that code where time_after(0,
MAX_UTYPE/2) == time_before(0, MAX_UTYPE/2). I don't like that, so I kept an
if statement that fixes that. I hope that's OK.
> 11. All global variables need to have one prefix. You are using both hsr_
> and framreg_. Can you just use hsr_?
Done (using hsr_ only now).
I have run the patch through checkpatch and the remaining errors and warnings are
intentional, except for the "C99 // comments" which will be fixed in due time. I
believe fixing the other problems would reduce the readability of the code. I am
however open to suggestions about this, of course.
--
Arvid Brodin | Consultant (Linux)
XDIN AB | Jan Stenbecks Torg 17 | SE-164 40 Kista | Sweden | xdin.com
Powered by blists - more mailing lists