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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
