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,
Hi,
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...
other GFP_* allocations can't fail ? This whole resizing isn't escpecially beautiful and asks for some love.
- 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.
Replacing kmalloc() with kzalloc() should do, right ?
- 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?
Thanks for catching this. I agree that this is not properly protected. All functions accessing orig_node->bcast_own(_sum) use orig_node->ogm_cnt_lock to lock each other out. Obviously we would need a global lock for the interface renumbering which will be as ugly as the current array resizing is. You don't happen to have a good example of a resizable array at hand ?
Cheers, Marek
On Mon, Apr 23, 2012 at 01:18:25PM +0800, Marek Lindner wrote:
Hi,
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...
other GFP_* allocations can't fail ? This whole resizing isn't escpecially beautiful and asks for some love.
Other GFP_* allocations fail only when system is in a really lousy state - killing processes, etc. GFP_ATOMIC can fail in much milder conditions; note that they can't e.g. swap a page out or write a dirty page out and free it, etc. _Any_ allocation failures need to be dealt with, of course, but with GFP_ATOMIC ones failures are just a fact of life - it's not even an emergency situation.
- 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.
Replacing kmalloc() with kzalloc() should do, right ?
*shrug* That would do it, all right, but since you memcpy() over all but the last element, I'd suggest cleaning that last element explicitly. Hell knows - depends on how large your arrays are...
- 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?
Thanks for catching this. I agree that this is not properly protected. All functions accessing orig_node->bcast_own(_sum) use orig_node->ogm_cnt_lock to lock each other out. Obviously we would need a global lock for the interface renumbering which will be as ugly as the current array resizing is. You don't happen to have a good example of a resizable array at hand ?
Depends... How large those arrays realistically get? I would probably consider allocating these guys separately and hashing them by orig_node/hwif pair, but feasibility of that depends on how many of each do you expect to see and how often do their numbers change...
On Monday, April 23, 2012 14:43:24 Al Viro wrote:
Other GFP_* allocations fail only when system is in a really lousy state - killing processes, etc. GFP_ATOMIC can fail in much milder conditions; note that they can't e.g. swap a page out or write a dirty page out and free it, etc. _Any_ allocation failures need to be dealt with, of course, but with GFP_ATOMIC ones failures are just a fact of life - it's not even an emergency situation.
Ok, that is what I thought.
Replacing kmalloc() with kzalloc() should do, right ?
*shrug* That would do it, all right, but since you memcpy() over all but the last element, I'd suggest cleaning that last element explicitly. Hell knows - depends on how large your arrays are...
Don't think that is worth the hassle. The index of these arrays is the number of interfaces batman is running on. In 90% of all cases it will be a single interface. Have yet to encounter a system with more than 3 interfaces.
Thanks for catching this. I agree that this is not properly protected. All functions accessing orig_node->bcast_own(_sum) use orig_node->ogm_cnt_lock to lock each other out. Obviously we would need a global lock for the interface renumbering which will be as ugly as the current array resizing is. You don't happen to have a good example of a resizable array at hand ?
Depends... How large those arrays realistically get? I would probably consider allocating these guys separately and hashing them by orig_node/hwif pair, but feasibility of that depends on how many of each do you expect to see and how often do their numbers change...
As I explained above: The index is not big and does not change often (when an interface is added or removed). Can you explain the "hashing them by orig_node/hwif pair" part in greater detail ?
Thanks, Marek
b.a.t.m.a.n@lists.open-mesh.org