From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web08.7221.1623411068865224903 for ; Fri, 11 Jun 2021 04:31:08 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 192.55.52.120, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: Pn6bX7UB423QyxVxHKjRaHpmeYJEycXzAu6zKpOINLO6io93rUiJ7AYN90MHZ4652R2a8+Xu6V vz9j5hTnPgkg== X-IronPort-AV: E=McAfee;i="6200,9189,10011"; a="203674990" X-IronPort-AV: E=Sophos;i="5.83,265,1616482800"; d="scan'208";a="203674990" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2021 04:31:06 -0700 IronPort-SDR: 9Vt3RbaoDWJgvn1w9+VBGFg13kzJ5HNj+aIuKrmwUFRdM8QeEWylcHFI9Sc0fJhtzKP07dAhqn NM5xSqLqcrQw== X-IronPort-AV: E=Sophos;i="5.83,265,1616482800"; d="scan'208";a="483235544" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.2.3]) ([10.213.2.3]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2021 04:31:04 -0700 Subject: Re: [PATCH 1/6] NetworkPkg/IScsiDxe: re-set session-level authentication state before login To: Laszlo Ersek , edk2-devel-groups-io Cc: Jiaxin Wu , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Siyuan Fu References: <20210608130652.2434-1-lersek@redhat.com> <20210608130652.2434-2-lersek@redhat.com> From: "Maciej Rabeda" Message-ID: Date: Fri, 11 Jun 2021 13:30:58 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210608130652.2434-2-lersek@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: pl Reviewed-by: Maciej Rabeda On 08-Jun-21 15:06, Laszlo Ersek wrote: > RFC 7143 explains that a single iSCSI session may use multiple TCP > connections. The first connection established is called the leading > connection. The login performed on the leading connection is called the > leading login. Before the session is considered full-featured, the leading > login must succeed. Further (non-leading) connections can be associated > with the session later. > > (It's unclear to me from RFC 7143 whether the non-leading connections > require individual (non-leading) logins as well, but that particular > question is irrelevant from the perspective of this patch; see below.) > > The data model in IScsiDxe exhibits some confusion, regarding connection / > session association: > > - On one hand, the "ISCSI_SESSION.Conns" field is a *set* (it has type > LIST_ENTRY), and accordingly, connections can be added to, and removed > from, a session, with the IScsiAttatchConnection() and > IScsiDetatchConnection() functions. > > - On the other hand, ISCSI_MAX_CONNS_PER_SESSION has value 1, therefore no > session will ever use more than 1 connection at a time (refer to > instances of "Session->MaxConnections" in > "NetworkPkg/IScsiDxe/IScsiProto.c"). > > This one-to-many confusion between ISCSI_SESSION and ISCSI_CONNECTION is > very visible in the CHAP logic, where the progress of the authentication > is maintained *per connection*, in the "ISCSI_CONNECTION.AuthStep" field > (with values such as ISCSI_AUTH_INITIAL, ISCSI_CHAP_STEP_ONE, etc), but > the *data* for the authentication are maintained *per session*, in the > "AuthType" and "AuthData" fields of ISCSI_SESSION. Clearly, this makes no > sense if multiple connections are eligible for logging in. > > Knowing that IScsiDxe uses only one connection per session (put > differently: knowing that any connection is a leading connection, and any > login is a leading login), there is no functionality bug. But the data > model is still broken: "AuthType", "AuthData", and "AuthStep" should be > maintained at the *same* level -- be it "session-level" or "(leading) > connection-level". > > Fixing this data model bug is more than what I'm signing up for. However, > I do need to add one function, in preparation for multi-hash support: > whenever a new login is attempted (put differently: whenever the leading > login is re-attempted), which always happens with a fresh connection, the > session-level authentication data needs to be rewound to a sane initial > state. > > Introduce the IScsiSessionResetAuthData() function. Call it from the > central -- session-level -- IScsiSessionLogin() function, just before the > latter calls the -- connection-level -- IScsiConnLogin() function. > > Right now, do nothing in IScsiSessionResetAuthData(); so functionally > speaking, the patch is a no-op. > > Cc: Jiaxin Wu > Cc: Maciej Rabeda > Cc: Philippe Mathieu-Daudé > Cc: Siyuan Fu > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3355 > Signed-off-by: Laszlo Ersek > --- > NetworkPkg/IScsiDxe/IScsiProto.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/NetworkPkg/IScsiDxe/IScsiProto.c b/NetworkPkg/IScsiDxe/IScsiProto.c > index 6983f0fa5973..69d1b39dbb1f 100644 > --- a/NetworkPkg/IScsiDxe/IScsiProto.c > +++ b/NetworkPkg/IScsiDxe/IScsiProto.c > @@ -401,38 +401,55 @@ IScsiGetIp6NicInfo ( > if (Ip6ModeData.GroupTable!= NULL) { > FreePool (Ip6ModeData.GroupTable); > } > if (Ip6ModeData.RouteTable!= NULL) { > FreePool (Ip6ModeData.RouteTable); > } > if (Ip6ModeData.NeighborCache!= NULL) { > FreePool (Ip6ModeData.NeighborCache); > } > if (Ip6ModeData.PrefixTable!= NULL) { > FreePool (Ip6ModeData.PrefixTable); > } > if (Ip6ModeData.IcmpTypeList!= NULL) { > FreePool (Ip6ModeData.IcmpTypeList); > } > > return Status; > } > > +/** > + Re-set any stateful session-level authentication information that is used by > + the leading login / leading connection. > + > + (Note that this driver only supports a single connection per session -- see > + ISCSI_MAX_CONNS_PER_SESSION.) > + > + @param[in,out] Session The iSCSI session. > +**/ > +STATIC > +VOID > +IScsiSessionResetAuthData ( > + IN OUT ISCSI_SESSION *Session > + ) > +{ > +} > + > /** > Login the iSCSI session. > > @param[in] Session The iSCSI session. > > @retval EFI_SUCCESS The iSCSI session login procedure finished. > @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. > @retval EFI_NO_MEDIA There was a media error. > @retval Others Other errors as indicated. > > **/ > EFI_STATUS > IScsiSessionLogin ( > IN ISCSI_SESSION *Session > ) > { > EFI_STATUS Status; > ISCSI_CONNECTION *Conn; > VOID *Tcp; > @@ -454,38 +471,39 @@ IScsiSessionLogin ( > // > CopyMem (Session->Isid, Session->ConfigData->SessionConfigData.IsId, 6); > > RetryCount = 0; > > do { > // > // Create a connection for the session. > // > Conn = IScsiCreateConnection (Session); > if (Conn == NULL) { > return EFI_OUT_OF_RESOURCES; > } > > IScsiAttatchConnection (Session, Conn); > > // > // Login through the newly created connection. > // > + IScsiSessionResetAuthData (Session); > Status = IScsiConnLogin (Conn, Session->ConfigData->SessionConfigData.ConnectTimeout); > if (EFI_ERROR (Status)) { > IScsiConnReset (Conn); > IScsiDetatchConnection (Conn); > IScsiDestroyConnection (Conn); > } > > if (Status != EFI_TIMEOUT) { > break; > } > > RetryCount++; > } while (RetryCount <= Session->ConfigData->SessionConfigData.ConnectRetryCount); > > if (!EFI_ERROR (Status)) { > Session->State = SESSION_STATE_LOGGED_IN; > > if (!Conn->Ipv6Flag) { > ProtocolGuid = &gEfiTcp4ProtocolGuid;