Skip to content

Commit

Permalink
fix: open blockdevices with exclusive flock for partitioning
Browse files Browse the repository at this point in the history 
This fixes spurious race conditions when user disks are partitioned
and formatted in `mountUserDisks` task. While this task runs, `udevd` is
running to allow various `/dev/` symlinks to be used for user disks.
At the same time `udevd` might trigger syscall `BLKRRPART` at any time
concurrently with Talos which leads to a race on kernel side when Talos
tries to update kernel partition table while kernel does it on its own
as a result of `udevd` call.

As part of the fix, `RereadPartitionTable()` calls were removed (they
trigger `BLKRRPART` and they're not needed as Talos updates partition
table on its own).

Some cleanups to make sure blockdevice is open/closed just in matching
pairs (no lingering open blockdevice instances). This is import for
`WithExclusiveLock()` calls, as it would lead to a deadlock if previous
blockdevice instance is not closed.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
  • Loading branch information
@smira @talos-bot
smira authored and talos-bot committed Jan 28, 2021
1 parent e0a0f58 commit 18db20d
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 95 deletions.
92 changes: 44 additions & 48 deletions cmd/installer/pkg/install/manifest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ func (m *Manifest) executeOnDevice(device Device, targets []*Target) (err error)

var bd *blockdevice.BlockDevice

if bd, err = blockdevice.Open(device.Device); err != nil {
if bd, err = blockdevice.Open(device.Device, blockdevice.WithExclusiveLock(true)); err != nil {
return err
}

Expand Down Expand Up @@ -267,66 +267,66 @@ func (m *Manifest) executeOnDevice(device Device, targets []*Target) (err error)
return err
}

if bd, err = blockdevice.Open(device.Device); err != nil {
if err = bd.Close(); err != nil {
return err
}

if bd, err = blockdevice.Open(device.Device, blockdevice.WithExclusiveLock(true)); err != nil {
return err
}

defer bd.Close() //nolint: errcheck

created = true
}

