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

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 <redacted>

--

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;
 	}

Brian Haley [off-list ref] 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

On 10/31/2012 04:47 PM, Andi Kleen wrote:

Brian Haley [off-list ref] writes:

quoted

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

From: Andi Kleen <redacted>
Date: Wed, 31 Oct 2012 13:47:01 -0700

Brian Haley [off-list ref] writes:

quoted

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.

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

Thanks,
Pavel

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 {

On 11/02/2012 05:36 AM, Pavel Emelyanov wrote:

quoted

+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

On Fri, 2012-11-02 at 11:02 -0400, Brian Haley wrote:

On 11/02/2012 05:36 AM, Pavel Emelyanov wrote:

quoted

quoted

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

On 11/02/2012 05:34 PM, Ben Hutchings wrote:

On Fri, 2012-11-02 at 11:02 -0400, Brian Haley wrote:

quoted

On 11/02/2012 05:36 AM, Pavel Emelyanov wrote:

quoted

quoted

+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

On Mon, 2012-11-05 at 21:01 -0700, Brian Haley wrote:

On 11/02/2012 05:34 PM, Ben Hutchings wrote:

quoted

On Fri, 2012-11-02 at 11:02 -0400, Brian Haley wrote:

quoted

On 11/02/2012 05:36 AM, Pavel Emelyanov wrote:

quoted

quoted

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

On 11/05/2012 09:01 PM, Brian Haley wrote:

quoted

quoted

quoted

quoted

+    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

From: Brian Haley <redacted>
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.