Received-SPF: pass (google.com: domain of linux-kernel+bounces-188889-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Javier Martinez Canillas <javierm@redhat.com>
To: Christian Brauner <brauner@kernel.org>
Cc: linux-kernel@vger.kernel.org, Daniel Lezcano
 <daniel.lezcano@linaro.org>, "Eric W . Biederman" <ebiederm@xmission.com>,
 javier@dowhile0.org, Andrew Morton <akpm@linux-foundation.org>, "Gustavo
 A. R. Silva" <gustavoars@kernel.org>, Masahiro Yamada
 <masahiroy@kernel.org>, Nhat Pham <nphamcs@gmail.com>, Petr Mladek
 <pmladek@suse.com>, Randy Dunlap <rdunlap@infradead.org>, Vincent Guittot
 <vincent.guittot@linaro.org>, Yoann Congal <yoann.congal@smile.fr>
Subject: Re: [PATCH] userns: Default to 'yes' when CONFIG_MEMCG option is
 enabled
In-Reply-To: <20240524-putzen-ablauf-12f514413be6@brauner>
References: <20240524082434.657573-1-javierm@redhat.com>
 <20240524-beurkunden-kantig-101649d6b5cf@brauner>
 <874jangzjk.fsf@minerva.mail-host-address-is-not-set>
 <20240524-putzen-ablauf-12f514413be6@brauner>
Date: Fri, 24 May 2024 17:39:02 +0200
Message-ID: <871q5rgqy1.fsf@minerva.mail-host-address-is-not-set>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Christian Brauner <brauner@kernel.org> writes:

> On Fri, May 24, 2024 at 02:33:19PM +0200, Javier Martinez Canillas wrote:
>> Christian Brauner <brauner@kernel.org> writes:
>>=20
>> Hello Christian,
>>=20
>> Thanks a lot for your feedback.
>>=20
>> > On Fri, May 24, 2024 at 10:24:16AM +0200, Javier Martinez Canillas wro=
te:
>> >> The default value for the CONFIG_USER_NS Kconfig symbol changed over =
time.
>> >>=20
>> >> When first was introduced by commit acce292c82d4 ("user namespace: ad=
d the
>> >> framework"), the default was 'no'. But then it was changed to 'yes' i=
f the
>> >> CONFIG_NAMESPACES option was enabled, by commit 17a6d4411a4d ("namesp=
aces:
>> >> default all the namespaces to 'yes' when CONFIG_NAMESPACES is selecte=
d").
>> >>=20
>> >> Then, commit 5673a94c1457 ("userns: Add a Kconfig option to enforce s=
trict
>> >> kuid and kgid type checks") changed the default to 'no' again and sel=
ected
>> >> the (now defunct) UIDGID_STRICT_TYPE_CHECKS option.
>> >>=20
>> >> This selected option was removed by commit 261000a56b63 ("userns: Rem=
ove
>> >> UIDGID_STRICT_TYPE_CHECKS"), but CONFIG_USER_NS default was left to '=
no'.
>> >>=20
>> >> Finally, the commit e11f0ae388f2 ("userns: Recommend use of memory co=
ntrol
>> >> groups") added to the Kconfig symbol's help text a recommendation tha=
t the
>> >> memory control groups should be used, to limit the amount of memory t=
hat a
>> >> user who can create user namespaces can consume.
>> >>=20
>> >> Looking at the changes' history, a default to 'yes' when the CONFIG_M=
EMCG
>> >> option is enabled seems like a sane thing to do. Specially since syst=
emd
>> >> requires user namespaces support for services that use the PrivateUse=
rs=3D
>> >> property in their unit files (e.g: the UPower daemon).
>> >
>> > Fyi, user namespaces are an entirely optional feature in systemd and it
>> > gracefully falls back if they are not available with PrivateUsers=3D s=
et.
>> > If that isn't the case then it's a bug in systemd with PrivateUsers=3D
>> > handling and should be reported.
>> >
>>=20
>> Interesting, it definitely failed for me:
>>=20
>> $ systemctl status upower
>> =E2=97=8F upower.service - Daemon for power management
>>      Loaded: loaded (/lib/systemd/system/upower.service; disabled; vendo=
r preset: enabled)
>>      Active: failed (Result: exit-code) since Fri 2024-05-24 12:23:49 UT=
C; 34s ago
>>        Docs: man:upowerd(8)
>>     Process: 390 ExecStart=3D/usr/libexec/upowerd (code=3Dexited, status=
=3D217/USER)
>>    Main PID: 390 (code=3Dexited, status=3D217/USER)
>>         CPU: 122ms
>>=20
>> May 24 12:23:49 igep systemd[1]: upower.service: Scheduled restart job, =
restart counter is at 5.
>> May 24 12:23:49 igep systemd[1]: Stopped Daemon for power management.
>> May 24 12:23:49 igep systemd[1]: upower.service: Start request repeated =
too quickly.
>> May 24 12:23:49 igep systemd[1]: upower.service: Failed with result 'exi=
t-code'.
>> May 24 12:23:49 igep systemd[1]: Failed to start Daemon for power manage=
ment.
>>=20
>> $ journalctl -u upower
>> May 24 12:23:49 igep systemd[1]: Starting Daemon for power management...
>> May 24 12:23:49 igep systemd[404]: upower.service: Failed to set up user=
 namespacing: Invalid argument
>> May 24 12:23:49 igep systemd[404]: upower.service: Failed at step USER s=
pawning /usr/libexec/upowerd: Invalid argument
>> May 24 12:23:49 igep systemd[1]: upower.service: Main process exited, co=
de=3Dexited, status=3D217/USER
>> May 24 12:23:49 igep systemd[1]: upower.service: Failed with result 'exi=
t-code'.
>> May 24 12:23:49 igep systemd[1]: Failed to start Daemon for power manage=
ment.
>> May 24 12:23:49 igep systemd[1]: upower.service: Scheduled restart job, =
restart counter is at 1.
>> May 24 12:23:49 igep systemd[1]: Stopped Daemon for power management.
>>=20
>> That lead me to https://gitlab.freedesktop.org/upower/upower/-/issues/104
>> and finally to systemd's README:
>>=20
>> https://github.com/systemd/systemd/blob/main/README#L89C22-L89C34=20
>>=20
>> But I'll investigate more if is upower or systemd to be blamed here...
>>=20
>> > But specifically to you change, afair CONFIG_MEMCG and userns are
>> > unrelated so tying them together like this in the kconfig seems
>> > misguided.
>> >
>>=20
>> Yes, but the config USER_NS help text already tieds them toghether:
>>=20
>> 	help
>> 	  This allows containers, i.e. vservers, to use user namespaces
>> 	  to provide different user info for different servers.
>>=20
>> 	  When user namespaces are enabled in the kernel it is
>> 	  recommended that the MEMCG option also be enabled and that
>
> But then the patch your patch is the wrong way around and you should
> make CONFIG_USER_NS select CONFIG_MEMCG. IOW, if you do userns, do memcg
> but _not_ if you do memcg, do userns.
>

You are right.

>> 	  user-space use the memory control groups to limit the amount
>> 	  of memory a memory unprivileged users can use.
>>=20
>> And as mentioned in the commit message, it seems to be the reason why the
>> default for this Kconfig symbol is no. Maybe I misunderstood though or do
>> you think that could be switched unconditionally to 'default y' ?
>
> No, definitely not.
>

Ok.

>>=20
>> Or is there a reason to be the only namespace to be default to no instead
>> of yes? Specially since important system services are trying to use it.
>
> Yes, it has a lot more security implications than all of the other ones
> and makes them available to unprivileged users by default. So that
> definitely requires a conscious choice.
>

Got it. Thanks for the explanation. Maybe the option help text could be a
little bit more verbose about it? Since it wasn't clear, at least to me.

Let's discard this patch then and sorry for the noise.

--=20
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat