Re: [B.A.T.M.A.N.] [PATCHv4] batman-adv: remove interfacing enslaving loop private check

2013-03-02 Thread Simon Wunderlich 
Hi,

I've tested this for compatibility on 2.6.32 (Debian stable) in my virtual 
machine.
Marek suggested to use:

batctl if add eth0
vconfig add bat0 5
batctl if add bat0.5

and the last command fails with:
Error - can't write to file '/sys/class/net/bat0.5/batman_adv/mesh_iface': 
Operation not permitted

which looks good and expected to me.

Without the patch, the directory /sys/class/net/bat0.5/batman_adv does not even 
exist - probably
because the check was removed in this patch.

Another thing I have tried, which behaves bad with and without this patch:
batctl if add eth0
brctl addbr br0
brctl addif br0 bat0
batctl if add br0
(wait a few seconds)
rmmod batman-adv

First, enslaving br0 into bat0 does not fail, but it makes no sense to enslave 
bat0 through br0 into bat0
again - we should probably prevent this too. Removing the module crashes the 
kernel, not always, but pretty
reliably (I have the feeling it happens more often after applying this patch, 
but not sure).
Unfortunately I couldn't get any useful backtraces so far, if it fails it 
usually instantly reboots
or hangs without output (this is in kvm with -curses). I've succeeded to get 
some trace part, it's
probably not useful but i've attached it to the bottom anyway [1].

Any thoughts on that? :)

Cheers,
Simon

[1]
[  130.688334]  [c1270aaf] ? do_page_fault+0x0/0x307
[  130.688334]  [c101bb73] ? bad_area_nosemaphore+0xa/0xc 
[  130.688334]  [c126f303] ? error_code+0x73/0x78   
[  130.688334]  [c8c60f89] ? br_nf_pre_routing+0x28/0x4a8 [bridge]   
[  130.688334]  [c11d007b] ? sock_rmalloc+0x1e/0x68 
[  130.688334]  [c11d9bee] ? netif_receive_skb+0x2dd/0x3d6  
[  130.688334]  [c8c60f89] ? br_nf_pre_routing+0x28/0x4a8 [bridge]  
[  130.688334]  [c11d9bee] ? netif_receive_skb+0x2dd/0x3d6
[  130.688334]  [c8c5d5b2] ? br_handle_frame_finish+0xd2/0xff [bridge]  
[  130.688334]  [c8c5d760] ? br_handle_frame+0x181/0x195 [bridge]
[  130.688334]  [c11d9bee] ? netif_receive_skb+0x2dd/0x3d6  
[  130.688334]  [c8c5d5b2] ? br_handle_frame_finish+0xd2/0xff [bridge]  
[  130.688334]  [c8c5d760] ? br_handle_frame+0x181/0x195 [bridge]   
[  130.688334]  [c11d9bee] ? netif_receive_skb+0x2dd/0x3d6
[  130.688334]  [c8c5d5b2] ? br_handle_frame_finish+0xd2/0xff [bridge]  
[  130.688334]  [c8c5d760] ? br_handle_frame+0x181/0x195 [bridge]
[  130.688334]  [c11d9bee] ? netif_receive_skb+0x2dd/0x3d6  
[  130.688334]  [c8c5d5b2] ? br_handle_frame_finish+0xd2/0xff [bridge]  
[  130.688334]  [c8c5d760] ? br_handle_frame+0x181/0x195 [bridge]   
[  130.688334]  [c11d9bee] ? netif_receive_skb+0x2dd/0x3d6
[  130.688334]  [c8c5d5b2] ? br_handle_frame_finish+0xd2/0xff [bridge]
[  130.688334]  [c8c5d760] ? br_handle_frame+0x181/0x195 [bridge]
[  130.688334]  [c11d9bee] ? netif_receive_skb+0x2dd/0x3d6
[  130.688334]  [c8c5d5b2] ? br_handle_frame_finish+0xd2/0xff [bridge]


