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

PATCH In review Brian Haley 3 participants 3 messages Nov 26, 2012 – Nov 27, 2012

Subsystems (3)

Networking Network drivers Sockets

lore

Share & tools

lore

Brian Haley

author

2012-11-26 15:21 UTC #0 lore raw

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

v3: Fixed word wrap in patch.

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

include/linux/netdevice.h +2 -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 +19 -2

index 7304ea8..2a5f558 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 +60 -4

95 lines not on this page · raw message

Ben Hutchings

Reviewed-by on v3 1/1

2012-11-27 20:34 UTC #1 lore raw

Quoted 18 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.
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.
v3: Fixed word wrap in patch.
Signed-off-by Brian Haley <[email protected]>

[...]

Reviewed-by Ben Hutchings <[email protected]>

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.

Pavel Emelyanov

Acked-by on v3 1/1

2012-11-27 20:36 UTC #2 lore raw

In reply to Brian Haley

Quoted 10 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.

Quoted 8 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.
v3: Fixed word wrap in patch.
Signed-off-by Brian Haley <[email protected]>

Acked-by Pavel Emelyanov <[email protected]>