All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] mptcp: allow priviledged operations from user namespaces
@ 2022-08-05 11:50 Thomas Haller
  2022-08-05 13:37 ` mptcp: allow priviledged operations from user namespaces: Tests Results MPTCP CI
  2022-08-09  0:04 ` [PATCH] mptcp: allow priviledged operations from user namespaces Mat Martineau
  0 siblings, 2 replies; 13+ messages in thread
From: Thomas Haller @ 2022-08-05 11:50 UTC (permalink / raw)
  To: mptcp; +Cc: pabeni, Thomas Haller

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

• * Re: mptcp: allow priviledged operations from user namespaces: Tests Results
    2022-08-05 11:50 [PATCH] mptcp: allow priviledged operations from user namespaces Thomas Haller
  @ 2022-08-05 13:37 ` MPTCP CI
    2022-08-09  0:04 ` [PATCH] mptcp: allow priviledged operations from user namespaces Mat Martineau
    1 sibling, 0 replies; 13+ messages in thread
  From: MPTCP CI @ 2022-08-05 13:37 UTC (permalink / raw)
    To: Thomas Haller; +Cc: mptcp
  
  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)
  
  ^ permalink raw reply	[flat|nested] 13+ messages in thread

• * Re: [PATCH] mptcp: allow priviledged operations from user namespaces
    2022-08-05 11:50 [PATCH] mptcp: allow priviledged operations from user namespaces Thomas Haller
    2022-08-05 13:37 ` mptcp: allow priviledged operations from user namespaces: Tests Results MPTCP CI
  @ 2022-08-09  0:04 ` Mat Martineau
    2022-08-09 10:23   ` Thomas Haller
                       ` (2 more replies)
    1 sibling, 3 replies; 13+ messages in thread
  From: Mat Martineau @ 2022-08-09  0:04 UTC (permalink / raw)
    To: Thomas Haller; +Cc: mptcp, Paolo Abeni, Florian Westphal, Matthieu Baerts
  
  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
  
  ^ permalink raw reply	[flat|nested] 13+ messages in thread