On Fri, Mar 01, 2013 at 10:44:47PM +0100, Antonio Quartulli wrote:
 Now that the new dev_upper API is used, the private check for enslaving loops 
 is
 not needed anymore as this is performed by the new API itself.
 
 Reported-by: Marek Lindner lindner_ma...@yahoo.de
 Signed-off-by: Antonio Quartulli or...@autistici.org
 ---
 
 v2, v3, v4:
  - no change to this patch
 
 v4:
  - added compat code
 
  compat.c | 53 +
  compat.h |  5 -
  hard-interface.c | 43 ---
  3 files changed, 57 insertions(+), 44 deletions(-)
 
 diff --git a/compat.c b/compat.c
 index 764878f..da63ea6 100644
 --- a/compat.c
 +++ b/compat.c
 @@ -24,6 +24,7 @@
  #include linux/in.h
  #include linux/version.h
  #include main.h
 +#include soft-interface.h
  
  #if LINUX_VERSION_CODE  KERNEL_VERSION(3, 0, 0)
  
 @@ -85,3 +86,55 @@ void batadv_free_rcu_nc_path(struct rcu_head *rcu)
  #endif
  
  #endif /*  KERNEL_VERSION(3, 0, 0) */
 +
 +#if LINUX_VERSION_CODE  KERNEL_VERSION(3, 9, 0)
 +
 +/**
 + * batadv_is_on_batman_iface - check if a device is a batman iface descendant
 + * @net_dev: the device to check
 + *
 + * If the user creates any virtual device on top of a batman-adv interface, 
 it
 + * is important to prevent this new interface to be used to create a new mesh
 + * network (this behaviour would lead to a batman-over-batman configuration).
 + * This function recursively checks all the fathers of the device passed as
 + * argument looking for a batman-adv soft interface.
 + *
 + * Returns true if the device is descendant of a batman-adv mesh interface 
 (or
 + * if it is a batman-adv interface itself), false otherwise
 + */
 +static bool batadv_is_on_batman_iface(const struct net_device *net_dev)
 +{
 + struct

Re: [B.A.T.M.A.N.] [PATCHv4] batman-adv: remove interfacing enslaving loop private check

2013-03-02 Thread Antonio Quartulli 
On Sat, Mar 02, 2013 at 03:01:14PM +0100, Simon Wunderlich wrote:
 Hi,
 
 I've tested this for compatibility on 2.6.32 (Debian stable) in my virtual 
 machine.

Hello Simon and thank yuou,

 Marek suggested to use:
 
 batctl if add eth0
 vconfig add bat0 5
 batctl if add bat0.5
 
 and the last command fails with:
 Error - can't write to file '/sys/class/net/bat0.5/batman_adv/mesh_iface': 
 Operation not permitted
 
 which looks good and expected to me.
 

yeah.

 Without the patch, the directory /sys/class/net/bat0.5/batman_adv does not 
 even exist - probably
 because the check was removed in this patch.
 

Well, with this patch the check is done later and therefore the folder is
created. We may want to try to move the check earlier, like it was before the
patch?


 Another thing I have tried, which behaves bad with and without this patch:
 batctl if add eth0
 brctl addbr br0
 brctl addif br0 bat0
 batctl if add br0
 (wait a few seconds)
 rmmod batman-adv
 
 First, enslaving br0 into bat0 does not fail, but it makes no sense to 
 enslave bat0 through br0 into bat0
 again - we should probably prevent this too. Removing the module crashes the 
 kernel, not always, but pretty
 reliably (I have the feeling it happens more often after applying this patch, 
 but not sure).
 Unfortunately I couldn't get any useful backtraces so far, if it fails it 
 usually instantly reboots
 or hangs without output (this is in kvm with -curses). I've succeeded to get 
 some trace part, it's
 probably not useful but i've attached it to the bottom anyway [1].
 
 Any thoughts on that? :)
 

How sure are we about being batman-adv the culprit for this?

Cheers,


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto Che Guevara


pgp4ycn_Xbbyf.pgp
Description: PGP signature

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Fix broadcast+ogm queue purging race condition

2013-03-02 Thread Marek Lindner 
On Wednesday, February 27, 2013 17:58:15 Linus Lüssing wrote:
 +/**
 + * batadv_canceled_packets_free - Frees canceled forward packets
 + * @head:  A list of to be freed forw_packets
 + *
 + * This function canceles the scheduling of any packet in the provided
 list, + * waits for any possibly running packet forwarding thread to
 finish and + * finally, safely frees this forward packet.
 + *
 + * This function might sleep.
 + */
 +static void batadv_canceled_packets_free(struct hlist_head *head)
 +{
 +   struct batadv_forw_packet *forw_packet;
 +   struct hlist_node *tmp_node, *safe_tmp_node;
 +
 +   hlist_for_each_entry_safe(forw_packet, tmp_node, safe_tmp_node,
 head, + canceled_list) {
 +   cancel_delayed_work_sync(forw_packet-delayed_work);
 +
 +   hlist_del(forw_packet-canceled_list);
 +   batadv_forw_packet_free(forw_packet);
 +   }
 +}

I don't quite see how your patch can work when only one interface is 
deactivated and not the mesh as a whole. batadv_purge_outstanding_packets() 
also is called from batadv_hardif_disable_interface() in which case the 
broadcast packets re-arm themselves by appending the work item to their 
respective queues. That is why we have the magic pending check in the 
cleanup functions.

Cheers,
Marek