lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 13 Jun 2007 22:19:39 +0530 From: "Satyam Sharma" <satyam.sharma@...il.com> To: "Keiichi KII" <k-keiichi@...jp.nec.com> Cc: "Matt Mackall" <mpm@...enic.com>, "Andrew Morton" <akpm@...ux-foundation.org>, "David Miller" <davem@...emloft.net>, linux-kernel@...r.kernel.org, netdev@...r.kernel.org Subject: Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device Hi, On 6/13/07, Keiichi KII <k-keiichi@...jp.nec.com> wrote: > [...] > +static DECLARE_MUTEX(netdev_change_sem); The preferred style these days is to use a DEFINE_MUTEX (and the struct mutex primitives) for such locks that are used as binary semaphores. BTW, a comment here to note what this lock protects is required. [ You don't really need to give a comment for the target_list_lock because it's defined just below the "target_list". It's not equally obvious at first glance what is protected by the netdev_change_sem, however. ] > + down(&netdev_change_sem); > + up(&netdev_change_sem); So these become mutex_lock and mutex_unlock. > +static int netconsole_event(struct notifier_block *this, unsigned long event, > + void *ptr) > +{ > + int error = 0; > + unsigned long flags; > + char *old_link_name = NULL, *new_link_name = NULL; > + struct netconsole_target *nt, *tmp; > + struct net_device *dev = ptr; > + LIST_HEAD(modify_target_list); > + > + if (event == NETDEV_CHANGENAME) { > + spin_lock_irqsave(&target_list_lock, flags); > + list_for_each_entry_safe(nt, tmp, &target_list, list) > + if (nt->np.dev == dev) > + list_move(&nt->list, &modify_target_list); > + spin_unlock_irqrestore(&target_list_lock, flags); > + > + down(&netdev_change_sem); > + list_for_each_entry(nt, &modify_target_list, list) { > + new_link_name = make_netdev_class_name(dev->name); > + old_link_name = make_netdev_class_name(nt->np.dev_name); > + if (!new_link_name || !old_link_name) { > + printk(KERN_ERR "netconsole: " > + "make_netdev_class_name() failed!\n"); > + kfree(new_link_name); > + kfree(old_link_name); > + continue; Sorry, but we're not covering from the error condition fully here. Note that later you merge the temporary modify_target_list entirely back into the target_list ... which would still contain these erroneous nodes. A full cleanup (kobject_unregister the entry, and then list_del from modify_target_list) is required here, before continuing. > + } > + sysfs_remove_link(&nt->obj, old_link_name); > + error = sysfs_create_link(&nt->obj, > + &nt->np.dev->dev.kobj, > + new_link_name); > + if (error) > + printk(KERN_ERR "can't create link: %s\n", > + new_link_name); Same here, a fuller cleanup is required to recover from the error condition here, then kfree()'s and then stick a "continue;" here, else ... > + strcpy(nt->np.dev_name, dev->name); ... you'll have move this up. > + kfree(new_link_name); > + kfree(old_link_name); > + } > + up(&netdev_change_sem); > + > + spin_lock_irqsave(&target_list_lock, flags); > + list_for_each_entry_safe(nt, tmp, &modify_target_list, list) > + list_move(&nt->list, &target_list); > + spin_unlock_irqrestore(&target_list_lock, flags); > + } > + > + return NOTIFY_DONE; > +} Satyam - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists