[PATCH] mptcp: allow priviledged operations from user namespaces

Thomas Haller posted 1 patch 1 month, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/20220805115020.525181-1-thaller@redhat.com
Maintainers: Eric Dumazet <edumazet@google.com>, Matthieu Baerts <matthieu.baerts@tessares.net>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Jakub Kicinski <kuba@kernel.org>, "David S. Miller" <davem@davemloft.net>, Paolo Abeni <pabeni@redhat.com>
net/mptcp/pm_netlink.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
[PATCH] mptcp: allow priviledged operations from user namespaces
Posted by Thomas Haller 1 month, 3 weeks ago
GENL_ADMIN_PERM checks that the user has CAP_NET_ADMIN in the initial
namespace by calling netlink_capable(). Instead, use GENL_UNS_ADMIN_PERM
which uses netlink_ns_capable(). This checks that the caller has
CAP_NET_ADMIN in the current user namespace.

See also commit 4a92602aa1cd ('openvswitch: allow management from inside
user namespaces') which introduced this mechanism. See also commit
5617c6cd6f84 ('nl80211: Allow privileged operations from user
namespaces'), which introduced this for nl80211.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 net/mptcp/pm_netlink.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 291b5da42fdb..2c145cdc7bdc 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -2218,17 +2218,17 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
 	{
 		.cmd    = MPTCP_PM_CMD_ADD_ADDR,
 		.doit   = mptcp_nl_cmd_add_addr,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_DEL_ADDR,
 		.doit   = mptcp_nl_cmd_del_addr,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_FLUSH_ADDRS,
 		.doit   = mptcp_nl_cmd_flush_addrs,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_GET_ADDR,
@@ -2238,7 +2238,7 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
 	{
 		.cmd    = MPTCP_PM_CMD_SET_LIMITS,
 		.doit   = mptcp_nl_cmd_set_limits,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_GET_LIMITS,
@@ -2247,27 +2247,27 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
 	{
 		.cmd    = MPTCP_PM_CMD_SET_FLAGS,
 		.doit   = mptcp_nl_cmd_set_flags,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_ANNOUNCE,
 		.doit   = mptcp_nl_cmd_announce,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_REMOVE,
 		.doit   = mptcp_nl_cmd_remove,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_SUBFLOW_CREATE,
 		.doit   = mptcp_nl_cmd_sf_create,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_SUBFLOW_DESTROY,
 		.doit   = mptcp_nl_cmd_sf_destroy,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 };
 
-- 
2.37.1


Re: [PATCH] mptcp: allow priviledged operations from user namespaces
Posted by Mat Martineau 1 month, 3 weeks ago
On Fri, 5 Aug 2022, Thomas Haller wrote:

> GENL_ADMIN_PERM checks that the user has CAP_NET_ADMIN in the initial
> namespace by calling netlink_capable(). Instead, use GENL_UNS_ADMIN_PERM
> which uses netlink_ns_capable(). This checks that the caller has
> CAP_NET_ADMIN in the current user namespace.
>
> See also commit 4a92602aa1cd ('openvswitch: allow management from inside
> user namespaces') which introduced this mechanism. See also commit
> 5617c6cd6f84 ('nl80211: Allow privileged operations from user
> namespaces'), which introduced this for nl80211.
>
> Signed-off-by: Thomas Haller <thaller@redhat.com>

Hi Thomas -

Thanks for the patch! This does seem like a good idea to me (and it seems 
to work ok with our MPTCP tests), but I'd like to get some more community 
input before merging (Paolo/Florian?).

We also need to figure out which branch this belongs in (we usually tag 
the patch subject with [PATCH mptcp-net] or [PATCH mptcp-next] to indicate 
this). If we upstream it to the net branch it will get in to 6.0-rcX and 
be eligible for backporting to the stable branches. Looks like it would 
need manual backporting to pre-5.19 kernels due to the incremental changes 
to mptcp_pm_ops over time.

To upstream this to net it also needs a Fixes tag:

Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow establishment")


Thomas, do you have any thoughts/preferences on upstreaming to net or 
net-next?


Thanks,
Mat


> ---
> net/mptcp/pm_netlink.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 291b5da42fdb..2c145cdc7bdc 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -2218,17 +2218,17 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
> 	{
> 		.cmd    = MPTCP_PM_CMD_ADD_ADDR,
> 		.doit   = mptcp_nl_cmd_add_addr,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_DEL_ADDR,
> 		.doit   = mptcp_nl_cmd_del_addr,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_FLUSH_ADDRS,
> 		.doit   = mptcp_nl_cmd_flush_addrs,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_GET_ADDR,
> @@ -2238,7 +2238,7 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
> 	{
> 		.cmd    = MPTCP_PM_CMD_SET_LIMITS,
> 		.doit   = mptcp_nl_cmd_set_limits,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_GET_LIMITS,
> @@ -2247,27 +2247,27 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
> 	{
> 		.cmd    = MPTCP_PM_CMD_SET_FLAGS,
> 		.doit   = mptcp_nl_cmd_set_flags,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_ANNOUNCE,
> 		.doit   = mptcp_nl_cmd_announce,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_REMOVE,
> 		.doit   = mptcp_nl_cmd_remove,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_SUBFLOW_CREATE,
> 		.doit   = mptcp_nl_cmd_sf_create,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_SUBFLOW_DESTROY,
> 		.doit   = mptcp_nl_cmd_sf_destroy,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> };
>
> -- 
> 2.37.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH] mptcp: allow priviledged operations from user namespaces
Posted by Florian Westphal 1 month, 2 weeks ago
Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
> On Fri, 5 Aug 2022, Thomas Haller wrote:
> 
> > GENL_ADMIN_PERM checks that the user has CAP_NET_ADMIN in the initial
> > namespace by calling netlink_capable(). Instead, use GENL_UNS_ADMIN_PERM
> > which uses netlink_ns_capable(). This checks that the caller has
> > CAP_NET_ADMIN in the current user namespace.
> > 
> > See also commit 4a92602aa1cd ('openvswitch: allow management from inside
> > user namespaces') which introduced this mechanism. See also commit
> > 5617c6cd6f84 ('nl80211: Allow privileged operations from user
> > namespaces'), which introduced this for nl80211.
> > 
> > Signed-off-by: Thomas Haller <thaller@redhat.com>
> 
> Hi Thomas -
> 
> Thanks for the patch! This does seem like a good idea to me (and it seems to
> work ok with our MPTCP tests), but I'd like to get some more community input
> before merging (Paolo/Florian?).

Looks good to me, I don't see any places where we'd accept e.g. a token
from a different netns.

mptcp_nl_cmd_add_addr should probably be changed to use
GFP_KERNEL_ACCOUNT so that the added entries can be limited via memcg.

mptcp_userspace_pm_append_new_local_addr() helper needs a bit of rework
for that, but I don't think this has do be done right away.

Personally I'd go for -next though.

Re: [PATCH] mptcp: allow priviledged operations from user namespaces
Posted by Florian Westphal 1 month, 2 weeks ago
Florian Westphal <fw@strlen.de> wrote:
> Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
> > On Fri, 5 Aug 2022, Thomas Haller wrote:
> > 
> > > GENL_ADMIN_PERM checks that the user has CAP_NET_ADMIN in the initial
> > > namespace by calling netlink_capable(). Instead, use GENL_UNS_ADMIN_PERM
> > > which uses netlink_ns_capable(). This checks that the caller has
> > > CAP_NET_ADMIN in the current user namespace.
> > > 
> > > See also commit 4a92602aa1cd ('openvswitch: allow management from inside
> > > user namespaces') which introduced this mechanism. See also commit
> > > 5617c6cd6f84 ('nl80211: Allow privileged operations from user
> > > namespaces'), which introduced this for nl80211.
> > > 
> > > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > 
> > Hi Thomas -
> > 
> > Thanks for the patch! This does seem like a good idea to me (and it seems to
> > work ok with our MPTCP tests), but I'd like to get some more community input
> > before merging (Paolo/Florian?).
> 
> Looks good to me, I don't see any places where we'd accept e.g. a token
> from a different netns.
> 
> mptcp_nl_cmd_add_addr should probably be changed to use
> GFP_KERNEL_ACCOUNT so that the added entries can be limited via memcg.

> mptcp_userspace_pm_append_new_local_addr() helper needs a bit of rework
> for that, but I don't think this has do be done right away.

Actually its fine; the allocation is accounted vs the socket.

mptcp_nl_cmd_add_addr has the only "permanent" kmalloc that I can see.

Re: [PATCH] mptcp: allow priviledged operations from user namespaces
Posted by Thomas Haller 1 month, 2 weeks ago
Hi Mat,

On Mon, 2022-08-08 at 17:04 -0700, Mat Martineau wrote:
> On Fri, 5 Aug 2022, Thomas Haller wrote:
> 
> > GENL_ADMIN_PERM checks that the user has CAP_NET_ADMIN in the
> > initial
> > namespace by calling netlink_capable(). Instead, use
> > GENL_UNS_ADMIN_PERM
> > which uses netlink_ns_capable(). This checks that the caller has
> > CAP_NET_ADMIN in the current user namespace.
> > 
> > See also commit 4a92602aa1cd ('openvswitch: allow management from
> > inside
> > user namespaces') which introduced this mechanism. See also commit
> > 5617c6cd6f84 ('nl80211: Allow privileged operations from user
> > namespaces'), which introduced this for nl80211.
> > 
> > Signed-off-by: Thomas Haller <thaller@redhat.com>
> 
> Hi Thomas -
> 
> Thanks for the patch! This does seem like a good idea to me (and it
> seems 
> to work ok with our MPTCP tests), but I'd like to get some more
> community 
> input before merging (Paolo/Florian?).

thanks for your feedback.

The background for this is that we run unit tests for NetworkManager in
a separate network namespace, started as non-root user. Configuring
MPTCP endpoints does currently not work in that environment.


> We also need to figure out which branch this belongs in (we usually
> tag 
> the patch subject with [PATCH mptcp-net] or [PATCH mptcp-next] to
> indicate 
> this). If we upstream it to the net branch it will get in to 6.0-rcX
> and 
> be eligible for backporting to the stable branches. Looks like it
> would 
> need manual backporting to pre-5.19 kernels due to the incremental
> changes 
> to mptcp_pm_ops over time.

> To upstream this to net it also needs a Fixes tag:
> 
> Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow
> establishment")


> Thomas, do you have any thoughts/preferences on upstreaming to net or
> net-next?

I don't have a strong opinion or need. Thanks for explaining the
process.

Ffrom what you said, it sounds as this would make sense as [PATCH
mptcp-net]. I will send a v2 with the "Fixes:" line.



Thomas

> > ---
> > net/mptcp/pm_netlink.c | 18 +++++++++---------
> > 1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 291b5da42fdb..2c145cdc7bdc 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -2218,17 +2218,17 @@ static const struct genl_small_ops
> > mptcp_pm_ops[] = {
> >         {
> >                 .cmd    = MPTCP_PM_CMD_ADD_ADDR,
> >                 .doit   = mptcp_nl_cmd_add_addr,
> > -               .flags  = GENL_ADMIN_PERM,
> > +               .flags  = GENL_UNS_ADMIN_PERM,
> >         },
> >         {
> >                 .cmd    = MPTCP_PM_CMD_DEL_ADDR,
> >                 .doit   = mptcp_nl_cmd_del_addr,
> > -               .flags  = GENL_ADMIN_PERM,
> > +               .flags  = GENL_UNS_ADMIN_PERM,
> >         },
> >         {
> >                 .cmd    = MPTCP_PM_CMD_FLUSH_ADDRS,
> >                 .doit   = mptcp_nl_cmd_flush_addrs,
> > -               .flags  = GENL_ADMIN_PERM,
> > +               .flags  = GENL_UNS_ADMIN_PERM,
> >         },
> >         {
> >                 .cmd    = MPTCP_PM_CMD_GET_ADDR,
> > @@ -2238,7 +2238,7 @@ static const struct genl_small_ops
> > mptcp_pm_ops[] = {
> >         {
> >                 .cmd    = MPTCP_PM_CMD_SET_LIMITS,
> >                 .doit   = mptcp_nl_cmd_set_limits,
> > -               .flags  = GENL_ADMIN_PERM,
> > +               .flags  = GENL_UNS_ADMIN_PERM,
> >         },
> >         {
> >                 .cmd    = MPTCP_PM_CMD_GET_LIMITS,
> > @@ -2247,27 +2247,27 @@ static const struct genl_small_ops
> > mptcp_pm_ops[] = {
> >         {
> >                 .cmd    = MPTCP_PM_CMD_SET_FLAGS,
> >                 .doit   = mptcp_nl_cmd_set_flags,
> > -               .flags  = GENL_ADMIN_PERM,
> > +               .flags  = GENL_UNS_ADMIN_PERM,
> >         },
> >         {
> >                 .cmd    = MPTCP_PM_CMD_ANNOUNCE,
> >                 .doit   = mptcp_nl_cmd_announce,
> > -               .flags  = GENL_ADMIN_PERM,
> > +               .flags  = GENL_UNS_ADMIN_PERM,
> >         },
> >         {
> >                 .cmd    = MPTCP_PM_CMD_REMOVE,
> >                 .doit   = mptcp_nl_cmd_remove,
> > -               .flags  = GENL_ADMIN_PERM,
> > +               .flags  = GENL_UNS_ADMIN_PERM,
> >         },
> >         {
> >                 .cmd    = MPTCP_PM_CMD_SUBFLOW_CREATE,
> >                 .doit   = mptcp_nl_cmd_sf_create,
> > -               .flags  = GENL_ADMIN_PERM,
> > +               .flags  = GENL_UNS_ADMIN_PERM,
> >         },
> >         {
> >                 .cmd    = MPTCP_PM_CMD_SUBFLOW_DESTROY,
> >                 .doit   = mptcp_nl_cmd_sf_destroy,
> > -               .flags  = GENL_ADMIN_PERM,
> > +               .flags  = GENL_UNS_ADMIN_PERM,
> >         },
> > };
> > 
> > -- 
> > 2.37.1
> > 
> > 
> > 
> 
> --
> Mat Martineau
> Intel
> 

[PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces
Posted by Thomas Haller 1 month, 2 weeks ago
GENL_ADMIN_PERM checks that the user has CAP_NET_ADMIN in the initial
namespace by calling netlink_capable(). Instead, use GENL_UNS_ADMIN_PERM
which uses netlink_ns_capable(). This checks that the caller has
CAP_NET_ADMIN in the current user namespace.

See also commit 4a92602aa1cd ('openvswitch: allow management from inside
user namespaces') which introduced this mechanism. See also commit
5617c6cd6f84 ('nl80211: Allow privileged operations from user
namespaces'), which introduced this for nl80211.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 net/mptcp/pm_netlink.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 291b5da42fdb..2c145cdc7bdc 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -2218,17 +2218,17 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
 	{
 		.cmd    = MPTCP_PM_CMD_ADD_ADDR,
 		.doit   = mptcp_nl_cmd_add_addr,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_DEL_ADDR,
 		.doit   = mptcp_nl_cmd_del_addr,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_FLUSH_ADDRS,
 		.doit   = mptcp_nl_cmd_flush_addrs,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_GET_ADDR,
@@ -2238,7 +2238,7 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
 	{
 		.cmd    = MPTCP_PM_CMD_SET_LIMITS,
 		.doit   = mptcp_nl_cmd_set_limits,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_GET_LIMITS,
@@ -2247,27 +2247,27 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
 	{
 		.cmd    = MPTCP_PM_CMD_SET_FLAGS,
 		.doit   = mptcp_nl_cmd_set_flags,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_ANNOUNCE,
 		.doit   = mptcp_nl_cmd_announce,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_REMOVE,
 		.doit   = mptcp_nl_cmd_remove,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_SUBFLOW_CREATE,
 		.doit   = mptcp_nl_cmd_sf_create,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 	{
 		.cmd    = MPTCP_PM_CMD_SUBFLOW_DESTROY,
 		.doit   = mptcp_nl_cmd_sf_destroy,
-		.flags  = GENL_ADMIN_PERM,
+		.flags  = GENL_UNS_ADMIN_PERM,
 	},
 };
 
-- 
2.37.1


Re: [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces
Posted by Mat Martineau 1 month, 2 weeks ago
On Wed, 10 Aug 2022, Thomas Haller wrote:

> GENL_ADMIN_PERM checks that the user has CAP_NET_ADMIN in the initial
> namespace by calling netlink_capable(). Instead, use GENL_UNS_ADMIN_PERM
> which uses netlink_ns_capable(). This checks that the caller has
> CAP_NET_ADMIN in the current user namespace.
>
> See also commit 4a92602aa1cd ('openvswitch: allow management from inside
> user namespaces') which introduced this mechanism. See also commit
> 5617c6cd6f84 ('nl80211: Allow privileged operations from user
> namespaces'), which introduced this for nl80211.
>
> Signed-off-by: Thomas Haller <thaller@redhat.com>

Thanks Thomas, looks good:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>


Florian had mentioned preferring net-next in the discussion of v1 (which 
Thomas has agreed with by labeling this for mptcp-next), and I agree. I 
don't think it quite meets the bar for -net or stable backporting and it 
would be easier to explain that "6.1 and later support user namespaces for 
MPTCP generic netlink commands". (Matthieu, if someone makes a convincing 
case for -net, it's up to you :) )


- Mat

> ---
> net/mptcp/pm_netlink.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 291b5da42fdb..2c145cdc7bdc 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -2218,17 +2218,17 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
> 	{
> 		.cmd    = MPTCP_PM_CMD_ADD_ADDR,
> 		.doit   = mptcp_nl_cmd_add_addr,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_DEL_ADDR,
> 		.doit   = mptcp_nl_cmd_del_addr,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_FLUSH_ADDRS,
> 		.doit   = mptcp_nl_cmd_flush_addrs,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_GET_ADDR,
> @@ -2238,7 +2238,7 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
> 	{
> 		.cmd    = MPTCP_PM_CMD_SET_LIMITS,
> 		.doit   = mptcp_nl_cmd_set_limits,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_GET_LIMITS,
> @@ -2247,27 +2247,27 @@ static const struct genl_small_ops mptcp_pm_ops[] = {
> 	{
> 		.cmd    = MPTCP_PM_CMD_SET_FLAGS,
> 		.doit   = mptcp_nl_cmd_set_flags,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_ANNOUNCE,
> 		.doit   = mptcp_nl_cmd_announce,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_REMOVE,
> 		.doit   = mptcp_nl_cmd_remove,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_SUBFLOW_CREATE,
> 		.doit   = mptcp_nl_cmd_sf_create,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> 	{
> 		.cmd    = MPTCP_PM_CMD_SUBFLOW_DESTROY,
> 		.doit   = mptcp_nl_cmd_sf_destroy,
> -		.flags  = GENL_ADMIN_PERM,
> +		.flags  = GENL_UNS_ADMIN_PERM,
> 	},
> };
>
> -- 
> 2.37.1
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces
Posted by Matthieu Baerts 1 month, 2 weeks ago
Hi Thomas, Mat, Florian,

On 11/08/2022 01:08, Mat Martineau wrote:
> On Wed, 10 Aug 2022, Thomas Haller wrote:
> 
>> GENL_ADMIN_PERM checks that the user has CAP_NET_ADMIN in the initial
>> namespace by calling netlink_capable(). Instead, use GENL_UNS_ADMIN_PERM
>> which uses netlink_ns_capable(). This checks that the caller has
>> CAP_NET_ADMIN in the current user namespace.
>>
>> See also commit 4a92602aa1cd ('openvswitch: allow management from inside
>> user namespaces') which introduced this mechanism. See also commit
>> 5617c6cd6f84 ('nl80211: Allow privileged operations from user
>> namespaces'), which introduced this for nl80211.
>>
>> Signed-off-by: Thomas Haller <thaller@redhat.com>
> 
> Thanks Thomas, looks good:
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Thank you for the patches and the reviews!

> Florian had mentioned preferring net-next in the discussion of v1 (which
> Thomas has agreed with by labeling this for mptcp-next), and I agree. I
> don't think it quite meets the bar for -net or stable backporting and it
> would be easier to explain that "6.1 and later support user namespaces
> for MPTCP generic netlink commands". (Matthieu, if someone makes a
> convincing case for -net, it's up to you :) )

I initially thought it was more a bug-fix but I also agree with Florian.
Also I see that the two mentioned commits above don't have a Fixes tag.

The only exception I found and related to this flag was for l2tp: the
commit 2abe05234f2e ("l2tp: Allow management of tunnels and session in
user namespace") has been selected by davem and backported to v5.4.

https://lore.kernel.org/stable/20200417.105100.821338189941807731.davem@davemloft.net/
(see patch 03/19)


Anyway, your 2 patches are now in our tree (feat. for net-next) with
Mat's RvB tag and without a typo (s/priviledged/privileged/) + a small
fix for checkpatch related to how the commit are mentioned, nothing
important:


New patches for t/upstream:
- 3d542a6c45ea: mptcp: allow privileged operations from user namespaces
- 11bdb1959854: mptcp: account memory allocation in
mptcp_nl_cmd_add_addr() to user
- Results: c4a0ae952875..004104cc8a77 (export)


Tests are now in progress:





https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220811T101116


@Thomas: BTW, thank you for maintaining libnl!
Funny that you sent these patches to MPTCP while earlier this week I
sent the latest libnl version to Debian :)

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1016867
https://tracker.debian.org/pkg/libnl3


Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces
Posted by Thomas Haller 1 month ago
Hi Matthieu,



On Thu, 2022-08-11 at 12:34 +0200, Matthieu Baerts wrote:
> Hi Thomas, Mat, Florian,
> 
> On 11/08/2022 01:08, Mat Martineau wrote:
> ...
> 
> > 
> > Thanks Thomas, looks good:
> > 
> > Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> 
> Thank you for the patches and the reviews!
> 
> > [...]
> 
> 
> I initially thought it was more a bug-fix but I also agree with
> Florian.
> Also I see that the two mentioned commits above don't have a Fixes
> tag.
> 
> The only exception I found and related to this flag was for l2tp: the
> commit 2abe05234f2e ("l2tp: Allow management of tunnels and session
> in
> user namespace") has been selected by davem and backported to v5.4.
> 
> https://lore.kernel.org/stable/20200417.105100.821338189941807731.davem@davemloft.net/
> (see patch 03/19)
> 
> 
> Anyway, your 2 patches are now in our tree (feat. for net-next) with
> Mat's RvB tag and without a typo (s/priviledged/privileged/) + a
> small
> fix for checkpatch related to how the commit are mentioned, nothing
> important:

Thank you for the adjustments, the feedback and applying!!


> New patches for t/upstream:
> - 3d542a6c45ea: mptcp: allow privileged operations from user
> namespaces
> - 11bdb1959854: mptcp: account memory allocation in
> mptcp_nl_cmd_add_addr() to user
> - Results: c4a0ae952875..004104cc8a77 (export)
> 
> 
> Tests are now in progress:
> 
> https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220811T101116
> 
> 
> @Thomas: BTW, thank you for maintaining libnl!
> Funny that you sent these patches to MPTCP while earlier this week I
> sent the latest libnl version to Debian :)
> 
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1016867
> https://tracker.debian.org/pkg/libnl3

Oh, that's nice. After I rather neglected libnl for quite some time, I
want do to a better job in the future. :)



On an unrelated note: upcoming NetworkManager 1.40.0 release will have
basic MPTCP support and configure endpoints. Similar to what mptcpd
does. This was the backstory for the patches.



best,
Thomas


Re: [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces
Posted by Matthieu Baerts 1 month ago
Hi Thomas,

On 24/08/2022 22:37, Thomas Haller wrote:
> On Thu, 2022-08-11 at 12:34 +0200, Matthieu Baerts wrote:

(...)

>> @Thomas: BTW, thank you for maintaining libnl!
>> Funny that you sent these patches to MPTCP while earlier this week I
>> sent the latest libnl version to Debian :)
>>
>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1016867
>> https://tracker.debian.org/pkg/libnl3
> 
> Oh, that's nice. After I rather neglected libnl for quite some time, I
> want do to a better job in the future. :)

:-)

> On an unrelated note: upcoming NetworkManager 1.40.0 release will have
> basic MPTCP support and configure endpoints. Similar to what mptcpd
> does. This was the backstory for the patches.

That's a really good news! Thank you for having worked on that,
preparing a setup to be used with MPTCP is going to be easier now!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[PATCH mptcp-next v2 2/2] mptcp: account memory allocation in mptcp_nl_cmd_add_addr() to user
Posted by Thomas Haller 1 month, 2 weeks ago
Now that non-root users can configure MPTCP endpoints, account
the memory allocation to the user.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 net/mptcp/pm_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 2c145cdc7bdc..79469178a2a8 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1327,7 +1327,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	}
 
-	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL_ACCOUNT);
 	if (!entry) {
 		GENL_SET_ERR_MSG(info, "can't allocate addr");
 		return -ENOMEM;
-- 
2.37.1


Re: [PATCH mptcp-next v2 2/2] mptcp: account memory allocation in mptcp_nl_cmd_add_addr() to user
Posted by Mat Martineau 1 month, 2 weeks ago
On Wed, 10 Aug 2022, Thomas Haller wrote:

> Now that non-root users can configure MPTCP endpoints, account
> the memory allocation to the user.
>
> Signed-off-by: Thomas Haller <thaller@redhat.com>

Looks good:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> ---
> net/mptcp/pm_netlink.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 2c145cdc7bdc..79469178a2a8 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1327,7 +1327,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
> 		return -EINVAL;
> 	}
>
> -	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL_ACCOUNT);
> 	if (!entry) {
> 		GENL_SET_ERR_MSG(info, "can't allocate addr");
> 		return -ENOMEM;
> -- 
> 2.37.1
>
>

--
Mat Martineau
Intel

Re: mptcp: allow priviledged operations from user namespaces: Tests Results
Posted by MPTCP CI 1 month, 3 weeks ago
Hi Thomas,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 2 failed test(s): kunit selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/4847158800154624
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4847158800154624/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 2 failed test(s): kunit packetdrill_add_addr 🔴:
  - Task: https://cirrus-ci.com/task/5973058706997248
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5973058706997248/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/c384b8e857ac


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)