From f24c7c359ea486438ecadf1496e7c9a77b132a0c Mon Sep 17 00:00:00 2001 From: Justin Gottula Date: Tue, 29 Jun 2021 21:29:09 -0700 Subject: [PATCH] Use substantially more robust program exit status logic in zvol_id Currently, there are several places in zvol_id where the program logic returns particular errno values, or even particular ioctl return values, as the program exit status, rather than a straightforward system of explicit zero on success and explicit nonzero value(s) on failure. This is problematic for multiple reasons. One particularly interesting problem that can arise, is that if any of these values happens to have all 8 least significant bits unset (i.e., it is a positive or negative multiple of 256), then although the C program sees a nonzero int value (presumed to be a failure exit status), the actual exit status as seen by the system is only the bottom 8 bits of that integer: zero. This can happen in practice, and I have encountered it myself. In a particularly weird situation, the zvol_open code in the zfs kernel module was behaving in such a manner that it caused the open() syscall to fail and for errno to be set to a kernel-private value (ERESTARTSYS, which happens to be defined as 512). It turns out that 512 is evenly divisible by 256; or, in other words, its least significant 8 bits are all-zero. So even though zvol_id believed it was returning a nonzero (failure) exit status of 512, the system modulo'd that value by 256, resulting in the actual exit status visible by other programs being 0! This actually-zero (non-failure) exit status caused problems: udev believed that the program was operating successfully, when in fact it was attempting to indicate failure via a nonzero exit status integer. Combined with another problem, this led to the creation of nonsense symlinks for zvol dev nodes by udev. Let's get rid of all this problematic logic, and simply return EXIT_SUCCESS (0) is everything went fine, and EXIT_FAILURE (1) if anything went wrong. Additionally, let's clarify some of the variable names (error is similar to errno, etc) and clean up the overall program flow a bit. Reviewed-by: Brian Behlendorf Reviewed-by: Pavel Zakharov Signed-off-by: Justin Gottula Closes #12302 --- cmd/zvol_id/zvol_id_main.c | 50 ++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/cmd/zvol_id/zvol_id_main.c b/cmd/zvol_id/zvol_id_main.c index 7e01028562bf..22f2e848cba1 100644 --- a/cmd/zvol_id/zvol_id_main.c +++ b/cmd/zvol_id/zvol_id_main.c @@ -38,40 +38,39 @@ static int ioctl_get_msg(char *var, int fd) { - int error = 0; + int ret; char msg[ZFS_MAX_DATASET_NAME_LEN]; - error = ioctl(fd, BLKZNAME, msg); - if (error < 0) { - return (error); + ret = ioctl(fd, BLKZNAME, msg); + if (ret < 0) { + return (ret); } snprintf(var, ZFS_MAX_DATASET_NAME_LEN, "%s", msg); - return (error); + return (ret); } int main(int argc, char **argv) { - int fd, error = 0; + int fd = -1, ret = 0, status = EXIT_FAILURE; char zvol_name[ZFS_MAX_DATASET_NAME_LEN]; char *zvol_name_part = NULL; char *dev_name; struct stat64 statbuf; int dev_minor, dev_part; int i; - int rc; if (argc < 2) { fprintf(stderr, "Usage: %s /dev/zvol_device_node\n", argv[0]); - return (EINVAL); + goto fail; } dev_name = argv[1]; - error = stat64(dev_name, &statbuf); - if (error != 0) { + ret = stat64(dev_name, &statbuf); + if (ret != 0) { fprintf(stderr, "Unable to access device file: %s\n", dev_name); - return (errno); + goto fail; } dev_minor = minor(statbuf.st_rdev); @@ -80,22 +79,22 @@ main(int argc, char **argv) fd = open(dev_name, O_RDONLY); if (fd < 0) { fprintf(stderr, "Unable to open device file: %s\n", dev_name); - return (errno); + goto fail; } - error = ioctl_get_msg(zvol_name, fd); - if (error < 0) { + ret = ioctl_get_msg(zvol_name, fd); + if (ret < 0) { fprintf(stderr, "ioctl_get_msg failed: %s\n", strerror(errno)); - return (errno); + goto fail; } if (dev_part > 0) - rc = asprintf(&zvol_name_part, "%s-part%d", zvol_name, + ret = asprintf(&zvol_name_part, "%s-part%d", zvol_name, dev_part); else - rc = asprintf(&zvol_name_part, "%s", zvol_name); + ret = asprintf(&zvol_name_part, "%s", zvol_name); - if (rc == -1 || zvol_name_part == NULL) - goto error; + if (ret == -1 || zvol_name_part == NULL) + goto fail; for (i = 0; i < strlen(zvol_name_part); i++) { if (isblank(zvol_name_part[i])) @@ -103,8 +102,13 @@ main(int argc, char **argv) } printf("%s\n", zvol_name_part); - free(zvol_name_part); -error: - close(fd); - return (error); + status = EXIT_SUCCESS; + +fail: + if (zvol_name_part) + free(zvol_name_part); + if (fd >= 0) + close(fd); + + return (status); }