Patchwork Don't allow multiple TPGs or targets to share a portal

login
register
mail settings
Submitter Andy Grover
Date Feb. 8, 2013, 11:05 p.m.
Message ID <1360364708-26104-1-git-send-email-agrover@redhat.com>
Download mbox | patch
Permalink /patch/219346/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Andy Grover - Feb. 8, 2013, 11:05 p.m.
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(-)
Nicholas A. Bellinger - Feb. 13, 2013, 8:31 p.m.
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) {


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Grover - Feb. 13, 2013, 10:09 p.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas A. Bellinger - Feb. 15, 2013, 3:46 p.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Grover - Feb. 18, 2013, 10:41 p.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas A. Bellinger - Feb. 19, 2013, 4:34 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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) {