Re: [PATCH net-next] sockopt: Make SO_BINDTODEVICE readable

On 10/22/2012 04:28 PM, Pavel Emelyanov wrote:

On 10/22/2012 10:04 PM, Brian Haley wrote:

quoted

On 10/19/2012 05:55 AM, Pavel Emelyanov wrote:

quoted

The SO_BINDTODEVICE option is the only SOL_SOCKET one that can be set, but
cannot be get via sockopt API. The only way we can find the device id a
socket is bound to is via sock-diag interface. But the diag works only on
hashed sockets, while the opt in question can be set for yet unhashed one.

That said, in order to know what device a socket is bound to (we do want
to know this in checkpoint-restore project) I propose to make this option
getsockopt-able and report the respective device index.

Another solution to the problem might be to teach the sock-diag reporting
info on unhashed sockets. Should I go this way instead?

Signed-off-by: Pavel Emelyanov <redacted>

---

diff --git a/net/core/sock.c b/net/core/sock.c
index 8a146cf..c49412c 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c

@@ -1074,6 +1074,9 @@ 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;
 	default:
 		return -ENOPROTOOPT;
 	}

Doesn't this make the set and get non-symmetrical?  For example, setsockopt()
would take "eth0", but getsockopt() would return 2.

It will, but since device name and index are two equal device "IDs" I assumed
it would be OK.

However, some comments inline.

quoted

The following patch would return a string, or -ENODEV if not set.

Sorry, my description is not quite right, this should return something like this
to be correct:

  0 on success
    optlen zero if interface not set
    optlen > zero if set and optval filled-in

  -errno on failure

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];
+
+	ret = 0;
+	if (sk->sk_bound_dev_if == 0)
+		goto out;

It will return 0 if device is not set, thus making it impossible to detect
this situation.

See below.

quoted

+	ret = -EINVAL;
+	if (len < IFNAMSIZ)
+		goto out;
+	if (len > IFNAMSIZ)
+		len = IFNAMSIZ;
+
+	rcu_read_lock();
+	dev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if);
+	if (dev)
+		strcpy(dev->name, devname);
+	rcu_read_unlock();
+	ret = -ENODEV;
+	if (!dev)
+		goto out;
+
+	ret = -EFAULT;
+	if (copy_to_user(optval, devname, len))
+		goto out;
+
+	if (put_user(len, optlen))
+		goto out;

What's the point in reporting IFNAMSIZ to the userspace always, taking
into account that this constant is exported there anyway?

I should have put an RFC on the patch :)

In the case that there is no interface, the length returned would be zero,
indicating nothing was there.

I can post another version.

-Brian