iscsiadm --logoutall=all vs. sessions without match in node database

[Christian Seiler]

Hello,

Recently there has been a bug report in Debian about -U all not
logging out of all active iSCSI sessions on shutdown:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=838540

I've isolated the problem, and what happens is as follows:

1. An external tool sets up a node configuration manually [1], but
makes a small mistake, by not setting a target portal group tag
at all, so open-iscsi will default to 1 - instead of the correct
value of 257 for that target. (In the case of the bug reporter.)

2. The external tool logs in on that portal via iscsiadm.

3. The login succeeds, but either iscsid or the kernel (I'm not
deep enough in the code to know exactly where that happens)
figures out "hey, the actual portal target group tag is 257,
so use that". Hence, login succeeds, but the node database
references the wrong portal group.

4. On shutdown, iscsiadm --logoutall=all is called. The
__logout_by_startup callback tries to run idbm_rec_read() on
the session, that fails, so it completely skips that session.

This behavior in (4) seems wrong to me in two ways:

a. If the tpgt can change after login, then the matching against
the tpgt should be relaxed.

i.e. first try to match the tpgt exactly, if that fails,
just try to find the same match but without fixing the tpgt.
That way, if a login changed the tpgt implicitly, this
would still match the right config with the right session.

b. Regardless of that, there could be active iSCSI sessions
without a corresponding configuration. For example, if the
configuration was wiped accidentally before shutting down
the session. Or someone accidentally used iscsistart on a
already booted system without knowing what they were doing.
In that case, would it not be better if --logoutall=all is
specify to not skip that session, even if there's no match
in the node database? After all, the manpage says -U all
will logout of all active sessions that are NOT onboot,
and it doesn't say "all sessions that have a config AND are
not onboot".

The counterpoint to that would be: if someone were have a
setup with root on iSCSI, and the session of the root
filesystem not configured in the node database at all (but
rather just in some separate config used by the initramfs
they were using), then the current behavior treats that as
if onboot were set in that case, whereas my proposed change
would be treat that as automatic or manual and -U all
would also kill that. In Debian that wouldn't be a problem,
as we already have logic in the shutdown script to
dynamically detect the session on which the root filesystem
(and potentially /usr) is located, but other distros
probably don't.

But maybe we could add --logoutall=reallyall or something?
(Maybe with a better name?)

I'm mostly interested in fixing (a), because that's what's
causing the immediate problem the bug reporter is experiencing.
But (b) deserves some consideration as well IMHO.

Now of course, the original bug is the external tool generating
a static node config with the wrong tpgt - and if open-iscsi
were to simply refuse logins in the first place, I'd also be
fine with that behavior. But since that's not the case (and it's
very likely true that a lot of people are in some way relying on
the fact that the tpgt isn't that important at login, so it's
not going to be realistic to change that), and the login does
succeed anyway, I think the automated logout should as well.

I can work on a patch, but before I start, I'd like to hear
some thoughts on this first. Thanks!

Regards,
Christian

[1] iscsiadm --mode node --portal ... --targetname ... --op new
The portal here is specified by the external tool as only
IP:PORT.

[Christian Seiler]

Hello again,

I would appreciate it if I could get some feedback on this, because
I would really like to see this fixed. While I'd be able and willing
to write a patch for that, I'd first like to hear back so I don't
waste time going on in a direction that'll ultimately be rejected.

Thanks!

Regards,
Christian

[The Lee-Man]

Hi Christian:

Apologies for not replying sooner.

Yes, this area of open-iscsi is a bit messy IMHO.

What about having open-iscsi update the node database entry when the TPG changes? I'd rather not see iscsid shutting down targets that were not specified.

What I would actually like to solve this problem more generally is a way to say "logout of all sessions except 'onboot' sessions", whether or not there's a node database entry.

[Christian Seiler]

On 11/01/2016 03:34 PM, The Lee-Man wrote:
> What about having open-iscsi update the node database entry when the TPG
> changes? I'd rather not see iscsid shutting down targets that were not
> specified.

I'm a bit unsure about magically updating the node database behind the
user's back. At the moment certain operations will update the node
database (-m discovery, -m node --op update, etc.), while others will
not (-m node --login). The former is something the user will call
themselves explicitly (hence knowing that the node database is updated)
while the latter isn't.

(Plus, in Debian we keep the database in /etc/iscsi for policy reasons,
because we consider it to be configuration, and /etc could be read-only
at session login time, think virtualization and the such.)

> What I would actually like to solve this problem more generally is a way to
> say "logout of all sessions except 'onboot' sessions", whether or not
> there's a node database entry.

Well, if you want to keep the current semantics for all, we could add
a new keyword ALL (capital letters, current versions reject it, so it
won't break anything) that has this new semantics

Then we'd have:

--logoutall=all current semantics, if session not in DB,
ignore it
--logoutall=ALL new semantics, if session not in DB,
consider it regardless

That retains backwards compatibility while still being reasonably
legible.

That said: if after login the resulting session can result in a
different TPGT, I would want to implement a two-step matching of
sessions regardless.

Example:

- target: iqn.dummy
- portal: 127.0.0.1:3260
- tpgt at target: 42
- tpgt in node database: 1

iscsiadm -m node -T iqn.dummy -p 127.0.0.1:3260 --login
=> logs into target
iscsiadm -m node -T iqn.dummy -p 127.0.0.1:3260 --logout
=> fails at the moment (IIRC)

I would therefore in addition want to implement session matching in
the node database in the following way (but for running sessions
only):

- if exact combination of <target, ip, port, tgpt> is available, use
that

- if <target, ip, port> has a _unique_ entry with a different tpgt,
use that instead

In that case, --logoutall=all (lowercase) would still match the
session (because it effectively is in the database), as well as the
manual logout command I presented above.

Would you agree to both of these changes? I'd be willing to write and
submit patches for that.

Regards,
Christian