Skip to content

Conversation

@martin-gpy
Copy link
Contributor

During nvme connect-all, if a discovery log page record reports the sectype as anything other than NVMF_TCP_SECTYPE_NONE in nvmf_connect_disc_entry(), it then assumes that tls should be default set for the same. But this holds true only for configured PSK TLS connections alone and not generated PSK TLS (i.e. secure channel concat) connections since both concat and tls flags are meant to be mutually exclusive. Fix the same.

Fixes: #3034

During nvme connect-all, if a discovery log page record reports
the sectype as anything other than NVMF_TCP_SECTYPE_NONE in
nvmf_connect_disc_entry(), it then assumes that tls should be
default set for the same. But this holds true only for configured
PSK TLS connections alone and not generated PSK TLS (i.e. secure
channel concat) connections since both concat and tls flags are
meant to be mutually exclusive. Fix the same.

Signed-off-by: Martin George <[email protected]>
@igaw
Copy link
Collaborator

igaw commented Jan 5, 2026

I think the nvmf_connect_disc_entry function should not blindly enable tls in this case.

Something like this here:

	if (e->trtype == NVMF_TRTYPE_TCP &&
	    e->tsas.tcp.sectype != NVMF_TCP_SECTYPE_NONE &&
	    !cfg->tls)
		c->cfg.tls = true;

If we check the command line for --concat and --tls and ensure that the user doesn't provide both options, then this here would be a good place to log what is happening. It seems relevant to report to me. What do you think?

@martin-gpy
Copy link
Contributor Author

martin-gpy commented Jan 5, 2026

I think the nvmf_connect_disc_entry function should not blindly enable tls in this case.

Yes, I agree. This code snippet at nvmf_connect_disc_entry() is really unwarranted IMO. But anyways...

Something like this here:

	if (e->trtype == NVMF_TRTYPE_TCP &&
	    e->tsas.tcp.sectype != NVMF_TCP_SECTYPE_NONE &&
	    !cfg->tls)
		c->cfg.tls = true;

If we check the command line for --concat and --tls and ensure that the user doesn't provide both options, then this here would be a good place to log what is happening. It seems relevant to report to me. What do you think?

This won't help because c->cfg.tls (or even c->cfg.concat for that matter) is always false at this point while parsing the discovery log page record at nvmf_connect_disc_entry(). IIRC, the user provided options for these flags are updated only in nvmf_add_ctrl() during the merge_config().

That's the reason I made this fix in nvmf_add_ctrl() itself in the above commit, and not in nvmf_connect_disc_entry().

@martin-gpy
Copy link
Contributor Author

Or should we just do away with the current code snippet of enabling tls if sectype is != NVMF_TCP_SECTYPE_NONE in nvmf_connect_disc_entry(), so that we completely rely on the user provided options itself for the same? That would be simple and straightforward. And then like you said, throw an additional error if tls and concat are invoked together.

What do you say?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

concat connections fail during connect-all

2 participants