Skip to contentSkip to conversation

moving the test for sockaddr->sa_family up

RFC In review Arnaldo Carvalho de Melo 3 participants 4 messages Nov 15, 2003 – Nov 17, 2003

Subsystems (9)
Networking Network drivers Bluetooth Sockets +5 more subsystems
lore
Share & tools
lore

Arnaldo Carvalho de Melo

author
#0 lore raw

RFC 1/1 moving the test for sockaddr->sa_family up

Does anybody see any problem with this simplification? Not for 2.6.0, of course...

- Arnaldo

===== net/socket.c 1.70 vs edited =====

net/socket.c +10 -6
@@ -1187,13 +1187,17 @@
 	if((sock = sockfd_lookup(fd,&err))!=NULL)
 	{
 		if((err=move_addr_to_kernel(umyaddr,addrlen,address))>=0) {
-			err = security_socket_bind(sock, (struct sockaddr *)address, addrlen);
-			if (err) {
-				sockfd_put(sock);
-				return err;
-			}
-			err = sock->ops->bind(sock, (struct sockaddr *)address, addrlen);
+			struct sockaddr *saddr = (struct sockaddr *)address;
+
+			err = -EAFNOSUPPORT;
+			if (saddr->sa_family != sock->sk->sk_family)
+				goto out_put;
+			err = security_socket_bind(sock, saddr, addrlen);
+			if (err)
+				goto out_put;
+			err = sock->ops->bind(sock, saddr, addrlen);
 		}
+out_put:
 		sockfd_put(sock);
 	}			
 	return err;

===== net/appletalk/ddp.c 1.40 vs edited =====

net/appletalk/ddp.c +0 -3
@@ -1158,9 +1158,6 @@
 	if (!sk->sk_zapped || addr_len != sizeof(struct sockaddr_at))
 		return -EINVAL;
 
-	if (addr->sat_family != AF_APPLETALK)
-		return -EAFNOSUPPORT;
-
 	if (addr->sat_addr.s_net == htons(ATADDR_ANYNET)) {
 		struct atalk_addr *ap = atalk_find_primary();

===== net/atm/pvc.c 1.15 vs edited =====

net/atm/pvc.c +0 -1
@@ -34,7 +34,6 @@
 
 	if (sockaddr_len != sizeof(struct sockaddr_atmpvc)) return -EINVAL;
 	addr = (struct sockaddr_atmpvc *) sockaddr;
-	if (addr->sap_family != AF_ATMPVC) return -EAFNOSUPPORT;
 	lock_sock(sk);
 	vcc = ATM_SD(sock);
 	if (!test_bit(ATM_VF_HASQOS, &vcc->flags)) {

===== net/ax25/af_ax25.c 1.33 vs edited =====

net/ax25/af_ax25.c +0 -3
@@ -1030,9 +1030,6 @@
 			current->comm);
 	}
 
