[PATCH net-next v2 0/4] genetlink: apply reject policy for split ops on the dispatch path

[PATCH net-next v2 0/4] genetlink: apply reject policy for split ops on the dispatch path

[Jakub Kicinski]

Looks like I somehow missed adding default reject policies to commands
in families using split Netlink ops. I realized this randomly trying
to dump page pools for a specific device and always getting all of them
back. The per-device dump is simply not implemented so the request
should have been rejected. Patch 2 is the real change, the rest is
just accompaniment.

v2:
 - add patch 1 to avoid breaking devlink
 - add a lot more tests
v1: https://lore.kernel.org/20260307204425.1900467-1-kuba@kernel.org

Jakub Kicinski (4):
  genetlink: use maxattr of 0 for the reject policy
  genetlink: apply reject policy for split ops on the dispatch path
  selftests: net: make sure that Netlink rejects unknown attrs in dump
  selftests: net: add test for Netlink policy dumps

 tools/testing/selftests/net/Makefile          |   1 +
 net/netlink/genetlink.c                       |  20 +--
 net/netlink/policy.c                          |   4 +-
 .../testing/selftests/net/lib/py/__init__.py  |   5 +-
 tools/testing/selftests/net/lib/py/ynl.py     |  10 +-
 tools/testing/selftests/net/nl_netdev.py      |  32 ++++-
 tools/testing/selftests/net/nl_nlctrl.py      | 135 ++++++++++++++++++
 7 files changed, 186 insertions(+), 21 deletions(-)
 create mode 100755 tools/testing/selftests/net/nl_nlctrl.py

-- 
2.53.0

[PATCH net-next v2 1/4] genetlink: use maxattr of 0 for the reject policy

[Jakub Kicinski]

