IP_FREEBIND and binding to in-use addr:ports

OK, this is weird:

https://bugzilla.redhat.com/show_bug.cgi?id=908368

It appears you can listen on the same address:port if you do it from a different iscsi target, or even a different tpg (so there are no configfs name collisions). I believe this is because we are setting IP_FREEBIND sockopt, so we can configure listening on iscsi portals (aka ip:port) before the IP is assigned.

from ip(7):
IP_FREEBIND (since Linux 2.4)
If enabled, this boolean option allows binding to an IP address that is nonlocal or does not (yet) exist. This permits listening on a socket, without requiring the underlying network interface or the specified dynamic IP address to be up at the time that the application is trying to bind to it. This option is the per-socket equivalent of the ip_nonlocal_bind /proc interface described below.

This doesn't say anything about if the address:port is already in use. Dave/netdev, should the network stack be returning an error when attempting to bind to an address:port already in use even if IP_FREEBIND is set, or should the caller be checking for this before trying to kernel_bind()?

Or is something else the issue?

RFC 3720 says "Each Network Portal, as utilized by a given iSCSI Node, belongs to exactly one portal group within that node." therefore iscsit_add_np should not check for existing matching portals, it should just go ahead and try to make the portal, and then kernel_bind() will return the proper error.

Signed-off-by Andy Grover <[email protected]>

---

 drivers/target/iscsi/iscsi_target.c |   64 -----------------------------------
 1 files changed, 0 insertions(+), 64 deletions(-)

drivers/target/iscsi/iscsi_target.c +0 -64

index 339f97f..73be05c 100644
@@ -264,64 +264,6 @@ int iscsit_deaccess_np(struct iscsi_np *np, struct iscsi_portal_group *tpg)
 	return 0;
 }
 
