pull request: batman-adv 2012-04-18

  Hello David,

this is the fixed version of my previous pull request (issued on 2012-04-17).
In this patchset the issues you reported have been fixed; moreover I'm
also including two new patches (1/13 and 2/13) which are respectively:

1/13) a fix for Al Viro's report about the missing htons()
2/13) a fix for a wrongly duplicated line in a comment

The following changes since commit ecffe75f934b4e3c5301fe5db278068e0efb0d6b:

  hippi: fix printk format in rrunner.c (2012-04-16 23:48:38 -0400)

are available in the git repository at:

  git://git.open-mesh.org/linux-merge.git tags/batman-adv-for-davem

for you to fetch changes up to 1e5cc266dbc401d11aefb966ad35e651c2f67414:

  batman-adv: skip the window protection test when the originator has no neighbours (2012-04-18 09:54:02 +0200)

----------------------------------------------------------------
Included changes:
* remove duplicated line in comment
* add htons() invocation for tt_crc as suggested by Al Viro
* OriGinator Message seqno initial value is now random
* some cleanups and fixes

----------------------------------------------------------------
Antonio Quartulli (5):
      batman-adv: convert the tt_crc to network order
      batman-adv: remove duplicated line in comment
      batman-adv: use ETH_HLEN instead of sizeof(struct ethhdr)
      batman-adv: print OGM seq numbers as unsigned int
      batman-adv: skip the window protection test when the originator has no neighbours

Marek Lindner (8):
      batman-adv: move ogm initialization into the proper function
      batman-adv: refactoring API: find generalized name for bat_ogm_init callback
      batman-adv: randomize initial seqno to avoid collision
      batman-adv: add iface_disable() callback to routing API
      batman-adv: handle routing code initialization properly
      batman-adv: refactoring API: find generalized name for bat_ogm_init_primary callback
      batman-adv: rename BATMAN_OGM_LEN to BATMAN_OGM_HLEN
      batman-adv: mark existing ogm variables as batman iv

 net/batman-adv/bat_iv_ogm.c            |   63 +++++++++++++++++++++-----------
 net/batman-adv/bridge_loop_avoidance.c |   11 ++----
 net/batman-adv/hard-interface.c        |   33 +++++++++--------
 net/batman-adv/icmp_socket.c           |    4 +-
 net/batman-adv/main.c                  |    5 ++-
 net/batman-adv/packet.h                |    6 +--
 net/batman-adv/routing.c               |   10 ++---
 net/batman-adv/send.c                  |   14 +++----
 net/batman-adv/soft-interface.c        |    2 +-
 net/batman-adv/translation-table.c     |    2 +-
 net/batman-adv/types.h                 |   12 +++---
 net/batman-adv/vis.c                   |    8 ++--
 12 files changed, 96 insertions(+), 74 deletions(-)
  

From: Antonio Quartulli <ordex@autistici.org>
Date: Wed, 18 Apr 2012 11:59:57 +0200

> this is the fixed version of my previous pull request (issued on 2012-04-17).
> In this patchset the issues you reported have been fixed; moreover I'm
> also including two new patches (1/13 and 2/13) which are respectively:
> 
> 1/13) a fix for Al Viro's report about the missing htons()
> 2/13) a fix for a wrongly duplicated line in a comment
> 
> The following changes since commit ecffe75f934b4e3c5301fe5db278068e0efb0d6b:
> 
>   hippi: fix printk format in rrunner.c (2012-04-16 23:48:38 -0400)
> 
> are available in the git repository at:
> 
>   git://git.open-mesh.org/linux-merge.git tags/batman-adv-for-davem

This looks a lot better, pulled, thanks!
  

On Wed, Apr 18, 2012 at 11:59:57AM +0200, Antonio Quartulli wrote:
> Hello David,
> 
> this is the fixed version of my previous pull request (issued on 2012-04-17).
> In this patchset the issues you reported have been fixed; moreover I'm
> also including two new patches (1/13 and 2/13) which are respectively:
> 
> 1/13) a fix for Al Viro's report about the missing htons()

Speaking of endianness stuff: here's a series of 4 patches on top of
your merge.git/master, hopefully getting all that stuff endian-clean,
at least from the sparse POV.  And AFAICS all on-the-wire stuff is
correctly annotated...
  

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
  

Hey,

I was about to merge the patch when I realized checkpatch is complaining. See 
below for details.


> @@ -618,7 +615,7 @@ int recv_tt_query(struct sk_buff *skb, struct
> hard_iface *recv_if) if (skb_linearize(skb) < 0)
>  				goto out;
> 
> -			tt_len = tt_query->tt_data * sizeof(struct tt_change);
> +			tt_len = ntohs(tt_query->tt_data) * sizeof(struct tt_change);

Line is too long.


> @@ -1727,7 +1727,7 @@ void handle_tt_response(struct bat_priv *bat_priv,
> 
>  	bat_dbg(DBG_TT, bat_priv,
>  		"Received TT_RESPONSE from %pM for ttvn %d t_size: %d [%c]\n",
> -		tt_response->src, tt_response->ttvn, tt_response->tt_data,
> +		tt_response->src, tt_response->ttvn, ntohs(tt_response->tt_data),
>  		(tt_response->flags & TT_FULL_TABLE ? 'F' : '.'));
> 
>  	/* we should have never asked a backbone gw */
> @@ -1741,7 +1741,7 @@ void handle_tt_response(struct bat_priv *bat_priv,
>  	if (tt_response->flags & TT_FULL_TABLE)
>  		tt_fill_gtable(bat_priv, tt_response);
>  	else
> -		tt_update_changes(bat_priv, orig_node, tt_response->tt_data,
> +		tt_update_changes(bat_priv, orig_node, ntohs(tt_response->tt_data),
>  				  tt_response->ttvn,
>  				  (struct tt_change *)(tt_response + 1));

Both changes are too long as well.

Regards,
Marek
  

On Sunday, April 22, 2012 14:50:29 Al Viro wrote:
> memcpy() arguments are void *, precisely to avoid that kind of pointless
> casts.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Applied in revision 815cfb5.

Thanks,
Marek
  

On Sunday, April 22, 2012 14:47:50 Al Viro wrote:
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  net/batman-adv/bridge_loop_avoidance.c |    6 +++---
>  net/batman-adv/distributed-arp-table.c |   16 ++++++++--------
>  net/batman-adv/distributed-arp-table.h |    4 ++--
>  net/batman-adv/packet.h                |   12 ++++++------
>  4 files changed, 19 insertions(+), 19 deletions(-)

Applied in revision 276f053.

Thanks,
Marek
  

On Sunday, April 22, 2012 14:46:29 Al Viro wrote:
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  net/batman-adv/bat_iv_ogm.c |   25 ++++++++++---------------
>  net/batman-adv/packet.h     |    2 +-
>  2 files changed, 11 insertions(+), 16 deletions(-)

Applied in revision e62e464.

Thanks,
Marek