Hello,
On Thu, Apr 19, 2012 at 02:48:54 +0100, Al Viro wrote:
On Thu, Apr 19, 2012 at 08:10:27AM +0200, Antonio Quartulli wrote:
Hello Al,
and thank you very much for your patches. Before committing them, do you mind if we reword the subject by substituting "batman" with "batman-adv"?
No problem...
Moreover, patch 3/4, fixes the memcpy() casting too other than the endianess
4/4, actually.
stuff. We would prefer to have two different patches for fixing those two issues. What about splitting it and resending them?
Can do; I've just grepped for memcpy() in there, to see if there are other places like that.
Thanks!
Haven't found any, but
- you do an awful lot of GFP_ATOMIC allocations and those can and
do fail from time to time. What's worse, you ignore some of those failures - e.g. failing allocation in orig_hash_{add,del}_if() will be ignored by the caller. I haven't looked into that code enough to tell if it could be exploited, but I really don't like the look of it...
- orig_node_add_if() leaves junk in added array elements. You do
kmalloc() followed by memcpy(), but leave the last element uninitialized. May be safe if you assign it soon enough, but I'd suggest checking that.
- orig_node_del_if() looks odd - it removes element #hard_iface->if_num
and shifts all subsequent ones down; then it renumbers interfaces to match that. So far, so good, and there's even a plausible comment about locking: /* renumber remaining batman interfaces _inside_ of orig_hash_lock */ except that no such lock exists since commit d007260. What protects us from the obvious race in there?
Thank you very much for your analysis. I really appreciated it! I'm CCing our mailing list so that other people can comment on it.
Cheers,