-static struct iscsi_np *iscsit_get_np(
-	struct __kernel_sockaddr_storage *sockaddr,
-	int network_transport)
-{
-	struct sockaddr_in *sock_in, *sock_in_e;
-	struct sockaddr_in6 *sock_in6, *sock_in6_e;
-	struct iscsi_np *np;
-	int ip_match = 0;
-	u16 port;
-
-	spin_lock_bh(&np_lock);
-	list_for_each_entry(np, &g_np_list, np_list) {
-		spin_lock(&np->np_thread_lock);
-		if (np->np_thread_state != ISCSI_NP_THREAD_ACTIVE) {
-			spin_unlock(&np->np_thread_lock);
-			continue;
-		}
-
-		if (sockaddr->ss_family == AF_INET6) {
-			sock_in6 = (struct sockaddr_in6 *)sockaddr;
-			sock_in6_e = (struct sockaddr_in6 *)&np->np_sockaddr;
-
-			if (!memcmp(&sock_in6->sin6_addr.in6_u,
-				    &sock_in6_e->sin6_addr.in6_u,
-				    sizeof(struct in6_addr)))
-				ip_match = 1;
-
-			port = ntohs(sock_in6->sin6_port);
-		} else {
-			sock_in = (struct sockaddr_in *)sockaddr;
-			sock_in_e = (struct sockaddr_in *)&np->np_sockaddr;
-
-			if (sock_in->sin_addr.s_addr ==
-			    sock_in_e->sin_addr.s_addr)
-				ip_match = 1;
-
-			port = ntohs(sock_in->sin_port);
-		}
-
-		if ((ip_match == 1) && (np->np_port == port) &&
-		    (np->np_network_transport == network_transport)) {
-			/*
-			 * Increment the np_exports reference count now to
-			 * prevent iscsit_del_np() below from being called
-			 * while iscsi_tpg_add_network_portal() is called.
-			 */
-			np->np_exports++;
-			spin_unlock(&np->np_thread_lock);
-			spin_unlock_bh(&np_lock);
-			return np;
-		}
-		spin_unlock(&np->np_thread_lock);
-	}
-	spin_unlock_bh(&np_lock);
-
-	return NULL;
-}
-
 struct iscsi_np *iscsit_add_np(
 	struct __kernel_sockaddr_storage *sockaddr,
 	char *ip_str,

@@ -331,12 +273,6 @@ struct iscsi_np *iscsit_add_np(
 	struct sockaddr_in6 *sock_in6;
 	struct iscsi_np *np;
 	int ret;
-	/*
-	 * Locate the existing struct iscsi_np if already active..
-	 */
-	np = iscsit_get_np(sockaddr, network_transport);
-	if (np)
-		return np;
 
 	np = kzalloc(sizeof(struct iscsi_np), GFP_KERNEL);
 	if (!np) {

Quoted 6 lines · Andy Grover

RFC 3720 says "Each Network Portal, as utilized by a given iSCSI Node, belongs to exactly one portal group within that node." therefore iscsit_add_np should not check for existing matching portals, it should just go ahead and try to make the portal, and then kernel_bind() will return the proper error.

Quoted 2 lines

Signed-off-by Andy Grover <[email protected]>

---

NACK. Your interpretation of RFC-3720 is incorrect. There is nothing that says that a single IP address cannot be shared across multiple TargetName+TargetPortalGroupTag endpoints.

Quoted 10 lines · Nicholas A. Bellinger

Quoted 8 lines · Andy Grover

RFC 3720 says "Each Network Portal, as utilized by a given iSCSI Node, belongs to exactly one portal group within that node." therefore iscsit_add_np should not check for existing matching portals, it should just go ahead and try to make the portal, and then kernel_bind() will return the proper error.

Signed-off-by Andy Grover <[email protected]>

---

Quoted 3 lines

NACK. Your interpretation of RFC-3720 is incorrect. There is nothing that says that a single IP address cannot be shared across multiple TargetName+TargetPortalGroupTag endpoints.

A Network Portal is ip:port, not just IP. I'd agree two TPGs can use the same IP as long as they listen on different ports.

But that bit I quoted seems pretty clear. How should it be alternatively interpreted?

The wording in section Section 3.4.1, e) that your referring to:

"Each Network Portal, as utilized by a given iSCSI Node, belongs to exactly one portal group within that node."

does not mean that individual network portals are limited to a single network entity, but that network portals are linked to a single TPG within an individual TargetName. Eg, 'that node' does not mean the entire physical machine (network entity), that may contain multiple nodes (TargetName+TargetPortalGroupTag endpoints).

However, in practice I've not yet seen a target implementation that supports multiple TPGs actually enforce this, considering this is not accompanied by a "SHOULD not" or "MUST not" anywhere in the spec. So unless you have a specific problem case where this is causing an issue with an initiator, I'm likely not going to accept a kernel patch to change existing behavior.

Quoted 11 lines · Nicholas A. Bellinger

The wording in section Section 3.4.1, e) that your referring to:

"Each Network Portal, as utilized by a given iSCSI Node, belongs to exactly one portal group within that node."

does not mean that individual network portals are limited to a single network entity, but that network portals are linked to a single TPG within an individual TargetName. Eg, 'that node' does not mean the entire physical machine (network entity), that may contain multiple nodes (TargetName+TargetPortalGroupTag endpoints).

Quoted 6 lines

However, in practice I've not yet seen a target implementation that supports multiple TPGs actually enforce this, considering this is not accompanied by a "SHOULD not" or "MUST not" anywhere in the spec. So unless you have a specific problem case where this is causing an issue with an initiator, I'm likely not going to accept a kernel patch to change existing behavior.

OK, so I'm clear now that a NetworkPortal can be shared among TargetNames, but not among TPGs within a TargetName.

But LIO currently allows it.
See https://bugzilla.redhat.com/show_bug.cgi?id=908368 .

The tester's actual issue may not be related to this area, but if you look at the attachment in comment 2, this configuration was allowed.

I don't think this is an issue where we need to worry about existing behavior. This *can't* work because the initiator passes the desired TargetName during iSCSI login, but not TargetPortalGroupTag. There's no way a target can tell which TPG the initiator wants if the TargetName for two are the same.

We could add a check for this to the rtslib userspace library, but this would mean the kernel could still be configured this way, if rtslib was not used to wrap configfs accesses. Therefore I'd push for the kernel to check for this. Would a patch for that fly?