if !created && device.ResetPartitionTable {
log.Printf("resetting partition table on %s", device.Device)

// TODO: how should it work with zero option above?
if err = bd.Reset(); err != nil {
return err
}
if !created {
if device.ResetPartitionTable {
log.Printf("resetting partition table on %s", device.Device)

if err = bd.RereadPartitionTable(); err != nil {
return err
}
} else {
// clean up partitions which are going to be recreated
keepPartitions := map[string]struct{}{}
// TODO: how should it work with zero option above?
if err = bd.Reset(); err != nil {
return err
}
} else {
// clean up partitions which are going to be recreated
keepPartitions := map[string]struct{}{}

for _, target := range targets {
if target.Skip {
keepPartitions[target.Label] = struct{}{}
for _, target := range targets {
if target.Skip {
keepPartitions[target.Label] = struct{}{}
}
}
}

// make sure all partitions to be skipped already exist
missingPartitions := map[string]struct{}{}
// make sure all partitions to be skipped already exist
missingPartitions := map[string]struct{}{}

for label := range keepPartitions {
missingPartitions[label] = struct{}{}
}
for label := range keepPartitions {
missingPartitions[label] = struct{}{}
}

for _, part := range pt.Partitions().Items() {
delete(missingPartitions, part.Name)
}
for _, part := range pt.Partitions().Items() {
delete(missingPartitions, part.Name)
}

if len(missingPartitions) > 0 {
return fmt.Errorf("some partitions to be skipped are missing: %v", missingPartitions)
}
if len(missingPartitions) > 0 {
return fmt.Errorf("some partitions to be skipped are missing: %v", missingPartitions)
}

// delete all partitions which are not skipped
for _, part := range pt.Partitions().Items() {
if _, ok := keepPartitions[part.Name]; !ok {
log.Printf("deleting partition %s", part.Name)
// delete all partitions which are not skipped
for _, part := range pt.Partitions().Items() {
if _, ok := keepPartitions[part.Name]; !ok {
log.Printf("deleting partition %s", part.Name)

if err = pt.Delete(part); err != nil {
return err
if err = pt.Delete(part); err != nil {
return err
}
}
}
}

if err = pt.Write(); err != nil {
return err
}

if err = bd.RereadPartitionTable(); err != nil {
return err
if err = pt.Write(); err != nil {
return err
}
}
}

Expand All @@ -345,10 +345,6 @@ func (m *Manifest) executeOnDevice(device Device, targets []*Target) (err error)
return err
}

if err = bd.RereadPartitionTable(); err != nil {
log.Printf("failed to re-read partition table on %q: %s, ignoring error...", device.Device, err)
}

for _, target := range targets {
target := target

Expand Down Expand Up @@ -494,7 +490,7 @@ func (m *Manifest) zeroDevice(device Device) (err error) {

log.Printf("wiping %q", device.Device)

if bd, err = blockdevice.Open(device.Device); err != nil {
if bd, err = blockdevice.Open(device.Device, blockdevice.WithExclusiveLock(true)); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ require (
github.com/stretchr/testify v1.7.0
github.com/syndtr/gocapability v0.0.0-20180916011248-d98352740cb2
github.com/talos-systems/crypto v0.2.1-0.20210125160556-cf75519cab82
github.com/talos-systems/go-blockdevice v0.1.1-0.20210126125338-5a1c7f768e01
github.com/talos-systems/go-blockdevice v0.2.0
github.com/talos-systems/go-loadbalancer v0.1.0
github.com/talos-systems/go-procfs v0.0.0-20210108152626-8cbc42d3dc24
github.com/talos-systems/go-retry v0.2.0
Expand Down
5 changes: 3 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -860,8 +860,8 @@ github.com/syndtr/gocapability v0.0.0-20180916011248-d98352740cb2 h1:b6uOv7YOFK0
github.com/syndtr/gocapability v0.0.0-20180916011248-d98352740cb2/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww=
github.com/talos-systems/crypto v0.2.1-0.20210125160556-cf75519cab82 h1:5TsM3o/yJJF6kakHyPee88D0yWNNDNKZJ2NCX9MFsKk=
github.com/talos-systems/crypto v0.2.1-0.20210125160556-cf75519cab82/go.mod h1:OXCK52Q0dzm88YRG4VdTBdidkPUtqrCxCyW7bUs4DAw=
github.com/talos-systems/go-blockdevice v0.1.1-0.20210126125338-5a1c7f768e01 h1:Efm20xEYOuiSh0Mct8kBudZoS88z5I+fegt5M6Xwqn4=
github.com/talos-systems/go-blockdevice v0.1.1-0.20210126125338-5a1c7f768e01/go.mod h1:DGbop5CJa0PYdhQK9cNVF61pPJNedas1m7Gi/qAnrsM=
github.com/talos-systems/go-blockdevice v0.2.0 h1:tTu0ak3GfF8iSxNsdsicVhTKebIcyBARQYxhRV86AF0=
github.com/talos-systems/go-blockdevice v0.2.0/go.mod h1:DGbop5CJa0PYdhQK9cNVF61pPJNedas1m7Gi/qAnrsM=
github.com/talos-systems/go-loadbalancer v0.1.0 h1:MQFONvSjoleU8RrKq1O1Z8CyTCJGd4SLqdAHDlR6o9s=
github.com/talos-systems/go-loadbalancer v0.1.0/go.mod h1:D5Qjfz+29WVjONWECZvOkmaLsBb3f5YeWME0u/5HmIc=
github.com/talos-systems/go-procfs v0.0.0-20210108152626-8cbc42d3dc24 h1:fN8vYvlB9XBQ5aImb1vLgR0ZaDwvfZfBMptqkpi3aEg=
Expand Down Expand Up @@ -1284,6 +1284,7 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8
gopkg.in/check.v1 v1.0.0-20141024133853-64131543e789/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f h1:BLraFXnmrev5lT+xlilqcH8XK9/i0At2xKjWk4p6zsU=
gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b h1:QRR6H1YWRnHb4Y/HeNFCTJLFVxaq6wH4YuVdsUOr75U=
gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
Expand Down
84 changes: 41 additions & 43 deletions internal/app/machined/pkg/runtime/v1alpha1/v1alpha1_sequencer_tasks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -835,72 +835,70 @@ func MountUserDisks(seq runtime.Sequence, data interface{}) (runtime.TaskExecuti

// TODO(andrewrynhard): We shouldn't pull in the installer command package
// here.
func partitionAndFormatDisks(logger *log.Logger, r runtime.Runtime) (err error) {
func partitionAndFormatDisks(logger *log.Logger, r runtime.Runtime) error {
m := &installer.Manifest{
Devices: map[string]installer.Device{},
Targets: map[string][]*installer.Target{},
}

for _, disk := range r.Config().Machine().Disks() {
var bd *blockdevice.BlockDevice
disk := disk

bd, err = blockdevice.Open(disk.Device())
if err != nil {
return err
}
if err := func() error {
bd, err := blockdevice.Open(disk.Device())
if err != nil {
return err
}

// nolint: errcheck
defer bd.Close()
// nolint: errcheck
defer bd.Close()

var pt *gpt.GPT
var pt *gpt.GPT

pt, err = bd.PartitionTable()
if err != nil {
if !errors.Is(err, blockdevice.ErrMissingPartitionTable) {
return err
pt, err = bd.PartitionTable()
if err != nil {
if !errors.Is(err, blockdevice.ErrMissingPartitionTable) {
return err
}
}
}

// Partitions will be created/recreated if either of the following
// conditions are true:
// - a partition table exists AND there are no partitions
// - a partition table does not exist
// Partitions will be created/recreated if either of the following
// conditions are true:
// - a partition table exists AND there are no partitions
// - a partition table does not exist

if pt != nil {
if len(pt.Partitions().Items()) > 0 {
logger.Printf(("skipping setup of %q, found existing partitions"), disk.Device())
if pt != nil {
if len(pt.Partitions().Items()) > 0 {
logger.Printf(("skipping setup of %q, found existing partitions"), disk.Device())

continue
return nil
}
}
}

m.Devices[disk.Device()] = installer.Device{
Device: disk.Device(),
ResetPartitionTable: true,
}
m.Devices[disk.Device()] = installer.Device{
Device: disk.Device(),
ResetPartitionTable: true,
}

if m.Targets[disk.Device()] == nil {
m.Targets[disk.Device()] = []*installer.Target{}
}
for _, part := range disk.Partitions() {
extraTarget := &installer.Target{
Device: disk.Device(),
Size: part.Size(),
Force: true,
PartitionType: installer.LinuxFilesystemData,
FileSystemType: installer.FilesystemTypeXFS,
}

for _, part := range disk.Partitions() {
extraTarget := &installer.Target{
Device: disk.Device(),
Size: part.Size(),
Force: true,
PartitionType: installer.LinuxFilesystemData,
FileSystemType: installer.FilesystemTypeXFS,
m.Targets[disk.Device()] = append(m.Targets[disk.Device()], extraTarget)
}

m.Targets[disk.Device()] = append(m.Targets[disk.Device()], extraTarget)
return nil
}(); err != nil {
return err
}
}

if err = m.Execute(); err != nil {
return err
}

return nil
return m.Execute()
}

func mountDisks(r runtime.Runtime) (err error) {
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/mount/mount.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ func (p *Point) ResizePartition() (resized bool, err error) {
return false, err
}

bd, err := blockdevice.Open("/dev/" + devname)
bd, err := blockdevice.Open("/dev/"+devname, blockdevice.WithExclusiveLock(true))
if err != nil {
return false, fmt.Errorf("error opening block device %q: %w", devname, err)
}
Expand Down

0 comments on commit 18db20d

Please sign in to comment.