From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Haley Subject: Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name Date: Mon, 05 Nov 2012 21:01:50 -0700 Message-ID: <50988BAE.6020602@hp.com> References: <509184D9.8030103@hp.com> <5093940B.6020207@parallels.com> <5093E09A.6000004@hp.com> <1351899276.2703.35.camel@bwh-desktop.uk.solarflarecom.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Pavel Emelyanov , David Miller , Eric Dumazet , "netdev@vger.kernel.org" To: Ben Hutchings Return-path: Received: from g6t0185.atlanta.hp.com ([15.193.32.62]:43688 "EHLO g6t0185.atlanta.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753012Ab2KFELX (ORCPT ); Mon, 5 Nov 2012 23:11:23 -0500 In-Reply-To: <1351899276.2703.35.camel@bwh-desktop.uk.solarflarecom.com> Sender: netdev-owner@vger.kernel.org List-ID: On 11/02/2012 05:34 PM, Ben Hutchings wrote: > On Fri, 2012-11-02 at 11:02 -0400, Brian Haley wrote: >> On 11/02/2012 05:36 AM, Pavel Emelyanov wrote: >>>> +static int sock_getbindtodevice(struct sock *sk, char __user *optval, >>>> + int __user *optlen, int len) >>>> +{ >>>> + int ret = -ENOPROTOOPT; >>>> +#ifdef CONFIG_NETDEVICES >>>> + struct net *net = sock_net(sk); >>>> + struct net_device *dev; >>>> + char devname[IFNAMSIZ]; >>>> + >>>> + if (sk->sk_bound_dev_if == 0) { >>>> + len = 0; >>>> + goto zero; >>>> + } >>>> + >>>> + ret = -EINVAL; >>>> + if (len < IFNAMSIZ) >>>> + goto out; >>>> + >>>> + rcu_read_lock(); >>>> + dev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if); >>>> + if (dev) >>>> + strcpy(devname, dev->name); >>> >>> This still races with the device name change, potentially providing >>> a name which never existed in the system, doesn't it? >> >> My only argument here is that SIOCGIFNAME has had this same code forever, and >> noone has ever complained about that returning a garbled name. Even >> dev_get_by_name() only holds an rcu lock when doing a strncmp(). >> >> We'd need to audit the whole kernel to catch all the places where we potentially >> look at dev->name while it could change. Is it really worth it? > > A net device name can't be changed while the device is up, or while > another task holds the RTNL lock. I think that covers almost all uses. > I don't know whether it's worth going out to look for exceptions, but we > might as well fix the cases we know about. So do you think we can fix these corner cases later and get the API right first? -Brian