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

fix: don't crash in cache refresh/update with nil fsnotify watcher. #254

Merged
merged 3 commits into from
Feb 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/cdi/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,14 @@ func (w *watch) update(dirErrors map[string]error, removed ...string) bool {
update bool
)

// If we failed to create an fsnotify.Watcher we have a nil watcher here
// (but with autoRefresh left on). One known case when this can happen is
// if we have too many open files. In that case we always return true and
// force a refresh.
if w.watcher == nil {
Copy link
Contributor

@bart0sh bart0sh Feb 23, 2025

Choose a reason for hiding this comment

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

Is it possible to check it earlier, e.g. when w.watcher is about to be assigned to nil? Or we want to keep it nil in a hope that it will be changed at some point?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall correctly we guard setting the watchers in a Mutex. Does that help to ensure this does not happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to check it earlier, e.g. when w.watcher is about to be assigned to nil? Or we want to keep it nil in a hope that it will be changed at some point?

Yes, I wanted to leave the door open for a better than the current recovery once we can again create fds (and that would probably be retrying a setup here).

In this very PR, I wanted to roll a minimum-footprint fix that we can review and merge quickly, and be fairly confident that it is safe to cherry pick and tag a v0.8.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I recall correctly we guard setting the watchers in a Mutex. Does that help to ensure this does not happen?

This happens if we hit EMFILE while trying to create the watch.

return true
}

for dir, ok = range w.tracked {
if ok {
continue
Expand Down
237 changes: 237 additions & 0 deletions pkg/cdi/cache_linux_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
/*
Copyright © The CDI Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package cdi

import (
"fmt"
"os"
"path/filepath"
"strconv"
"strings"
"syscall"
"testing"

oci "github.com/opencontainers/runtime-spec/specs-go"
"github.com/stretchr/testify/require"
)

func TestTooManyOpenFiles(t *testing.T) {
em, err := triggerEmfile()
require.NoError(t, err)
require.NotNil(t, em)
defer func() {
require.NoError(t, em.undo())
}()

_, err = syscall.Socket(syscall.AF_INET, syscall.SOCK_DGRAM, 0)
require.Equal(t, syscall.EMFILE, err)

cache := newCache(
WithAutoRefresh(true),
)
require.NotNil(t, cache)

// try to trigger original crash with a nil fsnotify.Watcher
_, _ = cache.InjectDevices(&oci.Spec{}, "vendor1.com/device=dev1")
}

func TestRecoveryAfterTooManyOpenFiles(t *testing.T) {
var (
etcDir = map[string]string{
"vendor1.yaml": `
cdiVersion: "0.3.0"
kind: "vendor1.com/device"
containerEdits:
env:
- VENDOR1_SPEC_VAR1=VAL1
devices:
- name: "dev1"
containerEdits:
env:
- "VENDOR1_VAR1=VAL1"
deviceNodes:
- path: "/dev/vendor1-dev1"
type: b
major: 10
minor: 1
`,
}

devices = []string{
"vendor1.com/device=dev1",
}

ociSpec = &oci.Spec{}

resultingSpec = &oci.Spec{
Process: &oci.Process{
Env: []string{
"VENDOR1_SPEC_VAR1=VAL1",
"VENDOR1_VAR1=VAL1",
},
},
Linux: &oci.Linux{
Devices: []oci.LinuxDevice{
{
Path: "/dev/vendor1-dev1",
Type: "b",
Major: 10,
Minor: 1,
},
},
Resources: &oci.LinuxResources{
Devices: []oci.LinuxDeviceCgroup{
{
Allow: true,
Type: "b",
Major: int64ptr(10),
Minor: int64ptr(1),
Access: "rwm",
},
},
},
},
}
)

dir, err := createSpecDirs(t, etcDir, nil)
require.NoError(t, err, "failed to create test directory")

// trigger EMFILE for fd creation: exhaust our file descriptor table
em, err := triggerEmfile()
require.NoError(t, err)
require.NotNil(t, em)
defer func() {
require.NoError(t, em.undo())
}()

_, err = syscall.Socket(syscall.AF_INET, syscall.SOCK_DGRAM, 0)
require.Equal(t, syscall.EMFILE, err)

cache := newCache(
WithSpecDirs(
filepath.Join(dir, "etc"),
),
WithAutoRefresh(true),
)
require.NotNil(t, cache)

// try to trigger original crash with a nil fsnotify.Watcher
_, _ = cache.InjectDevices(&oci.Spec{}, devices...)

// undo EMFILE for fd creation
require.NoError(t, em.undo())

// verify that injection works again
unresolved, err := cache.InjectDevices(ociSpec, devices...)
require.NoError(t, err)
require.Nil(t, unresolved)
require.Equal(t, resultingSpec, ociSpec)
}

type emfile struct {
limit syscall.Rlimit
fds []int
undone bool
}

// getFdTableSize reads the process' FD table size.
func getFdTableSize() (uint64, error) {
status, err := os.ReadFile("/proc/self/status")
if err != nil {
return 0, err
}

const fdSizeTag = "FDSize:"

for _, line := range strings.Split(string(status), "\n") {
if strings.HasPrefix(line, fdSizeTag) {
value := strings.TrimSpace(strings.TrimPrefix(line, fdSizeTag))
size, err := strconv.ParseUint(value, 10, 64)
if err != nil {
return 0, err
}
return size, nil
}
}

return 0, fmt.Errorf("tag %s not found in /proc/self/status", fdSizeTag)
}

// triggerEmfile exhausts the file descriptors of the process and triggers
// a failure with an EMFILE for any syscall that needs to create a new fd.
// On success, the returned emfile's undo() method can be used to undo the
// exhausted table and restore everything to a working state.
func triggerEmfile() (*emfile, error) {
// We exhaust our file descriptors by
// - checking the size of our current fd table
// - setting our soft RLIMIT_NOFILE limit to the table size
// - ensuring the fd table is full by creating new fd's
//
// We also save our original RLIMIT_NOFILE limit and any fd's we
// might need to create, so we can eventually restore everything
// to its original state.

fdsize, err := getFdTableSize()
if err != nil {
return nil, err
}

em := &emfile{}

if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, &em.limit); err != nil {
return nil, err
}

limit := em.limit
limit.Cur = fdsize

if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, &limit); err != nil {
return nil, err
}

for i := uint64(0); i < fdsize; i++ {
fd, err := syscall.Socket(syscall.AF_INET, syscall.SOCK_DGRAM, 0)
if err != nil {
return em, nil
}
em.fds = append(em.fds, fd)
}

return nil, fmt.Errorf("failed to trigger EMFILE")
}

// undo restores the process' state to its pre-EMFILE condition.
func (em *emfile) undo() error {
if em == nil || em.undone {
return nil
}

// we restore the process' state to pre-EMFILE condition by
// - restoring our saved RLIMIT_NOFILE
// - closing any extra file descriptors we might have created

if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, &em.limit); err != nil {
return err
}
for _, fd := range em.fds {
syscall.Close(fd)
}
em.undone = true

return nil
}
Loading