[PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[Brian Haley]

Instead of having the getsockopt() of SO_BINDTODEVICE return an index, which
will then require another call like if_indextoname() to get the actual interface
name, have it return the name directly.

This also matches the existing man page description on socket(7) which mentions
the argument being an interface name.

If the value has not been set, zero is returned and optlen will be set to zero
to indicate there is no interface name present.

Signed-off-by: Brian Haley <brian.haley@hp.com>

--

diff --git a/net/core/sock.c b/net/core/sock.c
index 0a023b8..9172ff4 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -505,7 +505,8 @@ struct dst_entry *sk_dst_check(struct sock *sk, u32 cookie)
 }
 EXPORT_SYMBOL(sk_dst_check);

-static int sock_bindtodevice(struct sock *sk, char __user *optval, int optlen)
+static int sock_setbindtodevice(struct sock *sk, char __user *optval,
+				int optlen)
 {
 	int ret = -ENOPROTOOPT;
 #ifdef CONFIG_NETDEVICES
@@ -562,6 +563,52 @@ out:
 	return ret;
 }

+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);
+	rcu_read_unlock();
+	ret = -ENODEV;
+	if (!dev)
+		goto out;
+
+	len = strlen(devname) + 1;
+
+	ret = -EFAULT;
+	if (copy_to_user(optval, devname, len))
+		goto out;
+
+zero:
+	ret = -EFAULT;
+	if (put_user(len, optlen))
+		goto out;
+
+	ret = 0;
+
+out:
+#endif
+
+	return ret;
+}
+
 static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
 {
 	if (valbool)
@@ -589,7 +636,7 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
 	 */

 	if (optname == SO_BINDTODEVICE)
-		return sock_bindtodevice(sk, optval, optlen);
+		return sock_setbindtodevice(sk, optval, optlen);

 	if (optlen < sizeof(int))
 		return -EINVAL;
@@ -1074,9 +1121,10 @@ int sock_getsockopt(struct socket *sock, int level, int
optname,
 	case SO_NOFCS:
 		v.val = sock_flag(sk, SOCK_NOFCS);
 		break;
+
 	case SO_BINDTODEVICE:
-		v.val = sk->sk_bound_dev_if;
-		break;
+		return sock_getbindtodevice(sk, optval, optlen, len);
+
 	default:
 		return -ENOPROTOOPT;
 	}

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[Andi Kleen]

Brian Haley <brian.haley@hp.com> writes:

> Instead of having the getsockopt() of SO_BINDTODEVICE return an index, which
> will then require another call like if_indextoname() to get the actual interface
> name, have it return the name directly.
>
> This also matches the existing man page description on socket(7) which mentions
> the argument being an interface name.
>
> If the value has not been set, zero is returned and optlen will be set to zero
> to indicate there is no interface name present.

That will break all existing programs using the return value, right?
Better to fix the manpage

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[Brian Haley]

On 10/31/2012 04:47 PM, Andi Kleen wrote:
> Brian Haley <brian.haley@hp.com> writes:
> 
>> Instead of having the getsockopt() of SO_BINDTODEVICE return an index, which
>> will then require another call like if_indextoname() to get the actual interface
>> name, have it return the name directly.
>>
>> This also matches the existing man page description on socket(7) which mentions
>> the argument being an interface name.
>>
>> If the value has not been set, zero is returned and optlen will be set to zero
>> to indicate there is no interface name present.
> 
> That will break all existing programs using the return value, right?
> Better to fix the manpage

Dave just accepted the original code for this into net-next last week, so I
don't think it's too late to change it to be correct.

-Brian

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Wed, 31 Oct 2012 13:47:01 -0700

> Brian Haley <brian.haley@hp.com> writes:
> 
>> Instead of having the getsockopt() of SO_BINDTODEVICE return an index, which
>> will then require another call like if_indextoname() to get the actual interface
>> name, have it return the name directly.
>>
>> This also matches the existing man page description on socket(7) which mentions
>> the argument being an interface name.
>>
>> If the value has not been set, zero is returned and optlen will be set to zero
>> to indicate there is no interface name present.
> 
> That will break all existing programs using the return value, right?
> Better to fix the manpage

It never returned a value before, you could not getsockopt() on
SO_BINDTODEVICE previously.

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[Pavel Emelyanov]

> +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?

> +	rcu_read_unlock();
> +	ret = -ENODEV;
> +	if (!dev)
> +		goto out;
> +
> +	len = strlen(devname) + 1;
> +
> +	ret = -EFAULT;
> +	if (copy_to_user(optval, devname, len))
> +		goto out;
> +
> +zero:
> +	ret = -EFAULT;
> +	if (put_user(len, optlen))
> +		goto out;
> +
> +	ret = 0;
> +
> +out:
> +#endif
> +
> +	return ret;
> +}

