Re: [RFC][PATCH -mm take5 4/7] using symlink for the net_device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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: [email protected]


-
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]
  Powered by Linux