-	if (addr->fsa_ax25.sax25_family != AF_AX25)
-		return -EINVAL;
-
 	call = ax25_findbyuid(current->euid);
 	if (call == NULL && ax25_uid_policy && !capable(CAP_NET_ADMIN)) {
 		return -EACCES;

===== net/bluetooth/hci_sock.c 1.26 vs edited =====

net/bluetooth/hci_sock.c +0 -3
@@ -264,9 +264,6 @@
 
 	BT_DBG("sock %p sk %p", sock, sk);
 
-	if (!haddr || haddr->hci_family != AF_BLUETOOTH)
-		return -EINVAL;
-
 	lock_sock(sk);
 
 	if (hci_pi(sk)->hdev) {

===== net/bluetooth/l2cap.c 1.37 vs edited =====

net/bluetooth/l2cap.c +0 -3
@@ -396,9 +396,6 @@
 
 	BT_DBG("sk %p, %s %d", sk, batostr(&la->l2_bdaddr), la->l2_psm);
 
-	if (!addr || addr->sa_family != AF_BLUETOOTH)
-		return -EINVAL;
-
 	lock_sock(sk);
 
 	if (sk->sk_state != BT_OPEN) {

===== net/bluetooth/sco.c 1.25 vs edited =====

net/bluetooth/sco.c +0 -3
@@ -472,9 +472,6 @@
 
 	BT_DBG("sk %p %s", sk, batostr(&sa->sco_bdaddr));
 
-	if (!addr || addr->sa_family != AF_BLUETOOTH)
-		return -EINVAL;
-
 	lock_sock(sk);
 
 	if (sk->sk_state != BT_OPEN) {

===== net/bluetooth/rfcomm/sock.c 1.27 vs edited =====

net/bluetooth/rfcomm/sock.c +0 -3
@@ -323,9 +323,6 @@
 
 	BT_DBG("sk %p %s", sk, batostr(&sa->rc_bdaddr));
 
-	if (!addr || addr->sa_family != AF_BLUETOOTH)
-		return -EINVAL;
-
 	lock_sock(sk);
 
 	if (sk->sk_state != BT_OPEN) {

===== net/decnet/af_decnet.c 1.37 vs edited =====

net/decnet/af_decnet.c +0 -3
@@ -707,9 +707,6 @@
 	if (addr_len != sizeof(struct sockaddr_dn))
 		return -EINVAL;
 
-	if (saddr->sdn_family != AF_DECnet)
-		return -EINVAL;
-
 	if (dn_ntohs(saddr->sdn_nodeaddrl) && (dn_ntohs(saddr->sdn_nodeaddrl) != 2))
 		return -EINVAL;

===== net/econet/af_econet.c 1.27 vs edited =====

net/econet/af_econet.c +1 -2
@@ -188,8 +188,7 @@
 	 *	Check legality
 	 */
 	 
-	if (addr_len < sizeof(struct sockaddr_ec) ||
-	    sec->sec_family != AF_ECONET)
+	if (addr_len < sizeof(struct sockaddr_ec))
 		return -EINVAL;
 	
 	eo->cb	    = sec->cb;

===== net/llc/af_llc.c 1.70 vs edited =====

net/llc/af_llc.c +0 -3
@@ -285,9 +285,6 @@
 	dprintk("%s: binding %02X\n", __FUNCTION__, addr->sllc_sap);
 	if (unlikely(!sk->sk_zapped || addrlen != sizeof(*addr)))
 		goto out;
-	rc = -EAFNOSUPPORT;
-	if (unlikely(addr->sllc_family != AF_LLC))
-		goto out;
 	if (!addr->sllc_sap) {
 		rc = -EUSERS;
 		addr->sllc_sap = llc_ui_autoport();

===== net/netlink/af_netlink.c 1.37 vs edited =====

net/netlink/af_netlink.c +0 -3
@@ -329,9 +329,6 @@
 	struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr;
 	int err;
 	
-	if (nladdr->nl_family != AF_NETLINK)
-		return -EINVAL;
-
 	/* Only superuser is allowed to listen multicasts */
 	if (nladdr->nl_groups && !netlink_capable(sock, NL_NONROOT_RECV))
 		return -EPERM;

===== net/netrom/af_netrom.c 1.40 vs edited =====

net/netrom/af_netrom.c +0 -4
@@ -570,10 +570,6 @@
 		release_sock(sk);
 		return -EINVAL;
 	}
-	if (addr->fsa_ax25.sax25_family != AF_NETROM) {
-		release_sock(sk);
-		return -EINVAL;
-	}
 	if ((dev = nr_dev_get(&addr->fsa_ax25.sax25_call)) == NULL) {
 		SOCK_DEBUG(sk, "NET/ROM: bind failed: invalid node callsign\n");
 		release_sock(sk);

===== net/packet/af_packet.c 1.35 vs edited =====

net/packet/af_packet.c +0 -2
@@ -901,8 +901,6 @@
 	 
 	if (addr_len < sizeof(struct sockaddr_ll))
 		return -EINVAL;
-	if (sll->sll_family != AF_PACKET)
-		return -EINVAL;
 
 	if (sll->sll_ifindex) {
 		err = -ENODEV;

===== net/rose/af_rose.c 1.33 vs edited =====

net/rose/af_rose.c +0 -3
@@ -654,9 +654,6 @@
 	if (addr_len != sizeof(struct sockaddr_rose) && addr_len != sizeof(struct full_sockaddr_rose))
 		return -EINVAL;
 
-	if (addr->srose_family != AF_ROSE)
-		return -EINVAL;
-
 	if (addr_len == sizeof(struct sockaddr_rose) && addr->srose_ndigis > 1)
 		return -EINVAL;

===== net/unix/af_unix.c 1.56 vs edited =====

net/unix/af_unix.c +0 -4
@@ -649,10 +649,6 @@
 	struct unix_address *addr;
 	struct hlist_head *list;
 
-	err = -EINVAL;
-	if (sunaddr->sun_family != AF_UNIX)
-		goto out;
-
 	if (addr_len==sizeof(short)) {
 		err = unix_autobind(sock);
 		goto out;

===== net/wanrouter/af_wanpipe.c 1.31 vs edited =====

net/wanrouter/af_wanpipe.c +0 -4
@@ -1340,10 +1340,6 @@
 		printk(KERN_INFO "wansock: Address length error\n");
 		return -EINVAL;
 	}
-	if (sll->sll_family != AF_WANPIPE){
-		printk(KERN_INFO "wansock: Illegal family name specified.\n");
-		return -EINVAL;
-	}
 
 	card = wanpipe_find_card (sll->sll_card);
 	if (!card){

===== net/x25/af_x25.c 1.38 vs edited =====

net/x25/af_x25.c +1 -2
@@ -587,8 +587,7 @@
 	struct sockaddr_x25 *addr = (struct sockaddr_x25 *)uaddr;
 
 	if (!sk->sk_zapped ||
-	    addr_len != sizeof(struct sockaddr_x25) ||
-	    addr->sx25_family != AF_X25)
+	    addr_len != sizeof(struct sockaddr_x25))
 		return -EINVAL;
 
 	x25_sk(sk)->source_addr = addr->sx25_addr;
#1 lore raw
Quoted 2 lines · Arnaldo Carvalho de Melo

Does anybody see any problem with this simplification? Not for 2.6.0, of course...

No, looks like a good idea. Might be able to push address length verification up there too.

- James

Signature · 2 lines
James Morris
<[email protected]>
author
#2 lore raw

Em Sun, Nov 16, 2003 at 08:56:04AM -0500, James Morris escreveu:

Quoted 7 lines · Arnaldo Carvalho de Melo
Quoted 3 lines

Does anybody see any problem with this simplification? Not for 2.6.0, of course...

No, looks like a good idea. Might be able to push address length verification up there too.

Not really, look at the ax25 code... :-\ They have to support two address types, if we want to keep this flexibility we can't check it at the upper layer, does anybody here knows if we want or if we can ditch that thing in ax25? this:

net/ax25/af_ax25.c

static int ax25_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)

{
        struct sock *sk = sock->sk;
        struct full_sockaddr_ax25 *addr = (struct full_sockaddr_ax25 *)uaddr;
        ax25_dev *ax25_dev = NULL;
        ax25_address *call;
        ax25_cb *ax25;
        int err = 0;

        if (addr_len != sizeof(struct sockaddr_ax25) &&
            addr_len != sizeof(struct full_sockaddr_ax25)) {
                /* support for old structure may go away some time */
                if ((addr_len < sizeof(struct sockaddr_ax25) + sizeof(ax25_address) * 6) ||
                    (addr_len > sizeof(struct full_sockaddr_ax25))) {
                        return -EINVAL;
        }

                printk(KERN_WARNING "ax25_bind(): %s uses old (6 digipeater) socket structure.\n",
                        current->comm);
        }

I haven't checked for how many major kernel versions this thing is there, but I'd love to trow this away if possible.

Ah, I tried to see if we could move the family test in the ->connect case as well, only to enlighten myself with a trick in PF_UNIX, namely:

static int unix_dgram_connect(struct socket *sock, struct sockaddr *addr,
int alen, int flags)
<SNIP>

        if (addr->sa_family != AF_UNSPEC) {
<SNIP>
	        } else {
                /*
                 *      1003.1g breaking connected state with AF_UNSPEC
                 */
                other = NULL;
                unix_state_wlock(sk);
        }

Oh well, the thing uses sa_family to a magic value to signal breaking connected state, the wonders never cease :(

- Arnaldo

#3 lore raw
Quoted 6 lines · On Sun, 16 Nov 2003 17:07:58 -0200 Arnaldo Carvalho de Melo…

Em Sun, Nov 16, 2003 at 08:56:04AM -0500, James Morris escreveu:

Quoted 5 lines · Arnaldo Carvalho de Melo
Quoted 3 lines

Does anybody see any problem with this simplification? Not for 2.6.0, of course...

Quoted 6 lines
Quoted 2 lines

No, looks like a good idea. Might be able to push address length verification up there too.

Not really, look at the ax25 code... :-\ They have to support two address types, if we want to keep this flexibility we can't check it at the upper layer, does anybody here knows if we want or if we can ditch that thing in ax25? this:

Right, this is the area where you need to be careful, where people need special semantics just like this ax25 case.

I'm %100 fine with the original cleanup for 2.6.1 or later.