1. Patchew
  2. MPTCP
  3. View series

[PATCH net-next] genetlink: Use internal flags for multicast groups

Ido Schimmel posted 1 patch 4 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/20231217075348.4040051-1-idosch@nvidia.com
Maintainers: "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Neil Horman <nhorman@tuxdriver.com>, Matthieu Baerts <matttbe@kernel.org>, Mat Martineau <martineau@kernel.org>, Yotam Gigi <yotam.gi@gmail.com>
There is a newer version of this series
include/net/genetlink.h | 7 ++++---
net/core/drop_monitor.c | 2 +-
net/mptcp/pm_netlink.c  | 2 +-
net/netlink/genetlink.c | 4 ++--
net/psample/psample.c   | 2 +-
5 files changed, 9 insertions(+), 8 deletions(-)
[PATCH net-next] genetlink: Use internal flags for multicast groups
Posted by Ido Schimmel 4 months, 2 weeks ago 
As explained in commit e03781879a0d ("drop_monitor: Require
'CAP_SYS_ADMIN' when joining "events" group"), the "flags" field in the
multicast group structure reuses uAPI flags despite the field not being
exposed to user space. This makes it impossible to extend its use
without adding new uAPI flags, which is inappropriate for internal
kernel checks.

Solve this by adding internal flags (i.e., "GENL_MCAST_*") and convert
the existing users to use them instead of the uAPI flags.

Tested using the reproducers in commit 44ec98ea5ea9 ("psample: Require
'CAP_NET_ADMIN' when joining "packets" group") and commit e03781879a0d
("drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group").

No functional changes intended.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 include/net/genetlink.h | 7 ++++---
 net/core/drop_monitor.c | 2 +-
 net/mptcp/pm_netlink.c  | 2 +-
 net/netlink/genetlink.c | 4 ++--
 net/psample/psample.c   | 2 +-
 5 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index c53244f20437..5a402fd24817 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -8,16 +8,17 @@
 
 #define GENLMSG_DEFAULT_SIZE (NLMSG_DEFAULT_SIZE - GENL_HDRLEN)
 
+#define GENL_MCAST_CAP_NET_ADMIN	BIT(0)
+#define GENL_MCAST_CAP_SYS_ADMIN	BIT(1)
+
 /**
  * struct genl_multicast_group - generic netlink multicast group
  * @name: name of the multicast group, names are per-family
- * @flags: GENL_* flags (%GENL_ADMIN_PERM or %GENL_UNS_ADMIN_PERM)
- * @cap_sys_admin: whether %CAP_SYS_ADMIN is required for binding
+ * @flags: GENL_MCAST_* flags
  */
 struct genl_multicast_group {
 	char			name[GENL_NAMSIZ];
 	u8			flags;
-	u8			cap_sys_admin:1;
 };
 
 struct genl_split_ops;
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index b240d9aae4a6..b0f221d658be 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -183,7 +183,7 @@ static struct sk_buff *reset_per_cpu_data(struct per_cpu_dm_data *data)
 }
 
 static const struct genl_multicast_group dropmon_mcgrps[] = {
-	{ .name = "events", .cap_sys_admin = 1 },
+	{ .name = "events", .flags = GENL_MCAST_CAP_SYS_ADMIN, },
 };
 
 static void send_dm_alert(struct work_struct *work)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index bf4d96f6f99a..272e93be1aad 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1100,7 +1100,7 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 static const struct genl_multicast_group mptcp_pm_mcgrps[] = {
 	[MPTCP_PM_CMD_GRP_OFFSET]	= { .name = MPTCP_PM_CMD_GRP_NAME, },
 	[MPTCP_PM_EV_GRP_OFFSET]        = { .name = MPTCP_PM_EV_GRP_NAME,
-					    .flags = GENL_UNS_ADMIN_PERM,
+					    .flags = GENL_MCAST_CAP_NET_ADMIN,
 					  },
 };
 
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 9c7ffd10df2a..f6d14c092d24 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1688,10 +1688,10 @@ static int genl_bind(struct net *net, int group)
 			continue;
 
 		grp = &family->mcgrps[i];
-		if ((grp->flags & GENL_UNS_ADMIN_PERM) &&
+		if ((grp->flags & GENL_MCAST_CAP_NET_ADMIN) &&
 		    !ns_capable(net->user_ns, CAP_NET_ADMIN))
 			ret = -EPERM;
-		if (grp->cap_sys_admin &&
+		if ((grp->flags & GENL_MCAST_CAP_SYS_ADMIN) &&
 		    !ns_capable(net->user_ns, CAP_SYS_ADMIN))
 			ret = -EPERM;
 
diff --git a/net/psample/psample.c b/net/psample/psample.c
index c34e902855db..ddd211a151d0 100644
--- a/net/psample/psample.c
+++ b/net/psample/psample.c
@@ -32,7 +32,7 @@ enum psample_nl_multicast_groups {
 static const struct genl_multicast_group psample_nl_mcgrps[] = {
 	[PSAMPLE_NL_MCGRP_CONFIG] = { .name = PSAMPLE_NL_MCGRP_CONFIG_NAME },
 	[PSAMPLE_NL_MCGRP_SAMPLE] = { .name = PSAMPLE_NL_MCGRP_SAMPLE_NAME,
-				      .flags = GENL_UNS_ADMIN_PERM },
+				      .flags = GENL_MCAST_CAP_NET_ADMIN, },
 };
 
 static struct genl_family psample_nl_family __ro_after_init;
-- 
2.40.1
Re: [PATCH net-next] genetlink: Use internal flags for multicast groups
Posted by Andy Shevchenko 4 months, 1 week ago 
On Sun, Dec 17, 2023 at 09:53:48AM +0200, Ido Schimmel wrote:
> As explained in commit e03781879a0d ("drop_monitor: Require
> 'CAP_SYS_ADMIN' when joining "events" group"), the "flags" field in the
> multicast group structure reuses uAPI flags despite the field not being
> exposed to user space. This makes it impossible to extend its use
> without adding new uAPI flags, which is inappropriate for internal
> kernel checks.
> 
> Solve this by adding internal flags (i.e., "GENL_MCAST_*") and convert
> the existing users to use them instead of the uAPI flags.
> 
> Tested using the reproducers in commit 44ec98ea5ea9 ("psample: Require
> 'CAP_NET_ADMIN' when joining "packets" group") and commit e03781879a0d
> ("drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group").
> 
> No functional changes intended.

...

> +#define GENL_MCAST_CAP_NET_ADMIN	BIT(0)
> +#define GENL_MCAST_CAP_SYS_ADMIN	BIT(1)
> +
>  /**
>   * struct genl_multicast_group - generic netlink multicast group
>   * @name: name of the multicast group, names are per-family
> - * @flags: GENL_* flags (%GENL_ADMIN_PERM or %GENL_UNS_ADMIN_PERM)
> - * @cap_sys_admin: whether %CAP_SYS_ADMIN is required for binding
> + * @flags: GENL_MCAST_* flags
>   */

No functional, but documentation changes...

So, I suggest to have these flags being described

/* ... */
#define FLAG1

/* ... */
#define FLAG2

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH net-next] genetlink: Use internal flags for multicast groups
Posted by Ido Schimmel 4 months, 1 week ago 
On Mon, Dec 18, 2023 at 01:37:41PM +0200, Andy Shevchenko wrote:
> No functional, but documentation changes...
> 
> So, I suggest to have these flags being described
> 
> /* ... */
> #define FLAG1
> 
> /* ... */
> #define FLAG2

OK, thanks. I will squash the following diff into v2:

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 5a402fd24817..bd6a6ffdec76 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -8,7 +8,9 @@
 
 #define GENLMSG_DEFAULT_SIZE (NLMSG_DEFAULT_SIZE - GENL_HDRLEN)
 
+/* Binding to multicast group requires %CAP_NET_ADMIN */
 #define GENL_MCAST_CAP_NET_ADMIN       BIT(0)
+/* Binding to multicast group requires %CAP_SYS_ADMIN */
 #define GENL_MCAST_CAP_SYS_ADMIN       BIT(1)
 
 /**
Re: genetlink: Use internal flags for multicast groups: Tests Results
Posted by MPTCP CI 4 months, 2 weeks ago 
Hi Ido,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5304333823115264
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5304333823115264/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6148758753247232
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6148758753247232/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): packetdrill_regressions 🔴:
  - Task: https://cirrus-ci.com/task/5022858846404608
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5022858846404608/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6430233729957888
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6430233729957888/summary/summary.txt

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


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 (NGI0 Core)

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.