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

Confile: Add lxc.sysctl config #2009

Merged
merged 1 commit into from Dec 11, 2017
Merged

Confile: Add lxc.sysctl config #2009

merged 1 commit into from Dec 11, 2017

Conversation

lifeng68
Copy link
Contributor

@lifeng68 lifeng68 commented Dec 7, 2017

Add lxc.sysctl config.
It can be useful to configure kernel parameters for the container.
Usage:
For e.g.

lxc.sysctl.net.ipv4.ip_forward = 1
lxc.sysctl.net.core.somaxconn = 256

Signed-off-by: LiFeng lifeng68@huawei.com

@lxc-jenkins
Copy link

This pull request didn't trigger Jenkins as its author isn't in the whitelist.

An organization member must perform one of the following:

  • To have this branch tested by Jenkins, use the "ok to test" command.
  • To have a one time test done, use the "test this please" command.

Those commands are simple Github comments of the format: "jenkins: COMMAND"

Copy link
Member

@brauner brauner left a comment

Choose a reason for hiding this comment

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

I like the idea of a generic lxc.sysctl.*. Thanks! I left a few comments in the code mostly related to coding style.

A few points though:

  1. whitelist
    I'd prefer however if we started with a whitelist of keys that we wan't the container to be able to set. Making all keys available sounds like a bad idea to me in case something gets added that we definitely don't want users to change or that is already in there that we don't want users to change.

  2. tests
    The new config key should get some tests in src/tests/getkeys.c and src/tests/parse_config_file.c. Doesn't need to be anything fancy. I just want to make sure that they basically work.

  3. maintainer feedback
    I'd like @stgraber's and @hallyn's opinion on this.

src/lxc/conf.c Outdated
struct lxc_list *it;
struct lxc_sysctl *elem;
char *tmp = NULL;
char filename[PATH_MAX] = {0};
Copy link
Member

Choose a reason for hiding this comment

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

This should be MAXPATHLEN which we define internally.

src/lxc/conf.c Outdated
struct lxc_sysctl *elem;
char *tmp = NULL;
char filename[PATH_MAX] = {0};
int r = 0;
Copy link
Member

Choose a reason for hiding this comment

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

nit: We use ret all over the codebase would be nice if you could change this to use ret as variable name. :)

src/lxc/conf.c Outdated
return -1;
}

snprintf(filename, sizeof(filename), "/proc/sys/%s", tmp);
Copy link
Member

Choose a reason for hiding this comment

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

Please use:

ret = snprintf()
if (ret < 0 || (size_t)ret >= sizeof(filename))
        /* handle error */

here just for good practice and because snprintf() can also fail with EIO.

@@ -3315,6 +3344,32 @@ int lxc_clear_limits(struct lxc_conf *c, const char *key)
return 0;
}

int lxc_clear_sysctls(struct lxc_conf *c, const char *key)
Copy link
Member

Choose a reason for hiding this comment

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

This function also needs to be called in conf.c:lxc_conf_free() when the config is deallocated otherwise you're leaking memory.

free(elem->value);
free(elem);
free(it);
}
Copy link
Member

Choose a reason for hiding this comment

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

We should probably return an error in case the requested specific key (e.g. ipv4_forward) wasn't found. Currently it always reports back that it succeeded to clear that value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need to do that, because our goal is to clean up a specific configuration, and now, it has reached its goal.If we return an error, the upper layer will be puzzled.

else
memset(retv, 0, inlen);

if (!strcmp(key, "lxc.sysctl"))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please use if (strcmp() != 0). (It's a coding convention @hallyn and I recently agreed upon.)


if (!strcmp(key, "lxc.sysctl"))
get_all = true;
else if (strncmp(key, "lxc.sysctl.", strlen("lxc.sysctl.")) == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Make this strlen("lxc.sysctl.") a sizeof("lxc.sysctl.") - 1 since the string size is not variable here.

if (!strcmp(key, "lxc.sysctl"))
get_all = true;
else if (strncmp(key, "lxc.sysctl.", strlen("lxc.sysctl.")) == 0)
key += strlen("lxc.sysctl.");
Copy link
Member

Choose a reason for hiding this comment

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

Make this strlen("lxc.sysctl.") a sizeof("lxc.sysctl.") - 1 since the string size is not variable here.

if (get_all) {
strprint(retv, inlen, "lxc.sysctl.%s = %s\n",
elem->key, elem->value);
} else if (!strcmp(elem->key, key)) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please use if (strcmp() != 0). (It's a coding convention @hallyn and I recently agreed upon.)

src/lxc/start.c Outdated
* For e.g. net.ipv4.ip_forward translated to /proc/sys/net/ipv4/ip_forward.
*/
if (!lxc_list_empty(&handler->conf->sysctls) && setup_sysctl_parameters(&handler->conf->sysctls))
goto out_warn_father;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'm trying to get rid of using non-boolean function calls directly in if-conditions since it's clearer that way. So In this case I'd prefer:

if (!lxc_list_empty(&handler->conf->sysctls) {
        ret = setup_sysctl_parameters(&handler->conf->sysctls);
        if (ret < 0)
                goto out_warn_father;
}

@brauner
Copy link
Member

brauner commented Dec 7, 2017

Ah, one final nit: The commit message of your first commit please 's/Confile:/confile:/`.

@brauner brauner added the Incomplete Waiting on more information from reporter label Dec 7, 2017
@brauner
Copy link
Member

brauner commented Dec 7, 2017

Also, if we go for the whitelist idea we should come up with - suprise - whitelist.

@lifeng68
Copy link
Contributor Author

lifeng68 commented Dec 8, 2017

@brauner ,
whitelist
Are you worried about that users shooting them selves in the foot, by attempting to set a sysctl that would effect not only one particular container, but the entire system? I think we can use strategy like runc runc PR. How do you feel about that?

@stgraber
Copy link
Member

stgraber commented Dec 8, 2017

Thanks, I like this!

I also don't think we need a whitelist for it. LXC's never prevented a user to shoot themselves in the foot and there's no way for the user inside the container to cause LXC outside of it to change arbitrary sysctls for them, so there's no security concern there.

We should certainly leave a note in the manpage that not all sysctls are namespaced and that changing one which isn't will cause the system-wide setting to be modified, but that's about as much as we can do since there's no way to know from userspace which sysctl is tied to a namespace and which isn't.

I've not looked at the code very closely but one thing we should make sure is that we change those sysctls before dropping privileges (in the case of a user namespace) as some sysctls are properly namespaced (thinking of the IPC ones for example) but don't allow an unprivileged root user to change them. This mechanism would allow for the administrator to set those up to the right values so long as real root is the one spawning the container (as is the case with LXD for example).

src/lxc/start.c Outdated
/* set sysctl value to a path under /proc/sys as determined from the key.
* For e.g. net.ipv4.ip_forward translated to /proc/sys/net/ipv4/ip_forward.
*/
if (!lxc_list_empty(&handler->conf->sysctls)) {
Copy link
Member

Choose a reason for hiding this comment

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

Right, so this need to move into conf.c:lxc_setup() right before we drop caps after setup personality so that we still have CAP_SYS_RESOURCE and friends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing out the error. @brauner @stgraber

@brauner
Copy link
Member

brauner commented Dec 8, 2017

We should certainly leave a note in the manpage that not all sysctls are namespaced and that

Non-namespaced keys was my only concern

changing one which isn't will cause the system-wide setting to be modified, but that's about as much as we can do since there's no way to know from userspace which sysctl is tied to a namespace and which isn't.

Unless you look at the kernel sources and compile the list from there. The problem rather would be that you don't know if the kernel you're running properly namespaced a given key.
.

@stgraber
Copy link
Member

stgraber commented Dec 8, 2017

Yeah and looking at the kernel source won't do you any good for any of the distro kernels + you'd need a per-version map, so it's way too crazy to be doable :)

The runc approach may be fine if they only care about a very limited set of keys, but I know for a fact that LXC users will jump on the lxc.sysctl.net.* keys which are notoriously messy in this regard. There are hundreds of keys, some of which are namespaced some of which aren't, some disappear on some kernel versions, some only work with using a userns, it's a giant mess and trying to keep track of all of that in LXC would be crazy :)

@brauner
Copy link
Member

brauner commented Dec 8, 2017

jenkins: test this please

@lifeng68
Copy link
Contributor Author

lifeng68 commented Dec 8, 2017

@brauner
I fix the issue at

@@ -3294,7 +3336,7 @@ int lxc_clear_limits(struct lxc_conf *c, const char *key)
        const char *k = NULL;
 
        if (strcmp(key, "lxc.limit") == 0
-           || strcmp(key, "lxc.prlimit"))
+           || strcmp(key, "lxc.prlimit") == 0)
                all = true;

so the get_item.c can test cases,otherwise the test willgoto out and return 0

--- a/src/tests/get_item.c
+++ b/src/tests/get_item.c
@@ -220,29 +220,108 @@ int main(int argc, char *argv[])
        ret = c->get_config_item(c, "lxc.limit", v3, 2047);
        if (ret != sizeof(ALL_LIMITS)-1) {
                fprintf(stderr, "%d: get_config_item(limit) returned %d\n", __LINE__, ret);
+               ret = -1;
                goto out;
        }
        if (strcmp(v3, ALL_LIMITS)) {
                fprintf(stderr, "%d: lxc.limit returned wrong value: %d %s not %d %s\n", __LINE__, ret, v3, (int)sizeof(ALL_LIMITS)-1, ALL_LIMITS);
+               ret = -1;
                goto out;
        }
        printf("lxc.limit returned %d %s\n", ret, v3);
 
        if (!c->clear_config_item(c, "lxc.limit.nofile")) {
                fprintf(stderr, "%d: failed clearing limit.nofile\n", __LINE__);
+               ret = -1;
                goto out;
        }
        ret = c->get_config_item(c, "lxc.limit", v3, 2047);
        if (ret != sizeof(LIMIT_STACK)-1) {
                fprintf(stderr, "%d: get_config_item(limit) returned %d\n", __LINE__, ret);
+               ret = -1;
                goto out; 
/*In the past, because of the issue at lxc_clear_limits,` clear_config_item(c, "lxc.limit.nofile")` will 
clear all the limit configures and `get_config_item(c, "lxc.limit", v3, 2047)` will 
return 0,  then it will` goto out and exit(0)`, so the next cases can not get executing*/
        } 

Now, the test suite failed at network test cases,

lxc.aa_profile returned 10 unconfined
417: after clearing ipv4 entries get_item(lxc.network.0.ipv4 returned -1
get_item.c: 60: main: Retrieving value for lxc.syslog correctly returned: local0.
get_item.c: 67: main: Successfully failed to set lxc.syslog to invalid value.

I check the test code, but since I'm not familiar with this code, i do not understand with the test code. How to fix it,can you help me?

	ret = c->get_config_item(c, "lxc.network.0.ipv4", v2, 255);
	if (ret <= 0) {
		fprintf(stderr, "%d: lxc.network.0.ipv4 returned %d\n", __LINE__, ret);
		goto out;
	}
	if (!c->clear_config_item(c, "lxc.network.0.ipv4")) {
		fprintf(stderr, "%d: failed clearing all ipv4 entries\n", __LINE__);
		goto out;
	}
	ret = c->get_config_item(c, "lxc.network.0.ipv4", v2, 255);
	if (ret != 0) {
		fprintf(stderr, "%d: after clearing ipv4 entries get_item(lxc.network.0.ipv4 returned %d\n", __LINE__, ret);
		goto out;
	} 

@brauner
Copy link
Member

brauner commented Dec 8, 2017

@lifeng68, this is unrelated to your branch. Please remove the get_item.c test and I'll merge. I'll send a branch tomorrow or Sunday that fixes the other issue.

brauner added a commit to brauner/lxc that referenced this pull request Dec 9, 2017
This fixes a bug introduced by:

commit 94f0035
Author: Christian Brauner <christian.brauner@ubuntu.com>
Date:   Thu Dec 7 15:07:26 2017 +0100

    coverity: #1425924

    remove logically dead condition

    Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Coverity's bug analysis is correct but my fix wasn't.

This commit fixes a bunch of other bugs I just spotted as well.

This unblocks lxc#2009.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
brauner added a commit to brauner/lxc that referenced this pull request Dec 9, 2017
This fixes a bug introduced by:

commit 94f0035
Author: Christian Brauner <christian.brauner@ubuntu.com>
Date:   Thu Dec 7 15:07:26 2017 +0100

    coverity: #1425924

    remove logically dead condition

    Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Coverity's bug analysis is correct but my fix wasn't.

This commit fixes a bunch of other bugs I just spotted as well.

This unblocks lxc#2009.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
@brauner brauner mentioned this pull request Dec 9, 2017
@brauner
Copy link
Member

brauner commented Dec 9, 2017

@lifeng68, I fixed the bug that is making your tests fail. So leave the test in there. I'm just going to request a few little changes in there. Sorry for the delay.

Copy link
Member

@brauner brauner left a comment

Choose a reason for hiding this comment

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

Right, the way we currently fail if there is an error in tests:get_item.c is error prone. That's why I asked you to remove the ret = -1 in the error conditions everywhere. Instead, please do


diff --git a/src/tests/get_item.c b/src/tests/get_item.c
index e0fd7b60..02b02eb9 100644
--- a/src/tests/get_item.c
+++ b/src/tests/get_item.c
@@ -34,8 +34,9 @@

 int main(int argc, char *argv[])
 {
-       int ret = EXIT_FAILURE;
+       int ret;
        struct lxc_container *c;
+       int fret = EXIT_FAILURE;
        char v1[2], v2[256], v3[2048];

        if ((c = lxc_container_new("testxyz", NULL)) == NULL) {
@@ -424,9 +425,9 @@ int main(int argc, char *argv[])
        }

        printf("All get_item tests passed\n");
-       ret = EXIT_SUCCESS;
+       fret = EXIT_SUCCESS;
 out:
        c->destroy(c);
        lxc_container_put(c);
-       exit(ret);
+       exit(fret);
 }

which defines a new variable used for exit only which doesn't get touched by any of the intermediate function calls.

@@ -220,29 +220,108 @@ int main(int argc, char *argv[])
ret = c->get_config_item(c, "lxc.limit", v3, 2047);
if (ret != sizeof(ALL_LIMITS)-1) {
fprintf(stderr, "%d: get_config_item(limit) returned %d\n", __LINE__, ret);
ret = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove.

goto out;
}
if (strcmp(v3, ALL_LIMITS)) {
fprintf(stderr, "%d: lxc.limit returned wrong value: %d %s not %d %s\n", __LINE__, ret, v3, (int)sizeof(ALL_LIMITS)-1, ALL_LIMITS);
ret = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove.

goto out;
}
printf("lxc.limit returned %d %s\n", ret, v3);

if (!c->clear_config_item(c, "lxc.limit.nofile")) {
fprintf(stderr, "%d: failed clearing limit.nofile\n", __LINE__);
ret = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove.

goto out;
}
ret = c->get_config_item(c, "lxc.limit", v3, 2047);
if (ret != sizeof(LIMIT_STACK)-1) {
fprintf(stderr, "%d: get_config_item(limit) returned %d\n", __LINE__, ret);
ret = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove.

goto out;
}
if (strcmp(v3, LIMIT_STACK)) {
fprintf(stderr, "%d: lxc.limit returned wrong value: %d %s not %d %s\n", __LINE__, ret, v3, (int)sizeof(LIMIT_STACK)-1, LIMIT_STACK);
ret = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove.

ret = c->get_config_item(c, "lxc.sysctl", v3, 2047);
if (ret != sizeof(ALL_SYSCTLS)-1) {
fprintf(stderr, "%d: get_config_item(sysctl) returned %d\n", __LINE__, ret);
ret = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove ret = -1;

}
if (strcmp(v3, ALL_SYSCTLS)) {
fprintf(stderr, "%d: lxc.sysctl returned wrong value: %d %s not %d %s\n", __LINE__, ret, v3, (int)sizeof(ALL_SYSCTLS) - 1, ALL_SYSCTLS);
ret = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove ret = -1;


if (!c->clear_config_item(c, "lxc.sysctl.net.ipv4.ip_forward")) {
fprintf(stderr, "%d: failed clearing lxc.sysctl.net.ipv4.ip_forward\n", __LINE__);
ret = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove ret = -1;

ret = c->get_config_item(c, "lxc.sysctl", v3, 2047);
if (ret != sizeof(SYSCTL_SOMAXCONN) - 1) {
fprintf(stderr, "%d: get_config_item(sysctl) returned %d\n", __LINE__, ret);
ret = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove ret = -1;

}
if (strcmp(v3, SYSCTL_SOMAXCONN)) {
fprintf(stderr, "%d: lxc.sysctl returned wrong value: %d %s not %d %s\n", __LINE__, ret, v3, (int)sizeof(SYSCTL_SOMAXCONN) - 1, SYSCTL_SOMAXCONN);
ret = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove ret = -1;

brauner added a commit to brauner/lxc that referenced this pull request Dec 9, 2017
This fixes a bug introduced by:

commit 94f0035
Author: Christian Brauner <christian.brauner@ubuntu.com>
Date:   Thu Dec 7 15:07:26 2017 +0100

    coverity: #1425924

    remove logically dead condition

    Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Coverity's bug analysis is correct but my fix wasn't.

This commit fixes a bunch of other bugs I just spotted as well.

This unblocks lxc#2009.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
@lifeng68
Copy link
Contributor Author

lifeng68 commented Dec 11, 2017

@brauner
Fixed the get_item.c, please re-trigger test after 6b269cc has been merged

brauner added a commit to brauner/lxc that referenced this pull request Dec 11, 2017
This fixes a bug introduced by:

commit 94f0035
Author: Christian Brauner <christian.brauner@ubuntu.com>
Date:   Thu Dec 7 15:07:26 2017 +0100

    coverity: #1425924

    remove logically dead condition

    Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Coverity's bug analysis is correct but my fix wasn't.

This commit fixes a bunch of other bugs I just spotted as well.

This unblocks lxc#2009.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
@brauner
Copy link
Member

brauner commented Dec 11, 2017

@lifeng68, thanks!

@brauner
Copy link
Member

brauner commented Dec 11, 2017

jenkins: test this please

1 similar comment
@brauner
Copy link
Member

brauner commented Dec 11, 2017

jenkins: test this please

Copy link
Member

@brauner brauner left a comment

Choose a reason for hiding this comment

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

Very pleased with this patch now. Thanks!

<para>
Specify the kernel parameters to be set. The parameters available
are those listed under /proc/sys/.
Note that not all sysctls are namespaced.Changing Non-namespaced
Copy link
Member

Choose a reason for hiding this comment

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

Missing space. :)

Copy link
Member

Choose a reason for hiding this comment

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

I'll just push another commit to fix this. No need to resend.

@@ -3293,8 +3335,7 @@ int lxc_clear_limits(struct lxc_conf *c, const char *key)
bool all = false;
const char *k = NULL;

if (strcmp(key, "lxc.limit") == 0
|| strcmp(key, "lxc.prlimit"))
if (strcmp(key, "lxc.limit") == 0 || strcmp(key, "lxc.prlimit") == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, you saw that bug too. :) Funny, I sent a branch this morning that fixes this as well. :)

@brauner brauner merged commit ed20740 into lxc:master Dec 11, 2017
Signed-off-by: LiFeng <lifeng68@huawei.com>
brauner added a commit that referenced this pull request Dec 15, 2017
This fixes a bug introduced by:

commit 94f0035
Author: Christian Brauner <christian.brauner@ubuntu.com>
Date:   Thu Dec 7 15:07:26 2017 +0100

    coverity: #1425924

    remove logically dead condition

    Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Coverity's bug analysis is correct but my fix wasn't.

This commit fixes a bunch of other bugs I just spotted as well.

This unblocks #2009.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
geaaru pushed a commit to geaaru/lxc that referenced this pull request Jul 4, 2018
This fixes a bug introduced by:

commit 94f0035
Author: Christian Brauner <christian.brauner@ubuntu.com>
Date:   Thu Dec 7 15:07:26 2017 +0100

    coverity: #1425924

    remove logically dead condition

    Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Coverity's bug analysis is correct but my fix wasn't.

This commit fixes a bunch of other bugs I just spotted as well.

This unblocks lxc#2009.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Incomplete Waiting on more information from reporter
Development

Successfully merging this pull request may close these issues.

None yet

4 participants