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

libsemanage: use mv instead of rename for container compat #342

Closed
wants to merge 1 commit into from
Closed

libsemanage: use mv instead of rename for container compat #342

wants to merge 1 commit into from

Conversation

jmarrero
Copy link

@jmarrero jmarrero commented Mar 8, 2022

Initial good path for: #343 but I am sure it needs input validation and/or error handling.

Signed-off-by: Joseph Marrero <jmarrero@redhat.com>
@cgzones
Copy link
Contributor

cgzones commented Mar 8, 2022

This should probably implemented in C instead of spawning a new shell.

Untested draft:

static int write_full(int fd, const void *data, size_t count)
{
        const unsigned char *p = (const unsigned char *)data;

        while (count > 0) {
                ssize_t n_rw;
                do {
                        n_rw = write(fd, p, count);
                } while (n_rw < 0 && errno == EINTR);
                if (n_rw < 0)
                        return -1;
                if (n_rw == 0)
                        break;
                p += n_rw;
                count -= (size_t)n_rw;
        }

        return 0;
}

static int mv(const char *src, const char *dest)
{
        struct stat oldst;
        char buffer[8192];
        int old = -1, new = -1, r;

        r = rename(src, dest);
        if (r == 0)
                return 0;
        if (errno != EXDEV)
                return -1;

        old = open(src, O_RDONLY | O_CLOEXEC);
        if (old < 0)
                return -1;

        r = fstat(old, &oldst);
        if (r < 0)
                goto err;

        r = unlink(dest);
        if (r < 0 && errno != ENOENT)
                goto err;

        new = open(dest, O_WRONLY | O_CLOEXEC | O_CREAT | O_EXCL, ACCESSPERMS & oldst.st_mode);
        if (new < 0)
                goto err;

        for (;;) {
                ssize_t n;

                n = read(old, buffer, sizeof(buffer));
                if (n == 0)
                        break;
                if (n > 0) {
                        r = write_full(new, buffer, (size_t)n);
                        if (r < 0)
                                goto err;
                        continue;
                }

                if (errno == EINTR)
                        continue;

                goto err;
        }

        close(old);
        r = close(new);
        if (r < 0)
                return -1;

        r = unlink(src);
        if (r < 0)
                return -1;

        return 0;
err:
        if (new >= 0)
                close(new);
        if (old >= 0)
                close(old);
        return -1;
}

@jmarrero
Copy link
Author

jmarrero commented Mar 8, 2022

Thanks @cgzones I will give this a try and report back.

@bachradsusi
Copy link
Member

Before you send it to the review on selinux@vger.kernel.org could you please add provide more information in the commit message about the problem this tries to fix?

Especially about the filesystem layout and why /etc/selinux/targeted/active is on a different filesystem than /etc/selinux/targeted/previous. What would be the impact on the filesystem layout when /etc/selinux/targeted/active is copied into /etc/selinux/targeted/previous and /etc/selinux/targeted/tmp, unlink()'ed and then created again with a copy of /etc/selinux/targeted/tmp?

@WOnder93
Copy link
Member

@bachradsusi They are not on different filesystems - the problem is that overlayfs doesn't support rename(2) even on the same mount in some circumstances:
https://docs.docker.com/storage/storagedriver/overlayfs-driver/#modifying-files-or-directories

Anyway, I don't think @cgzones's solution will work in this case, since we are renaming a directory, not a single file. Also, I believe we expect the rename here to be atomic, so we need to ensure that whatever fallback we use for EXDEV is still atomic (at least to the same extent as rename(2) is).

@bachradsusi
Copy link
Member

thanks @WOnder93 this was the information I was missing.

Btw libsemanage already implements copying directories see https://github.com/SELinuxProject/selinux/blob/master/libsemanage/src/semanage_store.c#L773

@jmarrero
Copy link
Author

jmarrero commented Mar 17, 2022

Sorry for the week delay, went on PTO just after my update here.

@cgzones using your draft I get:

libsemanage.semanage_commit_sandbox: Error while renaming /etc/selinux/targeted/active to /etc/selinux/targeted/previous. (Is a directory).

Which is what @WOnder93 suggested.

As for using @bachradsusi suggestion, I get:

Installing: usbguard-selinux-1.1.0-1.fc35.noarch (updates)
libsemanage.semanage_commit_sandbox: Error while renaming /etc/selinux/targeted/tmp to /etc/selinux/targeted/active. (File exists).
libsemanage.semanage_commit_sandbox: Error while renaming /etc/selinux/targeted/previous back to /etc/selinux/targeted/active. (File exists).
/usr/sbin/semodule:  Failed!

Also I tried a combination of both suggestions by replacing the rename on: https://github.com/SELinuxProject/selinux/blob/master/libsemanage/src/semanage_store.c#L763 with cgzones's mv implementation and using semanage_copy_dir for the other rename calls. But ended in the same error as above.

@bachradsusi
Copy link
Member

@jmarrero Please try the bachradsusi@03db464

It implements fallback to semanage_copy_dir() and semanage_remove_dir() if rename() failed on EXDEV.

Even though it's not an atomic operation, it should happen just once (based on my observation).

If it works for you, we should move this to selinux@vger.kernel.org

@jmarrero
Copy link
Author

It works on my end. Thank you!

@bachradsusi
Copy link
Member

c7a3b93

@bachradsusi bachradsusi closed this Apr 7, 2022
@cgwalters
Copy link
Contributor

cc coreos/layering-examples#16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants