| Message ID | 1267235382-4026-1-git-send-email-linus.luessing@web.de |
|---|---|
| State | Accepted, archived |
| Headers | show |
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 > [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 ]---
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
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; }
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(-)