net/mptcp/pm_netlink.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
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
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
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.
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.
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 >
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
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 -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
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 -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
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
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
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)
© 2016 - 2024 Red Hat, Inc.