Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751928AbaAPXfI (ORCPT ); Thu, 16 Jan 2014 18:35:08 -0500 Received: from out02.mta.xmission.com ([166.70.13.232]:44870 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751767AbaAPXfF (ORCPT ); Thu, 16 Jan 2014 18:35:05 -0500 From: ebiederm@xmission.com (Eric W. Biederman) To: Veaceslav Falico Cc: Tejun Heo , Greg KH , linux-kernel@vger.kernel.org, netdev@vger.kernel.org References: <20140114171740.GB1867@redhat.com> <20140114182135.GA29296@kroah.com> <20140114191208.GA9942@redhat.com> <20140114193139.GA3636@kroah.com> <20140114210610.GC9942@redhat.com> <87iotmc9ic.fsf@xmission.com> <20140115141618.GD7950@htj.dyndns.org> <87fvoo25gj.fsf@xmission.com> <20140116001116.GA27182@redhat.com> Date: Thu, 16 Jan 2014 15:34:56 -0800 In-Reply-To: <20140116001116.GA27182@redhat.com> (Veaceslav Falico's message of "Thu, 16 Jan 2014 01:11:16 +0100") Message-ID: <878uufwlen.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-AID: U2FsdGVkX19I2zDrydyvTdU3avC03G46JFYIk8lDmR4= X-SA-Exim-Connect-IP: 98.207.154.105 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 T_TM2_M_HEADER_IN_MSG BODY: T_TM2_M_HEADER_IN_MSG * 1.2 LotsOfNums_01 BODY: Lots of long strings of numbers * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.4969] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa06 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_01 4+ unique symbols in subject * 0.0 T_TooManySym_02 5+ unique symbols in subject X-Spam-DCC: XMission; sa06 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: *;Veaceslav Falico X-Spam-Relay-Country: Subject: Re: [RFC] sysfs_rename_link() and its usage X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Wed, 14 Nov 2012 14:26:46 -0700) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Veaceslav Falico writes: > On Wed, Jan 15, 2014 at 03:25:16PM -0800, Eric W. Biederman wrote: >>Tejun Heo writes: >> >>> Hey, Veaceslav, Eric. > > Hi Tejun, Eric, > >>> >>> On Tue, Jan 14, 2014 at 05:35:23PM -0800, Eric W. Biederman wrote: >>>> >>> >>This works like a charm. However, if I want to use (obviously, with the >>>> >>> >>symlink present): >>>> >>> >> >>>> >>> >>sysfs_rename_link(&(a->dev.kobj), &(b->dev.kobj), oldname, newname); >>>> >>> > >>>> >>> >You forgot the namespace option to this call, what kernel version are >>>> >>> >you using here? >>>> >>> >>>> >>> It's git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next , >>>> >>> 3.13-rc6 with some networking patches on top of it. >>> >>> Does this work on 3.12? How about Greg's driver-core-next? Do you >>> have a minimal test case that I can use to reproduce the issue? > > Sorry for the latency in responses, I'll update once I'll manage to test it > on those. > > ...snip... >>> Veaceslav, please confirm whether the issue is reproducible w/ v3.12. >> >>Anyway since a symlink living in a different namespace from it's target >>is just nonsense this (only compile tested) patch should fix the issue, >>and make sysfs_rename_link usable for people without a masters degree in >>sysfs again. > > It's still there :-(. I've used your patch and added my small addition[1] to > test the sysfs_rename_link() (on top of net-next, 3.13-rc7), however the > issue is still there: I expect the bug is my quick patch missed testing for sysfs_ns_type to see if we care at all about namespaces in sysfs_rename_link. Which would make just the sysfs_rename_link bit look like. Something like that. Eric diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c index 3ae3f1bf1a09..8b51d1b6cc21 100644 --- a/fs/sysfs/symlink.c +++ b/fs/sysfs/symlink.c @@ -194,15 +194,13 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link); * @targ: object we're pointing to. * @old: previous name of the symlink. * @new: new name of the symlink. - * @new_ns: new namespace of the symlink. - * * A helper function for the common rename symlink idiom. */ -int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ, - const char *old, const char *new, const void *new_ns) +int sysfs_rename_link(struct kobject *kobj, struct kobject *targ, + const char *old, const char *new) { struct sysfs_dirent *parent_sd, *sd = NULL; - const void *old_ns = NULL; + const void *old_ns = NULL, *new_ns = NULL; int result; if (!kobj) @@ -224,13 +222,16 @@ int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ, if (sd->s_symlink.target_sd->s_dir.kobj != targ) goto out; + if (sysfs_ns_type(parent_sd)) + new_ns = kobject_namespace(targ); + result = sysfs_rename(sd, parent_sd, new, new_ns); out: sysfs_put(sd); return result; } -EXPORT_SYMBOL_GPL(sysfs_rename_link_ns); +EXPORT_SYMBOL_GPL(sysfs_rename_link); static int sysfs_get_target_path(struct sysfs_dirent *parent_sd, struct sysfs_dirent *target_sd, char *path) > > [ 79.038340] net bond0: renaming to bondbla > [ 79.038380] ------------[ cut here ]------------ > [ 79.038411] WARNING: CPU: 1 PID: 5318 at fs/sysfs/dir.c:618 > sysfs_find_dirent+0x84/0x110() > [ 79.038449] sysfs: ns invalid in 'bridge0' for 'lower_bond0' > ...snip... > [ 79.038877] [] warn_slowpath_fmt+0x46/0x50 > [ 79.038903] [] ? sysfs_get_dirent_ns+0x30/0x80 > [ 79.038930] [] sysfs_find_dirent+0x84/0x110 > [ 79.038957] [] sysfs_get_dirent_ns+0x3e/0x80 > [ 79.038983] [] sysfs_rename_link+0x57/0xe0 > [ 79.039030] [] netdev_adjacent_rename_links+0xa2/0x160 > > The current scheme (sysfs_remove_link() + sysfs_add_link()) works perfectly > well without any namespaces. I'll dig into it once I have some spare time, > it's not at all critical. > > [1]: the patch (I've included your patch too, just in case): > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 67b180d..0c9377a 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -1825,9 +1825,8 @@ int device_rename(struct device *dev, const char *new_name) > } > if (dev->class) { > - error = sysfs_rename_link_ns(&dev->class->p->subsys.kobj, > - kobj, old_device_name, > - new_name, kobject_namespace(kobj)); > + error = sysfs_rename_link(&dev->class->p->subsys.kobj, > + kobj, old_device_name, new_name); > if (error) > goto out; > } > diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c > index 3ae3f1b..651444a 100644 > --- a/fs/sysfs/symlink.c > +++ b/fs/sysfs/symlink.c > @@ -194,12 +194,10 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link); > * @targ: object we're pointing to. > * @old: previous name of the symlink. > * @new: new name of the symlink. > - * @new_ns: new namespace of the symlink. > - * > * A helper function for the common rename symlink idiom. > */ > -int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ, > - const char *old, const char *new, const void *new_ns) > +int sysfs_rename_link(struct kobject *kobj, struct kobject *targ, > + const char *old, const char *new) > { > struct sysfs_dirent *parent_sd, *sd = NULL; > const void *old_ns = NULL; > @@ -224,13 +222,13 @@ int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ, > if (sd->s_symlink.target_sd->s_dir.kobj != targ) > goto out; > - result = sysfs_rename(sd, parent_sd, new, new_ns); > + result = sysfs_rename(sd, parent_sd, new, kobject_namespace(targ)); > out: > sysfs_put(sd); > return result; > } > -EXPORT_SYMBOL_GPL(sysfs_rename_link_ns); > +EXPORT_SYMBOL_GPL(sysfs_rename_link); > static int sysfs_get_target_path(struct sysfs_dirent *parent_sd, > struct sysfs_dirent *target_sd, char *path) > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > index 6695040..093d992 100644 > --- a/include/linux/sysfs.h > +++ b/include/linux/sysfs.h > @@ -213,9 +213,8 @@ int __must_check sysfs_create_link_nowarn(struct kobject *kobj, > const char *name); > void sysfs_remove_link(struct kobject *kobj, const char *name); > -int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *target, > - const char *old_name, const char *new_name, > - const void *new_ns); > +int sysfs_rename_link(struct kobject *kobj, struct kobject *target, > + const char *old_name, const char *new_name); > void sysfs_delete_link(struct kobject *dir, struct kobject *targ, > const char *name); > @@ -341,9 +340,8 @@ static inline void sysfs_remove_link(struct kobject *kobj, const char *name) > { > } > -static inline int sysfs_rename_link_ns(struct kobject *k, struct kobject *t, > - const char *old_name, > - const char *new_name, const void *ns) > +static inline int sysfs_rename_link(struct kobject *k, struct kobject *t, > + const char *old_name, const char *new_name) > { > return 0; > } > @@ -455,12 +453,6 @@ static inline void sysfs_remove_file(struct kobject *kobj, > return sysfs_remove_file_ns(kobj, attr, NULL); > } > -static inline int sysfs_rename_link(struct kobject *kobj, struct kobject > *target, > - const char *old_name, const char *new_name) > -{ > - return sysfs_rename_link_ns(kobj, target, old_name, new_name, NULL); > -} > - > static inline struct sysfs_dirent * > sysfs_get_dirent(struct sysfs_dirent *parent_sd, const unsigned char *name) > { > diff --git a/net/core/dev.c b/net/core/dev.c > index 9957557..5d24d8e 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -5005,19 +5005,20 @@ EXPORT_SYMBOL(netdev_upper_dev_unlink); > void netdev_adjacent_rename_links(struct net_device *dev, char *oldname) > { > struct netdev_adjacent *iter; > + char old_linkname[IFNAMSIZ+7], new_linkname[IFNAMSIZ+7]; > list_for_each_entry(iter, &dev->adj_list.upper, list) { > - netdev_adjacent_sysfs_del(iter->dev, oldname, > - &iter->dev->adj_list.lower); > - netdev_adjacent_sysfs_add(iter->dev, dev, > - &iter->dev->adj_list.lower); > + sprintf(old_linkname, "lower_%s", oldname); > + sprintf(new_linkname, "lower_%s", dev->name); > + sysfs_rename_link(&(iter->dev->dev.kobj), &(dev->dev.kobj), > + old_linkname, new_linkname); > } > list_for_each_entry(iter, &dev->adj_list.lower, list) { > - netdev_adjacent_sysfs_del(iter->dev, oldname, > - &iter->dev->adj_list.upper); > - netdev_adjacent_sysfs_add(iter->dev, dev, > - &iter->dev->adj_list.upper); > + sprintf(old_linkname, "upper_%s", oldname); > + sprintf(new_linkname, "upper_%s", dev->name); > + sysfs_rename_link(&(iter->dev->dev.kobj), &(dev->dev.kobj), > + old_linkname, new_linkname); > } > } > -- 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/