• • * Re: [PATCH] mptcp: allow priviledged operations from user namespaces
      2022-08-09  0:04 ` [PATCH] mptcp: allow priviledged operations from user namespaces Mat Martineau
    @ 2022-08-09 10:23   ` Thomas Haller
      2022-08-09 10:25   ` Florian Westphal
      2022-08-10  7:42   ` [PATCH mptcp-next v2 1/2] " Thomas Haller
      2 siblings, 0 replies; 13+ messages in thread
    From: Thomas Haller @ 2022-08-09 10:23 UTC (permalink / raw)
      To: Mat Martineau; +Cc: mptcp, Paolo Abeni, Florian Westphal, Matthieu Baerts
    
    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
    > 
    
    
    ^ permalink raw reply	[flat|nested] 13+ messages in thread

  • * Re: [PATCH] mptcp: allow priviledged operations from user namespaces
      2022-08-09  0:04 ` [PATCH] mptcp: allow priviledged operations from user namespaces Mat Martineau
      2022-08-09 10:23   ` Thomas Haller
    @ 2022-08-09 10:25   ` Florian Westphal
      2022-08-09 11:16     ` Florian Westphal
      2022-08-10  7:42   ` [PATCH mptcp-next v2 1/2] " Thomas Haller
      2 siblings, 1 reply; 13+ messages in thread
    From: Florian Westphal @ 2022-08-09 10:25 UTC (permalink / raw)
      To: Mat Martineau
      Cc: Thomas Haller, mptcp, Paolo Abeni, Florian Westphal,
    	Matthieu Baerts
    
    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.
    
    ^ permalink raw reply	[flat|nested] 13+ messages in thread

  • • * Re: [PATCH] mptcp: allow priviledged operations from user namespaces
        2022-08-09 10:25   ` Florian Westphal
      @ 2022-08-09 11:16     ` Florian Westphal
        0 siblings, 0 replies; 13+ messages in thread
      From: Florian Westphal @ 2022-08-09 11:16 UTC (permalink / raw)
        To: Florian Westphal
        Cc: Mat Martineau, Thomas Haller, mptcp, Paolo Abeni, Matthieu Baerts
      
      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.
      
      ^ permalink raw reply	[flat|nested] 13+ messages in thread

  • * [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces
      2022-08-09  0:04 ` [PATCH] mptcp: allow priviledged operations from user namespaces Mat Martineau
      2022-08-09 10:23   ` Thomas Haller
      2022-08-09 10:25   ` Florian Westphal
    @ 2022-08-10  7:42   ` Thomas Haller
      2022-08-10  7:42     ` [PATCH mptcp-next v2 2/2] mptcp: account memory allocation in mptcp_nl_cmd_add_addr() to user Thomas Haller
      2022-08-10 23:08     ` [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces Mat Martineau
      2 siblings, 2 replies; 13+ messages in thread
    From: Thomas Haller @ 2022-08-10  7:42 UTC (permalink / raw)
      To: mptcp
      Cc: Florian Westphal, Mat Martineau, Thomas Haller, Paolo Abeni,
    	Matthieu Baerts
    
    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
    
    
    ^ permalink raw reply related	[flat|nested] 13+ messages in thread

  • • * [PATCH mptcp-next v2 2/2] mptcp: account memory allocation in mptcp_nl_cmd_add_addr() to user
        2022-08-10  7:42   ` [PATCH mptcp-next v2 1/2] " Thomas Haller
      @ 2022-08-10  7:42     ` Thomas Haller
        2022-08-10 23:14       ` Mat Martineau
        2022-08-10 23:08     ` [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces Mat Martineau
        1 sibling, 1 reply; 13+ messages in thread
      From: Thomas Haller @ 2022-08-10  7:42 UTC (permalink / raw)
        To: mptcp
        Cc: Florian Westphal, Mat Martineau, Thomas Haller, Paolo Abeni,
      	Matthieu Baerts
      
      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
      
      
      ^ permalink raw reply related	[flat|nested] 13+ messages in thread

    • • * Re: [PATCH mptcp-next v2 2/2] mptcp: account memory allocation in mptcp_nl_cmd_add_addr() to user
          2022-08-10  7:42     ` [PATCH mptcp-next v2 2/2] mptcp: account memory allocation in mptcp_nl_cmd_add_addr() to user Thomas Haller
        @ 2022-08-10 23:14       ` Mat Martineau
          0 siblings, 0 replies; 13+ messages in thread
        From: Mat Martineau @ 2022-08-10 23:14 UTC (permalink / raw)
          To: Thomas Haller; +Cc: mptcp, Florian Westphal, Paolo Abeni, Matthieu Baerts
        
        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
        
        ^ permalink raw reply	[flat|nested] 13+ messages in thread

    • * Re: [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces
        2022-08-10  7:42   ` [PATCH mptcp-next v2 1/2] " Thomas Haller
        2022-08-10  7:42     ` [PATCH mptcp-next v2 2/2] mptcp: account memory allocation in mptcp_nl_cmd_add_addr() to user Thomas Haller
      @ 2022-08-10 23:08     ` Mat Martineau
        2022-08-11 10:34       ` Matthieu Baerts
        1 sibling, 1 reply; 13+ messages in thread
      From: Mat Martineau @ 2022-08-10 23:08 UTC (permalink / raw)
        To: Thomas Haller; +Cc: mptcp, Florian Westphal, Paolo Abeni, Matthieu Baerts
      
      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
      
      ^ permalink raw reply	[flat|nested] 13+ messages in thread

    • • * Re: [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces
          2022-08-10 23:08     ` [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces Mat Martineau
        @ 2022-08-11 10:34       ` Matthieu Baerts
          2022-08-24 20:37         ` Thomas Haller
          0 siblings, 1 reply; 13+ messages in thread
        From: Matthieu Baerts @ 2022-08-11 10:34 UTC (permalink / raw)
          To: Mat Martineau, Thomas Haller; +Cc: mptcp, Florian Westphal, Paolo Abeni
        
        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
        
        ^ permalink raw reply	[flat|nested] 13+ messages in thread

      • • * Re: [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces
            2022-08-11 10:34       ` Matthieu Baerts
          @ 2022-08-24 20:37         ` Thomas Haller
            2022-08-29 15:13           ` Matthieu Baerts
            0 siblings, 1 reply; 13+ messages in thread
          From: Thomas Haller @ 2022-08-24 20:37 UTC (permalink / raw)
            To: Matthieu Baerts, Mat Martineau; +Cc: mptcp, Florian Westphal, Paolo Abeni
          
          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
          
          
          ^ permalink raw reply	[flat|nested] 13+ messages in thread

        • • * Re: [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces
              2022-08-24 20:37         ` Thomas Haller
            @ 2022-08-29 15:13           ` Matthieu Baerts
              0 siblings, 0 replies; 13+ messages in thread
            From: Matthieu Baerts @ 2022-08-29 15:13 UTC (permalink / raw)
              To: Thomas Haller, Mat Martineau; +Cc: mptcp, Florian Westphal, Paolo Abeni
            
            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
            
            ^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-08-29 15:13 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-05 11:50 [PATCH] mptcp: allow priviledged operations from user namespaces Thomas Haller
2022-08-05 13:37 ` mptcp: allow priviledged operations from user namespaces: Tests Results MPTCP CI
2022-08-09  0:04 ` [PATCH] mptcp: allow priviledged operations from user namespaces Mat Martineau
2022-08-09 10:23   ` Thomas Haller
2022-08-09 10:25   ` Florian Westphal
2022-08-09 11:16     ` Florian Westphal
2022-08-10  7:42   ` [PATCH mptcp-next v2 1/2] " Thomas Haller
2022-08-10  7:42     ` [PATCH mptcp-next v2 2/2] mptcp: account memory allocation in mptcp_nl_cmd_add_addr() to user Thomas Haller
2022-08-10 23:14       ` Mat Martineau
2022-08-10 23:08     ` [PATCH mptcp-next v2 1/2] mptcp: allow priviledged operations from user namespaces Mat Martineau
2022-08-11 10:34       ` Matthieu Baerts
2022-08-24 20:37         ` Thomas Haller
2022-08-29 15:13           ` Matthieu Baerts

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.