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

PATCH Superseded Brian Haley 3 participants 5 messages Nov 16, 2012 – Nov 26, 2012

Subsystems (3)

Networking Network drivers Sockets

lore

Share & tools

lore

Brian Haley

author

2012-11-16 20:59 UTC #0 lore raw

PATCH v2 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.

Added a seqlock to protect this code path, and dev_ifname(), from someone changing the device name via dev_change_name().

v2: Added seqlock protection while copying device name.

Signed-off-by Brian Haley <[email protected]>

include/linux/netdevice.h +0 -0

index e46c830..e9929ab 100644
@@ -1567,6 +1567,8 @@ extern int call_netdevice_notifiers(unsigned long val,

struct net_device *dev);

extern rwlock_t dev_base_lock; /* Device list lock */

+extern seqlock_t devnet_rename_seq; /* Device rename lock */
+

#define for_each_netdev(net, d) \
list_for_each_entry(d, &(net)->dev_base_head, dev_list)

net/core/dev.c +2 -0

index 2705a2a..3a576ed 100644
@@ -203,6 +203,8 @@ static struct list_head offload_base __read_mostly;
 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);

@@ -1091,22 +1093,31 @@ int dev_change_name(struct net_device *dev, const char

*newname)
if (dev->flags & IFF_UP)
return -EBUSY;

- if (strncmp(newname, dev->name, IFNAMSIZ) == 0)
+ write_seqlock(&devnet_rename_seq);
+
+ if (strncmp(newname, dev->name, IFNAMSIZ) == 0) {
+ write_sequnlock(&devnet_rename_seq);
return 0;
+ }

memcpy(oldname, dev->name, IFNAMSIZ);

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);
@@ -1124,6 +1135,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 {
@@ -4148,6 +4160,7 @@ static int dev_ifname(struct net *net, struct ifreq __user *arg)
{
struct net_device *dev;
struct ifreq ifr;
+ unsigned seq;

/*
* Fetch the caller's info block.
@@ -4156,6 +4169,8 @@ static int dev_ifname(struct net *net, struct ifreq __user *arg)
if (copy_from_user(&ifr, arg, sizeof(struct ifreq)))
return -EFAULT;

+retry:
+ seq = read_seqbegin(&devnet_rename_seq);
rcu_read_lock();
dev = dev_get_by_index_rcu(net, ifr.ifr_ifindex);
if (!dev) {
@@ -4165,6 +4180,8 @@ static int dev_ifname(struct net *net, struct ifreq __user *arg)

strcpy(ifr.ifr_name, dev->name);
rcu_read_unlock();
+ if (read_seqretry(&devnet_rename_seq, seq))
+ goto retry;

if (copy_to_user(arg, &ifr, sizeof(struct ifreq)))
return -EFAULT;

net/core/sock.c +56 -2

76 lines not on this page · raw message

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);
+
case SO_GET_FILTER:
len = sk_get_filter(sk, (struct sock_filter __user *)optval, len); if (len < 0)
return len;

goto lenout;
+
default:
return -ENOPROTOOPT;
}

Pavel Emelyanov

2012-11-17 05:06 UTC #1 lore raw

net/core/sock.c static int dev_ifname(struct net *net, struct ifreq __user · line 4185

Quoted 7 lines

net/core/sock.c +2 -0

@@ -4165,6 +4180,8 @@ static int dev_ifname(struct net *net, struct ifreq __user
 *arg)
 
 	strcpy(ifr.ifr_name, dev->name);
 	rcu_read_unlock();
+	if (read_seqretry(&devnet_rename_seq, seq))
+		goto retry;

I believe it makes sense to make the seqcount protection as a separate patch with description of what may happen.

Quoted 21 lines

net/core/sock.c +13 -0

 	if (copy_to_user(arg, &ifr, sizeof(struct ifreq)))
 		return -EFAULT;

@@ -562,6 +563,59 @@ 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];
+	unsigned seq;
+
+	if (sk->sk_bound_dev_if == 0) {
+		len = 0;
+		goto zero;

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

Quoted 10 lines

net/core/sock.c +10 -0

+	}
+
+	ret = -EINVAL;
+	if (len < IFNAMSIZ)
+		goto out;
+
+retry:
+	seq = read_seqbegin(&devnet_rename_seq);
+	rcu_read_lock();
+	dev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if);

The sk->sk_bound_dev_if might have changed to 0 while we did read_seqretry (or did the len check above, but the race window is smaller) and this code will report -ENODEV instead of zero lenght.

Other than this, the intention looks OK to me.

Quoted 7 lines

net/core/sock.c +7 -0

+	ret = -ENODEV;
+	if (!dev) {
+		rcu_read_unlock();
+		goto out;
+	}
+
+	strcpy(devname, dev->name);

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

Quoted 10 lines

net/core/sock.c +10 -0

+	rcu_read_unlock();
+	if (read_seqretry(&devnet_rename_seq, seq))
+		goto retry;
+
+	len = strlen(devname) + 1;
+
+	ret = -EFAULT;
+	if (copy_to_user(optval, devname, len))
+		goto out;
+

Thanks,
Pavel

Brian Haley

author

2012-11-17 21:58 UTC #2 lore raw

Quoted 10 lines · Pavel Emelyanov

net/core/sock.c static int dev_ifname(struct net *net, struct ifreq __user · line 4185
Quoted 7 lines
net/core/sock.c +2 -0
@@ -4165,6 +4180,8 @@ static int dev_ifname(struct net *net, struct ifreq __user
 *arg)
 
 	strcpy(ifr.ifr_name, dev->name);
 	rcu_read_unlock();
+	if (read_seqretry(&devnet_rename_seq, seq))
+		goto retry;
I believe it makes sense to make the seqcount protection as a separate patch with description of what may happen.

I asked about that before and Dave said he "wanted all the races resolved". At best I could make this a series...

Quoted 8 lines

net/core/sock.c out: · line 588
Quoted 4 lines
net/core/sock.c +4 -0
+retry:
+	seq = read_seqbegin(&devnet_rename_seq);
+	rcu_read_lock();
+	dev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if);
The sk->sk_bound_dev_if might have changed to 0 while we did read_seqretry (or did the len check above, but the race window is smaller) and this code will report -ENODEV instead of zero lenght.

If there are two threads twiddling with the same socket like this the application is broken in my mind.

-Brian

David Miller

maintainer

2012-11-20 18:58 UTC #3 lore raw

In reply to Brian Haley

From: Brian Haley <[email protected]>
Date: Fri, 16 Nov 2012 15:58:35 -0500

Quoted 10 lines

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.

Quoted 6 lines

Added a seqlock to protect this code path, and dev_ifname(), from someone changing the device name via dev_change_name().
v2: Added seqlock protection while copying device name.
Signed-off-by Brian Haley <[email protected]>

Brian I was going to apply this, but something about how you email patches results in them being corrupted.

Go to:

http://patchwork.ozlabs.org/patch/199732/

Click on Download "mbox", and try to apply that to the net-next tree to see what I mean.

Brian Haley

author

2012-11-26 15:10 UTC #4 lore raw

Quoted 7 lines · David Miller

Quoted 3 lines
v2: Added seqlock protection while copying device name.
Signed-off-by Brian Haley <[email protected]>
Brian I was going to apply this, but something about how you email patches results in them being corrupted.

Quoted 6 lines

Go to:
http://patchwork.ozlabs.org/patch/199732/
Click on Download "mbox", and try to apply that to the net-next tree to see what I mean.

I'll take a look why that was wrapping and send a v3, been away...

-Brian