Browse Source

Utilize CSV file for EFI loader selection

Adds support to identify and utilize a CSV file to signal which
bootloader to utilize, and set it when the OS is running as opposed
to when EFI is running. This works around EFI loader potentially
crashing some vendors hardware types when entry stored in the
image does not match the EFI loader record which was utilzied to
boot.

Grub2+shim specifically specifically needs the CSV file name
and entry label to match what the system was booted with in order
to prevent the machine from potentially crashing.

See https://storyboard.openstack.org/#!/story/2008962
and https://bugzilla.redhat.com/show_bug.cgi?id=1966129#c37
for more information.

Change-Id: Ibf1ef4fe0764c0a6f1a39cb7eebc23ecc0ee177d
Story: 2008962
Task: 42598
Co-Authored-By: Bob Fournier <bfournie@redhat.com>
(cherry picked from commit 2fab70c36ba40a345a9dd01aeb5019681e567aa5)
changes/65/795965/1
Julia Kreger 5 days ago
committed by Riccardo Pittau
parent
commit
bfa97cbbc2
3 changed files with 137 additions and 14 deletions
  1. +43
    -10
      ironic_python_agent/extensions/image.py
  2. +72
    -4
      ironic_python_agent/tests/unit/extensions/test_image.py
  3. +22
    -0
      releasenotes/notes/support-bootloader-csv-file-use-c815b520c600cd98.yaml

+ 43
- 10
ironic_python_agent/extensions/image.py View File

@ -38,12 +38,16 @@ CONF = cfg.CONF
BIND_MOUNTS = ('/dev', '/proc', '/run')
# NOTE(TheJulia): Do not add bootia32.csv to this list. That is 32bit
# EFI booting and never really became popular.
BOOTLOADERS_EFI = [
'bootx64.csv', # Used by GRUB2 shim loader (Ubuntu, Red Hat)
'boot.csv', # Used by rEFInd, Centos7 Grub2
'bootia32.efi',
'bootx64.efi',
'bootx64.efi', # x86_64 Default
'bootia64.efi',
'bootarm.efi',
'bootaa64.efi',
'bootaa64.efi', # Arm64 Default
'bootriscv32.efi',
'bootriscv64.efi',
'bootriscv128.efi',
@ -224,9 +228,10 @@ def _is_bootloader_loaded(dev):
def _get_efi_bootloaders(location):
"""Get all valid efi bootloaders in a given location
:param location: the location where it should start looking for the
:param location: the location where it should start looking for the
efi files.
:return: a list of relative paths to valid efi bootloaders
:return: a list of relative paths to valid efi bootloaders or reference
files.
"""
# Let's find all files with .efi or .EFI extension
LOG.debug('Looking for all efi files on %s', location)
@ -242,15 +247,30 @@ def _get_efi_bootloaders(location):
v_bl = efi_f.split(location)[-1][1:]
LOG.debug('%s is a valid bootloader', v_bl)
valid_bootloaders.append(v_bl)
if 'csv' in efi_f.lower():
v_bl = efi_f.split(location)[-1][1:]
LOG.debug('%s is a pointer to a bootloader', v_bl)
# The CSV files are intended to be authortative as
# to the bootloader and the label to be used. Since
# we found one, we're going to point directly to it.
# centos7 did ship with 2, but with the same contents.
# TODO(TheJulia): Perhaps we extend this to make a list
# of CSVs instead and only return those?! But then the
# question is which is right/first/preferred.
return [v_bl]
return valid_bootloaders
def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition):
def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
mount_point):
"""Executes efibootmgr and removes duplicate entries.
:param valid_efi_bootloaders: the list of valid efi bootloaders
:param device: the device to be used
:param efi_partition: the efi partition on the device
:param mount_point: The mountpoint for the EFI partition so we can
read contents of files if necessary to perform
proper bootloader injection operations.
"""
# Before updating let's get information about the bootorder
@ -263,13 +283,25 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition):
r'Boot([0-9a-f-A-F]+)\s.*$')
label_id = 1
for v_bl in valid_efi_bootloaders:
v_efi_bl_path = '\\' + v_bl.replace('/', '\\')
# Update the nvram using efibootmgr
# https://linux.die.net/man/8/efibootmgr
label = 'ironic' + str(label_id)
if 'csv' in v_bl.lower():
# These files are always UTF-16 encoded, sometimes have a header.
# Positive bonus is python silently drops the FEFF header.
with open(mount_point + '/' + v_bl, 'r', encoding='utf-16') as csv:
contents = str(csv.read())
csv_contents = contents.split(',', maxsplit=3)
csv_filename = v_bl.split('/')[-1]
v_efi_bl_path = v_bl.replace(csv_filename, str(csv_contents[0]))
v_efi_bl_path = '\\' + v_efi_bl_path.replace('/', '\\')
label = csv_contents[1]
else:
v_efi_bl_path = '\\' + v_bl.replace('/', '\\')
label = 'ironic' + str(label_id)
LOG.debug("Adding loader %(path)s on partition %(part)s of device "
" %(dev)s", {'path': v_efi_bl_path, 'part': efi_partition,
'dev': device})
# Update the nvram using efibootmgr
# https://linux.die.net/man/8/efibootmgr
cmd = utils.execute('efibootmgr', '-c', '-d', device,
'-p', efi_partition, '-w', '-L', label,
'-l', v_efi_bl_path)
@ -345,7 +377,8 @@ def _manage_uefi(device, efi_system_part_uuid=None):
valid_efi_bootloaders = _get_efi_bootloaders(efi_partition_mount_point)
if valid_efi_bootloaders:
_run_efibootmgr(valid_efi_bootloaders, device, efi_partition)
_run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
efi_partition_mount_point)
return True
else:
# NOTE(dtantsur): if we have an empty EFI partition, try to use


+ 72
- 4
ironic_python_agent/tests/unit/extensions/test_image.py View File

@ -2154,6 +2154,51 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
mock_execute.assert_has_calls(expected)
self.assertEqual(7, mock_execute.call_count)
@mock.patch.object(os.path, 'exists', lambda *_: False)
@mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
@mock.patch.object(image, '_get_partition', autospec=True)
@mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
@mock.patch.object(os, 'makedirs', autospec=True)
def test__manage_uefi_found_csv(self, mkdir_mock, mock_utils_efi_part,
mock_get_part_uuid, mock_efi_bl,
mock_execute, mock_dispatch):
mock_utils_efi_part.return_value = '1'
mock_get_part_uuid.return_value = self.fake_dev
mock_efi_bl.return_value = ['EFI/vendor/BOOTX64.CSV']
# Format is <file>,<entry_name>,<options>,humanfriendlytextnotused
# https://www.rodsbooks.com/efi-bootloaders/fallback.html
# Mild difference, Ubuntu ships a file without a 0xFEFF delimiter
# at the start of the file, where as Red Hat *does*
csv_file_data = u'shimx64.efi,Vendor String,,Grub2MadeUSDoThis\n'
mock_execute.side_effect = iter([('', ''), ('', ''),
('', ''), ('', ''),
('', ''), ('', ''),
('', '')])
expected = [mock.call('partx', '-u', '/dev/fake', attempts=3,
delay_on_retry=True),
mock.call('udevadm', 'settle'),
mock.call('mount', self.fake_efi_system_part,
self.fake_dir + '/boot/efi'),
mock.call('efibootmgr'),
mock.call('efibootmgr', '-c', '-d', self.fake_dev,
'-p', '1', '-w',
'-L', 'Vendor String', '-l',
'\\EFI\\vendor\\shimx64.efi'),
mock.call('umount', self.fake_dir + '/boot/efi',
attempts=3, delay_on_retry=True),
mock.call('sync')]
with mock.patch('builtins.open',
mock.mock_open(read_data=csv_file_data)):
result = image._manage_uefi(self.fake_dev, self.fake_root_uuid)
self.assertTrue(result)
mkdir_mock.assert_called_once_with(self.fake_dir + '/boot/efi')
mock_efi_bl.assert_called_once_with(self.fake_dir + '/boot/efi')
mock_execute.assert_has_calls(expected)
self.assertEqual(7, mock_execute.call_count)
@mock.patch.object(os.path, 'exists', lambda *_: False)
@mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
@mock.patch.object(image, '_get_partition', autospec=True)
@ -2242,7 +2287,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
('/boot/efi', ['EFI'], []),
('/boot/efi/EFI', ['centos', 'BOOT'], []),
('/boot/efi/EFI/centos', ['fw', 'fonts'],
['shimx64-centos.efi', 'BOOT.CSV', 'BOOTX64.CSV',
['shimx64-centos.efi',
'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
'grub.cfg']),
@ -2263,7 +2308,28 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
('/boot/efi', ['EFI'], []),
('/boot/efi/EFI', ['centos', 'BOOT'], []),
('/boot/efi/EFI/centos', ['fw', 'fonts'],
['shimx64-centos.efi', 'BOOT.CSV', 'BOOTX64.CSV',
['shimx64-centos.efi', 'BOOTX64.CSV',
'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
'grub.cfg']),
('/boot/efi/EFI/centos/fw', [], []),
('/boot/efi/EFI/centos/fonts', [], ['unicode.pf2']),
('/boot/efi/EFI/BOOT', [],
['BOOTX64.EFI', 'fallback.efi', 'fbx64.efi'])
]
mock_access.return_value = True
result = image._get_efi_bootloaders("/boot/efi")
self.assertEqual(result[0], 'EFI/centos/BOOTX64.CSV')
@mock.patch.object(os, 'walk', autospec=True)
@mock.patch.object(os, 'access', autospec=True)
def test__get_efi_bootloaders_no_csv(
self, mock_access, mock_walk, mock_execute, mock_dispatch):
mock_walk.return_value = [
('/boot/efi', ['EFI'], []),
('/boot/efi/EFI', ['centos', 'BOOT'], []),
('/boot/efi/EFI/centos', ['fw', 'fonts'],
['shimx64-centos.efi',
'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
'grub.cfg']),
@ -2292,7 +2358,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
def test__run_efibootmgr_no_bootloaders(self, mock_execute, mock_dispatch):
result = image._run_efibootmgr([], self.fake_dev,
self.fake_efi_system_part)
self.fake_efi_system_part,
self.fake_dir)
expected = []
self.assertIsNone(result)
mock_execute.assert_has_calls(expected)
@ -2300,7 +2367,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
def test__run_efibootmgr(self, mock_execute, mock_dispatch):
result = image._run_efibootmgr(['EFI/BOOT/BOOTX64.EFI'],
self.fake_dev,
self.fake_efi_system_part)
self.fake_efi_system_part,
self.fake_dir)
expected = [mock.call('efibootmgr'),
mock.call('efibootmgr', '-c', '-d', self.fake_dev,
'-p', self.fake_efi_system_part, '-w',


+ 22
- 0
releasenotes/notes/support-bootloader-csv-file-use-c815b520c600cd98.yaml View File

@ -0,0 +1,22 @@
---
features:
- |
Adds the capability into the agent to read and act upon bootloader CSV
files which serve as authoritative indicators of what bootloader to load
instead of leaning towards utilizing the default.
fixes:
- |
Fixes nodes failing after deployment completes due to issues in the Grub2
EFI loader entry addition where a ``BOOT.CSV`` file provides the
authoritative pointer to the bootloader to be used for booting the OS. The
base issue with Grub2 is that it would update the UEFI bootloader NVRAM
entries with whatever is present in a vendor specific ``BOOT.CSV`` or
``BOOTX64.CSV`` file. In some cases, a baremetal machine *can* crash when
this occurs. More information can be found at
`story 2008962 <https://storyboard.openstack.org/#!/story/2008962>`_.
issues:
- |
If multiple bootloader CSV files are present on the EFI filesystem, the
first CSV file discovered will be utilized. The Ironic team considers
multiple files to be a defect in the image being deployed. This may be
changed in the future.

Loading…
Cancel
Save