From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [Bonding-devel] [PATCH] bonding: fix WARN_ON when writing to bond_master sysfs file Date: Mon, 4 Oct 2010 22:26:17 -0400 Message-ID: <20101005022617.GA10809@hmsreliant.think-freely.org> References: <20101004202112.GA27897@hmsreliant.think-freely.org> <20101005095713.624ca320@s6510> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, fubar@us.ibm.com, bonding-devel@lists.sourceforge.net, davem@davemloft.net To: Stephen Hemminger Return-path: Received: from charlotte.tuxdriver.com ([70.61.120.58]:36929 "EHLO smtp.tuxdriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757227Ab0JECa1 (ORCPT ); Mon, 4 Oct 2010 22:30:27 -0400 Content-Disposition: inline In-Reply-To: <20101005095713.624ca320@s6510> Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Oct 05, 2010 at 09:57:13AM +0900, Stephen Hemminger wrote: > On Mon, 4 Oct 2010 16:21:12 -0400 > Neil Horman wrote: > > > Fix a WARN_ON failure in bond_masters sysfs file > > > > Got a report of this warning recently > > > > bonding: bond0 is being created... > > ------------[ cut here ]------------ > > WARNING: at fs/proc/generic.c:590 proc_register+0x14d/0x185() > > Hardware name: ProLiant BL465c G1 > > proc_dir_entry 'bonding/bond0' already registered > > Modules linked in: bonding ipv6 tg3 bnx2 shpchp amd64_edac_mod edac_core > > ipmi_si > > ipmi_msghandler serio_raw i2c_piix4 k8temp edac_mce_amd hpwdt microcode hpsa > > cc > > iss radeon ttm drm_kms_helper drm i2c_algo_bit i2c_core [last unloaded: > > scsi_wai > > t_scan] > > Pid: 935, comm: ifup-eth Not tainted 2.6.33.5-124.fc13.x86_64 #1 > > Call Trace: > > [] warn_slowpath_common+0x77/0x8f > > [] warn_slowpath_fmt+0x3c/0x3e > > [] proc_register+0x14d/0x185 > > [] proc_create_data+0x87/0xa1 > > [] bond_create_proc_entry+0x55/0x95 [bonding] > > [] bond_init+0x95/0xd0 [bonding] > > [] register_netdevice+0xdd/0x29e > > [] bond_create+0x8e/0xb8 [bonding] > > [] bonding_store_bonds+0xb3/0x1c1 [bonding] > > [] class_attr_store+0x27/0x29 > > [] sysfs_write_file+0x10f/0x14b > > [] vfs_write+0xa9/0x106 > > [] sys_write+0x45/0x69 > > [] system_call_fastpath+0x16/0x1b > > ---[ end trace a677c3f7f8b16b1e ]--- > > bonding: Bond creation failed. > > > > It happens because a user space writer to bond_master can try to register and > > already existing bond interface name. Fix it by teaching bond_create to check > > for the existance of devices with that name first in cases where a non-NULL name > > parameter has been passed in > > > > Signed-off-by: Neil Horman > > > > bond_main.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > index fb70c3e..10e4ffe 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -5148,7 +5148,7 @@ static struct rtnl_link_ops bond_link_ops __read_mostly = { > > */ > > int bond_create(struct net *net, const char *name) > > { > > - struct net_device *bond_dev; > > + struct net_device *bond_dev, *check_dev; > > int res; > > > > rtnl_lock(); > > @@ -5168,6 +5168,20 @@ int bond_create(struct net *net, const char *name) > > res = dev_alloc_name(bond_dev, "bond%d"); > > if (res < 0) > > goto out; > > + } else { > > + /* > > + * If we're given a name to register > > + * we need to ensure that its not already > > + * registered > > + */ > > + check_dev = dev_get_by_name(net, name); > > + > > + res = (check_dev) ? 0 : -EEXIST; > > + > > + dev_put(check_dev); > > + > > + if (res) > > + goto out; > > } > > > > res = register_netdevice(bond_dev); > > Why is this necessary? > register_netdevice does already check for duplicate name so please > use it's return value instead > No, its the call to register_netdev that causes the WARN_ON. Check the stack trace, register_netdev call bond_init which in turn registers a proc interface by an existing name, which causes the WARN_ON > If that doesn't work then use __dev_get_by_name to avoid > ref count (ie dev_put). > Yeah, thats a good idea. I'll respin this in the AM. Thanks! Regards Neil