| Message ID | 20220810074251.31887-1-thaller@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 3d542a6c45ea3348049ee924fd5ad8c5caf7b33a |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-next,v2,1/2] mptcp: allow priviledged operations from user namespaces | expand |
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
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
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
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
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, }, };
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(-)