Hi again, Ok, so sysfs_create_link() would be illegal from inside spin_lock_irqsave(), and this is why we have to use the dual-list mechanism to react to the net device rename. This isn't so obvious, a comment at the point where you declare modify_target_list would be nice? (BTW temporary_list would be a better name for that, IMO)
On 6/13/07, Keiichi KII <[EMAIL PROTECTED]> 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. ]
Ok, so reading through the code makes it obvious that this mutex is used to protect against the following race: Thread #1 Thread #2 ========= ========= [ NETDEV_CHANGENAME notifier ] [ ioctl(NETCON_REMOVE_TARGET) ] netconsole_event() move from target_list to temp list work on temp list kobject_unregister() -> release_target() -> remove_target() move back to target_list Which would mean a deleted/removed target added back => *boom* But, the race still hasn't been closed properly! You're taking the mutex only around "work on temp list" which is insufficient, you need to ensure atomicity inside netconsole_event() _completely_ like this (renaming netdev_change_sem to netdev_changename_mtx):
> +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) {
mutex_lock(netdev_changename_mtx) here.
> + 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);
This goes away.
> + list_for_each_entry(nt, &modify_target_list, list) { > + [...] > + }
> + up(&netdev_change_sem);
So does this.
> + 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);
mutex_unlock(netdev_changename_mtx) comes here.
> + } > + > + return NOTIFY_DONE; > +}
@@ -239,12 +240,14 @@ static void remove_target(struct netcons { unsigned long flags; + down(&netdev_change_sem); spin_lock_irqsave(&target_list_lock, flags); list_del(&nt->list); if (list_empty(&target_list)) netpoll_cleanup(&nt->np); spin_unlock_irqrestore(&target_list_lock, flags); kfree(nt); + up(&netdev_change_sem); }
As I said earlier, the target_list_lock spin-locking needs to be pushed out from here to the callers of remove_target. => mutex_lock(netdev_changename_mtx) must also be done by them.
+static char *make_netdev_class_name(char *netdev_name) +{ + char *name; + + name = kasprintf(GFP_KERNEL, "net:%s", netdev_name);
Why the "net:" prefix in the filename?
+ if (!name) { + printk(KERN_ERR "netconsole: kmalloc() failed!\n"); + return NULL; + } + + return name; +}
And this doesn't want to be a separate function either.
static int setup_target_sysfs(struct netconsole_target *nt) { + int retval = 0; + char *name; + kobject_set_name(&nt->obj, "port%d", nt->id); nt->obj.parent = &netconsole_miscdev.this_device->kobj; nt->obj.ktype = &target_ktype; - return kobject_register(&nt->obj); + retval = kobject_register(&nt->obj); + name = make_netdev_class_name(nt->np.dev_name); + if (!name) + return -ENOMEM;
Just call kasprintf() directly, why the obfuscation? Satyam - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/