Merge branch 'genetlink-apply-reject-policy-for-split-ops-on-the-dispatch-path'

Jakub Kicinski says:

====================
genetlink: apply reject policy for split ops on the dispatch path

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.
====================

Link: https://patch.msgid.link/20260311032839.417748-1-kuba@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

7 files changed, 186 insertions, 21 deletions

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index a23d4c51c089..d251d894afd4 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)
@@ -250,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;
 		}
 
@@ -934,12 +929,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++) {
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 @@ except ModuleNotFoundError as e:
 __all__ = [
     "NlError", "NlPolicy", "Netlink", "YnlFamily", "SPEC_PATH",
     "EthtoolFamily", "RtnlFamily", "RtnlAddrFamily",
-    "NetdevFamily", "NetshaperFamily", "DevlinkFamily", "PSPFamily",
+    "NetdevFamily", "NetshaperFamily", "NlctrlFamily", "DevlinkFamily",
+    "PSPFamily",
 ]
 
 #
@@ -63,6 +64,13 @@ class NetshaperFamily(YnlFamily):
         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_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 @@ def lo_check(nf) -> None:
     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 @@ def page_pool_check(nf) -> None:
 
 
 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()
 
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()