Commit 4fa86555d1cd ("genetlink: piggy back on resv_op to default to
a reject policy") added genl_policy_reject_all to ensure that ops
without an explicit policy reject all attributes rather than silently
accepting them.

The reject policy had maxattr of 1. Passing info->attrs of size 2
may surprise families. Devlink, for instance, assumes that if
info->attrs is set it's safe to access DEVLINK_ATTR_BUS_NAME (1)
and DEVLINK_ATTR_DEV_NAME (2).

Before plugging reject policies into split ops we need to make sure
the genetlink code will not populate info->attrs if family
had no explicit policy for the op.

While even shared code paths within the families can figure out
that given op has no policy fairly easily themselves, passing attrs
with fixed size of 2 feels fairly useless and error prone.

This change has no user-visible impact, reject attrs are not
reported to the user space via getpolicy. We do have to remove
the safety check in netlink_policy_dump_get_policy_idx()
but it seems to have been there to catch likely faulty input,
the code can handle maxattr = 0 just fine.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/netlink/genetlink.c | 19 +++++++++----------
 net/netlink/policy.c    |  4 ++--
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index a23d4c51c089..c00f0586c8d6 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -92,10 +92,8 @@ static unsigned long mc_group_start = 0x3 | BIT(GENL_ID_CTRL) |
 static unsigned long *mc_groups = &mc_group_start;
 static unsigned long mc_groups_longs = 1;
 
-/* We need the last attribute with non-zero ID therefore a 2-entry array */
 static struct nla_policy genl_policy_reject_all[] = {
 	{ .type = NLA_REJECT },
-	{ .type = NLA_REJECT },
 };
 
 static int genl_ctrl_event(int event, const struct genl_family *family,
@@ -106,13 +104,10 @@ static void
 genl_op_fill_in_reject_policy(const struct genl_family *family,
 			      struct genl_ops *op)
 {
-	BUILD_BUG_ON(ARRAY_SIZE(genl_policy_reject_all) - 1 != 1);
-
 	if (op->policy || op->cmd < family->resv_start_op)
 		return;
 
 	op->policy = genl_policy_reject_all;
-	op->maxattr = 1;
 }
 
 static void
@@ -123,7 +118,6 @@ genl_op_fill_in_reject_policy_split(const struct genl_family *family,
 		return;
 
 	op->policy = genl_policy_reject_all;
-	op->maxattr = 1;
 }
 
 static const struct genl_family *genl_family_find_byid(unsigned int id)
@@ -934,12 +928,17 @@ genl_family_rcv_msg_attrs_parse(const struct genl_family *family,
 	struct nlattr **attrbuf;
 	int err;
 
-	if (!ops->maxattr)
+	if (!ops->policy)
 		return NULL;
 
-	attrbuf = kmalloc_objs(struct nlattr *, ops->maxattr + 1);
-	if (!attrbuf)
-		return ERR_PTR(-ENOMEM);
+	if (ops->maxattr) {
+		attrbuf = kmalloc_objs(struct nlattr *, ops->maxattr + 1);
+		if (!attrbuf)
+			return ERR_PTR(-ENOMEM);
+	} else {
+		/* Reject all policy, __nlmsg_parse() will just validate */
+		attrbuf = NULL;
+	}
 
 	err = __nlmsg_parse(nlh, hdrlen, attrbuf, ops->maxattr, ops->policy,
 			    validate, extack);
diff --git a/net/netlink/policy.c b/net/netlink/policy.c
index f39cd7cc4fb5..08b006c48f06 100644
--- a/net/netlink/policy.c
+++ b/net/netlink/policy.c
@@ -31,7 +31,7 @@ static int add_policy(struct netlink_policy_dump_state **statep,
 	struct netlink_policy_dump_state *state = *statep;
 	unsigned int old_n_alloc, n_alloc, i;
 
-	if (!policy || !maxtype)
+	if (!policy)
 		return 0;
 
 	for (i = 0; i < state->n_alloc; i++) {
@@ -85,7 +85,7 @@ int netlink_policy_dump_get_policy_idx(struct netlink_policy_dump_state *state,
 {
 	unsigned int i;
 
-	if (WARN_ON(!policy || !maxtype))
+	if (WARN_ON(!policy))
                 return 0;
 
 	for (i = 0; i < state->n_alloc; i++) {
-- 
2.53.0

[PATCH net-next v2 2/4] genetlink: apply reject policy for split ops on the dispatch path

[Jakub Kicinski]

Commit 4fa86555d1cd ("genetlink: piggy back on resv_op to default to
a reject policy") added genl_policy_reject_all to ensure that ops
without an explicit policy reject all attributes rather than silently
accepting them. This change was applied to net.

When split ops were later introduced in net-next in
commit b8fd60c36a44 ("genetlink: allow families to use split ops directly"),
genl_op_fill_in_reject_policy_split() was added and called from
genl_op_from_split() (used for policy dumping and registration).
However, genl_get_cmd_split(), which is called for incoming messages,
copies split_ops entries as-is without applying the reject policy.
This means that split ops without policy accept all inputs.

This looks like an omission / mistake made when splitting the changes
between net and net-next. Let's try to re-introduce the checking.
Not considering this a fix given the regression potential.
If anyone reports issues we should probably fill in fake policies
for specific ops rather than reverting this.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: kees@kernel.org
CC: alok.a.tiwari@oracle.com
---
 net/netlink/genetlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index c00f0586c8d6..d251d894afd4 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -244,6 +244,7 @@ genl_get_cmd_split(u32 cmd, u8 flag, const struct genl_family *family,
 		if (family->split_ops[i].cmd == cmd &&
 		    family->split_ops[i].flags & flag) {
 			*op = family->split_ops[i];
+			genl_op_fill_in_reject_policy_split(family, op);
 			return 0;
 		}
 
-- 
2.53.0

[PATCH net-next v2 3/4] selftests: net: make sure that Netlink rejects unknown attrs in dump

[Jakub Kicinski]

Add a test case for rejecting attrs if policy is not set.
dev_get dump has no input policy (accepts no attrs).

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: shuah@kernel.org
CC: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/net/nl_netdev.py | 32 +++++++++++++++++++-----
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
index 5c66421ab8aa..eff55c64a012 100755
--- a/tools/testing/selftests/net/nl_netdev.py
+++ b/tools/testing/selftests/net/nl_netdev.py
@@ -1,11 +1,15 @@
 #!/usr/bin/env python3
 # SPDX-License-Identifier: GPL-2.0
 
-import time
+"""
+Tests for the netdev netlink family.
+"""
+
+import errno
 from os import system
-from lib.py import ksft_run, ksft_exit, ksft_pr
-from lib.py import ksft_eq, ksft_ge, ksft_ne, ksft_busy_wait
-from lib.py import NetdevFamily, NetdevSimDev, ip
+from lib.py import ksft_run, ksft_exit
+from lib.py import ksft_eq, ksft_ge, ksft_ne, ksft_raises, ksft_busy_wait
+from lib.py import NetdevFamily, NetdevSimDev, NlError, ip
 
 
 def empty_check(nf) -> None:
@@ -19,6 +23,15 @@ from lib.py import NetdevFamily, NetdevSimDev, ip
     ksft_eq(len(lo_info['xdp-rx-metadata-features']), 0)
 
 
+def dev_dump_reject_attr(nf) -> None:
+    """Test that dev-get dump rejects attributes (no dump request policy)."""
+    with ksft_raises(NlError) as cm:
+        nf.dev_get({'ifindex': 1}, dump=True)
+    ksft_eq(cm.exception.nl_msg.error, -errno.EINVAL)
+    ksft_eq(cm.exception.nl_msg.extack['msg'], 'Unknown attribute type')
+    ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.ifindex')
+
+
 def napi_list_check(nf) -> None:
     with NetdevSimDev(queue_count=100) as nsimdev:
         nsim = nsimdev.nsims[0]
@@ -243,9 +256,16 @@ from lib.py import NetdevFamily, NetdevSimDev, ip
 
 
 def main() -> None:
+    """ Ksft boiler plate main """
     nf = NetdevFamily()
-    ksft_run([empty_check, lo_check, page_pool_check, napi_list_check,
-              dev_set_threaded, napi_set_threaded, nsim_rxq_reset_down],
+    ksft_run([empty_check,
+              lo_check,
+              dev_dump_reject_attr,
+              napi_list_check,
+              napi_set_threaded,
+              dev_set_threaded,
+              nsim_rxq_reset_down,
+              page_pool_check],
              args=(nf, ))
     ksft_exit()
 
-- 
2.53.0

[PATCH net-next v2 4/4] selftests: net: add test for Netlink policy dumps

[Jakub Kicinski]

Add validation for the nlctrl family, accessing family info and
dumping policies.

  TAP version 13
  1..4
  ok 1 nl_nlctrl.getfamily_do
  ok 2 nl_nlctrl.getfamily_dump
  ok 3 nl_nlctrl.getpolicy_dump
  ok 4 nl_nlctrl.getpolicy_by_op
  # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/Makefile          |   1 +
 .../testing/selftests/net/lib/py/__init__.py  |   5 +-
 tools/testing/selftests/net/lib/py/ynl.py     |  10 +-
 tools/testing/selftests/net/nl_nlctrl.py      | 135 ++++++++++++++++++
 4 files changed, 148 insertions(+), 3 deletions(-)
 create mode 100755 tools/testing/selftests/net/nl_nlctrl.py

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index a24ea64e2ae8..6bced3ed798b 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -65,6 +65,7 @@ TEST_PROGS := \
 	netns-name.sh \
 	netns-sysctl.sh \
 	nl_netdev.py \
+	nl_nlctrl.py \
 	pmtu.sh \
 	psock_snd.sh \
 	reuseaddr_ports_exhausted.sh \
diff --git a/tools/testing/selftests/net/lib/py/__init__.py b/tools/testing/selftests/net/lib/py/__init__.py
index 54e84282781c..e0c920041a58 100644
--- a/tools/testing/selftests/net/lib/py/__init__.py
+++ b/tools/testing/selftests/net/lib/py/__init__.py
@@ -15,7 +15,8 @@ from .nsim import NetdevSim, NetdevSimDev
 from .utils import CmdExitFailure, fd_read_timeout, cmd, bkg, defer, \
     bpftool, ip, ethtool, bpftrace, rand_port, rand_ports, wait_port_listen, \
     wait_file, tool
-from .ynl import NlError, YnlFamily, EthtoolFamily, NetdevFamily, RtnlFamily, RtnlAddrFamily
+from .ynl import NlError, NlctrlFamily, YnlFamily, \
+    EthtoolFamily, NetdevFamily, RtnlFamily, RtnlAddrFamily
 from .ynl import NetshaperFamily, DevlinkFamily, PSPFamily, Netlink
 
 __all__ = ["KSRC",
@@ -31,4 +32,4 @@ __all__ = ["KSRC",
            "NetdevSim", "NetdevSimDev",
            "NetshaperFamily", "DevlinkFamily", "PSPFamily", "NlError",
            "YnlFamily", "EthtoolFamily", "NetdevFamily", "RtnlFamily",
-           "RtnlAddrFamily", "Netlink"]
+           "NlctrlFamily", "RtnlAddrFamily", "Netlink"]
diff --git a/tools/testing/selftests/net/lib/py/ynl.py b/tools/testing/selftests/net/lib/py/ynl.py
index e8aa97936f6c..2e567062aa6c 100644
--- a/tools/testing/selftests/net/lib/py/ynl.py
+++ b/tools/testing/selftests/net/lib/py/ynl.py
@@ -30,7 +30,8 @@ from .ksft import ksft_pr, ktap_result
 __all__ = [
     "NlError", "NlPolicy", "Netlink", "YnlFamily", "SPEC_PATH",
     "EthtoolFamily", "RtnlFamily", "RtnlAddrFamily",
-    "NetdevFamily", "NetshaperFamily", "DevlinkFamily", "PSPFamily",
+    "NetdevFamily", "NetshaperFamily", "NlctrlFamily", "DevlinkFamily",
+    "PSPFamily",
 ]
 
 #
@@ -63,6 +64,13 @@ __all__ = [
         super().__init__((SPEC_PATH / Path('net_shaper.yaml')).as_posix(),
                          schema='', recv_size=recv_size)
 
+
+class NlctrlFamily(YnlFamily):
+    def __init__(self, recv_size=0):
+        super().__init__((SPEC_PATH / Path('nlctrl.yaml')).as_posix(),
+                         schema='', recv_size=recv_size)
+
+
 class DevlinkFamily(YnlFamily):
     def __init__(self, recv_size=0):
         super().__init__((SPEC_PATH / Path('devlink.yaml')).as_posix(),
diff --git a/tools/testing/selftests/net/nl_nlctrl.py b/tools/testing/selftests/net/nl_nlctrl.py
new file mode 100755
index 000000000000..d19e206c2c02
--- /dev/null
+++ b/tools/testing/selftests/net/nl_nlctrl.py
@@ -0,0 +1,135 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+"""
+Tests for the nlctrl genetlink family (family info and policy dumps).
+"""
+
+from lib.py import ksft_run, ksft_exit
+from lib.py import ksft_eq, ksft_ge, ksft_true, ksft_in, ksft_not_in
+from lib.py import NetdevFamily, EthtoolFamily, NlctrlFamily
+
+
+def getfamily_do(ctrl) -> None:
+    """Query a single family by name and validate its ops."""
+    fam = ctrl.getfamily({'family-name': 'netdev'})
+    ksft_eq(fam['family-name'], 'netdev')
+    ksft_true(fam['family-id'] > 0)
+
+    # The format of ops is quite odd, [{$idx: {"id"...}}, {$idx: {"id"...}}]
+    # Discard the indices and re-key by command id.
+    ops_by_id = {v['id']: v for op in fam['ops'] for v in op.values()}
+    ksft_eq(len(ops_by_id), len(fam['ops']))
+
+    # All ops should have a policy (either do or dump has one)
+    for op in ops_by_id.values():
+        ksft_in('cmd-cap-haspol', op['flags'],
+                comment=f"op {op['id']} missing haspol")
+
+    # dev-get (id 1) should support both do and dump
+    ksft_in('cmd-cap-do', ops_by_id[1]['flags'])
+    ksft_in('cmd-cap-dump', ops_by_id[1]['flags'])
+
+    # qstats-get (id 12) is dump-only
+    ksft_not_in('cmd-cap-do', ops_by_id[12]['flags'])
+    ksft_in('cmd-cap-dump', ops_by_id[12]['flags'])
+
+    # napi-set (id 14) is do-only and requires admin
+    ksft_in('cmd-cap-do', ops_by_id[14]['flags'])
+    ksft_not_in('cmd-cap-dump', ops_by_id[14]['flags'])
+    ksft_in('admin-perm', ops_by_id[14]['flags'])
+
+    # Notification-only commands (dev-add/del/change-ntf etc.) must
+    # not appear in the ops list since they have no do/dump handlers.
+    for ntf_id in [2, 3, 4, 6, 7, 8]:
+        ksft_not_in(ntf_id, ops_by_id,
+                    comment=f"ntf-only cmd {ntf_id} should not be in ops")
+
+
+def getfamily_dump(ctrl) -> None:
+    """Dump all families and verify expected entries."""
+    families = ctrl.getfamily({}, dump=True)
+    ksft_ge(len(families), 2)
+
+    names = [f['family-name'] for f in families]
+    ksft_in('nlctrl', names, comment="nlctrl not found in family dump")
+    ksft_in('netdev', names, comment="netdev not found in family dump")
+
+
+def getpolicy_dump(_ctrl) -> None:
+    """Dump policies for ops using get_policy() and validate results.
+
+    Test with netdev (split ops) where do and dump can have different
+    policies, and with ethtool (full ops) where they always share one.
+    """
+    # -- netdev (split ops) --
+    ndev = NetdevFamily()
+
+    # dev-get: do has a real policy with ifindex, dump has no policy
+    # (only the reject-all policy with maxattr=0)
+    pol = ndev.get_policy('dev-get', 'do')
+    ksft_in('ifindex', pol.attrs,
+            comment="dev-get do policy should have ifindex")
+    ksft_eq(pol.attrs.get('ifindex', {}).get('type'), 'u32')
+
+    pol_dump = ndev.get_policy('dev-get', 'dump')
+    ksft_eq(len(pol_dump.attrs), 0,
+            comment="dev-get should not accept any attrs")
+
+    # napi-get: both do and dump have real policies
+    pol_do = ndev.get_policy('napi-get', 'do')
+    ksft_ge(len(pol_do.attrs), 1)
+
+    pol_dump = ndev.get_policy('napi-get', 'dump')
+    ksft_ge(len(pol_dump.attrs), 1)
+
+    # -- ethtool (full ops) --
+    et = EthtoolFamily()
+
+    # strset-get (has both do and dump, full ops share policy)
+    pol_do = et.get_policy('strset-get', 'do')
+    ksft_ge(len(pol_do.attrs), 1, comment="strset-get should have a do policy")
+
+    pol_dump = et.get_policy('strset-get', 'dump')
+    ksft_ge(len(pol_dump.attrs), 1,
+            comment="strset-get should have a dump policy")
+
+    # Same policy means same attribute names
+    ksft_eq(set(pol_do.attrs.keys()), set(pol_dump.attrs.keys()))
+
+    # linkinfo-set is do-only (SET command), no dump
+    pol_do = et.get_policy('linkinfo-set', 'do')
+    ksft_ge(len(pol_do.attrs), 1,
+            comment="linkinfo-set should have a do policy")
+
+    pol_dump = et.get_policy('linkinfo-set', 'dump')
+    ksft_eq(pol_dump, None,
+            comment="linkinfo-set should not have a dump policy")
+
+
+def getpolicy_by_op(_ctrl) -> None:
+    """Query policy for specific ops, check attr names are resolved."""
+    ndev = NetdevFamily()
+
+    # dev-get do policy should have named attributes from the spec
+    pol = ndev.get_policy('dev-get', 'do')
+    ksft_ge(len(pol.attrs), 1)
+    # All attr names should be resolved (no 'attr-N' fallbacks)
+    for name in pol.attrs:
+        ksft_true(not name.startswith('attr-'),
+                  comment=f"unresolved attr name: {name}")
+
+
+def main() -> None:
+    """ Ksft boiler plate main """
+    ctrl = NlctrlFamily()
+    ksft_run([getfamily_do,
+              getfamily_dump,
+              getpolicy_dump,
+              getpolicy_by_op],
+             args=(ctrl, ))
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()
-- 
2.53.0

Re: [PATCH net-next v2 0/4] genetlink: apply reject policy for split ops on the dispatch path

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 10 Mar 2026 20:28:35 -0700 you wrote:
> Looks like I somehow missed adding default reject policies to commands
> in families using split Netlink ops. I realized this randomly trying
> to dump page pools for a specific device and always getting all of them
> back. The per-device dump is simply not implemented so the request
> should have been rejected. Patch 2 is the real change, the rest is
> just accompaniment.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/4] genetlink: use maxattr of 0 for the reject policy
    https://git.kernel.org/netdev/net-next/c/b93ec16310b4
  - [net-next,v2,2/4] genetlink: apply reject policy for split ops on the dispatch path
    https://git.kernel.org/netdev/net-next/c/e3a5b7f8ef2a
  - [net-next,v2,3/4] selftests: net: make sure that Netlink rejects unknown attrs in dump
    https://git.kernel.org/netdev/net-next/c/e911be835432
  - [net-next,v2,4/4] selftests: net: add test for Netlink policy dumps
    https://git.kernel.org/netdev/net-next/c/c1f9a89b0c90

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v2 0/4] genetlink: apply reject policy for split ops on the dispatch path

[Vladimir Oltean]

Hi Jakub,

On Tue, Mar 10, 2026 at 08:28:35PM -0700, Jakub Kicinski wrote:
> Looks like I somehow missed adding default reject policies to commands
> in families using split Netlink ops. I realized this randomly trying
> to dump page pools for a specific device and always getting all of them
> back. The per-device dump is simply not implemented so the request
> should have been rejected. Patch 2 is the real change, the rest is
> just accompaniment.
> 
> v2:
>  - add patch 1 to avoid breaking devlink
>  - add a lot more tests
> v1: https://lore.kernel.org/20260307204425.1900467-1-kuba@kernel.org
> 
> Jakub Kicinski (4):
>   genetlink: use maxattr of 0 for the reject policy
>   genetlink: apply reject policy for split ops on the dispatch path
>   selftests: net: make sure that Netlink rejects unknown attrs in dump
>   selftests: net: add test for Netlink policy dumps
> 
>  tools/testing/selftests/net/Makefile          |   1 +
>  net/netlink/genetlink.c                       |  20 +--
>  net/netlink/policy.c                          |   4 +-
>  .../testing/selftests/net/lib/py/__init__.py  |   5 +-
>  tools/testing/selftests/net/lib/py/ynl.py     |  10 +-
>  tools/testing/selftests/net/nl_netdev.py      |  32 ++++-
>  tools/testing/selftests/net/nl_nlctrl.py      | 135 ++++++++++++++++++
>  7 files changed, 186 insertions(+), 21 deletions(-)
>  create mode 100755 tools/testing/selftests/net/nl_nlctrl.py
> 
> -- 
> 2.53.0
> 
> 

It looks like this patch set broke at least filtered DEVLINK_CMD_GET/
DEVLINK_CMD_INFO_GET dumps in general and mv88e6xxx_dump in particular
(https://github.com/lunn/mv88e6xxx_dump).

I didn't follow the split ops development, but here it seems that the
idea to reject unknown attributes in dumps at the genetlink core layer
interacts negatively with devlink's use of split ops. See

int devlink_nl_dumpit(struct sk_buff *msg, struct netlink_callback *cb,
		      devlink_nl_dump_one_func_t *dump_one)
{
	const struct genl_info *info = genl_info_dump(cb);
	struct nlattr **attrs = info->attrs;
	int flags = NLM_F_MULTI;

	if (attrs &&								\
	    (attrs[DEVLINK_ATTR_BUS_NAME] || attrs[DEVLINK_ATTR_DEV_NAME] ||	|
	     attrs[DEVLINK_ATTR_INDEX]))					| Your patch is shunting
		return devlink_nl_inst_single_dumpit(msg, cb, flags, dump_one,	| this entire branch
						     attrs);			|
	else									/
		return devlink_nl_inst_iter_dumpit(msg, cb, flags, dump_one);   // only this will work
}

In mv88e6xxx_dump we have the following affected pattern
(DEVLINK_CMD_INFO_GET netlink command with the NLM_F_DUMP flag which
also attaches attributes):

static void get_info(struct mv88e6xxx_ctx *ctx)
{
	struct nlmsghdr *nlh;
	uint16_t flags = NLM_F_REQUEST | NLM_F_ACK | NLM_F_DUMP;
	int err;

	nlh = mnlg_msg_prepare(ctx->nlg, DEVLINK_CMD_INFO_GET, flags);
	mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, ctx->bus_name);
	mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, ctx->dev_name);

	err = _mnlg_socket_sndrcv(ctx->nlg, nlh, get_info_cb, ctx);
	if (err) {
		printf("Unable to get devices info\n");
		exit(EXIT_FAILURE);
	}
}

In the kernel, the devlink_nl_ops[] split_ops item for DEVLINK_CMD_INFO_GET
is _not_ defined with a policy for attributes. So your patch rejects it,
and that causes the regression.

I didn't review the entire devlink code, but I did review the entire
mv88e6xxx_dump code, and that is the only breakage.

We also have the following unaffected user space calls:

1.
static void list(struct mv88e6xxx_ctx *ctx)
{
	struct nlmsghdr *nlh;
	uint16_t flags = NLM_F_REQUEST | NLM_F_ACK | NLM_F_DUMP;

	nlh = mnlg_msg_prepare(ctx->nlg, DEVLINK_CMD_GET, flags);
	_mnlg_socket_sndrcv(ctx->nlg, nlh, list_cb, ctx);
}

The split_op for DEVLINK_CMD_GET also does not have a policy for
attributes, but we do not populate attributes (OK). The code does go
through devlink_nl_dumpit(), so the regression potential is there, it is
just not triggered with mv88e6xxx_dump.

2.
static void delete_snapshots(struct mv88e6xxx_ctx *ctx)
{
	struct nlmsghdr *nlh;
	uint16_t flags = NLM_F_REQUEST | NLM_F_ACK | NLM_F_DUMP;

	/* Sending a new message while decoding an older message
	 * results in problems. So keep repeating until all regions
	 * snapshots are gone. */
	do {
		ctx->repeat = false;
		nlh = mnlg_msg_prepare(ctx->nlg, DEVLINK_CMD_REGION_GET, flags);
		mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, ctx->bus_name);
		mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, ctx->dev_name);

		_mnlg_socket_sndrcv(ctx->nlg, nlh, delete_snapshots_cb, ctx);
	} while (ctx->repeat);
}

The split_op for DEVLINK_CMD_REGION_GET *does* have a policy for the
attributes that we add to this command when emitting it with NLM_F_DUMP.

3.
static int dump_snapshot_port_id(struct mv88e6xxx_ctx *ctx,
				 uint32_t port, const char *region_name,
				 uint32_t id)
{
	struct nlmsghdr *nlh;
	int err;

	nlh = mnlg_msg_prepare(ctx->nlg, DEVLINK_CMD_REGION_READ,
			       NLM_F_REQUEST | NLM_F_ACK | NLM_F_DUMP);

	mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, ctx->bus_name);
	mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, ctx->dev_name);
	if (port != ~0)
		mnl_attr_put_u32(nlh, DEVLINK_ATTR_PORT_INDEX, port);
	mnl_attr_put_strz(nlh, DEVLINK_ATTR_REGION_NAME, region_name);
	mnl_attr_put_u32(nlh, DEVLINK_ATTR_REGION_SNAPSHOT_ID, id);

	err = _mnlg_socket_sndrcv(ctx->nlg, nlh, dump_snapshot_cb, ctx);
	if (err)
		printf("Unable to dump snapshot %s\n", region_name);

	return err;
}

Same discussion for DEVLINK_CMD_REGION_READ. It does have a policy for
attributes.


I'm not sending a patch because I don't know what patch to send.

There are multiple directions, really.

(a) semantically, I don't know whether what get_info() does makes a lot
of sense. It requests NLM_F_DUMP, requesting a listing of all attributes,
and it then also passes DEVLINK_ATTR_BUS_NAME and DEVLINK_ATTR_DEV_NAME,
to filter for a single device out of that dump. That should be semantically
equivalent to not specifying NLM_F_DUMP, because in the kernel that goes
through the doit split op for DEVLINK_CMD_INFO_GET (which does have a policy),
not through the dumpit one. And it even makes more sense - instead of
dumping and telling the kernel to filter, just get the info for the
device to which you already have a handle.

I'm personally OK with saying user space is odd in this case, and fixing it.
If Andrew is OK too and will accept a patch removing NLM_F_DUMP from
get_info(), I can send this to mv88e6xxx_dump.

But technically it is an UAPI regression, so we should take that at face
value. Here is where the problems begin - I haven't reviewed all
possible dumpit commands, and I don't have time to do so.

(b) we can exempt all legacy split ops from your blanket policy rejection.
Essentially make genl_op_fill_in_reject_policy_split() do the same thing
as genl_op_fill_in_reject_policy():

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 0da39eaed255..61d249b9b089 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -114,7 +114,7 @@ static void
 genl_op_fill_in_reject_policy_split(const struct genl_family *family,
 				    struct genl_split_ops *op)
 {
-	if (op->policy)
+	if (op->policy || op->cmd < family->resv_start_op)
 		return;
 
 	op->policy = genl_policy_reject_all;

I think you are not in favour of this approach, because in commit
e3a5b7f8ef2a ("genetlink: apply reject policy for split ops on the
dispatch path") you said:

    If anyone reports issues we should probably fill in fake policies
    for specific ops rather than reverting this.

so in case that remains your desire, we have the following direction:

(c) add attributes to the netlink schema of all devlink commands that
support dumps (they all go through devlink_nl_dumpit() which parses them).

Essentially the patch below, generated with larger -U25 context for
clarity, and the tools/net/ynl/ynl-regen.sh output omitted for brevity.

--- a/Documentation/netlink/specs/devlink.yaml
+++ b/Documentation/netlink/specs/devlink.yaml
@@ -1318,50 +1318,52 @@ operations:
   enum-model: directional
   list:
     -
       name: get
       doc: Get devlink instances.
       attribute-set: devlink
       dont-validate: [strict, dump]
       do:
         pre: devlink-nl-pre-doit
         post: devlink-nl-post-doit
         request:
           value: 1
           attributes: &dev-id-attrs
             - bus-name
             - dev-name
             - index
         reply: &get-reply
           value: 3
           attributes:
             - bus-name
             - dev-name
             - index
             - reload-failed
             - dev-stats
       dump:
+        request:
+          attributes: *dev-id-attrs
         reply: *get-reply

     -
       name: port-get
       doc: Get devlink port instances.
       attribute-set: devlink
       dont-validate: [strict]
       do:
         pre: devlink-nl-pre-doit-port
         post: devlink-nl-post-doit
         request:
           value: 5
           attributes: &port-id-attrs
             - bus-name
             - dev-name
             - index
             - port-index
         reply:
           value: 7
           attributes: *port-id-attrs
       dump:
         request:
           attributes: *dev-id-attrs
         reply:
           value: 3  # due to a bug, port dump returns DEVLINK_CMD_NEW
@@ -1984,50 +1986,52 @@ operations:
       name: info-get
       doc: |
         Get device information, like driver name, hardware and firmware versions
         etc.
       attribute-set: devlink
       dont-validate: [strict, dump]
       do:
         pre: devlink-nl-pre-doit
         post: devlink-nl-post-doit
         request:
           value: 51
           attributes: *dev-id-attrs
         reply: &info-get-reply
           value: 51
           attributes:
             - bus-name
             - dev-name
             - index
             - info-driver-name
             - info-serial-number
             - info-version-fixed
             - info-version-running
             - info-version-stored
             - info-board-serial-number
       dump:
+        request:
+          attributes: *dev-id-attrs
         reply: *info-get-reply

     -
       name: health-reporter-get
       doc: Get health reporter instances.
       attribute-set: devlink
       dont-validate: [strict]
       do:
         pre: devlink-nl-pre-doit-port-optional
         post: devlink-nl-post-doit
         request:
           attributes: &health-reporter-id-attrs
             - bus-name
             - dev-name
             - index
             - port-index
             - health-reporter-name
         reply: &health-reporter-get-reply
           attributes: *health-reporter-id-attrs
       dump:
         request:
           attributes: *port-id-attrs
         reply: *health-reporter-get-reply

     -

But this is where things get a bit more interesting. Technically only
devlink info-get regressed in mv88e6xxx_dump. Do we fix devlink get too?
How far should this investigation go? I don't really want to go into a
very deep rabbit hole. In this sense option (b) may be preferable if
avoiding UAPI regressions is our main objective.

Thanks for reading what turned out a rather long email, and sorry if I
got anything wrong. I'm not a netlink expert by any stretch of imagination.