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

[Satyam Sharma]

Hi,

On 6/19/07, Keiichi KII <k-keiichi@xxxxxxxxxxxxx> 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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/