[B.A.T.M.A.N.] [PATCH] batman-adv: only modify hna-table on active module

[B.A.T.M.A.N.] [PATCH] batman-adv: only modify hna-table on active module

[Linus Lüssing]

If we haven't set the module to MODULE_ACTIVE state before (in general,
no interface has yet been added to batman-adv) then the hna table is not
initialised yet. If the kernel changes the mac address of the bat0
interface at this moment then an hna_local_add() called by interface_set_mac_addr()
then resulted in a null pointer derefernce. With this patch we are now
explicitly checking before if the state is MODULE_ACTIVE right now so
that we can assume having an initialised hna table.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 batman-adv-kernelland/soft-interface.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/batman-adv-kernelland/soft-interface.c b/batman-adv-kernelland/soft-interface.c
index f098a4f..582134f 100644
--- a/batman-adv-kernelland/soft-interface.c
+++ b/batman-adv-kernelland/soft-interface.c
@@ -154,9 +154,13 @@ int interface_set_mac_addr(struct net_device *dev, void *p)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
-	hna_local_remove(dev->dev_addr, "mac address changed");
+	/* only modify hna-table if it has been initialised before */
+	if (atomic_read(&module_state) == MODULE_ACTIVE) {
+		hna_local_remove(dev->dev_addr, "mac address changed");
+		hna_local_add(addr->sa_data);
+	}
+
 	memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
-	hna_local_add(dev->dev_addr);
 
 	return 0;
 }
-- 
1.7.0

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: only modify hna-table on active module

[Linus Lüssing]

[-- Attachment #1.1: Type: text/plain, Size: 2062 bytes --]

Just for some more clarification about this bug (have a look at
the attached call trace): It always occurs when I haven't put an
interface into batman-adv and when I'm then changing the
mac-address of bat0. I've now added a little check in
interface_set_mac_addr() which seems to work nicely here in my
setup. I'm also wondering if we should add another sanity check
somewhere in hna_local_add() to directly avoid any racy null
pointer dereferences in there.

Cheers, Linus

On Sat, Feb 27, 2010 at 02:49:42AM +0100, Linus Lüssing wrote:
> If we haven't set the module to MODULE_ACTIVE state before (in general,
> no interface has yet been added to batman-adv) then the hna table is not
> initialised yet. If the kernel changes the mac address of the bat0
> interface at this moment then an hna_local_add() called by interface_set_mac_addr()
> then resulted in a null pointer derefernce. With this patch we are now
> explicitly checking before if the state is MODULE_ACTIVE right now so
> that we can assume having an initialised hna table.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> ---
>  batman-adv-kernelland/soft-interface.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/batman-adv-kernelland/soft-interface.c b/batman-adv-kernelland/soft-interface.c
> index f098a4f..582134f 100644
> --- a/batman-adv-kernelland/soft-interface.c
> +++ b/batman-adv-kernelland/soft-interface.c
> @@ -154,9 +154,13 @@ int interface_set_mac_addr(struct net_device *dev, void *p)
>  	if (!is_valid_ether_addr(addr->sa_data))
>  		return -EADDRNOTAVAIL;
>  
> -	hna_local_remove(dev->dev_addr, "mac address changed");
> +	/* only modify hna-table if it has been initialised before */
> +	if (atomic_read(&module_state) == MODULE_ACTIVE) {
> +		hna_local_remove(dev->dev_addr, "mac address changed");
> +		hna_local_add(addr->sa_data);
> +	}
> +
>  	memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
> -	hna_local_add(dev->dev_addr);
>  
>  	return 0;
>  }
> -- 
> 1.7.0
> 

[-- Attachment #1.2: call-trace-2010-02-25-2057.log --]
[-- Type: text/plain, Size: 3574 bytes --]

[2236621.918952] batman-adv:B.A.T.M.A.N. advanced 0.2.1-beta (compatibility version 8) loaded
[2236634.418816] BUG: unable to handle kernel NULL pointer dereference at 00000004
[2236634.418839] IP: [<e1bbe1cc>] hna_local_add+0xf3/0x15b [batman_adv]
[2236634.418867] *pde = 00000000
[2236634.418874] Oops: 0000 [#1]
[2236634.418881] last sysfs file: /sys/devices/virtual/net/bat0/flags
[2236634.418889] Modules linked in: batman_adv ip6table_filter ip6table_mangle ip6_tables tcp_diag inet_diag tun sit tunnel4 xt_TCPMSS xt_tcpmss xt_tcpudp iptable_mangle ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables pppoe pppox ppp_generic fuse dm_snapshot dm_mirror dm_region_hash dm_log asb100 hwmon_vid loop tvaudio tda7432 msp3400 tuner_simple tuner_types tuner snd_wavefront bttv snd_cs4236 snd_wss_lib ir_common arc4 snd_opl3_lib hisax_fcpcipnp i2c_algo_bit snd_hwdep hisax_isac ecb v4l2_common snd_intel8x0 snd_mpu401 ath5k snd_mpu401_uart videodev snd_ac97_codec hfcpci hisax snd_seq_midi mac80211 v4l1_compat videobuf_dma_sg crc_ccitt ac97_bus snd_bt87x videobuf_core snd_rawmidi led_class snd_pcm_oss btcx_risc snd_seq_midi_event mISDN_core isdn tveeprom i2c_nforce2 slhc snd_mixer_oss snd_seq i2c_core btusb cfg80211 snd_pcm snd_timer shpchp snd_seq_device pci_hotplug snd_page_alloc bluetooth pcspkr evdev processor button snd parport_pc ns558 gameport soundcore parport ext3 jbd mbcache sha256_generic aes_i586 aes_generic cbc dm_crypt dm_mod ide_gd_mod ata_generic ohci_hcd ide_pci_generic sata_sil libata ehci_hcd amd74xx skge scsi_mod r8169 mii ide_core forcedeth usbcore nvidia_agp agpgart thermal fan thermal_sys [last unloaded: batman_adv]
[2236634.419089]
[2236634.419097] Pid: 20863, comm: ifconfig Not tainted (2.6.30-2-486 #1) A7N8X-E
[2236634.419107] EIP: 0060:[<e1bbe1cc>] EFLAGS: 00010002 CPU: 0
[2236634.419123] EIP is at hna_local_add+0xf3/0x15b [batman_adv]
[2236634.419131] EAX: ffffffff EBX: 00000202 ECX: 00000000 EDX: de2b6de0
[2236634.419140] ESI: de2b6de0 EDI: df3bf53c EBP: df3bf400 ESP: ccf65e98
[2236634.419148]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
[2236634.419156] Process ifconfig (pid: 20863, ti=ccf64000 task=de14e000 task.ti=ccf64000)
[2236634.419165] Stack:
[2236634.419170]  df3bf53c ccf65f0e ccf65f0c e1bbd3e9 ffffffed df3bf400 00000004 c04dfac8
[2236634.419184]  c0285c74 ccf65efc 00008924 c0286eb1 bfaf0898 00000000 00000000 00000000
[2236634.419199]  c0117c09 00000007 c13b52a0 00000000 00000000 c016b161 0804a440 decb68b4
[2236634.419216] Call Trace:
[2236634.419222]  [<e1bbd3e9>] ? interface_set_mac_addr+0x50/0x5e [batman_adv]
[2236634.419240]  [<c0285c74>] ? dev_set_mac_address+0x36/0x52
[2236634.419253]  [<c0286eb1>] ? dev_ioctl+0x4f0/0x5a4
[2236634.419262]  [<c0117c09>] ? kunmap_atomic+0x59/0x68
[2236634.419276]  [<c016b161>] ? __do_fault+0x2e0/0x310
[2236634.419290]  [<c0278fc8>] ? sock_ioctl+0x0/0x1e4
[2236634.419302]  [<c0185a72>] ? vfs_ioctl+0x16/0x42
[2236634.419313]  [<c0185fa4>] ? do_vfs_ioctl+0x41d/0x454
[2236634.419323]  [<c018601c>] ? sys_ioctl+0x41/0x58
[2236634.419332]  [<c0102f1c>] ? sysenter_do_call+0x12/0x28
[2236634.419343] Code: 89 c3 fa 8d 44 20 00 90 a1 bc 4b bc e1 89 f2 e8 a9 04 00 00 8b 0d bc 4b bc e1 66 ff 05 64 47 bc e1 c7 05 c0 4b bc e1 01 00 00 00 <8b> 41 04 8b 51 08 c1 e0 02 39 d0 7e 1f 01 d2 89 c8 e8 d6 05 00
[2236634.419404] EIP: [<e1bbe1cc>] hna_local_add+0xf3/0x15b [batman_adv] SS:ESP 0068:ccf65e98
[2236634.419424] CR2: 0000000000000004
[2236634.419434] ---[ end trace 13f2007552b7b72b ]---

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: only modify hna-table on active module

[Marek Lindner]

Hey,

nice patch and this time well formatted.  ;-)
Applied in 1578.

> Just for some more clarification about this bug (have a look at
> the attached call trace): It always occurs when I haven't put an
> interface into batman-adv and when I'm then changing the
> mac-address of bat0. I've now added a little check in
> interface_set_mac_addr() which seems to work nicely here in my
> setup. I'm also wondering if we should add another sanity check
> somewhere in hna_local_add() to directly avoid any racy null
> pointer dereferences in there.

I'm not sure it is a good place to put such a check since hna_local_add() is 
more of a library function which does not know much about the context. 
However, it could check whether the hna_local_hash had been initialized.

Cheers,
Marek