Thanks,
Pavel

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[Eric Dumazet]

On Fri, 2012-11-02 at 13:36 +0400, Pavel Emelyanov wrote:

> This still races with the device name change, potentially providing
> a name which never existed in the system, doesn't it?

It does.

Maybe add a seqlock_t, to avoid taking rtnl here.
(as it could bring interesting lockdep/deadlock issues)

Here is the write side. Read side is trivial.

diff --git a/net/core/dev.c b/net/core/dev.c
index b4978e2..0184ec9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -201,6 +201,8 @@ static struct list_head ptype_all __read_mostly;	/* Taps */
 DEFINE_RWLOCK(dev_base_lock);
 EXPORT_SYMBOL(dev_base_lock);
 
+DEFINE_SEQLOCK(devnet_rename_seq);
+
 static inline void dev_base_seq_inc(struct net *net)
 {
 	while (++net->dev_base_seq == 0);
@@ -1018,17 +1020,22 @@ int dev_change_name(struct net_device *dev, const char *newname)
 
 	memcpy(oldname, dev->name, IFNAMSIZ);
 
+	write_seqlock(&devnet_rename_seq);
 	err = dev_get_valid_name(net, dev, newname);
-	if (err < 0)
+	if (err < 0) {
+		write_sequnlock(&devnet_rename_seq);
 		return err;
-
+	}
 rollback:
 	ret = device_rename(&dev->dev, dev->name);
 	if (ret) {
 		memcpy(dev->name, oldname, IFNAMSIZ);
+		write_sequnlock(&devnet_rename_seq);
 		return ret;
 	}
 
+	write_sequnlock(&devnet_rename_seq);
+
 	write_lock_bh(&dev_base_lock);
 	hlist_del_rcu(&dev->name_hlist);
 	write_unlock_bh(&dev_base_lock);
@@ -1046,6 +1053,7 @@ rollback:
 		/* err >= 0 after dev_alloc_name() or stores the first errno */
 		if (err >= 0) {
 			err = ret;
+			write_seqlock(&devnet_rename_seq);
 			memcpy(dev->name, oldname, IFNAMSIZ);
 			goto rollback;
 		} else {

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[Brian Haley]

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?

-Brian

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[Ben Hutchings]

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.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[Brian Haley]

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

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[Ben Hutchings]

On Mon, 2012-11-05 at 21:01 -0700, Brian Haley wrote:
> 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?

Well you can avoid the problem here by using rtnl_{,un}lock() and
__dev_get_by_index().  But if that's too heavyweight (it depends on how
often you think this might need to be called) then I suppose it can be
left until we have a general solution for races with renaming.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[Brian Haley]

On 11/05/2012 09:01 PM, Brian Haley wrote:
>>>>> +    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?

Hi Dave,

I noticed this isn't in patchwork anymore.  Is it possible to get this 
in and then work on the other issue in a separate patch since it's not 
just this code that has this problem?

Of course I'm still not convinced that extra check is necessary (but 
I'll do it to make others happy) because if you do a setsockopt("eth0"), 
all you care is that a getsockopt() returns "eth0" - if it returns 
anything else, eth0_renamed, eth0_foo, etc you'll notice it's not the 
same and take action.  And the fact that the name can't change when UP 
means the odds are small it can happen anyways.

Thanks,

-Brian

Re: [PATCH net-next] sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

[David Miller]

From: Brian Haley <brian.haley@hp.com>
Date: Thu, 08 Nov 2012 08:36:02 -0700

> I noticed this isn't in patchwork anymore.

I want all the races resolved, and in any event the thing to do
when something isn't in patchwork IS TO RESUBMIT IT!

Every time you ask me stuff like this you take up more of my already
low amount of time.

Today I'm again without internet access other than my cell phone, so
everything you can do TO REDUCE my overhead would be greatly
appreciated.