Skip to contentSkip to conversation

sockopt: Change getsockopt() of SO_BINDTODEVICE to return an interface name

PATCH Superseded Brian Haley 6 participants 12 messages Oct 31, 2012 – Nov 8, 2012

Subsystems (3)
Networking Network drivers Sockets

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Brian Haley

author
#0 lore raw

PATCH 1/1 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 <[email protected]>

--

net/core/sock.c +49 -2
index 0a023b8..9172ff4 100644
@@ -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;
}

#1 lore raw
Quoted 9 lines · 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.

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

-Andi

Signature · 1 line
[email protected] -- Speaking for myself only
author
#2 lore raw
Quoted 6 lines · Andi Kleen
Quoted 4 lines · 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.

Quoted 8 lines
Quoted 5 lines

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

maintainer
#3 lore raw
In reply to Andi Kleen

From: Andi Kleen <[email protected]>
Date: Wed, 31 Oct 2012 13:47:01 -0700

Quoted 6 lines
Quoted 4 lines · 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.

Quoted 8 lines
Quoted 5 lines

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.

#4 lore raw
In reply to Brian Haley
Quoted 12 lines
net/core/sock.c +12 -0
+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;

net/core/sock.c out: · line 587

Quoted 10 lines
net/core/sock.c +10 -0
+	}
+
+	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?

Quoted 13 lines
net/core/sock.c +13 -0
+	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;

net/core/sock.c out: · line 610

Quoted 10 lines
net/core/sock.c +10 -0
+	if (put_user(len, optlen))
+		goto out;
+
+	ret = 0;
+
+out:
+#endif
+
+	return ret;
+}

Thanks,
Pavel

#5 lore raw
Quoted 2 lines · Pavel Emelyanov

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.

net/core/dev.c +10 -2
index b4978e2..0184ec9 100644
@@ -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 {
author
#6 lore raw
In reply to Pavel Emelyanov
Quoted 17 lines · Pavel Emelyanov
Quoted 17 lines
net/core/sock.c +17 -0
+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;
Quoted 8 lines

net/core/sock.c out: · line 587

Quoted 5 lines
net/core/sock.c +5 -0
+
+	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

#7 lore raw
Quoted 27 lines · Brian Haley
Quoted 17 lines · Pavel Emelyanov
Quoted 17 lines
net/core/sock.c +17 -0
+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;
Quoted 8 lines

net/core/sock.c out: · line 587

Quoted 5 lines
net/core/sock.c +5 -0
+
+	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?

Quoted 6 lines

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.

Signature · 3 lines
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.
author
#8 lore raw
Quoted 32 lines · Ben Hutchings
Quoted 31 lines · Brian Haley
Quoted 17 lines · Pavel Emelyanov
Quoted 17 lines
net/core/sock.c +17 -0
+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;
Quoted 8 lines

net/core/sock.c out: · line 587

Quoted 5 lines
net/core/sock.c +5 -0
+
+	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().

Quoted 7 lines
Quoted 2 lines

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

#9 lore raw
Quoted 36 lines · Brian Haley
Quoted 35 lines · Ben Hutchings
Quoted 33 lines · Brian Haley
Quoted 17 lines · Pavel Emelyanov
Quoted 17 lines
net/core/sock.c +17 -0
+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;
Quoted 8 lines

net/core/sock.c out: · line 587

Quoted 5 lines
net/core/sock.c +5 -0
+
+	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?

Quoted 7 lines
Quoted 4 lines

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.

Signature · 3 lines
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.
author
#10 lore raw
In reply to Brian Haley
Quoted 17 lines · Brian Haley
Quoted 17 lines
Quoted 16 lines
Quoted 7 lines

net/core/sock.c out: · line 587

Quoted 4 lines
net/core/sock.c +4 -0
+    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?

Quoted 7 lines
Quoted 4 lines

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

maintainer
#11 lore raw

From: Brian Haley <[email protected]>
Date: Thu, 08 Nov 2012 08:36:02 -0700

Quoted 1 line

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.