* IP_FREEBIND and binding to in-use addr:ports @ 2013-02-07 0:47 Andy Grover 2013-02-07 18:42 ` Andy Grover 0 siblings, 1 reply; 8+ messages in thread From: Andy Grover @ 2013-02-07 0:47 UTC (permalink / raw) To: target-devel; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: IP_FREEBIND and binding to in-use addr:ports 2013-02-07 0:47 IP_FREEBIND and binding to in-use addr:ports Andy Grover @ 2013-02-07 18:42 ` Andy Grover 2013-02-08 23:05 ` [PATCH] Don't allow multiple TPGs or targets to share a portal Andy Grover 0 siblings, 1 reply; 8+ messages in thread From: Andy Grover @ 2013-02-07 18:42 UTC (permalink / raw) To: target-devel; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Don't allow multiple TPGs or targets to share a portal 2013-02-07 18:42 ` Andy Grover @ 2013-02-08 23:05 ` Andy Grover 2013-02-13 20:31 ` Nicholas A. Bellinger 0 siblings, 1 reply; 8+ messages in thread From: Andy Grover @ 2013-02-08 23:05 UTC (permalink / raw) To: target-devel; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Don't allow multiple TPGs or targets to share a portal 2013-02-08 23:05 ` [PATCH] Don't allow multiple TPGs or targets to share a portal Andy Grover @ 2013-02-13 20:31 ` Nicholas A. Bellinger 2013-02-13 22:09 ` Andy Grover 0 siblings, 1 reply; 8+ messages in thread From: Nicholas A. Bellinger @ 2013-02-13 20:31 UTC (permalink / raw) To: Andy Grover; +Cc: target-devel, netdev 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) { ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Don't allow multiple TPGs or targets to share a portal 2013-02-13 20:31 ` Nicholas A. Bellinger @ 2013-02-13 22:09 ` Andy Grover 2013-02-15 15:46 ` Nicholas A. Bellinger 0 siblings, 1 reply; 8+ messages in thread From: Andy Grover @ 2013-02-13 22:09 UTC (permalink / raw) To: Nicholas A. Bellinger; +Cc: target-devel, netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Don't allow multiple TPGs or targets to share a portal 2013-02-13 22:09 ` Andy Grover @ 2013-02-15 15:46 ` Nicholas A. Bellinger 2013-02-18 22:41 ` Andy Grover 0 siblings, 1 reply; 8+ messages in thread From: Nicholas A. Bellinger @ 2013-02-15 15:46 UTC (permalink / raw) To: Andy Grover; +Cc: target-devel, netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Don't allow multiple TPGs or targets to share a portal 2013-02-15 15:46 ` Nicholas A. Bellinger @ 2013-02-18 22:41 ` Andy Grover 2013-02-19 4:34 ` Nicholas A. Bellinger 0 siblings, 1 reply; 8+ messages in thread From: Andy Grover @ 2013-02-18 22:41 UTC (permalink / raw) To: Nicholas A. Bellinger; +Cc: target-devel, netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Don't allow multiple TPGs or targets to share a portal 2013-02-18 22:41 ` Andy Grover @ 2013-02-19 4:34 ` Nicholas A. Bellinger 0 siblings, 0 replies; 8+ messages in thread From: Nicholas A. Bellinger @ 2013-02-19 4:34 UTC (permalink / raw) To: Andy Grover; +Cc: target-devel, netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-02-19 4:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-02-07 0:47 IP_FREEBIND and binding to in-use addr:ports Andy Grover 2013-02-07 18:42 ` Andy Grover 2013-02-08 23:05 ` [PATCH] Don't allow multiple TPGs or targets to share a portal Andy Grover 2013-02-13 20:31 ` Nicholas A. Bellinger 2013-02-13 22:09 ` Andy Grover 2013-02-15 15:46 ` Nicholas A. Bellinger 2013-02-18 22:41 ` Andy Grover 2013-02-19 4:34 ` Nicholas A. Bellinger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.