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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 19 Jun 2007 20:10:00 +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/19/07, Keiichi KII <k-keiichi@...jp.nec.com> wrote: > 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 . Hum, then why have we introduced that lock (netdev_change_sem) in the first place, as in what exactly is it protecting? In any case, however, the point to extend the critical section here to encapsulate all the three parts still stands. We wouldn't want ioctl(NETCON_REMOVE_TARGET) on the specified target to return without removing the target that the user specified just because that target's ethernet interface happens to be currently undergoing a name change. The correct behaviour would be to sleep on a mutex till the renaming has completed (which will then relinquish the mutex) and then (after acquiring the mutex) proceed to remove it, IMHO. > >> +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. I think you're referring to make_class_name() here? That seems to be somewhat bulkier than simply being a wrapper over kasprintf() like the make_netdev_class_name() here. I'd definitely recommend not obfuscating this simple functionality here. Thanks, Satyam - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists