x/bareon
Code Issues Proposed changes

Avoid swapspace uuid's time_mid collision with minix fs magic

Browse Source 
mkswap defaults to generating a random UUID. The second field
of that UUID, time_mid, is written at offset 0x410. If a
random generated value yields to have 0x8f13 in this very
location, blkid mistakenly treats the block device as a minix
filesystem superblock and then fails reading the structure.

This bug is triggered with probability of 2^-16.

We can work around this bug by re-running mkfs several times
unless blkid is able to examine the newly-created filesystem.

Change-Id: I13209faeb4e1d2798b7b0e0e8382e43803998d36
Closes-Bug: #1550227
This commit is contained in:
Dmitry Bilunov 2016-02-26 17:07:37 +03:00 committed by Alexander Gordeev
parent cb36d98227
commit 6f3c24eace
2 changed files with 84 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

68
bareon/tests/test_fs_utils.py
View File

@ -25,22 +25,40 @@ class TestFSUtils(unittest2.TestCase):
def test_make_xfs_add_f_flag(self, mock_exec):
fu.make_fs('xfs', '--other-options --passed', '', '/dev/fake')
mock_exec.assert_called_once_with('mkfs.xfs', '--other-options',
'--passed', '-f', '/dev/fake')
expected_calls = [
mock.call('mkfs.xfs', '--other-options', '--passed',
'-f', '/dev/fake'),
mock.call('blkid', '-c', '/dev/null', '-o', 'value',
'-s', 'UUID', '/dev/fake')
]
self.assertEqual(mock_exec.call_args_list, expected_calls)
def test_make_xfs_empty_options(self, mock_exec):
fu.make_fs('xfs', '', '', '/dev/fake')
mock_exec.assert_called_once_with('mkfs.xfs', '-f', '/dev/fake')
expected_calls = [
mock.call('mkfs.xfs', '-f', '/dev/fake'),
mock.call('blkid', '-c', '/dev/null', '-o', 'value',
'-s', 'UUID', '/dev/fake')
]
self.assertEqual(mock_exec.call_args_list, expected_calls)
def test_make_fs(self, mock_exec):
fu.make_fs('ext4', '-F', 'fake_label', '/dev/fake')
mock_exec.assert_called_once_with('mkfs.ext4', '-F', '-L',
'fake_label', '/dev/fake')
expected_calls = [
mock.call('mkfs.ext4', '-F', '-L', 'fake_label', '/dev/fake'),
mock.call('blkid', '-c', '/dev/null', '-o', 'value',
'-s', 'UUID', '/dev/fake')
]
self.assertEqual(mock_exec.call_args_list, expected_calls)
def test_make_fs_swap(self, mock_exec):
fu.make_fs('swap', '-f', 'fake_label', '/dev/fake')
mock_exec.assert_called_once_with('mkswap', '-f', '-L', 'fake_label',
'/dev/fake')
fu.make_fs('swap', '', 'fake_label', '/dev/fake')
expected_calls = [
mock.call('mkswap', '-f', '-L', 'fake_label', '/dev/fake'),
mock.call('blkid', '-c', '/dev/null', '-o', 'value',
'-s', 'UUID', '/dev/fake')
]
self.assertEqual(mock_exec.call_args_list, expected_calls)
def test_extend_fs_ok_ext2(self, mock_exec):
fu.extend_fs('ext2', '/dev/fake')
@ -142,3 +160,37 @@ class TestFSUtils(unittest2.TestCase):
self.assertEqual(fu.format_fs_label(long_label),
template.format(long_label_trimmed))
class TestFSRetry(unittest2.TestCase):
def test_make_fs_bad_swap_retry(self):
# We mock utils.execute to throw an exception on first two
# invocations of blkid to test the retry loop.
rvs = [
None, errors.ProcessExecutionError(),
None, errors.ProcessExecutionError(),
None, None
]
with mock.patch.object(utils, 'execute', side_effect=rvs) as mock_exec:
fu.make_fs('swap', '', 'fake_label', '/dev/fake')
expected_calls = 3 * [
mock.call('mkswap', '-f', '-L', 'fake_label', '/dev/fake'),
mock.call('blkid', '-c', '/dev/null', '-o', 'value',
'-s', 'UUID', '/dev/fake')
]
self.assertEqual(mock_exec.call_args_list, expected_calls)
def test_make_fs_bad_swap_failure(self):
# We mock utils.execute to throw an exception on invocations
# of blkid (MAX_MKFS_TRIES times) to see if it fails.
rvs = fu.MAX_MKFS_TRIES * [None, errors.ProcessExecutionError()]
with mock.patch.object(utils, 'execute', side_effect=rvs) as mock_exec:
with self.assertRaises(errors.FsUtilsError):
fu.make_fs('swap', '', 'fake_label', '/dev/fake')
expected_calls = 3 * [
mock.call('mkswap', '-f', '-L', 'fake_label', '/dev/fake'),
mock.call('blkid', '-c', '/dev/null', '-o', 'value',
'-s', 'UUID', '/dev/fake')
]
self.assertEqual(mock_exec.call_args_list, expected_calls)

25
bareon/utils/fs.py
View File

@ -19,7 +19,10 @@ from bareon import errors
from bareon.openstack.common import log as logging
from bareon.utils import utils
import six
LOG = logging.getLogger(__name__)
MAX_MKFS_TRIES = 5
def format_fs_label(label):
@ -45,11 +48,31 @@ def make_fs(fs_type, fs_options, fs_label, dev):
# NOTE(agordeev): force xfs creation.
# Othwerwise, it will fail to proceed if filesystem exists.
fs_options += ' -f '
if fs_type == 'swap':
fs_options += ' -f '
cmd_line.append(cmd_name)
for opt in (fs_options, format_fs_label(fs_label)):
cmd_line.extend([s for s in opt.split(' ') if s])
cmd_line.append(dev)
utils.execute(*cmd_line)
# NOTE(dbilunov): make sure the newly-created fs can
# be observed by blkid. Currently known problem is
# that generated UUID could possibly collide with
# minix filesystem magic (0x8f13)
mkfs_ok = False
for _ in six.moves.range(MAX_MKFS_TRIES):
utils.execute(*cmd_line)
try:
utils.execute('blkid', '-c', '/dev/null', '-o', 'value',
'-s', 'UUID', dev)
except errors.ProcessExecutionError:
LOG.warning('blkid has failed on %s, retrying...', dev)
else:
mkfs_ok = True
break
if not mkfs_ok:
raise errors.FsUtilsError('Cannot get UUID of a newly-created ' +
'{0} on {1}'.format(fs_type, dev))
def extend_fs(fs_type, fs_dev):