Messages in this thread |  | | | Date | Tue, 19 Jun 2007 19:04:19 +0900 | | From | Keiichi KII <> | | Subject | Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device |
| |
Hello Satyam,
> 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)
All right, my patches are short of comments. So, I will add comments to the ambiguous codes.
> 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):
After the target moves from target_list to temporary_list, the kobject_unregister() of possible raced target isn't called in ioctl(NETCON_REMOVE_TARGET) because the target_list doesn't contain the target .
I have the wrong idea?
>> +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?
Because I drew upon dev_change_name() method in net/core/dev.c. The device_rename() in the above function makes use of same prefix related to netdev.
>> 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? >
I drew upon dev_change_name() method in net/core/dev.c.
Thanks -- Keiichi KII NEC Corporation OSS Platform Development Division E-mail: k-keiichi@bx.jp.nec.com
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |