From 618997f0b34960f18b524b413d079b7ee84d7f72 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Wed, 3 Apr 2019 16:18:36 +0200 Subject: [PATCH 1/4] gpuclockctl: fix return value of get_gpu_state_amd() --- daemon/gpuclockctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemon/gpuclockctl.c b/daemon/gpuclockctl.c index 73c17c5..8598a9f 100644 --- a/daemon/gpuclockctl.c +++ b/daemon/gpuclockctl.c @@ -303,7 +303,7 @@ static int get_gpu_state_amd(struct GameModeGPUInfo *info) /* Copy in the value from the file */ strncpy(info->amd_performance_level, buff, CONFIG_VALUE_MAX - 1); info->amd_performance_level[CONFIG_VALUE_MAX - 1] = '\0'; - return info == NULL; + return 0; } /* From d7394cbeb23990c42c2a5f49244bae9b5fbdeeb3 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Wed, 3 Apr 2019 16:27:12 +0200 Subject: [PATCH 2/4] daemon: fix file descriptor leaks --- daemon/daemon_config.c | 1 + daemon/governors-query.c | 1 + daemon/gpu-control.c | 5 ++++- daemon/gpuclockctl.c | 26 +++++++++++++++++--------- 4 files changed, 23 insertions(+), 10 deletions(-) diff --git a/daemon/daemon_config.c b/daemon/daemon_config.c index 675c195..23e0e76 100644 --- a/daemon/daemon_config.c +++ b/daemon/daemon_config.c @@ -362,6 +362,7 @@ static void load_config_files(GameModeConfig *self) if (error) { LOG_MSG("Failed to parse config file - error on line %d!\n", error); } + fclose(f); } free(path); } diff --git a/daemon/governors-query.c b/daemon/governors-query.c index 9e0af0f..9d483c0 100644 --- a/daemon/governors-query.c +++ b/daemon/governors-query.c @@ -133,6 +133,7 @@ const char *get_gov_state(void) /* Don't handle the mixed case, this shouldn't ever happen * But it is a clear sign we shouldn't carry on */ LOG_ERROR("Governors malformed: got \"%s\", expected \"%s\"", contents, governor); + fclose(f); return "malformed"; } diff --git a/daemon/gpu-control.c b/daemon/gpu-control.c index 147cdd1..a2699c0 100644 --- a/daemon/gpu-control.c +++ b/daemon/gpu-control.c @@ -50,7 +50,10 @@ enum GPUVendor gamemode_get_gpu_vendor(long device) return Vendor_Invalid; } char buff[64]; - if (fgets(buff, 64, file) != NULL) { + bool got_line = fgets(buff, 64, file) != NULL; + fclose(file); + + if (got_line) { vendor = strtol(buff, NULL, 0); } else { LOG_ERROR("Coudn't read contents of file %s, will not apply optimisations!\n", path); diff --git a/daemon/gpuclockctl.c b/daemon/gpuclockctl.c index 8598a9f..873ab54 100644 --- a/daemon/gpuclockctl.c +++ b/daemon/gpuclockctl.c @@ -289,21 +289,27 @@ static int get_gpu_state_amd(struct GameModeGPUInfo *info) return -1; } + int ret = 0; + char buff[CONFIG_VALUE_MAX] = { 0 }; if (!fgets(buff, CONFIG_VALUE_MAX, file)) { LOG_ERROR("Could not read file %s (%s)!\n", path, strerror(errno)); - return -1; + ret = -1; } if (fclose(file) != 0) { LOG_ERROR("Could not close %s after reading (%s)!\n", path, strerror(errno)); - return -1; + ret = -1; } - /* Copy in the value from the file */ - strncpy(info->amd_performance_level, buff, CONFIG_VALUE_MAX - 1); - info->amd_performance_level[CONFIG_VALUE_MAX - 1] = '\0'; - return 0; + if( ret == 0 ) + { + /* Copy in the value from the file */ + strncpy(info->amd_performance_level, buff, CONFIG_VALUE_MAX - 1); + info->amd_performance_level[CONFIG_VALUE_MAX - 1] = '\0'; + } + + return ret; } /* @@ -320,17 +326,19 @@ static int set_gpu_state_amd_file(const char *filename, long device, const char return -1; } + int ret = 0; + if (fprintf(file, "%s", value) < 0) { LOG_ERROR("Could not write to %s (%s)!\n", path, strerror(errno)); - return -1; + ret = -1; } if (fclose(file) != 0) { LOG_ERROR("Could not close %s after writing (%s)!\n", path, strerror(errno)); - return -1; + ret = -1; } - return 0; + return ret; } /** From b411cfff6b0aadfdc940bad9beb8a52e0e8c9f89 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Wed, 3 Apr 2019 16:46:38 +0200 Subject: [PATCH 3/4] gpuclockctl: refactor buffer size specification and avoid redundancies --- daemon/gpuclockctl.c | 116 ++++++++++++++++++++++++------------------- 1 file changed, 64 insertions(+), 52 deletions(-) diff --git a/daemon/gpuclockctl.c b/daemon/gpuclockctl.c index 873ab54..cac49bd 100644 --- a/daemon/gpuclockctl.c +++ b/daemon/gpuclockctl.c @@ -36,6 +36,8 @@ POSSIBILITY OF SUCH DAMAGE. #include "external-helper.h" #include "gpu-control.h" +#include + /* NV constants */ #define NV_CORE_OFFSET_ATTRIBUTE "GPUGraphicsClockOffset" #define NV_MEM_OFFSET_ATTRIBUTE "GPUMemoryTransferRateOffset" @@ -44,6 +46,7 @@ POSSIBILITY OF SUCH DAMAGE. #define NV_PCIDEVICE_ATTRIBUTE "PCIDevice" #define NV_ATTRIBUTE_FORMAT "[gpu:%ld]/%s" #define NV_PERF_LEVEL_FORMAT "[%ld]" +#define NV_ARG_MAX 128 /* AMD constants */ #define AMD_DRM_PATH "/sys/class/drm/card%ld/device/%s" @@ -64,6 +67,30 @@ static void print_usage_and_exit(void) exit(EXIT_FAILURE); } +static const char* get_nv_attr(const char *attr) +{ + static char out[EXTERNAL_BUFFER_MAX]; + const char *exec_args[] = { "/usr/bin/nvidia-settings", "-q", attr, "-t", NULL }; + if (run_external_process(exec_args, out, -1) != 0) { + LOG_ERROR("Failed to get %s!\n", attr); + out[0] = 0; + return NULL; + } + + return &out[0]; +} + +static int set_nv_attr(const char *attr) +{ + const char *exec_args_core[] = { "/usr/bin/nvidia-settings", "-a", attr, NULL }; + if (run_external_process(exec_args_core, NULL, -1) != 0) { + LOG_ERROR("Failed to set %s!\n", attr); + return -1; + } + + return 0; +} + /* Get the nvidia driver index for the current GPU */ static long get_gpu_index_id_nv(struct GameModeGPUInfo *info) { @@ -108,23 +135,21 @@ static long get_max_perf_level_nv(struct GameModeGPUInfo *info) if (!getenv("DISPLAY")) LOG_ERROR("Getting Nvidia parameters requires DISPLAY to be set - will likely fail!\n"); - char arg[128] = { 0 }; - char buf[EXTERNAL_BUFFER_MAX] = { 0 }; + char arg[NV_ARG_MAX] = { 0 }; + const char *attr; - snprintf(arg, 128, NV_ATTRIBUTE_FORMAT, info->device, NV_PERFMODES_ATTRIBUTE); - const char *exec_args[] = { "/usr/bin/nvidia-settings", "-q", arg, "-t", NULL }; - if (run_external_process(exec_args, buf, -1) != 0) { - LOG_ERROR("Failed to get %s!\n", arg); + snprintf(arg, NV_ARG_MAX, NV_ATTRIBUTE_FORMAT, info->device, NV_PERFMODES_ATTRIBUTE); + if ((attr = get_nv_attr(arg)) == NULL) { return -1; } - char *ptr = strrchr(buf, ';'); + char *ptr = strrchr(attr, ';'); long level = -1; if (!ptr || sscanf(ptr, "; perf=%ld", &level) != 1) { LOG_ERROR( "Output didn't match expected format, couldn't discern highest perf level from " "nvidia-settings!\n"); - LOG_ERROR("Output:%s\n", buf); + LOG_ERROR("Output:%s\n", attr); return -1; } @@ -142,59 +167,53 @@ static int get_gpu_state_nv(struct GameModeGPUInfo *info) long perf_level = get_max_perf_level_nv(info); - char arg[128] = { 0 }; - char buf[EXTERNAL_BUFFER_MAX] = { 0 }; + char arg[NV_ARG_MAX] = { 0 }; + const char *attr; char *end; /* Get the GPUGraphicsClockOffset parameter */ snprintf(arg, - 128, + NV_ARG_MAX, NV_ATTRIBUTE_FORMAT NV_PERF_LEVEL_FORMAT, info->device, NV_CORE_OFFSET_ATTRIBUTE, perf_level); - const char *exec_args_core[] = { "/usr/bin/nvidia-settings", "-q", arg, "-t", NULL }; - if (run_external_process(exec_args_core, buf, -1) != 0) { - LOG_ERROR("Failed to get %s!\n", arg); + if ((attr = get_nv_attr(arg)) == NULL) { return -1; } - info->nv_core = strtol(buf, &end, 10); - if (end == buf) { - LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, buf); + info->nv_core = strtol(attr, &end, 10); + if (end == attr) { + LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, attr); return -1; } /* Get the GPUMemoryTransferRateOffset parameter */ snprintf(arg, - 128, + NV_ARG_MAX, NV_ATTRIBUTE_FORMAT NV_PERF_LEVEL_FORMAT, info->device, NV_MEM_OFFSET_ATTRIBUTE, perf_level); - const char *exec_args_mem[] = { "/usr/bin/nvidia-settings", "-q", arg, "-t", NULL }; - if (run_external_process(exec_args_mem, buf, -1) != 0) { - LOG_ERROR("Failed to get %s!\n", arg); + if ((attr = get_nv_attr(arg)) == NULL) { return -1; } - info->nv_mem = strtol(buf, &end, 10); - if (end == buf) { - LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, buf); + info->nv_mem = strtol(attr, &end, 10); + if (end == attr) { + LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, attr); return -1; } /* Get the GPUPowerMizerMode parameter */ - snprintf(arg, 128, NV_ATTRIBUTE_FORMAT, info->device, NV_POWERMIZER_MODE_ATTRIBUTE); - const char *exec_args_pm[] = { "/usr/bin/nvidia-settings", "-q", arg, "-t", NULL }; - if (run_external_process(exec_args_pm, buf, -1) != 0) { - LOG_ERROR("Failed to get %s!\n", arg); + snprintf(arg, NV_ARG_MAX, NV_ATTRIBUTE_FORMAT, info->device, NV_POWERMIZER_MODE_ATTRIBUTE); + if ((attr = get_nv_attr(arg)) == NULL) { return -1; } - info->nv_powermizer_mode = strtol(buf, &end, 10); - if (end == buf) { - LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, buf); + info->nv_powermizer_mode = strtol(attr, &end, 10); + if (end == attr) { + LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, attr); return -1; } @@ -218,20 +237,18 @@ static int set_gpu_state_nv(struct GameModeGPUInfo *info) long perf_level = get_max_perf_level_nv(info); - char arg[128] = { 0 }; + char arg[NV_ARG_MAX] = { 0 }; /* Set the GPUGraphicsClockOffset parameter */ if (info->nv_core != -1) { snprintf(arg, - 128, + NV_ARG_MAX, NV_ATTRIBUTE_FORMAT NV_PERF_LEVEL_FORMAT "=%ld", info->device, NV_CORE_OFFSET_ATTRIBUTE, perf_level, info->nv_core); - const char *exec_args_core[] = { "/usr/bin/nvidia-settings", "-a", arg, NULL }; - if (run_external_process(exec_args_core, NULL, -1) != 0) { - LOG_ERROR("Failed to set %s!\n", arg); + if (set_nv_attr(arg) != 0) { status = -1; } } @@ -239,15 +256,13 @@ static int set_gpu_state_nv(struct GameModeGPUInfo *info) /* Set the GPUMemoryTransferRateOffset parameter */ if (info->nv_mem != -1) { snprintf(arg, - 128, + NV_ARG_MAX, NV_ATTRIBUTE_FORMAT NV_PERF_LEVEL_FORMAT "=%ld", info->device, NV_MEM_OFFSET_ATTRIBUTE, perf_level, info->nv_mem); - const char *exec_args_mem[] = { "/usr/bin/nvidia-settings", "-a", arg, NULL }; - if (run_external_process(exec_args_mem, NULL, -1) != 0) { - LOG_ERROR("Failed to set %s!\n", arg); + if (set_nv_attr(arg) != 0) { status = -1; } } @@ -255,14 +270,12 @@ static int set_gpu_state_nv(struct GameModeGPUInfo *info) /* Set the GPUPowerMizerMode parameter if requested */ if (info->nv_powermizer_mode != -1) { snprintf(arg, - 128, + NV_ARG_MAX, NV_ATTRIBUTE_FORMAT "=%ld", info->device, NV_POWERMIZER_MODE_ATTRIBUTE, info->nv_powermizer_mode); - const char *exec_args_pm[] = { "/usr/bin/nvidia-settings", "-a", arg, NULL }; - if (run_external_process(exec_args_pm, NULL, -1) != 0) { - LOG_ERROR("Failed to set %s!\n", arg); + if (set_nv_attr(arg) != 0) { status = -1; } } @@ -280,8 +293,8 @@ static int get_gpu_state_amd(struct GameModeGPUInfo *info) return -1; /* Get the contents of power_dpm_force_performance_level */ - char path[64]; - snprintf(path, 64, AMD_DRM_PATH, info->device, "power_dpm_force_performance_level"); + char path[PATH_MAX]; + snprintf(path, PATH_MAX, AMD_DRM_PATH, info->device, "power_dpm_force_performance_level"); FILE *file = fopen(path, "r"); if (!file) { @@ -317,8 +330,8 @@ static int get_gpu_state_amd(struct GameModeGPUInfo *info) */ static int set_gpu_state_amd_file(const char *filename, long device, const char *value) { - char path[64]; - snprintf(path, 64, AMD_DRM_PATH, device, filename); + char path[PATH_MAX]; + snprintf(path, PATH_MAX, AMD_DRM_PATH, device, filename); FILE *file = fopen(path, "w"); if (!file) { @@ -398,10 +411,11 @@ static long get_generic_value(const char *val) */ int main(int argc, char *argv[]) { + struct GameModeGPUInfo info; + memset(&info, 0, sizeof(info)); + if (argc == 3 && strncmp(argv[2], "get", 3) == 0) { /* Get and verify the vendor and device */ - struct GameModeGPUInfo info; - memset(&info, 0, sizeof(info)); info.device = get_device(argv[1]); info.vendor = gamemode_get_gpu_vendor(info.device); @@ -428,8 +442,6 @@ int main(int argc, char *argv[]) } else if (argc >= 4 && argc <= 7 && strncmp(argv[2], "set", 3) == 0) { /* Get and verify the vendor and device */ - struct GameModeGPUInfo info; - memset(&info, 0, sizeof(info)); info.device = get_device(argv[1]); info.vendor = gamemode_get_gpu_vendor(info.device); From 78c2ced7d746a1c653eb17ca3017b8c0124fab6d Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Thu, 4 Apr 2019 11:39:17 +0200 Subject: [PATCH 4/4] external-helper: improve run_external_process() robustness run_external_process() contained pipe file descriptors leaks (e.g. one pipe end was never closed). Also the stdout might have been captured incomplete, since only a single read() was performed on the pipe. Furthermore should a child process try to write a larger amount of data onto the pipe then it will become stuck, because the parent process isn't consuming the data. Thus the timeout would trigger in these cases although the child process does nothing wrong. This commit changes the implementation to follow a select() based approach that continually reads from the pipe, but discards data that doesn't fit in the provided buffer. --- daemon/external-helper.c | 136 ++++++++++++++++++++++++++------------- daemon/gpuclockctl.c | 5 +- 2 files changed, 93 insertions(+), 48 deletions(-) diff --git a/daemon/external-helper.c b/daemon/external-helper.c index 52361f9..62cf29a 100644 --- a/daemon/external-helper.c +++ b/daemon/external-helper.c @@ -36,11 +36,75 @@ POSSIBILITY OF SUCH DAMAGE. #include #include +#include +#include #include -#include #include -static const int default_timeout = 5; +static const int DEFAULT_TIMEOUT = 5; + +static int read_child_stdout(int pipe_fd, char buffer[EXTERNAL_BUFFER_MAX], int tsec) +{ + fd_set fds; + struct timeval timeout; + int num_readable = 0; + ssize_t buffer_bytes_read = 0; + ssize_t just_read = 0; + bool buffer_full = false; + char discard_buffer[EXTERNAL_BUFFER_MAX]; + + /* Set up the timout */ + timeout.tv_sec = tsec; + timeout.tv_usec = 0; + + FD_ZERO(&fds); + + /* Wait for the child to finish up with a timout */ + while (true) { + FD_SET(pipe_fd, &fds); + num_readable = select(pipe_fd + 1, &fds, NULL, NULL, &timeout); + + if (num_readable < 0) { + if (errno == EINTR) { + continue; + } else { + LOG_ERROR("sigtimedwait failed: %s\n", strerror(errno)); + return -1; + } + } else if (num_readable == 0) { + return -2; + } + + if (!buffer_full) { + just_read = read(pipe_fd, + buffer + buffer_bytes_read, + EXTERNAL_BUFFER_MAX - (size_t)buffer_bytes_read - 1); + } else { + just_read = read(pipe_fd, discard_buffer, EXTERNAL_BUFFER_MAX - 1); + } + + if (just_read < 0) { + return -1; + } else if (just_read == 0) { + // EOF encountered + break; + } + + if (!buffer_full) { + buffer_bytes_read += just_read; + + if (buffer_bytes_read == EXTERNAL_BUFFER_MAX - 1) { + // our buffer is exhausted, discard the rest + // of the output + buffer_full = true; + } + } + } + + buffer[buffer_bytes_read] = 0; + + return 0; +} /** * Call an external process @@ -50,6 +114,7 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF pid_t p; int status = 0; int pipes[2]; + int ret = 0; char internal[EXTERNAL_BUFFER_MAX] = { 0 }; if (pipe(pipes) == -1) { @@ -59,20 +124,12 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF /* Set the default timeout */ if (tsec == -1) { - tsec = default_timeout; - } - - /* set up our signaling for the child and the timout */ - sigset_t mask; - sigset_t omask; - sigemptyset(&mask); - sigaddset(&mask, SIGCHLD); - if (sigprocmask(SIG_BLOCK, &mask, &omask) < 0) { - LOG_ERROR("sigprocmask failed: %s\n", strerror(errno)); - return -1; + tsec = DEFAULT_TIMEOUT; } if ((p = fork()) < 0) { + close(pipes[0]); + close(pipes[1]); LOG_ERROR("Failed to fork(): %s\n", strerror(errno)); return false; } else if (p == 0) { @@ -91,38 +148,30 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF LOG_ERROR("Failed to execute external process: %s %s\n", exec_args[0], strerror(errno)); exit(EXIT_FAILURE); } - _exit(EXIT_SUCCESS); - } - - /* Set up the timout */ - struct timespec timeout; - timeout.tv_sec = tsec; - timeout.tv_nsec = 0; - - /* Wait for the child to finish up with a timout */ - while (true) { - if (sigtimedwait(&mask, NULL, &timeout) < 0) { - if (errno == EINTR) { - continue; - } else if (errno == EAGAIN) { - LOG_ERROR("Child process timed out for %s, killing and returning\n", exec_args[0]); - kill(p, SIGKILL); - } else { - LOG_ERROR("sigtimedwait failed: %s\n", strerror(errno)); - return -1; - } - } - break; + // should never be reached + abort(); } + // close the write end of the pipe so we get signaled EOF once the + // child exits close(pipes[1]); - ssize_t output_size = read(pipes[0], internal, EXTERNAL_BUFFER_MAX - 1); - if (output_size < 0) { - LOG_ERROR("Failed to read from process %s: %s\n", exec_args[0], strerror(errno)); - return -1; - } + ret = read_child_stdout(pipes[0], internal, tsec); + close(pipes[0]); - internal[output_size] = 0; + if (ret != 0) { + if (ret == -2) { + LOG_ERROR("Child process timed out for %s, killing and returning\n", exec_args[0]); + kill(p, SIGKILL); + } else { + LOG_ERROR("Failed to read from process %s: %s\n", exec_args[0], strerror(errno)); + } + if (buffer) { + // make sure the buffer is a terminated empty string on error + buffer[0] = 0; + } + } else if (buffer) { + memcpy(buffer, internal, EXTERNAL_BUFFER_MAX); + } if (waitpid(p, &status, 0) < 0) { LOG_ERROR("Failed to waitpid(%d): %s\n", (int)p, strerror(errno)); @@ -134,12 +183,9 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF LOG_ERROR("Child process '%s' exited abnormally\n", exec_args[0]); } else if (WEXITSTATUS(status) != 0) { LOG_ERROR("External process failed with exit code %d\n", WEXITSTATUS(status)); - LOG_ERROR("Output was: %s\n", buffer ? buffer : internal); + LOG_ERROR("Output was: %s\n", internal); return -1; } - if (buffer) - memcpy(buffer, internal, EXTERNAL_BUFFER_MAX); - return 0; } diff --git a/daemon/gpuclockctl.c b/daemon/gpuclockctl.c index cac49bd..850bd05 100644 --- a/daemon/gpuclockctl.c +++ b/daemon/gpuclockctl.c @@ -67,7 +67,7 @@ static void print_usage_and_exit(void) exit(EXIT_FAILURE); } -static const char* get_nv_attr(const char *attr) +static const char *get_nv_attr(const char *attr) { static char out[EXTERNAL_BUFFER_MAX]; const char *exec_args[] = { "/usr/bin/nvidia-settings", "-q", attr, "-t", NULL }; @@ -315,8 +315,7 @@ static int get_gpu_state_amd(struct GameModeGPUInfo *info) ret = -1; } - if( ret == 0 ) - { + if (ret == 0) { /* Copy in the value from the file */ strncpy(info->amd_performance_level, buff, CONFIG_VALUE_MAX - 1); info->amd_performance_level[CONFIG_VALUE_MAX - 1] = '\0';