netdev - Re: [patch v2, kernel version 3.2.1] net/ipv4/ip

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGsizz+kgBB9APmU9dPExkGQvPUVa3m6qz3NvSguPYNuJQQ_7Q@mail.gmail.com>
Date:	Tue, 17 Jan 2012 09:04:00 +0100
From:	Štefan Gula <steweg@...t.sk>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Alexey Kuznetsov <kuznet@....inr.ac.ru>,
	"David S. Miller" <davem@...emloft.net>,
	James Morris <jmorris@...ei.org>,
	Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
	Patrick McHardy <kaber@...sh.net>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch v2, kernel version 3.2.1] net/ipv4/ip_gre: Ethernet
 multipoint GRE over IP

Dňa 17. januára 2012 5:47, Eric Dumazet <eric.dumazet@...il.com> napísal/a:
> Le mardi 17 janvier 2012 à 00:11 +0100, Štefan Gula a écrit :
>
>> I agree with you, but for the start of this feature I believe static
>> slots size is enough here - same limitation is inside the original
>> linux bridge code. I have merged hopefully all your comments and here
>> is the newest patch:
>
>
>
> 1) Before sending a new version of your patch, please fix your mailer,
> you can read Documentation/email-clients.txt for hints.
>
Sorry, I have to test it as I am using clasical gmail web GUI to send emails

> Send it to you and check you can apply it.
> Then, once you are confident its OK, you can send it to netdev.
>
> Right now, it doesnt apply, because of unexpected line wraps.
>
> # cd next-next
> # cat /tmp/patch | patch -p1
> patching file include/net/ipip.h
> patching file net/ipv4/Kconfig
> patching file net/ipv4/ip_gre.c
> patch: **** malformed patch at line 495: ipgre_tap_bridge_entry *entry)
>
>
> 2) You call ipgre_tap_bridge_fini() from ipgre_exit_net() and
> ipgre_init_net(), thats completely bogus if CONFIG_NET_NS=y
>
> Just remove the struct kmem_cache *ipgre_tap_bridge_cache
> and use instead kmalloc(sizeof(...))/kfree(ptr) instead.
>
As this is completely the same part of code from net/bridge/br_fdb.c,
can you give me a hint about how change that as I believe it should be
changed also there?

> 3) ipgre_tap_bridge_init() should not be __net_init, but __init
>
Ok

>
> 4) I cant find why you store 'struct net_device *dev;' in a
> ipgre_tap_bridge_entry, it seems you never read it ?
>
yes you are right, it is not needed - removed from code

> 5) Also please add an empty line between variable declaration and
> function body. Also, we prefer an alignement of parameters.
>
I used scripts/checkpatch.pl to check my coding styles, but apparently
this is missing from it as it never complains before about this.
Anyway hopefully done ok based your comments

> For example :
>
> static __be32 ipgre_tap_bridge_get_raddr(struct ip_tunnel *tunnel,
>        const unsigned char *addr)
> {
>        __be32 raddr;
>        struct ipgre_tap_bridge_entry *entry;
>        rcu_read_lock();
>        entry = __ipgre_tap_bridge_get(tunnel, addr);
>        if (entry == NULL)
>                raddr = 0;
>        else
>                raddr = entry->raddr;
>        rcu_read_unlock();
>        return raddr;
> }
>
> should be :
>
> static __be32 ipgre_tap_bridge_get_raddr(struct ip_tunnel *tunnel,
>                                         const unsigned char *addr)
> {
>        __be32 raddr = 0;
>        struct ipgre_tap_bridge_entry *entry;
>
>        rcu_read_lock();
>        entry = __ipgre_tap_bridge_get(tunnel, addr);
>        if (entry)
>                raddr = entry->raddr;
>        rcu_read_unlock();
>
>        return raddr;
> }
>
>
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html