IP_FREEBIND and binding to in-use addr:ports

IP_FREEBIND and binding to in-use addr:ports

[Andy Grover]

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?

Thanks -- Regards -- Andy

p.s. see drivers/target/iscsi/iscsi_target_login.c line ~846 for caller 
code.

Re: IP_FREEBIND and binding to in-use addr:ports

[Andy Grover]

On 02/06/2013 04:47 PM, Andy Grover wrote:
> 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?

Looks like IP_FREEBIND doesn't make a difference. More shortly. -- Andy

[PATCH] Don't allow multiple TPGs or targets to share a portal

[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 <agrover@redhat.com>
---
 drivers/target/iscsi/iscsi_target.c |   64 -----------------------------------
 1 files changed, 0 insertions(+), 64 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 339f97f..73be05c 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -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) {
-- 
1.7.1

Re: [PATCH] Don't allow multiple TPGs or targets to share a portal

[Nicholas A. Bellinger]

On Fri, 2013-02-08 at 15:05 -0800, Andy Grover wrote:
> 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 <agrover@redhat.com>
> ---

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.

--nab

>  drivers/target/iscsi/iscsi_target.c |   64 -----------------------------------
>  1 files changed, 0 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 339f97f..73be05c 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -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) {

Re: [PATCH] Don't allow multiple TPGs or targets to share a portal

[Andy Grover]

On 02/13/2013 12:31 PM, Nicholas A. Bellinger wrote:
> On Fri, 2013-02-08 at 15:05 -0800, Andy Grover wrote:
>> 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 <agrover@redhat.com>
>> ---
>
> 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?

Thanks -- Andy

Re: [PATCH] Don't allow multiple TPGs or targets to share a portal

[Nicholas A. Bellinger]

On Wed, 2013-02-13 at 14:09 -0800, Andy Grover wrote:
> On 02/13/2013 12:31 PM, Nicholas A. Bellinger wrote:
> > On Fri, 2013-02-08 at 15:05 -0800, Andy Grover wrote:
> >> 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 <agrover@redhat.com>
> >> ---
> >
> > 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.
> 

No.  The whole point of having IQNs is to decouple the network portal
access from the target node, so that network portals can be shared
across the network entity.  

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

Your completely ignoring all the previous context to reach this
conclusion.  Consider:

3.4.  SCSI to iSCSI Concepts Mapping Model

   The following diagram shows an example of how multiple iSCSI Nodes
   (targets in this case) can coexist within the same Network Entity and
   can share Network Portals (IP addresses and TCP ports).
 
   ....

and,

3.4.1 iSCSI Architecture Model

      a)  Network Entity - represents a device or gateway that is
          accessible from the IP network.  A Network Entity must have
          one or more Network Portals (see item d), each of which can be
          used by some iSCSI Nodes (see item (b)) contained in that
          Network Entity to gain access to the IP network.

and,

      b)  iSCSI Node - 

          .....

          The separation of the iSCSI Name from the addresses used by
          and for the iSCSI node allows multiple iSCSI nodes to use the
          same addresses, and the same iSCSI node to use multiple
          addresses.

and,

Appendix D.  SendTargets Operation

   The next example has two internal iSCSI targets, each accessible via
   two different ports with different IP addresses.  The following is
   the text response:

      TargetName=iqn.1993-11.com.example:diskarray.sn.8675309
      TargetAddress=10.1.0.45:3000,1 TargetAddress=10.1.1.45:3000,2
      TargetName=iqn.1993-11.com.example:diskarray.sn.1234567
      TargetAddress=10.1.0.45:3000,1 TargetAddress=10.1.1.45:3000,2

   Both targets share both addresses; the multiple addresses are likely
   used to provide multi-path support.  The initiator may connect to
   either target name on either address.

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.

--nab

Re: [PATCH] Don't allow multiple TPGs or targets to share a portal

[Andy Grover]

On 02/15/2013 07:46 AM, Nicholas A. Bellinger wrote:
> 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.

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?

Thanks -- Regards -- Andy

Re: [PATCH] Don't allow multiple TPGs or targets to share a portal

[Nicholas A. Bellinger]

On Mon, 2013-02-18 at 14:41 -0800, Andy Grover wrote:
> On 02/15/2013 07:46 AM, Nicholas A. Bellinger wrote:
> > 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.
> 
> 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.
> 

Yes, it's related.  He will want to be using multiple IQNs for this type
of setup.

> 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?
> 

So considering in this special case that an target cannot distinguish
between TargetPortalGroup for an incoming Login Request, enforcing from
the kernel that individual network portals only be mapped to a single
TargetPortalGroup within TargetName context is going to be the proper
resolution here.

I'm working on a patch for this, and will post shortly..

Thanks,

--nab