Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support OPAL 2 self-encrypting NVMe disk drives (fix #2475) #2488

Merged
merged 5 commits into from Nov 5, 2020

Conversation

OliverO2
Copy link
Contributor

@OliverO2 OliverO2 commented Sep 4, 2020

This patch enables support for OPAL 2 self-encrypting NVMe disk drives.

I will amend this PR and/or propose merging it as soon as testing results come in (cf. progress in #2475).


case "$device" in
(*/nvme*)
echo "$device"n[0-9] # consider all namespace block devices (NOTE: relies on nullglob)
Copy link
Member

@jsmeix jsmeix Sep 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"$device"n[0-9] matches only up to namespace number 9 i.e. /dev/nvme...n9
so e.g. /dev/nvme0n10 and higher namespace numbers are ignored.

To be more on the safe side I suggest

# consider all namespace block devices up to /dev/nvme...n99 (NOTE: relies on nullglob)
echo "$device"n[1-9] "$device"n[1-9][0-9]

I think more than 9 namespaces might happen in practice
while more than 99 namespaces never happen in real world.

But the latter is probably only wishful thinking because
https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4-2019.06.10-Ratified.pdf
reads (excerpts)

... namespace ID (NSID) ...

6.1.2 Valid and Invalid NSIDs
...
Any NSID is valid, except if that NSID is 0h or greater than the
Number of Namespaces field reported in the Identify Controller
data structure (refer to Figure 247).
NSID FFFFFFFFh is a broadcast value that is used to specify
all namespaces. 

so it seems any number up to FFFFFFFFh could appear in real world.

Copy link
Member

@jsmeix jsmeix Sep 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next question is how namespaces > 9 block device nodes are shown:
As decimal numbers like /dev/nvme0n12
or as hex numbers like /dev/nvme0nc or /dev/nvme0nC or /dev/nvme0n0c

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, I have not seen namespace IDs beyond 1 being used in the wild. But I don't mind if we extend the scheme.

The kernel will use decimal digits:

	sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance);

@jsmeix jsmeix added the enhancement Adaptions and new features label Sep 17, 2020
@jsmeix jsmeix added this to the ReaR v2.7 milestone Sep 17, 2020
@jsmeix jsmeix self-assigned this Sep 17, 2020
@jsmeix
Copy link
Member

jsmeix commented Sep 18, 2020

@OliverO2
thank you for your improvement in
48669d2

It is such a pleasure for my eyes!
Perfect code that is 100% to the point with a complete explanatory comment
and that with even the same number of characters as my clumsy attempt before.

Have a nice and relaxed weekend!

@OliverO2
Copy link
Contributor Author

@jsmeix
My pleasure, I just happend to cross-check and found out that extglob is actually enabled throughout ReaR. :-)

Have a nice weekend, too!

@jsmeix
Copy link
Member

jsmeix commented Sep 18, 2020

Yes, there are some well hidded 'secrets' (like the global extglob) in ReaR.

Right now I adapted the "Beware of the emptiness" section in
https://github.com/rear/rear/wiki/Coding-Style
so that it now at least mentiones that also extglob is globally set as follows:

There is a global 'shopt -s nullglob extglob' setting in usr/sbin/rear
where the 'nullglob' setting means that ...

@enzolis
Copy link

enzolis commented Oct 27, 2020

@OliverO2
Thanks for providing these patches.
Previously, the disks were unlocked but "unlocked_device_count" did not increase due to failing partprobe. Thus the password prompt was showing up again.
Your patches fix the issue for me (NVMe disks using UEFI boot).

I would like to make you aware that at least in my PBA environment the shell options nullglob and extglob are NOT set (contrary to the situation in the booted OS). Thus the shell was complaining about incorrect syntax.
Adding "shopt -s nullglob extglob" to the top of "unlock-opal-disks", re-creation and upload of PBA fixed this issue, but I am not sure if it is the correct place to put it.

@OliverO2
Copy link
Contributor Author

@enzolis
Thank you so much for your feedback. Good to hear that it works in general. I'll take care of safely integrating shopt -s nullglob extglob. It is my plan to come up with a ready-to-merge solution within 7 days.

@OliverO2
Copy link
Contributor Author

OliverO2 commented Nov 3, 2020

With the successful test report by @enzolis and the fix for missing 'nullglob extglob' options I'd consider this patch ready to merge.

@OliverO2 OliverO2 changed the title [DO NOT MERGE] Support OPAL 2 self-encrypting NVMe disk drives (fix #2475) Support OPAL 2 self-encrypting NVMe disk drives (fix #2475) Nov 3, 2020
@jsmeix jsmeix requested a review from a team November 3, 2020 13:53
@jsmeix
Copy link
Member

jsmeix commented Nov 3, 2020

@rear/contributors
please have a look.
Perhaps you see some obvious issues.
If there are no objections I would like to merge it on Thursday (05.Oct.2020) afternoon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants