* [PATCH net-next 0/6] ethtool: allow dumping policies to user space
@ 2020-10-05 15:57 Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 1/6] ethtool: wire up get policies to ops Jakub Kicinski
` (5 more replies)
0 siblings, 6 replies; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 15:57 UTC (permalink / raw)
To: davem
Cc: netdev, kernel-team, johannes, jiri, andrew, mkubecek, Jakub Kicinski
Hi!
This series wires up ethtool policies to ops, so they can be
dumped to user space for feature discovery.
First two patches wire up GET, third patch wires up SET.
Next - take care of linking up nested policies for the header
(which is what we actually care about right now). And once header
policy is linked make sure that attribute range validation is
done by policy, not code conditions for flags. New type of
policy is needed to validate masks (patch 5).
Netlink as always staying a step ahead of all the other kernel
API interfaces :)
Jakub Kicinski (6):
ethtool: wire up get policies to ops
ethtool: use the attributes parsed by the core in get commands
ethtool: wire up set policies to ops
ethtool: link up ethnl_header_policy as a nested policy
netlink: add mask validation
ethtool: specify which header flags are supported per command
include/net/netlink.h | 11 ++++
include/uapi/linux/netlink.h | 2 +
lib/nlattr.c | 36 ++++++++++
net/ethtool/cabletest.c | 30 +++------
net/ethtool/channels.c | 22 +++----
net/ethtool/coalesce.c | 22 +++----
net/ethtool/debug.c | 20 ++----
net/ethtool/eee.c | 21 +++---
net/ethtool/features.c | 22 +++----
net/ethtool/linkinfo.c | 22 +++----
net/ethtool/linkmodes.c | 22 +++----
net/ethtool/linkstate.c | 8 +--
net/ethtool/netlink.c | 123 +++++++++++++++++++++++++----------
net/ethtool/netlink.h | 33 +++++++++-
net/ethtool/pause.c | 19 ++----
net/ethtool/privflags.c | 22 +++----
net/ethtool/rings.c | 20 ++----
net/ethtool/strset.c | 6 +-
net/ethtool/tsinfo.c | 7 +-
net/ethtool/tunnels.c | 42 +++++-------
net/ethtool/wol.c | 19 ++----
net/netlink/policy.c | 8 +++
22 files changed, 303 insertions(+), 234 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 15:57 [PATCH net-next 0/6] ethtool: allow dumping policies to user space Jakub Kicinski
@ 2020-10-05 15:57 ` Jakub Kicinski
2020-10-05 18:56 ` Johannes Berg
2020-10-05 15:57 ` [PATCH net-next 2/6] ethtool: use the attributes parsed by the core in get commands Jakub Kicinski
` (4 subsequent siblings)
5 siblings, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 15:57 UTC (permalink / raw)
To: davem
Cc: netdev, kernel-team, johannes, jiri, andrew, mkubecek, Jakub Kicinski
To make use of genetlink code validating and parsing attributes
for us, as well as dumping policies to user space wrie up policies
for get commands in struct nla_policy of the ethtool family.
For ease of review this commit just does the repetitive wiring up.
Next changes will use the core-parsed attrs and migrate set commands.
For every ETHTOOL_MSG_*_GET:
- add 'ethnl_' prefix to policy name
- add extern declaration in net/ethtool/netlink.h
- wire up the policy in ethtool_genl_ops[].
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/ethtool/channels.c | 6 +++---
net/ethtool/coalesce.c | 6 +++---
net/ethtool/debug.c | 5 ++---
net/ethtool/eee.c | 5 ++---
net/ethtool/features.c | 6 +++---
net/ethtool/linkinfo.c | 6 +++---
net/ethtool/linkmodes.c | 6 +++---
net/ethtool/linkstate.c | 6 +++---
net/ethtool/netlink.c | 32 ++++++++++++++++++++++++++++++++
net/ethtool/netlink.h | 16 ++++++++++++++++
net/ethtool/pause.c | 5 ++---
net/ethtool/privflags.c | 6 +++---
net/ethtool/rings.c | 5 ++---
net/ethtool/strset.c | 4 ++--
net/ethtool/tsinfo.c | 5 ++---
net/ethtool/tunnels.c | 6 +++---
net/ethtool/wol.c | 5 ++---
17 files changed, 86 insertions(+), 44 deletions(-)
diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c
index 9ecda09ecb11..d3ef94c5ee59 100644
--- a/net/ethtool/channels.c
+++ b/net/ethtool/channels.c
@@ -17,8 +17,8 @@ struct channels_reply_data {
#define CHANNELS_REPDATA(__reply_base) \
container_of(__reply_base, struct channels_reply_data, base)
-static const struct nla_policy
-channels_get_policy[ETHTOOL_A_CHANNELS_MAX + 1] = {
+const struct nla_policy
+ethnl_channels_get_policy[ETHTOOL_A_CHANNELS_MAX + 1] = {
[ETHTOOL_A_CHANNELS_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_CHANNELS_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_CHANNELS_RX_MAX] = { .type = NLA_REJECT },
@@ -102,7 +102,7 @@ const struct ethnl_request_ops ethnl_channels_request_ops = {
.max_attr = ETHTOOL_A_CHANNELS_MAX,
.req_info_size = sizeof(struct channels_req_info),
.reply_data_size = sizeof(struct channels_reply_data),
- .request_policy = channels_get_policy,
+ .request_policy = ethnl_channels_get_policy,
.prepare_data = channels_prepare_data,
.reply_size = channels_reply_size,
diff --git a/net/ethtool/coalesce.c b/net/ethtool/coalesce.c
index 6afd99042d67..e0d8a269a46d 100644
--- a/net/ethtool/coalesce.c
+++ b/net/ethtool/coalesce.c
@@ -51,8 +51,8 @@ __CHECK_SUPPORTED_OFFSET(COALESCE_TX_USECS_HIGH);
__CHECK_SUPPORTED_OFFSET(COALESCE_TX_MAX_FRAMES_HIGH);
__CHECK_SUPPORTED_OFFSET(COALESCE_RATE_SAMPLE_INTERVAL);
-static const struct nla_policy
-coalesce_get_policy[ETHTOOL_A_COALESCE_MAX + 1] = {
+const struct nla_policy
+ethnl_coalesce_get_policy[ETHTOOL_A_COALESCE_MAX + 1] = {
[ETHTOOL_A_COALESCE_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_COALESCE_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_COALESCE_RX_USECS] = { .type = NLA_REJECT },
@@ -206,7 +206,7 @@ const struct ethnl_request_ops ethnl_coalesce_request_ops = {
.max_attr = ETHTOOL_A_COALESCE_MAX,
.req_info_size = sizeof(struct coalesce_req_info),
.reply_data_size = sizeof(struct coalesce_reply_data),
- .request_policy = coalesce_get_policy,
+ .request_policy = ethnl_coalesce_get_policy,
.prepare_data = coalesce_prepare_data,
.reply_size = coalesce_reply_size,
diff --git a/net/ethtool/debug.c b/net/ethtool/debug.c
index 1bd026a29f3f..a007730320a4 100644
--- a/net/ethtool/debug.c
+++ b/net/ethtool/debug.c
@@ -16,8 +16,7 @@ struct debug_reply_data {
#define DEBUG_REPDATA(__reply_base) \
container_of(__reply_base, struct debug_reply_data, base)
-static const struct nla_policy
-debug_get_policy[ETHTOOL_A_DEBUG_MAX + 1] = {
+const struct nla_policy ethnl_debug_get_policy[ETHTOOL_A_DEBUG_MAX + 1] = {
[ETHTOOL_A_DEBUG_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_DEBUG_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_DEBUG_MSGMASK] = { .type = NLA_REJECT },
@@ -72,7 +71,7 @@ const struct ethnl_request_ops ethnl_debug_request_ops = {
.max_attr = ETHTOOL_A_DEBUG_MAX,
.req_info_size = sizeof(struct debug_req_info),
.reply_data_size = sizeof(struct debug_reply_data),
- .request_policy = debug_get_policy,
+ .request_policy = ethnl_debug_get_policy,
.prepare_data = debug_prepare_data,
.reply_size = debug_reply_size,
diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index 94aa19cff22f..ea9d071b5360 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -19,8 +19,7 @@ struct eee_reply_data {
#define EEE_REPDATA(__reply_base) \
container_of(__reply_base, struct eee_reply_data, base)
-static const struct nla_policy
-eee_get_policy[ETHTOOL_A_EEE_MAX + 1] = {
+const struct nla_policy ethnl_eee_get_policy[ETHTOOL_A_EEE_MAX + 1] = {
[ETHTOOL_A_EEE_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_EEE_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_EEE_MODES_OURS] = { .type = NLA_REJECT },
@@ -122,7 +121,7 @@ const struct ethnl_request_ops ethnl_eee_request_ops = {
.max_attr = ETHTOOL_A_EEE_MAX,
.req_info_size = sizeof(struct eee_req_info),
.reply_data_size = sizeof(struct eee_reply_data),
- .request_policy = eee_get_policy,
+ .request_policy = ethnl_eee_get_policy,
.prepare_data = eee_prepare_data,
.reply_size = eee_reply_size,
diff --git a/net/ethtool/features.c b/net/ethtool/features.c
index 495635f152ba..c2979b3890f9 100644
--- a/net/ethtool/features.c
+++ b/net/ethtool/features.c
@@ -20,8 +20,8 @@ struct features_reply_data {
#define FEATURES_REPDATA(__reply_base) \
container_of(__reply_base, struct features_reply_data, base)
-static const struct nla_policy
-features_get_policy[ETHTOOL_A_FEATURES_MAX + 1] = {
+const struct nla_policy
+ethnl_features_get_policy[ETHTOOL_A_FEATURES_MAX + 1] = {
[ETHTOOL_A_FEATURES_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_FEATURES_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_FEATURES_HW] = { .type = NLA_REJECT },
@@ -123,7 +123,7 @@ const struct ethnl_request_ops ethnl_features_request_ops = {
.max_attr = ETHTOOL_A_FEATURES_MAX,
.req_info_size = sizeof(struct features_req_info),
.reply_data_size = sizeof(struct features_reply_data),
- .request_policy = features_get_policy,
+ .request_policy = ethnl_features_get_policy,
.prepare_data = features_prepare_data,
.reply_size = features_reply_size,
diff --git a/net/ethtool/linkinfo.c b/net/ethtool/linkinfo.c
index 5eaf173eaaca..676aa5a00ac0 100644
--- a/net/ethtool/linkinfo.c
+++ b/net/ethtool/linkinfo.c
@@ -16,8 +16,8 @@ struct linkinfo_reply_data {
#define LINKINFO_REPDATA(__reply_base) \
container_of(__reply_base, struct linkinfo_reply_data, base)
-static const struct nla_policy
-linkinfo_get_policy[ETHTOOL_A_LINKINFO_MAX + 1] = {
+const struct nla_policy
+ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_MAX + 1] = {
[ETHTOOL_A_LINKINFO_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKINFO_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_LINKINFO_PORT] = { .type = NLA_REJECT },
@@ -86,7 +86,7 @@ const struct ethnl_request_ops ethnl_linkinfo_request_ops = {
.max_attr = ETHTOOL_A_LINKINFO_MAX,
.req_info_size = sizeof(struct linkinfo_req_info),
.reply_data_size = sizeof(struct linkinfo_reply_data),
- .request_policy = linkinfo_get_policy,
+ .request_policy = ethnl_linkinfo_get_policy,
.prepare_data = linkinfo_prepare_data,
.reply_size = linkinfo_reply_size,
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index 29dcd675b65a..f32ab3239d15 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -18,8 +18,8 @@ struct linkmodes_reply_data {
#define LINKMODES_REPDATA(__reply_base) \
container_of(__reply_base, struct linkmodes_reply_data, base)
-static const struct nla_policy
-linkmodes_get_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
+const struct nla_policy
+ethnl_linkmodes_get_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
[ETHTOOL_A_LINKMODES_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKMODES_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_LINKMODES_AUTONEG] = { .type = NLA_REJECT },
@@ -151,7 +151,7 @@ const struct ethnl_request_ops ethnl_linkmodes_request_ops = {
.max_attr = ETHTOOL_A_LINKMODES_MAX,
.req_info_size = sizeof(struct linkmodes_req_info),
.reply_data_size = sizeof(struct linkmodes_reply_data),
- .request_policy = linkmodes_get_policy,
+ .request_policy = ethnl_linkmodes_get_policy,
.prepare_data = linkmodes_prepare_data,
.reply_size = linkmodes_reply_size,
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index 4834091ec24c..c86ae67e2486 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -20,8 +20,8 @@ struct linkstate_reply_data {
#define LINKSTATE_REPDATA(__reply_base) \
container_of(__reply_base, struct linkstate_reply_data, base)
-static const struct nla_policy
-linkstate_get_policy[ETHTOOL_A_LINKSTATE_MAX + 1] = {
+const struct nla_policy
+ethnl_linkstate_get_policy[ETHTOOL_A_LINKSTATE_MAX + 1] = {
[ETHTOOL_A_LINKSTATE_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKSTATE_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_LINKSTATE_LINK] = { .type = NLA_REJECT },
@@ -182,7 +182,7 @@ const struct ethnl_request_ops ethnl_linkstate_request_ops = {
.max_attr = ETHTOOL_A_LINKSTATE_MAX,
.req_info_size = sizeof(struct linkstate_req_info),
.reply_data_size = sizeof(struct linkstate_reply_data),
- .request_policy = linkstate_get_policy,
+ .request_policy = ethnl_linkstate_get_policy,
.prepare_data = linkstate_prepare_data,
.reply_size = linkstate_reply_size,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 5c2072765be7..dbabac38c191 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -696,6 +696,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_strset_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_strset_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_LINKINFO_GET,
@@ -703,6 +705,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_linkinfo_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_linkinfo_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_LINKINFO_SET,
@@ -715,6 +719,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_linkmodes_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_linkmodes_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_LINKMODES_SET,
@@ -727,6 +733,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_linkstate_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_linkstate_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_DEBUG_GET,
@@ -734,6 +742,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_debug_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_debug_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_DEBUG_SET,
@@ -747,6 +757,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_wol_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_wol_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_WOL_SET,
@@ -759,6 +771,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_features_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_features_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_FEATURES_SET,
@@ -771,6 +785,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_privflags_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_privflags_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_PRIVFLAGS_SET,
@@ -783,6 +799,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_rings_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_rings_get_policy) - 1,
+
},
{
.cmd = ETHTOOL_MSG_RINGS_SET,
@@ -795,6 +814,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_channels_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_channels_get_policy) - 1,
+
},
{
.cmd = ETHTOOL_MSG_CHANNELS_SET,
@@ -807,6 +829,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_coalesce_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_coalesce_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_COALESCE_SET,
@@ -819,6 +843,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_pause_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_pause_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_PAUSE_SET,
@@ -831,6 +857,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_eee_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_eee_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_EEE_SET,
@@ -843,6 +871,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.start = ethnl_default_start,
.dumpit = ethnl_default_dumpit,
.done = ethnl_default_done,
+ .policy = ethnl_tsinfo_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_tsinfo_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_CABLE_TEST_ACT,
@@ -859,6 +889,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.doit = ethnl_tunnel_info_doit,
.start = ethnl_tunnel_info_start,
.dumpit = ethnl_tunnel_info_dumpit,
+ .policy = ethnl_tunnel_info_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_tunnel_info_get_policy) - 1,
},
};
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index e2085005caac..0100fab5829e 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -349,6 +349,22 @@ extern const struct ethnl_request_ops ethnl_pause_request_ops;
extern const struct ethnl_request_ops ethnl_eee_request_ops;
extern const struct ethnl_request_ops ethnl_tsinfo_request_ops;
+extern const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_MAX + 1];
+extern const struct nla_policy ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_MAX + 1];
+extern const struct nla_policy ethnl_linkmodes_get_policy[ETHTOOL_A_LINKMODES_MAX + 1];
+extern const struct nla_policy ethnl_linkstate_get_policy[ETHTOOL_A_LINKSTATE_MAX + 1];
+extern const struct nla_policy ethnl_debug_get_policy[ETHTOOL_A_DEBUG_MAX + 1];
+extern const struct nla_policy ethnl_wol_get_policy[ETHTOOL_A_WOL_MAX + 1];
+extern const struct nla_policy ethnl_features_get_policy[ETHTOOL_A_FEATURES_MAX + 1];
+extern const struct nla_policy ethnl_privflags_get_policy[ETHTOOL_A_PRIVFLAGS_MAX + 1];
+extern const struct nla_policy ethnl_rings_get_policy[ETHTOOL_A_RINGS_MAX + 1];
+extern const struct nla_policy ethnl_channels_get_policy[ETHTOOL_A_CHANNELS_MAX + 1];
+extern const struct nla_policy ethnl_coalesce_get_policy[ETHTOOL_A_COALESCE_MAX + 1];
+extern const struct nla_policy ethnl_pause_get_policy[ETHTOOL_A_PAUSE_MAX + 1];
+extern const struct nla_policy ethnl_eee_get_policy[ETHTOOL_A_EEE_MAX + 1];
+extern const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1];
+extern const struct nla_policy ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_MAX + 1];
+
int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
int ethnl_set_debug(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/ethtool/pause.c b/net/ethtool/pause.c
index 1980aa7eb2b6..40f36f8d6291 100644
--- a/net/ethtool/pause.c
+++ b/net/ethtool/pause.c
@@ -16,8 +16,7 @@ struct pause_reply_data {
#define PAUSE_REPDATA(__reply_base) \
container_of(__reply_base, struct pause_reply_data, base)
-static const struct nla_policy
-pause_get_policy[ETHTOOL_A_PAUSE_MAX + 1] = {
+const struct nla_policy ethnl_pause_get_policy[ETHTOOL_A_PAUSE_MAX + 1] = {
[ETHTOOL_A_PAUSE_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_PAUSE_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_PAUSE_AUTONEG] = { .type = NLA_REJECT },
@@ -133,7 +132,7 @@ const struct ethnl_request_ops ethnl_pause_request_ops = {
.max_attr = ETHTOOL_A_PAUSE_MAX,
.req_info_size = sizeof(struct pause_req_info),
.reply_data_size = sizeof(struct pause_reply_data),
- .request_policy = pause_get_policy,
+ .request_policy = ethnl_pause_get_policy,
.prepare_data = pause_prepare_data,
.reply_size = pause_reply_size,
diff --git a/net/ethtool/privflags.c b/net/ethtool/privflags.c
index 77447dceb109..7ebab33f2244 100644
--- a/net/ethtool/privflags.c
+++ b/net/ethtool/privflags.c
@@ -18,8 +18,8 @@ struct privflags_reply_data {
#define PRIVFLAGS_REPDATA(__reply_base) \
container_of(__reply_base, struct privflags_reply_data, base)
-static const struct nla_policy
-privflags_get_policy[ETHTOOL_A_PRIVFLAGS_MAX + 1] = {
+const struct nla_policy
+ethnl_privflags_get_policy[ETHTOOL_A_PRIVFLAGS_MAX + 1] = {
[ETHTOOL_A_PRIVFLAGS_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_PRIVFLAGS_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_PRIVFLAGS_FLAGS] = { .type = NLA_REJECT },
@@ -127,7 +127,7 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = {
.max_attr = ETHTOOL_A_PRIVFLAGS_MAX,
.req_info_size = sizeof(struct privflags_req_info),
.reply_data_size = sizeof(struct privflags_reply_data),
- .request_policy = privflags_get_policy,
+ .request_policy = ethnl_privflags_get_policy,
.prepare_data = privflags_prepare_data,
.reply_size = privflags_reply_size,
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 5422526f4eef..eb52639b6ed3 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -15,8 +15,7 @@ struct rings_reply_data {
#define RINGS_REPDATA(__reply_base) \
container_of(__reply_base, struct rings_reply_data, base)
-static const struct nla_policy
-rings_get_policy[ETHTOOL_A_RINGS_MAX + 1] = {
+const struct nla_policy ethnl_rings_get_policy[ETHTOOL_A_RINGS_MAX + 1] = {
[ETHTOOL_A_RINGS_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_RINGS_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_RINGS_RX_MAX] = { .type = NLA_REJECT },
@@ -100,7 +99,7 @@ const struct ethnl_request_ops ethnl_rings_request_ops = {
.max_attr = ETHTOOL_A_RINGS_MAX,
.req_info_size = sizeof(struct rings_req_info),
.reply_data_size = sizeof(struct rings_reply_data),
- .request_policy = rings_get_policy,
+ .request_policy = ethnl_rings_get_policy,
.prepare_data = rings_prepare_data,
.reply_size = rings_reply_size,
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 82707b662fe4..8d30fe0fbe3b 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -99,7 +99,7 @@ struct strset_reply_data {
#define STRSET_REPDATA(__reply_base) \
container_of(__reply_base, struct strset_reply_data, base)
-static const struct nla_policy strset_get_policy[ETHTOOL_A_STRSET_MAX + 1] = {
+const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_MAX + 1] = {
[ETHTOOL_A_STRSET_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_STRSET_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_STRSET_STRINGSETS] = { .type = NLA_NESTED },
@@ -448,7 +448,7 @@ const struct ethnl_request_ops ethnl_strset_request_ops = {
.max_attr = ETHTOOL_A_STRSET_MAX,
.req_info_size = sizeof(struct strset_req_info),
.reply_data_size = sizeof(struct strset_reply_data),
- .request_policy = strset_get_policy,
+ .request_policy = ethnl_strset_get_policy,
.allow_nodev_do = true,
.parse_request = strset_parse_request,
diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
index 7cb5b512b77c..05d501563dc9 100644
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -18,8 +18,7 @@ struct tsinfo_reply_data {
#define TSINFO_REPDATA(__reply_base) \
container_of(__reply_base, struct tsinfo_reply_data, base)
-static const struct nla_policy
-tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1] = {
+const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1] = {
[ETHTOOL_A_TSINFO_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_TSINFO_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_TSINFO_TIMESTAMPING] = { .type = NLA_REJECT },
@@ -135,7 +134,7 @@ const struct ethnl_request_ops ethnl_tsinfo_request_ops = {
.max_attr = ETHTOOL_A_TSINFO_MAX,
.req_info_size = sizeof(struct tsinfo_req_info),
.reply_data_size = sizeof(struct tsinfo_reply_data),
- .request_policy = tsinfo_get_policy,
+ .request_policy = ethnl_tsinfo_get_policy,
.prepare_data = tsinfo_prepare_data,
.reply_size = tsinfo_reply_size,
diff --git a/net/ethtool/tunnels.c b/net/ethtool/tunnels.c
index d93bf2da0f34..d7d4964ea773 100644
--- a/net/ethtool/tunnels.c
+++ b/net/ethtool/tunnels.c
@@ -8,8 +8,8 @@
#include "common.h"
#include "netlink.h"
-static const struct nla_policy
-ethtool_tunnel_info_policy[ETHTOOL_A_TUNNEL_INFO_MAX + 1] = {
+const struct nla_policy
+ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_MAX + 1] = {
[ETHTOOL_A_TUNNEL_INFO_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_TUNNEL_INFO_HEADER] = { .type = NLA_NESTED },
};
@@ -170,7 +170,7 @@ ethnl_tunnel_info_req_parse(struct ethnl_req_info *req_info,
int ret;
ret = nlmsg_parse(nlhdr, GENL_HDRLEN, tb, ETHTOOL_A_TUNNEL_INFO_MAX,
- ethtool_tunnel_info_policy, extack);
+ ethnl_tunnel_info_get_policy, extack);
if (ret < 0)
return ret;
diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
index 1798421e9f1c..d234885fc265 100644
--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c
@@ -17,8 +17,7 @@ struct wol_reply_data {
#define WOL_REPDATA(__reply_base) \
container_of(__reply_base, struct wol_reply_data, base)
-static const struct nla_policy
-wol_get_policy[ETHTOOL_A_WOL_MAX + 1] = {
+const struct nla_policy ethnl_wol_get_policy[ETHTOOL_A_WOL_MAX + 1] = {
[ETHTOOL_A_WOL_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_WOL_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_WOL_MODES] = { .type = NLA_REJECT },
@@ -92,7 +91,7 @@ const struct ethnl_request_ops ethnl_wol_request_ops = {
.max_attr = ETHTOOL_A_WOL_MAX,
.req_info_size = sizeof(struct wol_req_info),
.reply_data_size = sizeof(struct wol_reply_data),
- .request_policy = wol_get_policy,
+ .request_policy = ethnl_wol_get_policy,
.prepare_data = wol_prepare_data,
.reply_size = wol_reply_size,
--
2.26.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 2/6] ethtool: use the attributes parsed by the core in get commands
2020-10-05 15:57 [PATCH net-next 0/6] ethtool: allow dumping policies to user space Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 1/6] ethtool: wire up get policies to ops Jakub Kicinski
@ 2020-10-05 15:57 ` Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 3/6] ethtool: wire up set policies to ops Jakub Kicinski
` (3 subsequent siblings)
5 siblings, 0 replies; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 15:57 UTC (permalink / raw)
To: davem
Cc: netdev, kernel-team, johannes, jiri, andrew, mkubecek, Jakub Kicinski
Now that policies are placed in op structs core will do the
attribute parsing for us. Obviously core only record the first
"layer" of parsed attrs so we still need to parse the sub-attrs
of the nested header attribute.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/ethtool/channels.c | 1 -
net/ethtool/coalesce.c | 1 -
net/ethtool/debug.c | 1 -
net/ethtool/eee.c | 1 -
net/ethtool/features.c | 1 -
net/ethtool/linkinfo.c | 1 -
net/ethtool/linkmodes.c | 1 -
net/ethtool/linkstate.c | 1 -
net/ethtool/netlink.c | 32 ++++++++++----------------------
net/ethtool/netlink.h | 2 --
net/ethtool/pause.c | 1 -
net/ethtool/privflags.c | 1 -
net/ethtool/rings.c | 1 -
net/ethtool/strset.c | 1 -
net/ethtool/tsinfo.c | 1 -
net/ethtool/tunnels.c | 35 +++++++++++------------------------
net/ethtool/wol.c | 1 -
17 files changed, 21 insertions(+), 62 deletions(-)
diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c
index d3ef94c5ee59..76994c8f32a0 100644
--- a/net/ethtool/channels.c
+++ b/net/ethtool/channels.c
@@ -102,7 +102,6 @@ const struct ethnl_request_ops ethnl_channels_request_ops = {
.max_attr = ETHTOOL_A_CHANNELS_MAX,
.req_info_size = sizeof(struct channels_req_info),
.reply_data_size = sizeof(struct channels_reply_data),
- .request_policy = ethnl_channels_get_policy,
.prepare_data = channels_prepare_data,
.reply_size = channels_reply_size,
diff --git a/net/ethtool/coalesce.c b/net/ethtool/coalesce.c
index e0d8a269a46d..77d83062faa2 100644
--- a/net/ethtool/coalesce.c
+++ b/net/ethtool/coalesce.c
@@ -206,7 +206,6 @@ const struct ethnl_request_ops ethnl_coalesce_request_ops = {
.max_attr = ETHTOOL_A_COALESCE_MAX,
.req_info_size = sizeof(struct coalesce_req_info),
.reply_data_size = sizeof(struct coalesce_reply_data),
- .request_policy = ethnl_coalesce_get_policy,
.prepare_data = coalesce_prepare_data,
.reply_size = coalesce_reply_size,
diff --git a/net/ethtool/debug.c b/net/ethtool/debug.c
index a007730320a4..f553ef6a99e4 100644
--- a/net/ethtool/debug.c
+++ b/net/ethtool/debug.c
@@ -71,7 +71,6 @@ const struct ethnl_request_ops ethnl_debug_request_ops = {
.max_attr = ETHTOOL_A_DEBUG_MAX,
.req_info_size = sizeof(struct debug_req_info),
.reply_data_size = sizeof(struct debug_reply_data),
- .request_policy = ethnl_debug_get_policy,
.prepare_data = debug_prepare_data,
.reply_size = debug_reply_size,
diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index ea9d071b5360..9ee03444fb8f 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -121,7 +121,6 @@ const struct ethnl_request_ops ethnl_eee_request_ops = {
.max_attr = ETHTOOL_A_EEE_MAX,
.req_info_size = sizeof(struct eee_req_info),
.reply_data_size = sizeof(struct eee_reply_data),
- .request_policy = ethnl_eee_get_policy,
.prepare_data = eee_prepare_data,
.reply_size = eee_reply_size,
diff --git a/net/ethtool/features.c b/net/ethtool/features.c
index c2979b3890f9..99944190f398 100644
--- a/net/ethtool/features.c
+++ b/net/ethtool/features.c
@@ -123,7 +123,6 @@ const struct ethnl_request_ops ethnl_features_request_ops = {
.max_attr = ETHTOOL_A_FEATURES_MAX,
.req_info_size = sizeof(struct features_req_info),
.reply_data_size = sizeof(struct features_reply_data),
- .request_policy = ethnl_features_get_policy,
.prepare_data = features_prepare_data,
.reply_size = features_reply_size,
diff --git a/net/ethtool/linkinfo.c b/net/ethtool/linkinfo.c
index 676aa5a00ac0..b7142d8e2baf 100644
--- a/net/ethtool/linkinfo.c
+++ b/net/ethtool/linkinfo.c
@@ -86,7 +86,6 @@ const struct ethnl_request_ops ethnl_linkinfo_request_ops = {
.max_attr = ETHTOOL_A_LINKINFO_MAX,
.req_info_size = sizeof(struct linkinfo_req_info),
.reply_data_size = sizeof(struct linkinfo_reply_data),
- .request_policy = ethnl_linkinfo_get_policy,
.prepare_data = linkinfo_prepare_data,
.reply_size = linkinfo_reply_size,
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index f32ab3239d15..e23e600b73cb 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -151,7 +151,6 @@ const struct ethnl_request_ops ethnl_linkmodes_request_ops = {
.max_attr = ETHTOOL_A_LINKMODES_MAX,
.req_info_size = sizeof(struct linkmodes_req_info),
.reply_data_size = sizeof(struct linkmodes_reply_data),
- .request_policy = ethnl_linkmodes_get_policy,
.prepare_data = linkmodes_prepare_data,
.reply_size = linkmodes_reply_size,
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index c86ae67e2486..3f0ab6e84fce 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -182,7 +182,6 @@ const struct ethnl_request_ops ethnl_linkstate_request_ops = {
.max_attr = ETHTOOL_A_LINKSTATE_MAX,
.req_info_size = sizeof(struct linkstate_req_info),
.reply_data_size = sizeof(struct linkstate_reply_data),
- .request_policy = ethnl_linkstate_get_policy,
.prepare_data = linkstate_prepare_data,
.reply_size = linkstate_reply_size,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index dbabac38c191..2f317caa077a 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -247,7 +247,7 @@ static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
/**
* ethnl_default_parse() - Parse request message
* @req_info: pointer to structure to put data into
- * @nlhdr: pointer to request message header
+ * @tb: parsed attributes
* @net: request netns
* @request_ops: struct request_ops for request type
* @extack: netlink extack for error reporting
@@ -259,37 +259,24 @@ static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
* Return: 0 on success or negative error code
*/
static int ethnl_default_parse(struct ethnl_req_info *req_info,
- const struct nlmsghdr *nlhdr, struct net *net,
+ struct nlattr **tb, struct net *net,
const struct ethnl_request_ops *request_ops,
struct netlink_ext_ack *extack, bool require_dev)
{
- struct nlattr **tb;
int ret;
- tb = kmalloc_array(request_ops->max_attr + 1, sizeof(tb[0]),
- GFP_KERNEL);
- if (!tb)
- return -ENOMEM;
-
- ret = nlmsg_parse(nlhdr, GENL_HDRLEN, tb, request_ops->max_attr,
- request_ops->request_policy, extack);
- if (ret < 0)
- goto out;
ret = ethnl_parse_header_dev_get(req_info, tb[request_ops->hdr_attr],
net, extack, require_dev);
if (ret < 0)
- goto out;
+ return ret;
if (request_ops->parse_request) {
ret = request_ops->parse_request(req_info, tb, extack);
if (ret < 0)
- goto out;
+ return ret;
}
- ret = 0;
-out:
- kfree(tb);
- return ret;
+ return 0;
}
/**
@@ -334,8 +321,8 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
return -ENOMEM;
}
- ret = ethnl_default_parse(req_info, info->nlhdr, genl_info_net(info), ops,
- info->extack, !ops->allow_nodev_do);
+ ret = ethnl_default_parse(req_info, info->attrs, genl_info_net(info),
+ ops, info->extack, !ops->allow_nodev_do);
if (ret < 0)
goto err_dev;
ethnl_init_reply_data(reply_data, ops, req_info->dev);
@@ -480,6 +467,7 @@ static int ethnl_default_dumpit(struct sk_buff *skb,
/* generic ->start() handler for GET requests */
static int ethnl_default_start(struct netlink_callback *cb)
{
+ const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct ethnl_dump_ctx *ctx = ethnl_dump_context(cb);
struct ethnl_reply_data *reply_data;
const struct ethnl_request_ops *ops;
@@ -502,8 +490,8 @@ static int ethnl_default_start(struct netlink_callback *cb)
goto free_req_info;
}
- ret = ethnl_default_parse(req_info, cb->nlh, sock_net(cb->skb->sk), ops,
- cb->extack, false);
+ ret = ethnl_default_parse(req_info, info->attrs, sock_net(cb->skb->sk),
+ ops, cb->extack, false);
if (req_info->dev) {
/* We ignore device specification in dump requests but as the
* same parser as for non-dump (doit) requests is used, it
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 0100fab5829e..b67c41efaf7e 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -269,7 +269,6 @@ static inline void ethnl_ops_complete(struct net_device *dev)
* @max_attr: maximum (top level) attribute type
* @req_info_size: size of request info
* @reply_data_size: size of reply data
- * @request_policy: netlink policy for message contents
* @allow_nodev_do: allow non-dump request with no device identification
* @parse_request:
* Parse request except common header (struct ethnl_req_info). Common
@@ -315,7 +314,6 @@ struct ethnl_request_ops {
unsigned int max_attr;
unsigned int req_info_size;
unsigned int reply_data_size;
- const struct nla_policy *request_policy;
bool allow_nodev_do;
int (*parse_request)(struct ethnl_req_info *req_info,
diff --git a/net/ethtool/pause.c b/net/ethtool/pause.c
index 40f36f8d6291..32f37240e04d 100644
--- a/net/ethtool/pause.c
+++ b/net/ethtool/pause.c
@@ -132,7 +132,6 @@ const struct ethnl_request_ops ethnl_pause_request_ops = {
.max_attr = ETHTOOL_A_PAUSE_MAX,
.req_info_size = sizeof(struct pause_req_info),
.reply_data_size = sizeof(struct pause_reply_data),
- .request_policy = ethnl_pause_get_policy,
.prepare_data = pause_prepare_data,
.reply_size = pause_reply_size,
diff --git a/net/ethtool/privflags.c b/net/ethtool/privflags.c
index 7ebab33f2244..c255c75a7ac0 100644
--- a/net/ethtool/privflags.c
+++ b/net/ethtool/privflags.c
@@ -127,7 +127,6 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = {
.max_attr = ETHTOOL_A_PRIVFLAGS_MAX,
.req_info_size = sizeof(struct privflags_req_info),
.reply_data_size = sizeof(struct privflags_reply_data),
- .request_policy = ethnl_privflags_get_policy,
.prepare_data = privflags_prepare_data,
.reply_size = privflags_reply_size,
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index eb52639b6ed3..80545845a3c8 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -99,7 +99,6 @@ const struct ethnl_request_ops ethnl_rings_request_ops = {
.max_attr = ETHTOOL_A_RINGS_MAX,
.req_info_size = sizeof(struct rings_req_info),
.reply_data_size = sizeof(struct rings_reply_data),
- .request_policy = ethnl_rings_get_policy,
.prepare_data = rings_prepare_data,
.reply_size = rings_reply_size,
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 8d30fe0fbe3b..e893f98505d0 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -448,7 +448,6 @@ const struct ethnl_request_ops ethnl_strset_request_ops = {
.max_attr = ETHTOOL_A_STRSET_MAX,
.req_info_size = sizeof(struct strset_req_info),
.reply_data_size = sizeof(struct strset_reply_data),
- .request_policy = ethnl_strset_get_policy,
.allow_nodev_do = true,
.parse_request = strset_parse_request,
diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
index 05d501563dc9..185333d377c9 100644
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -134,7 +134,6 @@ const struct ethnl_request_ops ethnl_tsinfo_request_ops = {
.max_attr = ETHTOOL_A_TSINFO_MAX,
.req_info_size = sizeof(struct tsinfo_req_info),
.reply_data_size = sizeof(struct tsinfo_reply_data),
- .request_policy = ethnl_tsinfo_get_policy,
.prepare_data = tsinfo_prepare_data,
.reply_size = tsinfo_reply_size,
diff --git a/net/ethtool/tunnels.c b/net/ethtool/tunnels.c
index d7d4964ea773..330817adcf62 100644
--- a/net/ethtool/tunnels.c
+++ b/net/ethtool/tunnels.c
@@ -161,35 +161,19 @@ ethnl_tunnel_info_fill_reply(const struct ethnl_req_info *req_base,
return -EMSGSIZE;
}
-static int
-ethnl_tunnel_info_req_parse(struct ethnl_req_info *req_info,
- const struct nlmsghdr *nlhdr, struct net *net,
- struct netlink_ext_ack *extack, bool require_dev)
-{
- struct nlattr *tb[ETHTOOL_A_TUNNEL_INFO_MAX + 1];
- int ret;
-
- ret = nlmsg_parse(nlhdr, GENL_HDRLEN, tb, ETHTOOL_A_TUNNEL_INFO_MAX,
- ethnl_tunnel_info_get_policy, extack);
- if (ret < 0)
- return ret;
-
- return ethnl_parse_header_dev_get(req_info,
- tb[ETHTOOL_A_TUNNEL_INFO_HEADER],
- net, extack, require_dev);
-}
-
int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info)
{
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
struct sk_buff *rskb;
void *reply_payload;
int reply_len;
int ret;
- ret = ethnl_tunnel_info_req_parse(&req_info, info->nlhdr,
- genl_info_net(info), info->extack,
- true);
+ ret = ethnl_parse_header_dev_get(&req_info,
+ tb[ETHTOOL_A_TUNNEL_INFO_HEADER],
+ genl_info_net(info), info->extack,
+ true);
if (ret < 0)
return ret;
@@ -233,16 +217,19 @@ struct ethnl_tunnel_info_dump_ctx {
int ethnl_tunnel_info_start(struct netlink_callback *cb)
{
+ const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct ethnl_tunnel_info_dump_ctx *ctx = (void *)cb->ctx;
+ struct nlattr **tb = info->attrs;
int ret;
BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
memset(ctx, 0, sizeof(*ctx));
- ret = ethnl_tunnel_info_req_parse(&ctx->req_info, cb->nlh,
- sock_net(cb->skb->sk), cb->extack,
- false);
+ ret = ethnl_parse_header_dev_get(&ctx->req_info,
+ tb[ETHTOOL_A_TUNNEL_INFO_HEADER],
+ sock_net(cb->skb->sk), cb->extack,
+ false);
if (ctx->req_info.dev) {
dev_put(ctx->req_info.dev);
ctx->req_info.dev = NULL;
diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
index d234885fc265..4a88c597890c 100644
--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c
@@ -91,7 +91,6 @@ const struct ethnl_request_ops ethnl_wol_request_ops = {
.max_attr = ETHTOOL_A_WOL_MAX,
.req_info_size = sizeof(struct wol_req_info),
.reply_data_size = sizeof(struct wol_reply_data),
- .request_policy = ethnl_wol_get_policy,
.prepare_data = wol_prepare_data,
.reply_size = wol_reply_size,
--
2.26.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 3/6] ethtool: wire up set policies to ops
2020-10-05 15:57 [PATCH net-next 0/6] ethtool: allow dumping policies to user space Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 1/6] ethtool: wire up get policies to ops Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 2/6] ethtool: use the attributes parsed by the core in get commands Jakub Kicinski
@ 2020-10-05 15:57 ` Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 4/6] ethtool: link up ethnl_header_policy as a nested policy Jakub Kicinski
` (2 subsequent siblings)
5 siblings, 0 replies; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 15:57 UTC (permalink / raw)
To: davem
Cc: netdev, kernel-team, johannes, jiri, andrew, mkubecek, Jakub Kicinski
Similarly to get commands wire up the policies of set commands
to get parsing by the core and policy dumps.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/ethtool/cabletest.c | 24 ++++++------------------
net/ethtool/channels.c | 11 +++--------
net/ethtool/coalesce.c | 11 +++--------
net/ethtool/debug.c | 10 ++--------
net/ethtool/eee.c | 11 +++--------
net/ethtool/features.c | 11 +++--------
net/ethtool/linkinfo.c | 11 +++--------
net/ethtool/linkmodes.c | 11 +++--------
net/ethtool/netlink.c | 26 ++++++++++++++++++++++++++
net/ethtool/netlink.h | 13 +++++++++++++
net/ethtool/pause.c | 9 ++-------
net/ethtool/privflags.c | 11 +++--------
net/ethtool/rings.c | 10 ++--------
net/ethtool/wol.c | 9 ++-------
14 files changed, 74 insertions(+), 104 deletions(-)
diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index 888f6e101f34..beb85e0b7fc6 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -11,8 +11,8 @@
*/
#define MAX_CABLE_LENGTH_CM (150 * 100)
-static const struct nla_policy
-cable_test_act_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
+const struct nla_policy
+ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
[ETHTOOL_A_CABLE_TEST_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_CABLE_TEST_HEADER] = { .type = NLA_NESTED },
};
@@ -56,18 +56,12 @@ static int ethnl_cable_test_started(struct phy_device *phydev, u8 cmd)
int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_CABLE_TEST_MAX + 1];
struct ethnl_req_info req_info = {};
const struct ethtool_phy_ops *ops;
+ struct nlattr **tb = info->attrs;
struct net_device *dev;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
- ETHTOOL_A_CABLE_TEST_MAX,
- cable_test_act_policy, info->extack);
- if (ret < 0)
- return ret;
-
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_CABLE_TEST_HEADER],
genl_info_net(info), info->extack,
@@ -226,8 +220,8 @@ cable_test_tdr_act_cfg_policy[ETHTOOL_A_CABLE_TEST_TDR_CFG_MAX + 1] = {
[ETHTOOL_A_CABLE_TEST_TDR_CFG_PAIR] = { .type = NLA_U8 },
};
-static const struct nla_policy
-cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_MAX + 1] = {
+const struct nla_policy
+ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_MAX + 1] = {
[ETHTOOL_A_CABLE_TEST_TDR_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_CABLE_TEST_TDR_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_CABLE_TEST_TDR_CFG] = { .type = NLA_NESTED },
@@ -313,19 +307,13 @@ static int ethnl_act_cable_test_tdr_cfg(const struct nlattr *nest,
int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_CABLE_TEST_TDR_MAX + 1];
struct ethnl_req_info req_info = {};
const struct ethtool_phy_ops *ops;
+ struct nlattr **tb = info->attrs;
struct phy_tdr_config cfg;
struct net_device *dev;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
- ETHTOOL_A_CABLE_TEST_TDR_MAX,
- cable_test_tdr_act_policy, info->extack);
- if (ret < 0)
- return ret;
-
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
genl_info_net(info), info->extack,
diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c
index 76994c8f32a0..7e14e93adedb 100644
--- a/net/ethtool/channels.c
+++ b/net/ethtool/channels.c
@@ -110,8 +110,8 @@ const struct ethnl_request_ops ethnl_channels_request_ops = {
/* CHANNELS_SET */
-static const struct nla_policy
-channels_set_policy[ETHTOOL_A_CHANNELS_MAX + 1] = {
+const struct nla_policy
+ethnl_channels_set_policy[ETHTOOL_A_CHANNELS_MAX + 1] = {
[ETHTOOL_A_CHANNELS_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_CHANNELS_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_CHANNELS_RX_MAX] = { .type = NLA_REJECT },
@@ -126,22 +126,17 @@ channels_set_policy[ETHTOOL_A_CHANNELS_MAX + 1] = {
int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_CHANNELS_MAX + 1];
unsigned int from_channel, old_total, i;
bool mod = false, mod_combined = false;
struct ethtool_channels channels = {};
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
const struct nlattr *err_attr;
const struct ethtool_ops *ops;
struct net_device *dev;
u32 max_rx_in_use = 0;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
- ETHTOOL_A_CHANNELS_MAX, channels_set_policy,
- info->extack);
- if (ret < 0)
- return ret;
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_CHANNELS_HEADER],
genl_info_net(info), info->extack,
diff --git a/net/ethtool/coalesce.c b/net/ethtool/coalesce.c
index 77d83062faa2..261ef40f7f98 100644
--- a/net/ethtool/coalesce.c
+++ b/net/ethtool/coalesce.c
@@ -214,8 +214,8 @@ const struct ethnl_request_ops ethnl_coalesce_request_ops = {
/* COALESCE_SET */
-static const struct nla_policy
-coalesce_set_policy[ETHTOOL_A_COALESCE_MAX + 1] = {
+const struct nla_policy
+ethnl_coalesce_set_policy[ETHTOOL_A_COALESCE_MAX + 1] = {
[ETHTOOL_A_COALESCE_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_COALESCE_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_COALESCE_RX_USECS] = { .type = NLA_U32 },
@@ -244,9 +244,9 @@ coalesce_set_policy[ETHTOOL_A_COALESCE_MAX + 1] = {
int ethnl_set_coalesce(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_COALESCE_MAX + 1];
struct ethtool_coalesce coalesce = {};
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
const struct ethtool_ops *ops;
struct net_device *dev;
u32 supported_params;
@@ -254,11 +254,6 @@ int ethnl_set_coalesce(struct sk_buff *skb, struct genl_info *info)
int ret;
u16 a;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
- ETHTOOL_A_COALESCE_MAX, coalesce_set_policy,
- info->extack);
- if (ret < 0)
- return ret;
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_COALESCE_HEADER],
genl_info_net(info), info->extack,
diff --git a/net/ethtool/debug.c b/net/ethtool/debug.c
index f553ef6a99e4..33a4126ed80d 100644
--- a/net/ethtool/debug.c
+++ b/net/ethtool/debug.c
@@ -79,8 +79,7 @@ const struct ethnl_request_ops ethnl_debug_request_ops = {
/* DEBUG_SET */
-static const struct nla_policy
-debug_set_policy[ETHTOOL_A_DEBUG_MAX + 1] = {
+const struct nla_policy ethnl_debug_set_policy[ETHTOOL_A_DEBUG_MAX + 1] = {
[ETHTOOL_A_DEBUG_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_DEBUG_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_DEBUG_MSGMASK] = { .type = NLA_NESTED },
@@ -88,18 +87,13 @@ debug_set_policy[ETHTOOL_A_DEBUG_MAX + 1] = {
int ethnl_set_debug(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_DEBUG_MAX + 1];
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
struct net_device *dev;
bool mod = false;
u32 msg_mask;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
- ETHTOOL_A_DEBUG_MAX, debug_set_policy,
- info->extack);
- if (ret < 0)
- return ret;
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_DEBUG_HEADER],
genl_info_net(info), info->extack,
diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index 9ee03444fb8f..25b99f1cfe12 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -129,8 +129,7 @@ const struct ethnl_request_ops ethnl_eee_request_ops = {
/* EEE_SET */
-static const struct nla_policy
-eee_set_policy[ETHTOOL_A_EEE_MAX + 1] = {
+const struct nla_policy ethnl_eee_set_policy[ETHTOOL_A_EEE_MAX + 1] = {
[ETHTOOL_A_EEE_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_EEE_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_EEE_MODES_OURS] = { .type = NLA_NESTED },
@@ -143,18 +142,14 @@ eee_set_policy[ETHTOOL_A_EEE_MAX + 1] = {
int ethnl_set_eee(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_EEE_MAX + 1];
- struct ethtool_eee eee = {};
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
const struct ethtool_ops *ops;
+ struct ethtool_eee eee = {};
struct net_device *dev;
bool mod = false;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb, ETHTOOL_A_EEE_MAX,
- eee_set_policy, info->extack);
- if (ret < 0)
- return ret;
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_EEE_HEADER],
genl_info_net(info), info->extack,
diff --git a/net/ethtool/features.c b/net/ethtool/features.c
index 99944190f398..00c5b77852ec 100644
--- a/net/ethtool/features.c
+++ b/net/ethtool/features.c
@@ -131,8 +131,8 @@ const struct ethnl_request_ops ethnl_features_request_ops = {
/* FEATURES_SET */
-static const struct nla_policy
-features_set_policy[ETHTOOL_A_FEATURES_MAX + 1] = {
+const struct nla_policy
+ethnl_features_set_policy[ETHTOOL_A_FEATURES_MAX + 1] = {
[ETHTOOL_A_FEATURES_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_FEATURES_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_FEATURES_HW] = { .type = NLA_REJECT },
@@ -228,17 +228,12 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
DECLARE_BITMAP(new_wanted, NETDEV_FEATURE_COUNT);
DECLARE_BITMAP(req_wanted, NETDEV_FEATURE_COUNT);
DECLARE_BITMAP(req_mask, NETDEV_FEATURE_COUNT);
- struct nlattr *tb[ETHTOOL_A_FEATURES_MAX + 1];
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
struct net_device *dev;
bool mod;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
- ETHTOOL_A_FEATURES_MAX, features_set_policy,
- info->extack);
- if (ret < 0)
- return ret;
if (!tb[ETHTOOL_A_FEATURES_WANTED])
return -EINVAL;
ret = ethnl_parse_header_dev_get(&req_info,
diff --git a/net/ethtool/linkinfo.c b/net/ethtool/linkinfo.c
index b7142d8e2baf..eb801271b9bc 100644
--- a/net/ethtool/linkinfo.c
+++ b/net/ethtool/linkinfo.c
@@ -94,8 +94,8 @@ const struct ethnl_request_ops ethnl_linkinfo_request_ops = {
/* LINKINFO_SET */
-static const struct nla_policy
-linkinfo_set_policy[ETHTOOL_A_LINKINFO_MAX + 1] = {
+const struct nla_policy
+ethnl_linkinfo_set_policy[ETHTOOL_A_LINKINFO_MAX + 1] = {
[ETHTOOL_A_LINKINFO_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKINFO_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_LINKINFO_PORT] = { .type = NLA_U8 },
@@ -107,19 +107,14 @@ linkinfo_set_policy[ETHTOOL_A_LINKINFO_MAX + 1] = {
int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_LINKINFO_MAX + 1];
struct ethtool_link_ksettings ksettings = {};
struct ethtool_link_settings *lsettings;
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
struct net_device *dev;
bool mod = false;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
- ETHTOOL_A_LINKINFO_MAX, linkinfo_set_policy,
- info->extack);
- if (ret < 0)
- return ret;
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_LINKINFO_HEADER],
genl_info_net(info), info->extack,
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index e23e600b73cb..b5f061f192b9 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -275,8 +275,8 @@ static const struct link_mode_info link_mode_params[] = {
__DEFINE_LINK_MODE_PARAMS(100, FX, Full),
};
-static const struct nla_policy
-linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
+const struct nla_policy
+ethnl_linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
[ETHTOOL_A_LINKMODES_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKMODES_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_LINKMODES_AUTONEG] = { .type = NLA_U8 },
@@ -391,18 +391,13 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_LINKMODES_MAX + 1];
struct ethtool_link_ksettings ksettings = {};
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
struct net_device *dev;
bool mod = false;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
- ETHTOOL_A_LINKMODES_MAX, linkmodes_set_policy,
- info->extack);
- if (ret < 0)
- return ret;
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_LINKMODES_HEADER],
genl_info_net(info), info->extack,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 2f317caa077a..f2d197e6c9d0 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -700,6 +700,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_LINKINFO_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_linkinfo,
+ .policy = ethnl_linkinfo_set_policy,
+ .maxattr = ARRAY_SIZE(ethnl_linkinfo_set_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_LINKMODES_GET,
@@ -714,6 +716,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_LINKMODES_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_linkmodes,
+ .policy = ethnl_linkmodes_set_policy,
+ .maxattr = ARRAY_SIZE(ethnl_linkmodes_set_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_LINKSTATE_GET,
@@ -737,6 +741,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_DEBUG_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_debug,
+ .policy = ethnl_debug_set_policy,
+ .maxattr = ARRAY_SIZE(ethnl_debug_set_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_WOL_GET,
@@ -752,6 +758,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_WOL_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_wol,
+ .policy = ethnl_wol_set_policy,
+ .maxattr = ARRAY_SIZE(ethnl_wol_set_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_FEATURES_GET,
@@ -766,6 +774,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_FEATURES_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_features,
+ .policy = ethnl_features_set_policy,
+ .maxattr = ARRAY_SIZE(ethnl_features_set_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_PRIVFLAGS_GET,
@@ -780,6 +790,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_PRIVFLAGS_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_privflags,
+ .policy = ethnl_privflags_set_policy,
+ .maxattr = ARRAY_SIZE(ethnl_privflags_set_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_RINGS_GET,
@@ -795,6 +807,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_RINGS_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_rings,
+ .policy = ethnl_rings_set_policy,
+ .maxattr = ARRAY_SIZE(ethnl_rings_set_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_CHANNELS_GET,
@@ -810,6 +824,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_CHANNELS_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_channels,
+ .policy = ethnl_channels_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_channels_get_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_COALESCE_GET,
@@ -824,6 +840,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_COALESCE_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_coalesce,
+ .policy = ethnl_coalesce_set_policy,
+ .maxattr = ARRAY_SIZE(ethnl_coalesce_set_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_PAUSE_GET,
@@ -838,6 +856,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_PAUSE_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_pause,
+ .policy = ethnl_pause_set_policy,
+ .maxattr = ARRAY_SIZE(ethnl_pause_set_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_EEE_GET,
@@ -852,6 +872,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_EEE_SET,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_set_eee,
+ .policy = ethnl_eee_set_policy,
+ .maxattr = ARRAY_SIZE(ethnl_eee_set_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_TSINFO_GET,
@@ -866,11 +888,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_MSG_CABLE_TEST_ACT,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_act_cable_test,
+ .policy = ethnl_cable_test_act_policy,
+ .maxattr = ARRAY_SIZE(ethnl_cable_test_act_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
.flags = GENL_UNS_ADMIN_PERM,
.doit = ethnl_act_cable_test_tdr,
+ .policy = ethnl_cable_test_tdr_act_policy,
+ .maxattr = ARRAY_SIZE(ethnl_cable_test_tdr_act_policy) - 1,
},
{
.cmd = ETHTOOL_MSG_TUNNEL_INFO_GET,
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index b67c41efaf7e..53eeda056441 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -349,18 +349,31 @@ extern const struct ethnl_request_ops ethnl_tsinfo_request_ops;
extern const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_MAX + 1];
extern const struct nla_policy ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_MAX + 1];
+extern const struct nla_policy ethnl_linkinfo_set_policy[ETHTOOL_A_LINKINFO_MAX + 1];
extern const struct nla_policy ethnl_linkmodes_get_policy[ETHTOOL_A_LINKMODES_MAX + 1];
+extern const struct nla_policy ethnl_linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1];
extern const struct nla_policy ethnl_linkstate_get_policy[ETHTOOL_A_LINKSTATE_MAX + 1];
extern const struct nla_policy ethnl_debug_get_policy[ETHTOOL_A_DEBUG_MAX + 1];
+extern const struct nla_policy ethnl_debug_set_policy[ETHTOOL_A_DEBUG_MAX + 1];
extern const struct nla_policy ethnl_wol_get_policy[ETHTOOL_A_WOL_MAX + 1];
+extern const struct nla_policy ethnl_wol_set_policy[ETHTOOL_A_WOL_MAX + 1];
extern const struct nla_policy ethnl_features_get_policy[ETHTOOL_A_FEATURES_MAX + 1];
+extern const struct nla_policy ethnl_features_set_policy[ETHTOOL_A_FEATURES_MAX + 1];
extern const struct nla_policy ethnl_privflags_get_policy[ETHTOOL_A_PRIVFLAGS_MAX + 1];
+extern const struct nla_policy ethnl_privflags_set_policy[ETHTOOL_A_PRIVFLAGS_MAX + 1];
extern const struct nla_policy ethnl_rings_get_policy[ETHTOOL_A_RINGS_MAX + 1];
+extern const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_MAX + 1];
extern const struct nla_policy ethnl_channels_get_policy[ETHTOOL_A_CHANNELS_MAX + 1];
+extern const struct nla_policy ethnl_channels_set_policy[ETHTOOL_A_CHANNELS_MAX + 1];
extern const struct nla_policy ethnl_coalesce_get_policy[ETHTOOL_A_COALESCE_MAX + 1];
+extern const struct nla_policy ethnl_coalesce_set_policy[ETHTOOL_A_COALESCE_MAX + 1];
extern const struct nla_policy ethnl_pause_get_policy[ETHTOOL_A_PAUSE_MAX + 1];
+extern const struct nla_policy ethnl_pause_set_policy[ETHTOOL_A_PAUSE_MAX + 1];
extern const struct nla_policy ethnl_eee_get_policy[ETHTOOL_A_EEE_MAX + 1];
+extern const struct nla_policy ethnl_eee_set_policy[ETHTOOL_A_EEE_MAX + 1];
extern const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1];
+extern const struct nla_policy ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_MAX + 1];
+extern const struct nla_policy ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_MAX + 1];
extern const struct nla_policy ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_MAX + 1];
int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/ethtool/pause.c b/net/ethtool/pause.c
index 32f37240e04d..26c73ba987d1 100644
--- a/net/ethtool/pause.c
+++ b/net/ethtool/pause.c
@@ -140,8 +140,7 @@ const struct ethnl_request_ops ethnl_pause_request_ops = {
/* PAUSE_SET */
-static const struct nla_policy
-pause_set_policy[ETHTOOL_A_PAUSE_MAX + 1] = {
+const struct nla_policy ethnl_pause_set_policy[ETHTOOL_A_PAUSE_MAX + 1] = {
[ETHTOOL_A_PAUSE_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_PAUSE_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_PAUSE_AUTONEG] = { .type = NLA_U8 },
@@ -152,18 +151,14 @@ pause_set_policy[ETHTOOL_A_PAUSE_MAX + 1] = {
int ethnl_set_pause(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_PAUSE_MAX + 1];
struct ethtool_pauseparam params = {};
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
const struct ethtool_ops *ops;
struct net_device *dev;
bool mod = false;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb, ETHTOOL_A_PAUSE_MAX,
- pause_set_policy, info->extack);
- if (ret < 0)
- return ret;
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_PAUSE_HEADER],
genl_info_net(info), info->extack,
diff --git a/net/ethtool/privflags.c b/net/ethtool/privflags.c
index c255c75a7ac0..f8164e0f2f87 100644
--- a/net/ethtool/privflags.c
+++ b/net/ethtool/privflags.c
@@ -136,8 +136,8 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = {
/* PRIVFLAGS_SET */
-static const struct nla_policy
-privflags_set_policy[ETHTOOL_A_PRIVFLAGS_MAX + 1] = {
+const struct nla_policy
+ethnl_privflags_set_policy[ETHTOOL_A_PRIVFLAGS_MAX + 1] = {
[ETHTOOL_A_PRIVFLAGS_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_PRIVFLAGS_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_PRIVFLAGS_FLAGS] = { .type = NLA_NESTED },
@@ -145,9 +145,9 @@ privflags_set_policy[ETHTOOL_A_PRIVFLAGS_MAX + 1] = {
int ethnl_set_privflags(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_PRIVFLAGS_MAX + 1];
const char (*names)[ETH_GSTRING_LEN] = NULL;
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
const struct ethtool_ops *ops;
struct net_device *dev;
unsigned int nflags;
@@ -156,11 +156,6 @@ int ethnl_set_privflags(struct sk_buff *skb, struct genl_info *info)
u32 flags;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
- ETHTOOL_A_PRIVFLAGS_MAX, privflags_set_policy,
- info->extack);
- if (ret < 0)
- return ret;
if (!tb[ETHTOOL_A_PRIVFLAGS_FLAGS])
return -EINVAL;
ret = ethnl_bitset_is_compact(tb[ETHTOOL_A_PRIVFLAGS_FLAGS], &compact);
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 80545845a3c8..73ee664f9b0b 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -107,8 +107,7 @@ const struct ethnl_request_ops ethnl_rings_request_ops = {
/* RINGS_SET */
-static const struct nla_policy
-rings_set_policy[ETHTOOL_A_RINGS_MAX + 1] = {
+const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_MAX + 1] = {
[ETHTOOL_A_RINGS_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_RINGS_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_RINGS_RX_MAX] = { .type = NLA_REJECT },
@@ -123,20 +122,15 @@ rings_set_policy[ETHTOOL_A_RINGS_MAX + 1] = {
int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *tb[ETHTOOL_A_RINGS_MAX + 1];
struct ethtool_ringparam ringparam = {};
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
const struct nlattr *err_attr;
const struct ethtool_ops *ops;
struct net_device *dev;
bool mod = false;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
- ETHTOOL_A_RINGS_MAX, rings_set_policy,
- info->extack);
- if (ret < 0)
- return ret;
ret = ethnl_parse_header_dev_get(&req_info,
tb[ETHTOOL_A_RINGS_HEADER],
genl_info_net(info), info->extack,
diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
index 4a88c597890c..a5f396c8c69a 100644
--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c
@@ -99,8 +99,7 @@ const struct ethnl_request_ops ethnl_wol_request_ops = {
/* WOL_SET */
-static const struct nla_policy
-wol_set_policy[ETHTOOL_A_WOL_MAX + 1] = {
+const struct nla_policy ethnl_wol_set_policy[ETHTOOL_A_WOL_MAX + 1] = {
[ETHTOOL_A_WOL_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_WOL_HEADER] = { .type = NLA_NESTED },
[ETHTOOL_A_WOL_MODES] = { .type = NLA_NESTED },
@@ -111,16 +110,12 @@ wol_set_policy[ETHTOOL_A_WOL_MAX + 1] = {
int ethnl_set_wol(struct sk_buff *skb, struct genl_info *info)
{
struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
- struct nlattr *tb[ETHTOOL_A_WOL_MAX + 1];
struct ethnl_req_info req_info = {};
+ struct nlattr **tb = info->attrs;
struct net_device *dev;
bool mod = false;
int ret;
- ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb, ETHTOOL_A_WOL_MAX,
- wol_set_policy, info->extack);
- if (ret < 0)
- return ret;
ret = ethnl_parse_header_dev_get(&req_info, tb[ETHTOOL_A_WOL_HEADER],
genl_info_net(info), info->extack,
true);
--
2.26.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 4/6] ethtool: link up ethnl_header_policy as a nested policy
2020-10-05 15:57 [PATCH net-next 0/6] ethtool: allow dumping policies to user space Jakub Kicinski
` (2 preceding siblings ...)
2020-10-05 15:57 ` [PATCH net-next 3/6] ethtool: wire up set policies to ops Jakub Kicinski
@ 2020-10-05 15:57 ` Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 5/6] netlink: add mask validation Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 6/6] ethtool: specify which header flags are supported per command Jakub Kicinski
5 siblings, 0 replies; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 15:57 UTC (permalink / raw)
To: davem
Cc: netdev, kernel-team, johannes, jiri, andrew, mkubecek, Jakub Kicinski
To get the most out of parsing by the core, and to allow dumping
full policies we need to specify which policy applies to nested
attrs. For headers it's ethnl_header_policy.
$ sed -i 's@\(ETHTOOL_A_.*HEADER\].*=\) { .type = NLA_NESTED },@\1\n\t\tNLA_POLICY_NESTED(ethnl_header_policy),@' net/ethtool/*
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/ethtool/cabletest.c | 6 ++++--
net/ethtool/channels.c | 6 ++++--
net/ethtool/coalesce.c | 6 ++++--
net/ethtool/debug.c | 6 ++++--
net/ethtool/eee.c | 6 ++++--
net/ethtool/features.c | 6 ++++--
net/ethtool/linkinfo.c | 6 ++++--
net/ethtool/linkmodes.c | 6 ++++--
net/ethtool/linkstate.c | 3 ++-
net/ethtool/netlink.c | 2 +-
net/ethtool/netlink.h | 1 +
net/ethtool/pause.c | 6 ++++--
net/ethtool/privflags.c | 6 ++++--
net/ethtool/rings.c | 6 ++++--
net/ethtool/strset.c | 3 ++-
net/ethtool/tsinfo.c | 3 ++-
net/ethtool/tunnels.c | 3 ++-
net/ethtool/wol.c | 6 ++++--
18 files changed, 58 insertions(+), 29 deletions(-)
diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index beb85e0b7fc6..17019ed74a02 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -14,7 +14,8 @@
const struct nla_policy
ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
[ETHTOOL_A_CABLE_TEST_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_CABLE_TEST_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_CABLE_TEST_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
};
static int ethnl_cable_test_started(struct phy_device *phydev, u8 cmd)
@@ -223,7 +224,8 @@ cable_test_tdr_act_cfg_policy[ETHTOOL_A_CABLE_TEST_TDR_CFG_MAX + 1] = {
const struct nla_policy
ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_MAX + 1] = {
[ETHTOOL_A_CABLE_TEST_TDR_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_CABLE_TEST_TDR_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_CABLE_TEST_TDR_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_CABLE_TEST_TDR_CFG] = { .type = NLA_NESTED },
};
diff --git a/net/ethtool/channels.c b/net/ethtool/channels.c
index 7e14e93adedb..dbbe2dcb21d6 100644
--- a/net/ethtool/channels.c
+++ b/net/ethtool/channels.c
@@ -20,7 +20,8 @@ struct channels_reply_data {
const struct nla_policy
ethnl_channels_get_policy[ETHTOOL_A_CHANNELS_MAX + 1] = {
[ETHTOOL_A_CHANNELS_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_CHANNELS_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_CHANNELS_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_CHANNELS_RX_MAX] = { .type = NLA_REJECT },
[ETHTOOL_A_CHANNELS_TX_MAX] = { .type = NLA_REJECT },
[ETHTOOL_A_CHANNELS_OTHER_MAX] = { .type = NLA_REJECT },
@@ -113,7 +114,8 @@ const struct ethnl_request_ops ethnl_channels_request_ops = {
const struct nla_policy
ethnl_channels_set_policy[ETHTOOL_A_CHANNELS_MAX + 1] = {
[ETHTOOL_A_CHANNELS_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_CHANNELS_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_CHANNELS_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_CHANNELS_RX_MAX] = { .type = NLA_REJECT },
[ETHTOOL_A_CHANNELS_TX_MAX] = { .type = NLA_REJECT },
[ETHTOOL_A_CHANNELS_OTHER_MAX] = { .type = NLA_REJECT },
diff --git a/net/ethtool/coalesce.c b/net/ethtool/coalesce.c
index 261ef40f7f98..15adc9861421 100644
--- a/net/ethtool/coalesce.c
+++ b/net/ethtool/coalesce.c
@@ -54,7 +54,8 @@ __CHECK_SUPPORTED_OFFSET(COALESCE_RATE_SAMPLE_INTERVAL);
const struct nla_policy
ethnl_coalesce_get_policy[ETHTOOL_A_COALESCE_MAX + 1] = {
[ETHTOOL_A_COALESCE_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_COALESCE_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_COALESCE_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_COALESCE_RX_USECS] = { .type = NLA_REJECT },
[ETHTOOL_A_COALESCE_RX_MAX_FRAMES] = { .type = NLA_REJECT },
[ETHTOOL_A_COALESCE_RX_USECS_IRQ] = { .type = NLA_REJECT },
@@ -217,7 +218,8 @@ const struct ethnl_request_ops ethnl_coalesce_request_ops = {
const struct nla_policy
ethnl_coalesce_set_policy[ETHTOOL_A_COALESCE_MAX + 1] = {
[ETHTOOL_A_COALESCE_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_COALESCE_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_COALESCE_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_COALESCE_RX_USECS] = { .type = NLA_U32 },
[ETHTOOL_A_COALESCE_RX_MAX_FRAMES] = { .type = NLA_U32 },
[ETHTOOL_A_COALESCE_RX_USECS_IRQ] = { .type = NLA_U32 },
diff --git a/net/ethtool/debug.c b/net/ethtool/debug.c
index 33a4126ed80d..b72980698ecb 100644
--- a/net/ethtool/debug.c
+++ b/net/ethtool/debug.c
@@ -18,7 +18,8 @@ struct debug_reply_data {
const struct nla_policy ethnl_debug_get_policy[ETHTOOL_A_DEBUG_MAX + 1] = {
[ETHTOOL_A_DEBUG_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_DEBUG_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_DEBUG_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_DEBUG_MSGMASK] = { .type = NLA_REJECT },
};
@@ -81,7 +82,8 @@ const struct ethnl_request_ops ethnl_debug_request_ops = {
const struct nla_policy ethnl_debug_set_policy[ETHTOOL_A_DEBUG_MAX + 1] = {
[ETHTOOL_A_DEBUG_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_DEBUG_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_DEBUG_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_DEBUG_MSGMASK] = { .type = NLA_NESTED },
};
diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index 25b99f1cfe12..646456d5d78a 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -21,7 +21,8 @@ struct eee_reply_data {
const struct nla_policy ethnl_eee_get_policy[ETHTOOL_A_EEE_MAX + 1] = {
[ETHTOOL_A_EEE_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_EEE_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_EEE_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_EEE_MODES_OURS] = { .type = NLA_REJECT },
[ETHTOOL_A_EEE_MODES_PEER] = { .type = NLA_REJECT },
[ETHTOOL_A_EEE_ACTIVE] = { .type = NLA_REJECT },
@@ -131,7 +132,8 @@ const struct ethnl_request_ops ethnl_eee_request_ops = {
const struct nla_policy ethnl_eee_set_policy[ETHTOOL_A_EEE_MAX + 1] = {
[ETHTOOL_A_EEE_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_EEE_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_EEE_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_EEE_MODES_OURS] = { .type = NLA_NESTED },
[ETHTOOL_A_EEE_MODES_PEER] = { .type = NLA_REJECT },
[ETHTOOL_A_EEE_ACTIVE] = { .type = NLA_REJECT },
diff --git a/net/ethtool/features.c b/net/ethtool/features.c
index 00c5b77852ec..63ead0ca9eac 100644
--- a/net/ethtool/features.c
+++ b/net/ethtool/features.c
@@ -23,7 +23,8 @@ struct features_reply_data {
const struct nla_policy
ethnl_features_get_policy[ETHTOOL_A_FEATURES_MAX + 1] = {
[ETHTOOL_A_FEATURES_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_FEATURES_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_FEATURES_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_FEATURES_HW] = { .type = NLA_REJECT },
[ETHTOOL_A_FEATURES_WANTED] = { .type = NLA_REJECT },
[ETHTOOL_A_FEATURES_ACTIVE] = { .type = NLA_REJECT },
@@ -134,7 +135,8 @@ const struct ethnl_request_ops ethnl_features_request_ops = {
const struct nla_policy
ethnl_features_set_policy[ETHTOOL_A_FEATURES_MAX + 1] = {
[ETHTOOL_A_FEATURES_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_FEATURES_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_FEATURES_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_FEATURES_HW] = { .type = NLA_REJECT },
[ETHTOOL_A_FEATURES_WANTED] = { .type = NLA_NESTED },
[ETHTOOL_A_FEATURES_ACTIVE] = { .type = NLA_REJECT },
diff --git a/net/ethtool/linkinfo.c b/net/ethtool/linkinfo.c
index eb801271b9bc..9d66679b3e21 100644
--- a/net/ethtool/linkinfo.c
+++ b/net/ethtool/linkinfo.c
@@ -19,7 +19,8 @@ struct linkinfo_reply_data {
const struct nla_policy
ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_MAX + 1] = {
[ETHTOOL_A_LINKINFO_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_LINKINFO_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_LINKINFO_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_LINKINFO_PORT] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKINFO_PHYADDR] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKINFO_TP_MDIX] = { .type = NLA_REJECT },
@@ -97,7 +98,8 @@ const struct ethnl_request_ops ethnl_linkinfo_request_ops = {
const struct nla_policy
ethnl_linkinfo_set_policy[ETHTOOL_A_LINKINFO_MAX + 1] = {
[ETHTOOL_A_LINKINFO_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_LINKINFO_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_LINKINFO_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_LINKINFO_PORT] = { .type = NLA_U8 },
[ETHTOOL_A_LINKINFO_PHYADDR] = { .type = NLA_U8 },
[ETHTOOL_A_LINKINFO_TP_MDIX] = { .type = NLA_REJECT },
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index b5f061f192b9..951ab02e688e 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -21,7 +21,8 @@ struct linkmodes_reply_data {
const struct nla_policy
ethnl_linkmodes_get_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
[ETHTOOL_A_LINKMODES_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_LINKMODES_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_LINKMODES_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_LINKMODES_AUTONEG] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKMODES_OURS] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKMODES_PEER] = { .type = NLA_REJECT },
@@ -278,7 +279,8 @@ static const struct link_mode_info link_mode_params[] = {
const struct nla_policy
ethnl_linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
[ETHTOOL_A_LINKMODES_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_LINKMODES_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_LINKMODES_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_LINKMODES_AUTONEG] = { .type = NLA_U8 },
[ETHTOOL_A_LINKMODES_OURS] = { .type = NLA_NESTED },
[ETHTOOL_A_LINKMODES_PEER] = { .type = NLA_REJECT },
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index 3f0ab6e84fce..ed713552029c 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -23,7 +23,8 @@ struct linkstate_reply_data {
const struct nla_policy
ethnl_linkstate_get_policy[ETHTOOL_A_LINKSTATE_MAX + 1] = {
[ETHTOOL_A_LINKSTATE_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_LINKSTATE_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_LINKSTATE_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_LINKSTATE_LINK] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKSTATE_SQI] = { .type = NLA_REJECT },
[ETHTOOL_A_LINKSTATE_SQI_MAX] = { .type = NLA_REJECT },
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index f2d197e6c9d0..e78ff7ce2a7d 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -9,7 +9,7 @@ static struct genl_family ethtool_genl_family;
static bool ethnl_ok __read_mostly;
static u32 ethnl_bcast_seq;
-static const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_MAX + 1] = {
+const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_MAX + 1] = {
[ETHTOOL_A_HEADER_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_HEADER_DEV_INDEX] = { .type = NLA_U32 },
[ETHTOOL_A_HEADER_DEV_NAME] = { .type = NLA_NUL_STRING,
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 53eeda056441..4fcd2e8b259b 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -347,6 +347,7 @@ extern const struct ethnl_request_ops ethnl_pause_request_ops;
extern const struct ethnl_request_ops ethnl_eee_request_ops;
extern const struct ethnl_request_ops ethnl_tsinfo_request_ops;
+extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_MAX + 1];
extern const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_MAX + 1];
extern const struct nla_policy ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_MAX + 1];
extern const struct nla_policy ethnl_linkinfo_set_policy[ETHTOOL_A_LINKINFO_MAX + 1];
diff --git a/net/ethtool/pause.c b/net/ethtool/pause.c
index 26c73ba987d1..a7fbe0e4dca6 100644
--- a/net/ethtool/pause.c
+++ b/net/ethtool/pause.c
@@ -18,7 +18,8 @@ struct pause_reply_data {
const struct nla_policy ethnl_pause_get_policy[ETHTOOL_A_PAUSE_MAX + 1] = {
[ETHTOOL_A_PAUSE_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_PAUSE_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_PAUSE_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_PAUSE_AUTONEG] = { .type = NLA_REJECT },
[ETHTOOL_A_PAUSE_RX] = { .type = NLA_REJECT },
[ETHTOOL_A_PAUSE_TX] = { .type = NLA_REJECT },
@@ -142,7 +143,8 @@ const struct ethnl_request_ops ethnl_pause_request_ops = {
const struct nla_policy ethnl_pause_set_policy[ETHTOOL_A_PAUSE_MAX + 1] = {
[ETHTOOL_A_PAUSE_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_PAUSE_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_PAUSE_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_PAUSE_AUTONEG] = { .type = NLA_U8 },
[ETHTOOL_A_PAUSE_RX] = { .type = NLA_U8 },
[ETHTOOL_A_PAUSE_TX] = { .type = NLA_U8 },
diff --git a/net/ethtool/privflags.c b/net/ethtool/privflags.c
index f8164e0f2f87..4a77a8a547f7 100644
--- a/net/ethtool/privflags.c
+++ b/net/ethtool/privflags.c
@@ -21,7 +21,8 @@ struct privflags_reply_data {
const struct nla_policy
ethnl_privflags_get_policy[ETHTOOL_A_PRIVFLAGS_MAX + 1] = {
[ETHTOOL_A_PRIVFLAGS_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_PRIVFLAGS_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_PRIVFLAGS_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_PRIVFLAGS_FLAGS] = { .type = NLA_REJECT },
};
@@ -139,7 +140,8 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = {
const struct nla_policy
ethnl_privflags_set_policy[ETHTOOL_A_PRIVFLAGS_MAX + 1] = {
[ETHTOOL_A_PRIVFLAGS_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_PRIVFLAGS_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_PRIVFLAGS_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_PRIVFLAGS_FLAGS] = { .type = NLA_NESTED },
};
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 73ee664f9b0b..142d0902293a 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -17,7 +17,8 @@ struct rings_reply_data {
const struct nla_policy ethnl_rings_get_policy[ETHTOOL_A_RINGS_MAX + 1] = {
[ETHTOOL_A_RINGS_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_RINGS_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_RINGS_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_RINGS_RX_MAX] = { .type = NLA_REJECT },
[ETHTOOL_A_RINGS_RX_MINI_MAX] = { .type = NLA_REJECT },
[ETHTOOL_A_RINGS_RX_JUMBO_MAX] = { .type = NLA_REJECT },
@@ -109,7 +110,8 @@ const struct ethnl_request_ops ethnl_rings_request_ops = {
const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_MAX + 1] = {
[ETHTOOL_A_RINGS_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_RINGS_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_RINGS_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_RINGS_RX_MAX] = { .type = NLA_REJECT },
[ETHTOOL_A_RINGS_RX_MINI_MAX] = { .type = NLA_REJECT },
[ETHTOOL_A_RINGS_RX_JUMBO_MAX] = { .type = NLA_REJECT },
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index e893f98505d0..8aec735216ca 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -101,7 +101,8 @@ struct strset_reply_data {
const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_MAX + 1] = {
[ETHTOOL_A_STRSET_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_STRSET_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_STRSET_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_STRSET_STRINGSETS] = { .type = NLA_NESTED },
};
diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
index 185333d377c9..8a26e1620083 100644
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -20,7 +20,8 @@ struct tsinfo_reply_data {
const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1] = {
[ETHTOOL_A_TSINFO_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_TSINFO_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_TSINFO_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_TSINFO_TIMESTAMPING] = { .type = NLA_REJECT },
[ETHTOOL_A_TSINFO_TX_TYPES] = { .type = NLA_REJECT },
[ETHTOOL_A_TSINFO_RX_FILTERS] = { .type = NLA_REJECT },
diff --git a/net/ethtool/tunnels.c b/net/ethtool/tunnels.c
index 330817adcf62..734e12147d34 100644
--- a/net/ethtool/tunnels.c
+++ b/net/ethtool/tunnels.c
@@ -11,7 +11,8 @@
const struct nla_policy
ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_MAX + 1] = {
[ETHTOOL_A_TUNNEL_INFO_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_TUNNEL_INFO_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_TUNNEL_INFO_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
};
static_assert(ETHTOOL_UDP_TUNNEL_TYPE_VXLAN == ilog2(UDP_TUNNEL_TYPE_VXLAN));
diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
index a5f396c8c69a..0e1aa6acb4aa 100644
--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c
@@ -19,7 +19,8 @@ struct wol_reply_data {
const struct nla_policy ethnl_wol_get_policy[ETHTOOL_A_WOL_MAX + 1] = {
[ETHTOOL_A_WOL_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_WOL_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_WOL_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_WOL_MODES] = { .type = NLA_REJECT },
[ETHTOOL_A_WOL_SOPASS] = { .type = NLA_REJECT },
};
@@ -101,7 +102,8 @@ const struct ethnl_request_ops ethnl_wol_request_ops = {
const struct nla_policy ethnl_wol_set_policy[ETHTOOL_A_WOL_MAX + 1] = {
[ETHTOOL_A_WOL_UNSPEC] = { .type = NLA_REJECT },
- [ETHTOOL_A_WOL_HEADER] = { .type = NLA_NESTED },
+ [ETHTOOL_A_WOL_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
[ETHTOOL_A_WOL_MODES] = { .type = NLA_NESTED },
[ETHTOOL_A_WOL_SOPASS] = { .type = NLA_BINARY,
.len = SOPASS_MAX },
--
2.26.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 15:57 [PATCH net-next 0/6] ethtool: allow dumping policies to user space Jakub Kicinski
` (3 preceding siblings ...)
2020-10-05 15:57 ` [PATCH net-next 4/6] ethtool: link up ethnl_header_policy as a nested policy Jakub Kicinski
@ 2020-10-05 15:57 ` Jakub Kicinski
2020-10-05 19:05 ` Johannes Berg
2020-10-05 15:57 ` [PATCH net-next 6/6] ethtool: specify which header flags are supported per command Jakub Kicinski
5 siblings, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 15:57 UTC (permalink / raw)
To: davem
Cc: netdev, kernel-team, johannes, jiri, andrew, mkubecek,
Jakub Kicinski, dsahern, pablo
We don't have good validation policy for existing unsigned int attrs
which serve as flags (for new ones we could use NLA_BITFIELD32).
With increased use of policy dumping having the validation be
expressed as part of the policy is important. Add validation
policy in form of a mask of supported/valid bits.
Support u64 in the uAPI to be future-proof, but really for now
the embedded mask member can only hold 32 bits, so anything with
bit 32+ set will always fail validation.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jiri@resnulli.us
CC: dsahern@gmail.com
CC: pablo@netfilter.org
---
include/net/netlink.h | 11 +++++++++++
include/uapi/linux/netlink.h | 2 ++
lib/nlattr.c | 36 ++++++++++++++++++++++++++++++++++++
net/netlink/policy.c | 8 ++++++++
4 files changed, 57 insertions(+)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 5a5ff97cc596..844b53dbba68 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -200,6 +200,7 @@ enum nla_policy_validation {
NLA_VALIDATE_RANGE_WARN_TOO_LONG,
NLA_VALIDATE_MIN,
NLA_VALIDATE_MAX,
+ NLA_VALIDATE_MASK,
NLA_VALIDATE_RANGE_PTR,
NLA_VALIDATE_FUNCTION,
};
@@ -317,6 +318,7 @@ struct nla_policy {
u16 len;
union {
const u32 bitfield32_valid;
+ const u32 mask;
const char *reject_message;
const struct nla_policy *nested_policy;
struct netlink_range_validation *range;
@@ -363,6 +365,9 @@ struct nla_policy {
{ .type = NLA_BITFIELD32, .bitfield32_valid = valid }
#define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
+#define NLA_ENSURE_UINT_TYPE(tp) \
+ (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
+ tp == NLA_U32 || tp == NLA_U64) + tp)
#define NLA_ENSURE_UINT_OR_BINARY_TYPE(tp) \
(__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
tp == NLA_U32 || tp == NLA_U64 || \
@@ -415,6 +420,12 @@ struct nla_policy {
.max = _max, \
}
+#define NLA_POLICY_MASK(tp, _mask) { \
+ .type = NLA_ENSURE_UINT_TYPE(tp), \
+ .validation_type = NLA_VALIDATE_MASK, \
+ .mask = _mask, \
+}
+
#define NLA_POLICY_VALIDATE_FN(tp, fn, ...) { \
.type = NLA_ENSURE_NO_VALIDATION_PTR(tp), \
.validation_type = NLA_VALIDATE_FUNCTION, \
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index eac8a6a648ea..d02e472ba54c 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -331,6 +331,7 @@ enum netlink_attribute_type {
* the index, if limited inside the nesting (U32)
* @NL_POLICY_TYPE_ATTR_BITFIELD32_MASK: valid mask for the
* bitfield32 type (U32)
+ * @NL_POLICY_TYPE_ATTR_MASK: mask of valid bits for unsigned integers (U64)
* @NL_POLICY_TYPE_ATTR_PAD: pad attribute for 64-bit alignment
*/
enum netlink_policy_type_attr {
@@ -346,6 +347,7 @@ enum netlink_policy_type_attr {
NL_POLICY_TYPE_ATTR_POLICY_MAXTYPE,
NL_POLICY_TYPE_ATTR_BITFIELD32_MASK,
NL_POLICY_TYPE_ATTR_PAD,
+ NL_POLICY_TYPE_ATTR_MASK,
/* keep last */
__NL_POLICY_TYPE_ATTR_MAX,
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 80ff9fe83696..9c99f5daa4d2 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -323,6 +323,37 @@ static int nla_validate_int_range(const struct nla_policy *pt,
}
}
+static int nla_validate_mask(const struct nla_policy *pt,
+ const struct nlattr *nla,
+ struct netlink_ext_ack *extack)
+{
+ u64 value;
+
+ switch (pt->type) {
+ case NLA_U8:
+ value = nla_get_u8(nla);
+ break;
+ case NLA_U16:
+ value = nla_get_u16(nla);
+ break;
+ case NLA_U32:
+ value = nla_get_u32(nla);
+ break;
+ case NLA_U64:
+ value = nla_get_u64(nla);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (value & ~(u64)pt->mask) {
+ NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy, unsigned int validate,
struct netlink_ext_ack *extack, unsigned int depth)
@@ -503,6 +534,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
if (err)
return err;
break;
+ case NLA_VALIDATE_MASK:
+ err = nla_validate_mask(pt, nla, extack);
+ if (err)
+ return err;
+ break;
case NLA_VALIDATE_FUNCTION:
if (pt->validate) {
err = pt->validate(nla, extack);
diff --git a/net/netlink/policy.c b/net/netlink/policy.c
index cf23c0151721..ee26d01328ee 100644
--- a/net/netlink/policy.c
+++ b/net/netlink/policy.c
@@ -263,6 +263,14 @@ int netlink_policy_dump_write(struct sk_buff *skb,
else
type = NL_ATTR_TYPE_U64;
+ if (pt->validation_type == NLA_VALIDATE_MASK) {
+ if (nla_put_u64_64bit(skb, NL_POLICY_TYPE_ATTR_MASK,
+ pt->mask,
+ NL_POLICY_TYPE_ATTR_PAD))
+ goto nla_put_failure;
+ break;
+ }
+
nla_get_range_unsigned(pt, &range);
if (nla_put_u64_64bit(skb, NL_POLICY_TYPE_ATTR_MIN_VALUE_U,
--
2.26.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 6/6] ethtool: specify which header flags are supported per command
2020-10-05 15:57 [PATCH net-next 0/6] ethtool: allow dumping policies to user space Jakub Kicinski
` (4 preceding siblings ...)
2020-10-05 15:57 ` [PATCH net-next 5/6] netlink: add mask validation Jakub Kicinski
@ 2020-10-05 15:57 ` Jakub Kicinski
2020-10-05 18:58 ` Johannes Berg
5 siblings, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 15:57 UTC (permalink / raw)
To: davem
Cc: netdev, kernel-team, johannes, jiri, andrew, mkubecek, Jakub Kicinski
Perform header flags validation through the policy.
Only pause command supports ETHTOOL_FLAG_STATS. Create a separate
policy to be able to express that in policy dumps to user space.
Note that even though the core will validate the header policy,
it cannot record multiple layers of attributes and we have to
re-parse header sub-attrs. When doing so we could skip attribute
validation, or use most permissive policy.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/ethtool/netlink.c | 31 +++++++++++++++++++++----------
net/ethtool/netlink.h | 1 +
net/ethtool/pause.c | 2 +-
3 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index e78ff7ce2a7d..b972789d6b8d 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -9,12 +9,26 @@ static struct genl_family ethtool_genl_family;
static bool ethnl_ok __read_mostly;
static u32 ethnl_bcast_seq;
+#define ETHTOOL_FLAGS_BASIC (ETHTOOL_FLAG_COMPACT_BITSETS | \
+ ETHTOOL_FLAG_OMIT_REPLY)
+#define ETHTOOL_FLAGS_STATS (ETHTOOL_FLAGS_BASIC | ETHTOOL_FLAG_STATS)
+
const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_MAX + 1] = {
[ETHTOOL_A_HEADER_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_HEADER_DEV_INDEX] = { .type = NLA_U32 },
[ETHTOOL_A_HEADER_DEV_NAME] = { .type = NLA_NUL_STRING,
.len = ALTIFNAMSIZ - 1 },
- [ETHTOOL_A_HEADER_FLAGS] = { .type = NLA_U32 },
+ [ETHTOOL_A_HEADER_FLAGS] = NLA_POLICY_MASK(NLA_U32,
+ ETHTOOL_FLAGS_BASIC),
+};
+
+const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_MAX + 1] = {
+ [ETHTOOL_A_HEADER_UNSPEC] = { .type = NLA_REJECT },
+ [ETHTOOL_A_HEADER_DEV_INDEX] = { .type = NLA_U32 },
+ [ETHTOOL_A_HEADER_DEV_NAME] = { .type = NLA_NUL_STRING,
+ .len = ALTIFNAMSIZ - 1 },
+ [ETHTOOL_A_HEADER_FLAGS] = NLA_POLICY_MASK(NLA_U32,
+ ETHTOOL_FLAGS_STATS),
};
/**
@@ -47,19 +61,16 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
NL_SET_ERR_MSG(extack, "request header missing");
return -EINVAL;
}
+ /* Use most permissive header policy here, ops should specify their
+ * actual header policy via NLA_POLICY_NESTED(), and the real
+ * validation will happen in genetlink code.
+ */
ret = nla_parse_nested(tb, ETHTOOL_A_HEADER_MAX, header,
- ethnl_header_policy, extack);
+ ethnl_header_policy_stats, extack);
if (ret < 0)
return ret;
- if (tb[ETHTOOL_A_HEADER_FLAGS]) {
+ if (tb[ETHTOOL_A_HEADER_FLAGS])
flags = nla_get_u32(tb[ETHTOOL_A_HEADER_FLAGS]);
- if (flags & ~ETHTOOL_FLAG_ALL) {
- NL_SET_ERR_MSG_ATTR(extack, tb[ETHTOOL_A_HEADER_FLAGS],
- "unrecognized request flags");
- nl_set_extack_cookie_u32(extack, ETHTOOL_FLAG_ALL);
- return -EOPNOTSUPP;
- }
- }
devname_attr = tb[ETHTOOL_A_HEADER_DEV_NAME];
if (tb[ETHTOOL_A_HEADER_DEV_INDEX]) {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 4fcd2e8b259b..f04d94e5fa77 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -348,6 +348,7 @@ extern const struct ethnl_request_ops ethnl_eee_request_ops;
extern const struct ethnl_request_ops ethnl_tsinfo_request_ops;
extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_MAX + 1];
+extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_MAX + 1];
extern const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_MAX + 1];
extern const struct nla_policy ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_MAX + 1];
extern const struct nla_policy ethnl_linkinfo_set_policy[ETHTOOL_A_LINKINFO_MAX + 1];
diff --git a/net/ethtool/pause.c b/net/ethtool/pause.c
index a7fbe0e4dca6..a5c2abddc992 100644
--- a/net/ethtool/pause.c
+++ b/net/ethtool/pause.c
@@ -19,7 +19,7 @@ struct pause_reply_data {
const struct nla_policy ethnl_pause_get_policy[ETHTOOL_A_PAUSE_MAX + 1] = {
[ETHTOOL_A_PAUSE_UNSPEC] = { .type = NLA_REJECT },
[ETHTOOL_A_PAUSE_HEADER] =
- NLA_POLICY_NESTED(ethnl_header_policy),
+ NLA_POLICY_NESTED(ethnl_header_policy_stats),
[ETHTOOL_A_PAUSE_AUTONEG] = { .type = NLA_REJECT },
[ETHTOOL_A_PAUSE_RX] = { .type = NLA_REJECT },
[ETHTOOL_A_PAUSE_TX] = { .type = NLA_REJECT },
--
2.26.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 15:57 ` [PATCH net-next 1/6] ethtool: wire up get policies to ops Jakub Kicinski
@ 2020-10-05 18:56 ` Johannes Berg
2020-10-05 19:16 ` Jakub Kicinski
0 siblings, 1 reply; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 18:56 UTC (permalink / raw)
To: Jakub Kicinski, davem; +Cc: netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
>
> @@ -783,6 +799,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
> .start = ethnl_default_start,
> .dumpit = ethnl_default_dumpit,
> .done = ethnl_default_done,
> + .policy = ethnl_rings_get_policy,
> + .maxattr = ARRAY_SIZE(ethnl_rings_get_policy) - 1,
> +
> },
If you find some other reason to respin, perhaps remove that blank line
:)
Unrelated to that, it bothers me a bit that you put here the maxattr as
the ARRAY_SIZE(), which is of course fine, but then still have
> @@ -127,7 +127,7 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = {
> .max_attr = ETHTOOL_A_PRIVFLAGS_MAX,
max_attr here, using the original define - yes, mostly the policies use
the define to size them, but they didn't really *need* to, and one might
make an argument that on the policy arrays the size might as well be
removed (and it be sized automatically based on the contents) since all
the unspecified attrs are rejected anyway.
But with the difference it seems to me that it'd be possible to get this
mixed up?
I do see that you still need this to size the attrs for parsing them
even after patch 2 where this:
> .req_info_size = sizeof(struct privflags_req_info),
> .reply_data_size = sizeof(struct privflags_reply_data),
> - .request_policy = privflags_get_policy,
> + .request_policy = ethnl_privflags_get_policy,
gets removed completely.
Perhaps we can look up the genl_ops pointer, or add the ops pointer to
struct genl_info (could point to the temporary full struct that gets
populated, size of genl_info itself doesn't matter much since it's on
the stack and temporary), and then use ops->maxattr instead of
request_ops->max_attr in ethnl_default_parse()?
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 6/6] ethtool: specify which header flags are supported per command
2020-10-05 15:57 ` [PATCH net-next 6/6] ethtool: specify which header flags are supported per command Jakub Kicinski
@ 2020-10-05 18:58 ` Johannes Berg
2020-10-05 19:25 ` Jakub Kicinski
0 siblings, 1 reply; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 18:58 UTC (permalink / raw)
To: Jakub Kicinski, davem; +Cc: netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
>
> @@ -47,19 +61,16 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
> NL_SET_ERR_MSG(extack, "request header missing");
> return -EINVAL;
> }
> + /* Use most permissive header policy here, ops should specify their
> + * actual header policy via NLA_POLICY_NESTED(), and the real
> + * validation will happen in genetlink code.
> + */
> ret = nla_parse_nested(tb, ETHTOOL_A_HEADER_MAX, header,
> - ethnl_header_policy, extack);
> + ethnl_header_policy_stats, extack);
Would it make sense to just remove the validation here? It's already
done, so it just costs extra cycles and can't really fail, and if there
are more diverse policies in the future this might also very quickly get
out of hand?
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 15:57 ` [PATCH net-next 5/6] netlink: add mask validation Jakub Kicinski
@ 2020-10-05 19:05 ` Johannes Berg
2020-10-05 19:22 ` Jakub Kicinski
2020-10-05 19:28 ` Michal Kubecek
0 siblings, 2 replies; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 19:05 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: netdev, kernel-team, jiri, andrew, mkubecek, dsahern, pablo
On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
> We don't have good validation policy for existing unsigned int attrs
> which serve as flags (for new ones we could use NLA_BITFIELD32).
> With increased use of policy dumping having the validation be
> expressed as part of the policy is important. Add validation
> policy in form of a mask of supported/valid bits.
Nice, I'll surely use this as well somewhere :)
> #define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
> +#define NLA_ENSURE_UINT_TYPE(tp) \
> + (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
> + tp == NLA_U32 || tp == NLA_U64) + tp)
> #define NLA_ENSURE_UINT_OR_BINARY_TYPE(tp) \
nit: maybe express this (_OR_BINARY_TYPE) in terms of UINT_TYPE() ||
tp==NLA_BINARY? Doesn't matter much though.
> +static int nla_validate_mask(const struct nla_policy *pt,
> + const struct nlattr *nla,
> + struct netlink_ext_ack *extack)
> +{
> + u64 value;
> +
> + switch (pt->type) {
> + case NLA_U8:
> + value = nla_get_u8(nla);
> + break;
> + case NLA_U16:
> + value = nla_get_u16(nla);
> + break;
> + case NLA_U32:
> + value = nla_get_u32(nla);
> + break;
> + case NLA_U64:
> + value = nla_get_u64(nla);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (value & ~(u64)pt->mask) {
> + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> + return -EINVAL;
You had an export of the valid bits there in ethtool, using the cookie.
Just pointing out you lost it now. I'm not sure I like using the cookie,
that seems a bit strange, but we could easily define a different attr?
OTOH, one can always query the policy export too (which hopefully got
wired up) so it wouldn't really matter much.
Either way is fine with me on both of these points.
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Thanks!
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 18:56 ` Johannes Berg
@ 2020-10-05 19:16 ` Jakub Kicinski
2020-10-05 19:21 ` Johannes Berg
0 siblings, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 19:16 UTC (permalink / raw)
To: Johannes Berg; +Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 05 Oct 2020 20:56:29 +0200 Johannes Berg wrote:
> On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
> > @@ -783,6 +799,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
> > .start = ethnl_default_start,
> > .dumpit = ethnl_default_dumpit,
> > .done = ethnl_default_done,
> > + .policy = ethnl_rings_get_policy,
> > + .maxattr = ARRAY_SIZE(ethnl_rings_get_policy) - 1,
> > +
> > },
>
> If you find some other reason to respin, perhaps remove that blank line
> :)
>
> Unrelated to that, it bothers me a bit that you put here the maxattr as
> the ARRAY_SIZE(), which is of course fine, but then still have
>
> > @@ -127,7 +127,7 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = {
> > .max_attr = ETHTOOL_A_PRIVFLAGS_MAX,
>
> max_attr here, using the original define
Ah, another good catch, this is obviously no longer needed. I will
remove those members in v2.
> yes, mostly the policies use
> the define to size them, but they didn't really *need* to, and one might
> make an argument that on the policy arrays the size might as well be
> removed (and it be sized automatically based on the contents) since all
> the unspecified attrs are rejected anyway.
>
> But with the difference it seems to me that it'd be possible to get this
> mixed up?
Right, I prefer not to have the unnecessary NLA_REJECTS, so my thinking
was - use the format I like for the new code, but leave the existing
rejects for a separate series / discussion.
If we remove the rejects we still need something like
extern struct nla_policy policy[lastattr + 1];
For array_size to work, but I think that's fine. And we'd get a
compiler errors if the sizes don't match up.
> I do see that you still need this to size the attrs for parsing them
> even after patch 2 where this:
>
> > .req_info_size = sizeof(struct privflags_req_info),
> > .reply_data_size = sizeof(struct privflags_reply_data),
> > - .request_policy = privflags_get_policy,
> > + .request_policy = ethnl_privflags_get_policy,
>
> gets removed completely.
>
>
> Perhaps we can look up the genl_ops pointer, or add the ops pointer to
> struct genl_info (could point to the temporary full struct that gets
> populated, size of genl_info itself doesn't matter much since it's on
> the stack and temporary), and then use ops->maxattr instead of
> request_ops->max_attr in ethnl_default_parse()?
Hm, maybe my split of patches 1 and 2 hurts more than it helps.
Let me merge the two in v2.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 19:16 ` Jakub Kicinski
@ 2020-10-05 19:21 ` Johannes Berg
2020-10-05 19:31 ` Jakub Kicinski
0 siblings, 1 reply; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 19:21 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 2020-10-05 at 12:16 -0700, Jakub Kicinski wrote:
> On Mon, 05 Oct 2020 20:56:29 +0200 Johannes Berg wrote:
> > On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
> > > @@ -783,6 +799,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
> > > .start = ethnl_default_start,
> > > .dumpit = ethnl_default_dumpit,
> > > .done = ethnl_default_done,
> > > + .policy = ethnl_rings_get_policy,
> > > + .maxattr = ARRAY_SIZE(ethnl_rings_get_policy) - 1,
> > > +
> > > },
> >
> > If you find some other reason to respin, perhaps remove that blank line
> > :)
> >
> > Unrelated to that, it bothers me a bit that you put here the maxattr as
> > the ARRAY_SIZE(), which is of course fine, but then still have
> >
> > > @@ -127,7 +127,7 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = {
> > > .max_attr = ETHTOOL_A_PRIVFLAGS_MAX,
> >
> > max_attr here, using the original define
>
> Ah, another good catch, this is obviously no longer needed. I will
> remove those members in v2.
Good point, I misread/misunderstood the code and thought it was still
being used to size the parsing array, but that's of course no longer
there since the genl core now does it.
> > But with the difference it seems to me that it'd be possible to get this
> > mixed up?
>
> Right, I prefer not to have the unnecessary NLA_REJECTS, so my thinking
> was - use the format I like for the new code, but leave the existing
> rejects for a separate series / discussion.
>
> If we remove the rejects we still need something like
>
> extern struct nla_policy policy[lastattr + 1];
Not sure I understand? You're using strict validation (I think), so
attrs that are out of range will be rejected same as NLA_REJECT (well,
with a different message) in __nla_validate_parse():
nla_for_each_attr(nla, head, len, rem) {
u16 type = nla_type(nla);
if (type == 0 || type > maxtype) {
if (validate & NL_VALIDATE_MAXTYPE) {
NL_SET_ERR_MSG_ATTR(extack, nla,
"Unknown attribute type");
return -EINVAL;
}
In fact, if you're using strict validation even the default
(0==NLA_UNSPEC) will be rejected, just like NLA_REJECT.
Or am I confused somewhere?
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 19:05 ` Johannes Berg
@ 2020-10-05 19:22 ` Jakub Kicinski
2020-10-05 19:25 ` Johannes Berg
2020-10-05 19:28 ` Michal Kubecek
1 sibling, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 19:22 UTC (permalink / raw)
To: Johannes Berg
Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek, dsahern, pablo
On Mon, 05 Oct 2020 21:05:23 +0200 Johannes Berg wrote:
> On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
> > We don't have good validation policy for existing unsigned int attrs
> > which serve as flags (for new ones we could use NLA_BITFIELD32).
> > With increased use of policy dumping having the validation be
> > expressed as part of the policy is important. Add validation
> > policy in form of a mask of supported/valid bits.
>
> Nice, I'll surely use this as well somewhere :)
>
> > #define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
> > +#define NLA_ENSURE_UINT_TYPE(tp) \
> > + (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
> > + tp == NLA_U32 || tp == NLA_U64) + tp)
> > #define NLA_ENSURE_UINT_OR_BINARY_TYPE(tp) \
>
> nit: maybe express this (_OR_BINARY_TYPE) in terms of UINT_TYPE() ||
> tp==NLA_BINARY? Doesn't matter much though.
Will do!
> > +static int nla_validate_mask(const struct nla_policy *pt,
> > + const struct nlattr *nla,
> > + struct netlink_ext_ack *extack)
> > +{
> > + u64 value;
> > +
> > + switch (pt->type) {
> > + case NLA_U8:
> > + value = nla_get_u8(nla);
> > + break;
> > + case NLA_U16:
> > + value = nla_get_u16(nla);
> > + break;
> > + case NLA_U32:
> > + value = nla_get_u32(nla);
> > + break;
> > + case NLA_U64:
> > + value = nla_get_u64(nla);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (value & ~(u64)pt->mask) {
> > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> > + return -EINVAL;
>
> You had an export of the valid bits there in ethtool, using the cookie.
> Just pointing out you lost it now. I'm not sure I like using the cookie,
> that seems a bit strange, but we could easily define a different attr?
>
> OTOH, one can always query the policy export too (which hopefully got
> wired up) so it wouldn't really matter much.
My thinking is that there are no known uses of the cookie, it'd only
have practical use to test for new flags - and we're adding first new
flag in 5.10.
> Either way is fine with me on both of these points.
>
> Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Thanks!
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 6/6] ethtool: specify which header flags are supported per command
2020-10-05 18:58 ` Johannes Berg
@ 2020-10-05 19:25 ` Jakub Kicinski
2020-10-05 19:28 ` Johannes Berg
0 siblings, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 19:25 UTC (permalink / raw)
To: Johannes Berg; +Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 05 Oct 2020 20:58:57 +0200 Johannes Berg wrote:
> On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
> >
> > @@ -47,19 +61,16 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
> > NL_SET_ERR_MSG(extack, "request header missing");
> > return -EINVAL;
> > }
> > + /* Use most permissive header policy here, ops should specify their
> > + * actual header policy via NLA_POLICY_NESTED(), and the real
> > + * validation will happen in genetlink code.
> > + */
> > ret = nla_parse_nested(tb, ETHTOOL_A_HEADER_MAX, header,
> > - ethnl_header_policy, extack);
> > + ethnl_header_policy_stats, extack);
>
> Would it make sense to just remove the validation here? It's already
> done, so it just costs extra cycles and can't really fail, and if there
> are more diverse policies in the future this might also very quickly get
> out of hand?
I was slightly worried I missed a command and need last line of defence,
but you're right, I'll just pass a NULL for the policy in v2 :)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 19:22 ` Jakub Kicinski
@ 2020-10-05 19:25 ` Johannes Berg
2020-10-05 19:34 ` Jakub Kicinski
0 siblings, 1 reply; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 19:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek, dsahern, pablo
On Mon, 2020-10-05 at 12:22 -0700, Jakub Kicinski wrote:
> > > + if (value & ~(u64)pt->mask) {
> > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> > > + return -EINVAL;
> >
> > You had an export of the valid bits there in ethtool, using the cookie.
> > Just pointing out you lost it now. I'm not sure I like using the cookie,
> > that seems a bit strange, but we could easily define a different attr?
> >
> > OTOH, one can always query the policy export too (which hopefully got
> > wired up) so it wouldn't really matter much.
>
> My thinking is that there are no known uses of the cookie, it'd only
> have practical use to test for new flags - and we're adding first new
> flag in 5.10.
Hm, wait, not sure I understand?
You _had_ this in ethtool, but you removed it now. And you're not
keeping it here, afaict.
I can't disagree on the "no known uses of the cookie" part, but it feels
odd to me anyway - since that is just another netlink message (*), you
could as well add a "NLMSGERR_ATTR_VALID_FLAGS" instead of sticking the
data into the cookie?
But then are you saying the new flags are only in 5.10 so the policy
export will be sufficient, since that's also wired up now?
johannes
(*) in a way - the ack message has the "legacy" fixed part before the
attrs, of course
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 6/6] ethtool: specify which header flags are supported per command
2020-10-05 19:25 ` Jakub Kicinski
@ 2020-10-05 19:28 ` Johannes Berg
0 siblings, 0 replies; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 19:28 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 2020-10-05 at 12:25 -0700, Jakub Kicinski wrote:
> On Mon, 05 Oct 2020 20:58:57 +0200 Johannes Berg wrote:
> > On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
> > > @@ -47,19 +61,16 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
> > > NL_SET_ERR_MSG(extack, "request header missing");
> > > return -EINVAL;
> > > }
> > > + /* Use most permissive header policy here, ops should specify their
> > > + * actual header policy via NLA_POLICY_NESTED(), and the real
> > > + * validation will happen in genetlink code.
> > > + */
> > > ret = nla_parse_nested(tb, ETHTOOL_A_HEADER_MAX, header,
> > > - ethnl_header_policy, extack);
> > > + ethnl_header_policy_stats, extack);
> >
> > Would it make sense to just remove the validation here? It's already
> > done, so it just costs extra cycles and can't really fail, and if there
> > are more diverse policies in the future this might also very quickly get
> > out of hand?
>
> I was slightly worried I missed a command and need last line of defence,
Ah. I was just about to suggest to put it into the family policy/maxattr
but that won't work of course since this is nested.
But actually what you _could_ put there is a dummy policy (non-NULL
pointer) with a maxattr of 0, and then all attrs will be completely
rejected for a command where the policy was missed.
Not if you missed the NLA_POLICY_NESTED() link, though.
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 19:05 ` Johannes Berg
2020-10-05 19:22 ` Jakub Kicinski
@ 2020-10-05 19:28 ` Michal Kubecek
2020-10-05 19:31 ` Johannes Berg
1 sibling, 1 reply; 34+ messages in thread
From: Michal Kubecek @ 2020-10-05 19:28 UTC (permalink / raw)
To: Johannes Berg
Cc: Jakub Kicinski, davem, netdev, kernel-team, jiri, andrew, dsahern, pablo
On Mon, Oct 05, 2020 at 09:05:23PM +0200, Johannes Berg wrote:
> On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
>
> > +static int nla_validate_mask(const struct nla_policy *pt,
> > + const struct nlattr *nla,
> > + struct netlink_ext_ack *extack)
> > +{
> > + u64 value;
> > +
> > + switch (pt->type) {
> > + case NLA_U8:
> > + value = nla_get_u8(nla);
> > + break;
> > + case NLA_U16:
> > + value = nla_get_u16(nla);
> > + break;
> > + case NLA_U32:
> > + value = nla_get_u32(nla);
> > + break;
> > + case NLA_U64:
> > + value = nla_get_u64(nla);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (value & ~(u64)pt->mask) {
> > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> > + return -EINVAL;
>
> You had an export of the valid bits there in ethtool, using the cookie.
> Just pointing out you lost it now. I'm not sure I like using the cookie,
> that seems a bit strange, but we could easily define a different attr?
The idea behind the cookie was that if new userspace sends a request
with multiple flags which may not be supported by an old kernel, getting
only -EOPNOTSUPP (and badattr pointing to the flags) would not be very
helpful as multiple iteration would be necessary to find out which flags
are supported and which not.
> OTOH, one can always query the policy export too (which hopefully got
> wired up) so it wouldn't really matter much.
But yes, if userspace can get supported flags from policy dump, it can
check them in advance and either bail out (if one of essential flags is
unsupported) or send only supported flags.
I'm not exactly happy with the prospect of having to do a full policy
dump before each such request, thought.
Michal
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 19:28 ` Michal Kubecek
@ 2020-10-05 19:31 ` Johannes Berg
2020-10-05 19:40 ` Jakub Kicinski
0 siblings, 1 reply; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 19:31 UTC (permalink / raw)
To: Michal Kubecek
Cc: Jakub Kicinski, davem, netdev, kernel-team, jiri, andrew, dsahern, pablo
On Mon, 2020-10-05 at 21:28 +0200, Michal Kubecek wrote:
> > > + if (value & ~(u64)pt->mask) {
> > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> > > + return -EINVAL;
> >
> > You had an export of the valid bits there in ethtool, using the cookie.
> > Just pointing out you lost it now. I'm not sure I like using the cookie,
> > that seems a bit strange, but we could easily define a different attr?
>
> The idea behind the cookie was that if new userspace sends a request
> with multiple flags which may not be supported by an old kernel, getting
> only -EOPNOTSUPP (and badattr pointing to the flags) would not be very
> helpful as multiple iteration would be necessary to find out which flags
> are supported and which not.
Message crossing, I guess.
I completely agree. I just don't like using the (somewhat vague)
_cookie_ for that vs. adding a new explicit NLMSGERR_ATTR_SOMETHING :)
I would totally support doing that here in the general validation code,
but (again) don't really think NLMSGERR_ATTR_COOKIE is an appropriate
attribute for it.
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 19:21 ` Johannes Berg
@ 2020-10-05 19:31 ` Jakub Kicinski
2020-10-05 19:33 ` Johannes Berg
2020-10-05 21:33 ` Jacob Keller
0 siblings, 2 replies; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 19:31 UTC (permalink / raw)
To: Johannes Berg; +Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 05 Oct 2020 21:21:36 +0200 Johannes Berg wrote:
> > > But with the difference it seems to me that it'd be possible to get this
> > > mixed up?
> >
> > Right, I prefer not to have the unnecessary NLA_REJECTS, so my thinking
> > was - use the format I like for the new code, but leave the existing
> > rejects for a separate series / discussion.
> >
> > If we remove the rejects we still need something like
> >
> > extern struct nla_policy policy[lastattr + 1];
>
> Not sure I understand? You're using strict validation (I think), so
> attrs that are out of range will be rejected same as NLA_REJECT (well,
> with a different message) in __nla_validate_parse():
>
> nla_for_each_attr(nla, head, len, rem) {
> u16 type = nla_type(nla);
>
> if (type == 0 || type > maxtype) {
> if (validate & NL_VALIDATE_MAXTYPE) {
> NL_SET_ERR_MSG_ATTR(extack, nla,
> "Unknown attribute type");
> return -EINVAL;
> }
>
>
> In fact, if you're using strict validation even the default
> (0==NLA_UNSPEC) will be rejected, just like NLA_REJECT.
>
>
> Or am I confused somewhere?
Yea, I think we're both confused. Agreed with the above.
Are you suggesting:
const struct nla_policy policy[/* no size */] = {
[HEADER] = NLA_POLICY(...)
[OTHER_ATTR] = NLA_POLICY(...)
};
extern const struct nla_policy policy[/* no size */];
op = {
.policy = policy,
.max_attr = OTHER_ATTR,
}
?
What I'm saying is that my preference would be:
const struct nla_policy policy[OTHER_ATTR + 1] = {
[HEADER] = NLA_POLICY(...)
[OTHER_ATTR] = NLA_POLICY(...)
};
extern const struct nla_policy policy[OTHER_ATTR + 1];
op = {
.policy = policy,
.max_attr = ARRAY_SIZE(policy) - 1,
}
Since it's harder to forget to update the op (you don't have to update
op, and compiler will complain about the extern out of sync).
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 19:31 ` Jakub Kicinski
@ 2020-10-05 19:33 ` Johannes Berg
2020-10-05 19:41 ` Jakub Kicinski
2020-10-05 21:52 ` Jacob Keller
2020-10-05 21:33 ` Jacob Keller
1 sibling, 2 replies; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 19:33 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 2020-10-05 at 12:31 -0700, Jakub Kicinski wrote:
> Yea, I think we're both confused. Agreed with the above.
>
> Are you suggesting:
>
> const struct nla_policy policy[/* no size */] = {
> [HEADER] = NLA_POLICY(...)
> [OTHER_ATTR] = NLA_POLICY(...)
> };
>
> extern const struct nla_policy policy[/* no size */];
>
> op = {
> .policy = policy,
> .max_attr = OTHER_ATTR,
> }
No, that'd be awkward, for the reason you stated below.
> What I'm saying is that my preference would be:
>
> const struct nla_policy policy[OTHER_ATTR + 1] = {
> [HEADER] = NLA_POLICY(...)
> [OTHER_ATTR] = NLA_POLICY(...)
> };
>
> extern const struct nla_policy policy[OTHER_ATTR + 1];
>
> op = {
> .policy = policy,
> .max_attr = ARRAY_SIZE(policy) - 1,
> }
>
> Since it's harder to forget to update the op (you don't have to update
> op, and compiler will complain about the extern out of sync).
Yeah.
I was thinking the third way ;-)
const struct nla_policy policy[] = {
[HEADER] = NLA_POLICY(...)
[OTHER_ATTR] = NLA_POLICY(...)
};
op = {
.policy = policy,
.maxattr = ARRAY_SIZE(policy) - 1,
};
Now you can freely add any attributes, and, due to strict validation,
anything not specified in the policy will be rejected, whether by being
out of range (> maxattr) or not specified (NLA_UNSPEC).
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 19:25 ` Johannes Berg
@ 2020-10-05 19:34 ` Jakub Kicinski
2020-10-05 19:37 ` Johannes Berg
2020-10-05 19:47 ` Michal Kubecek
0 siblings, 2 replies; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 19:34 UTC (permalink / raw)
To: Johannes Berg
Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek, dsahern, pablo
On Mon, 05 Oct 2020 21:25:57 +0200 Johannes Berg wrote:
> On Mon, 2020-10-05 at 12:22 -0700, Jakub Kicinski wrote:
>
> > > > + if (value & ~(u64)pt->mask) {
> > > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> > > > + return -EINVAL;
> > >
> > > You had an export of the valid bits there in ethtool, using the cookie.
> > > Just pointing out you lost it now. I'm not sure I like using the cookie,
> > > that seems a bit strange, but we could easily define a different attr?
> > >
> > > OTOH, one can always query the policy export too (which hopefully got
> > > wired up) so it wouldn't really matter much.
> >
> > My thinking is that there are no known uses of the cookie, it'd only
> > have practical use to test for new flags - and we're adding first new
> > flag in 5.10.
>
> Hm, wait, not sure I understand?
>
> You _had_ this in ethtool, but you removed it now. And you're not
> keeping it here, afaict.
>
> I can't disagree on the "no known uses of the cookie" part, but it feels
> odd to me anyway - since that is just another netlink message (*), you
> could as well add a "NLMSGERR_ATTR_VALID_FLAGS" instead of sticking the
> data into the cookie?
>
> But then are you saying the new flags are only in 5.10 so the policy
> export will be sufficient, since that's also wired up now?
Right, I was commenting on the need to keep the cookie for backward
compat.
My preference is to do a policy dump to check the capabilities of the
kernel rather than shoot messages at it and then try to work backward
based on the info returned in extack.
> johannes
>
> (*) in a way - the ack message has the "legacy" fixed part before the
> attrs, of course
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 19:34 ` Jakub Kicinski
@ 2020-10-05 19:37 ` Johannes Berg
2020-10-05 19:47 ` Michal Kubecek
1 sibling, 0 replies; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 19:37 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek, dsahern, pablo
On Mon, 2020-10-05 at 12:34 -0700, Jakub Kicinski wrote:
> > > My thinking is that there are no known uses of the cookie, it'd only
Ahh. I completely misinterpreted the word "uses" here - you meant, I
think (now), "uses of the cookie in the way that it was done in ethtool
before". I read over this because it seemed in a way obviously right and
also obviously wrong (no other uses of the cookie in ethtool and clearly
uses of the cookie elsewhere, respectively)...
> Right, I was commenting on the need to keep the cookie for backward
> compat.
Right ...
> My preference is to do a policy dump to check the capabilities of the
> kernel rather than shoot messages at it and then try to work backward
> based on the info returned in extack.
I guess Michal disagrees ;-)
I can see both points of view though - if you have just a single
attribute it's basically the same, but once you have two or more it's
way less complex to just query before, I'd think.
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 19:31 ` Johannes Berg
@ 2020-10-05 19:40 ` Jakub Kicinski
2020-10-05 19:53 ` Johannes Berg
0 siblings, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 19:40 UTC (permalink / raw)
To: Johannes Berg
Cc: Michal Kubecek, davem, netdev, kernel-team, jiri, andrew, dsahern, pablo
On Mon, 05 Oct 2020 21:31:13 +0200 Johannes Berg wrote:
> On Mon, 2020-10-05 at 21:28 +0200, Michal Kubecek wrote:
>
> > > > + if (value & ~(u64)pt->mask) {
> > > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> > > > + return -EINVAL;
> > >
> > > You had an export of the valid bits there in ethtool, using the cookie.
> > > Just pointing out you lost it now. I'm not sure I like using the cookie,
> > > that seems a bit strange, but we could easily define a different attr?
> >
> > The idea behind the cookie was that if new userspace sends a request
> > with multiple flags which may not be supported by an old kernel, getting
> > only -EOPNOTSUPP (and badattr pointing to the flags) would not be very
> > helpful as multiple iteration would be necessary to find out which flags
> > are supported and which not.
>
> Message crossing, I guess.
>
> I completely agree. I just don't like using the (somewhat vague)
> _cookie_ for that vs. adding a new explicit NLMSGERR_ATTR_SOMETHING :)
>
> I would totally support doing that here in the general validation code,
> but (again) don't really think NLMSGERR_ATTR_COOKIE is an appropriate
> attribute for it.
Hm. Perhaps we can do a partial policy dump into the extack?
NL_POLICY_TYPE_ATTR_TYPE etc.?
Either way, I don't feel like this series needs it.
> > I'm not exactly happy with the prospect of having to do a full policy
> > dump before each such request, thought.
I tried to code it up and it gets rather ugly.
One has to selectively suppress error messages for stuff that's known
to be optional, and keep retrying.
The policy dump is much, much cleaner (and the policy for an op is
rather tiny - one nested policy of the header with 3? attrs in it).
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 19:33 ` Johannes Berg
@ 2020-10-05 19:41 ` Jakub Kicinski
2020-10-05 19:46 ` Johannes Berg
2020-10-05 21:52 ` Jacob Keller
1 sibling, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 19:41 UTC (permalink / raw)
To: Johannes Berg; +Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 05 Oct 2020 21:33:55 +0200 Johannes Berg wrote:
> > What I'm saying is that my preference would be:
> >
> > const struct nla_policy policy[OTHER_ATTR + 1] = {
> > [HEADER] = NLA_POLICY(...)
> > [OTHER_ATTR] = NLA_POLICY(...)
> > };
> >
> > extern const struct nla_policy policy[OTHER_ATTR + 1];
> >
> > op = {
> > .policy = policy,
> > .max_attr = ARRAY_SIZE(policy) - 1,
> > }
> >
> > Since it's harder to forget to update the op (you don't have to update
> > op, and compiler will complain about the extern out of sync).
>
> Yeah.
>
> I was thinking the third way ;-)
>
> const struct nla_policy policy[] = {
> [HEADER] = NLA_POLICY(...)
> [OTHER_ATTR] = NLA_POLICY(...)
> };
>
> op = {
> .policy = policy,
> .maxattr = ARRAY_SIZE(policy) - 1,
> };
>
>
> Now you can freely add any attributes, and, due to strict validation,
> anything not specified in the policy will be rejected, whether by being
> out of range (> maxattr) or not specified (NLA_UNSPEC).
100%, but in ethtool policy is defined in a different compilation unit
than the op array.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 19:41 ` Jakub Kicinski
@ 2020-10-05 19:46 ` Johannes Berg
2020-10-05 19:51 ` Jakub Kicinski
0 siblings, 1 reply; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 19:46 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 2020-10-05 at 12:41 -0700, Jakub Kicinski wrote:
> > Now you can freely add any attributes, and, due to strict validation,
> > anything not specified in the policy will be rejected, whether by being
> > out of range (> maxattr) or not specified (NLA_UNSPEC).
>
> 100%, but in ethtool policy is defined in a different compilation unit
> than the op array.
Ah. OK, then that won't work, of course, never mind.
I'd probably go with your preference then, but perhaps drop the actual
size definition:
const struct nla_policy policy[] = {
...
};
extern const struct nla_policy policy[OTHER_ATTR + 1];
op = {
.policy = policy,
.max_attr = ARRAY_SIZE(policy) - 1,
}
But that'd really just be to save typing copying it if it ever changes,
since it's compiler checked for consistency.
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 19:34 ` Jakub Kicinski
2020-10-05 19:37 ` Johannes Berg
@ 2020-10-05 19:47 ` Michal Kubecek
1 sibling, 0 replies; 34+ messages in thread
From: Michal Kubecek @ 2020-10-05 19:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Johannes Berg, davem, netdev, kernel-team, jiri, andrew, dsahern, pablo
On Mon, Oct 05, 2020 at 12:34:14PM -0700, Jakub Kicinski wrote:
> On Mon, 05 Oct 2020 21:25:57 +0200 Johannes Berg wrote:
> > On Mon, 2020-10-05 at 12:22 -0700, Jakub Kicinski wrote:
> >
> > > > > + if (value & ~(u64)pt->mask) {
> > > > > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> > > > > + return -EINVAL;
> > > >
> > > > You had an export of the valid bits there in ethtool, using the cookie.
> > > > Just pointing out you lost it now. I'm not sure I like using the cookie,
> > > > that seems a bit strange, but we could easily define a different attr?
> > > >
> > > > OTOH, one can always query the policy export too (which hopefully got
> > > > wired up) so it wouldn't really matter much.
> > >
> > > My thinking is that there are no known uses of the cookie, it'd only
> > > have practical use to test for new flags - and we're adding first new
> > > flag in 5.10.
> >
> > Hm, wait, not sure I understand?
> >
> > You _had_ this in ethtool, but you removed it now. And you're not
> > keeping it here, afaict.
> >
> > I can't disagree on the "no known uses of the cookie" part, but it feels
> > odd to me anyway - since that is just another netlink message (*), you
> > could as well add a "NLMSGERR_ATTR_VALID_FLAGS" instead of sticking the
> > data into the cookie?
> >
> > But then are you saying the new flags are only in 5.10 so the policy
> > export will be sufficient, since that's also wired up now?
>
> Right, I was commenting on the need to keep the cookie for backward
> compat.
>
> My preference is to do a policy dump to check the capabilities of the
> kernel rather than shoot messages at it and then try to work backward
> based on the info returned in extack.
The reason I used the extack was that that way, the "standard" case of
kernel and ethtool in sync (or even old ethtool with new kernel) would
still use one request and only new ethtool against old kernel would end
up doing two. Also, the extra request would be relatively short and so
would be the reply to it.
On the other hand, using the policy dump would allow checking not only
support for new flags but also support for new request attributes so in
the long term, it could actually make things simpler.
Michal
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 19:46 ` Johannes Berg
@ 2020-10-05 19:51 ` Jakub Kicinski
0 siblings, 0 replies; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 19:51 UTC (permalink / raw)
To: Johannes Berg; +Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On Mon, 05 Oct 2020 21:46:02 +0200 Johannes Berg wrote:
> On Mon, 2020-10-05 at 12:41 -0700, Jakub Kicinski wrote:
>
> > > Now you can freely add any attributes, and, due to strict validation,
> > > anything not specified in the policy will be rejected, whether by being
> > > out of range (> maxattr) or not specified (NLA_UNSPEC).
> >
> > 100%, but in ethtool policy is defined in a different compilation unit
> > than the op array.
>
> Ah. OK, then that won't work, of course, never mind.
>
> I'd probably go with your preference then, but perhaps drop the actual
> size definition:
>
> const struct nla_policy policy[] = {
> ...
> };
>
> extern const struct nla_policy policy[OTHER_ATTR + 1];
>
> op = {
> .policy = policy,
> .max_attr = ARRAY_SIZE(policy) - 1,
> }
>
>
> But that'd really just be to save typing copying it if it ever changes,
> since it's compiler checked for consistency.
Sounds good, will do (unless Michal speaks up and prefers otherwise :)).
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 19:40 ` Jakub Kicinski
@ 2020-10-05 19:53 ` Johannes Berg
2020-10-05 20:12 ` Johannes Berg
0 siblings, 1 reply; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 19:53 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Michal Kubecek, davem, netdev, kernel-team, jiri, andrew, dsahern, pablo
On Mon, 2020-10-05 at 12:40 -0700, Jakub Kicinski wrote:
> > I would totally support doing that here in the general validation code,
> > but (again) don't really think NLMSGERR_ATTR_COOKIE is an appropriate
> > attribute for it.
>
> Hm. Perhaps we can do a partial policy dump into the extack?
Hm. I like that idea.
If we have NLMSGERR_ATTR_OFFS we could accompany that with the sub-
policy for that particular attribute, something like
[NLMSGERR_ATTR_POLICY] = nested {
[NL_POLICY_TYPE_ATTR_TYPE] = ...
[NL_POLICY_TYPE_ATTR_MASK] = ...
}
which we could basically do by factoring out the inner portion of
netlink_policy_dump_write():
attr = nla_nest_start(skb, state->attr_idx);
if (!attr)
goto nla_put_failure;
...
nla_nest_end(skb, attr);
from there into a separate function, give it the pt and the nested
attribute (what's "state->attr_idx" here) as arguments, and then we call
it with NLMSGERR_ATTR_POLICY from here, and with "state->attr_idx" from
netlink_policy_dump_write() :-)
Nice, easy & useful, maybe I'll code it up tomorrow.
> Either way, I don't feel like this series needs it.
Fair enough.
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 19:53 ` Johannes Berg
@ 2020-10-05 20:12 ` Johannes Berg
2020-10-05 22:21 ` Jakub Kicinski
0 siblings, 1 reply; 34+ messages in thread
From: Johannes Berg @ 2020-10-05 20:12 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Michal Kubecek, davem, netdev, kernel-team, jiri, andrew, dsahern, pablo
On Mon, 2020-10-05 at 21:53 +0200, Johannes Berg wrote:
> On Mon, 2020-10-05 at 12:40 -0700, Jakub Kicinski wrote:
>
> > > I would totally support doing that here in the general validation code,
> > > but (again) don't really think NLMSGERR_ATTR_COOKIE is an appropriate
> > > attribute for it.
> >
> > Hm. Perhaps we can do a partial policy dump into the extack?
>
> Hm. I like that idea.
>
> If we have NLMSGERR_ATTR_OFFS we could accompany that with the sub-
> policy for that particular attribute, something like
>
> [NLMSGERR_ATTR_POLICY] = nested {
> [NL_POLICY_TYPE_ATTR_TYPE] = ...
> [NL_POLICY_TYPE_ATTR_MASK] = ...
> }
>
> which we could basically do by factoring out the inner portion of
> netlink_policy_dump_write():
>
> attr = nla_nest_start(skb, state->attr_idx);
> if (!attr)
> goto nla_put_failure;
> ...
> nla_nest_end(skb, attr);
>
> from there into a separate function, give it the pt and the nested
> attribute (what's "state->attr_idx" here) as arguments, and then we call
> it with NLMSGERR_ATTR_POLICY from here, and with "state->attr_idx" from
> netlink_policy_dump_write() :-)
>
> Nice, easy & useful, maybe I'll code it up tomorrow.
OK I thought about it a bit more and looked at the code, and it's not
actually possible to do easily right now, because we can't actually
point to the bad attribute from the general lib/nlattr.c code ...
Why? Because we don't know right now, e.g. for nla_validate(), where in
the message we started validation, i.e. the offset of the "head" inside
the particular message.
For nlmsg_parse() and friends that's a bit easier, but it needs more
rejiggering than I'm willing to do tonight ;)
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 19:31 ` Jakub Kicinski
2020-10-05 19:33 ` Johannes Berg
@ 2020-10-05 21:33 ` Jacob Keller
1 sibling, 0 replies; 34+ messages in thread
From: Jacob Keller @ 2020-10-05 21:33 UTC (permalink / raw)
To: Jakub Kicinski, Johannes Berg
Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On 10/5/2020 12:31 PM, Jakub Kicinski wrote:
> On Mon, 05 Oct 2020 21:21:36 +0200 Johannes Berg wrote:
>>>> But with the difference it seems to me that it'd be possible to get this
>>>> mixed up?
>>>
>>> Right, I prefer not to have the unnecessary NLA_REJECTS, so my thinking
>>> was - use the format I like for the new code, but leave the existing
>>> rejects for a separate series / discussion.
>>>
>>> If we remove the rejects we still need something like
>>>
>>> extern struct nla_policy policy[lastattr + 1];
>>
>> Not sure I understand? You're using strict validation (I think), so
>> attrs that are out of range will be rejected same as NLA_REJECT (well,
>> with a different message) in __nla_validate_parse():
>>
>> nla_for_each_attr(nla, head, len, rem) {
>> u16 type = nla_type(nla);
>>
>> if (type == 0 || type > maxtype) {
>> if (validate & NL_VALIDATE_MAXTYPE) {
>> NL_SET_ERR_MSG_ATTR(extack, nla,
>> "Unknown attribute type");
>> return -EINVAL;
>> }
>>
>>
>> In fact, if you're using strict validation even the default
>> (0==NLA_UNSPEC) will be rejected, just like NLA_REJECT.
>>
>>
>> Or am I confused somewhere?
>
> Yea, I think we're both confused. Agreed with the above.
>
> Are you suggesting:
>
> const struct nla_policy policy[/* no size */] = {
> [HEADER] = NLA_POLICY(...)
> [OTHER_ATTR] = NLA_POLICY(...)
> };
>
> extern const struct nla_policy policy[/* no size */];
>
> op = {
> .policy = policy,
> .max_attr = OTHER_ATTR,
> }
>
Why can't .max_attr here just be derived from ARRAY_SIZE? In this
example, ARRAY_SIZE should return 2, so max_attr would be ARRAY_SIZE - 1.
Well, I guess HEADER/OTHER_ATTR could make this a sparse array... in
which case it might not work?
> ?
>
> What I'm saying is that my preference would be:
>
> const struct nla_policy policy[OTHER_ATTR + 1] = {
> [HEADER] = NLA_POLICY(...)
> [OTHER_ATTR] = NLA_POLICY(...)
> };
>
> extern const struct nla_policy policy[OTHER_ATTR + 1];
>
> op = {
> .policy = policy,
> .max_attr = ARRAY_SIZE(policy) - 1,
> }
>
> Since it's harder to forget to update the op (you don't have to update
> op, and compiler will complain about the extern out of sync).
>
Ahh.. Ok so I guess what you're doing here is defining the array size as
OTHER_ATTR + 1, so that ARRAY_SIZE() - 1 guarantees to equal
OTHER_ATTR.. so as long as OTHER_ATTR is the largest element in the array...
But wouldn't that be the case in the previous example where array size
is automatically determined... as long as you keep the index ordering in
ascending order, then the size of the array would be 1 larger than the
last element...
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops
2020-10-05 19:33 ` Johannes Berg
2020-10-05 19:41 ` Jakub Kicinski
@ 2020-10-05 21:52 ` Jacob Keller
1 sibling, 0 replies; 34+ messages in thread
From: Jacob Keller @ 2020-10-05 21:52 UTC (permalink / raw)
To: Johannes Berg, Jakub Kicinski
Cc: davem, netdev, kernel-team, jiri, andrew, mkubecek
On 10/5/2020 12:33 PM, Johannes Berg wrote:
> On Mon, 2020-10-05 at 12:31 -0700, Jakub Kicinski wrote:
>
>> Yea, I think we're both confused. Agreed with the above.
>>
>> Are you suggesting:
>>
>> const struct nla_policy policy[/* no size */] = {
>> [HEADER] = NLA_POLICY(...)
>> [OTHER_ATTR] = NLA_POLICY(...)
>> };
>>
>> extern const struct nla_policy policy[/* no size */];
>>
>> op = {
>> .policy = policy,
>> .max_attr = OTHER_ATTR,
>> }
>
> No, that'd be awkward, for the reason you stated below.
>
>> What I'm saying is that my preference would be:
>>
>> const struct nla_policy policy[OTHER_ATTR + 1] = {
>> [HEADER] = NLA_POLICY(...)
>> [OTHER_ATTR] = NLA_POLICY(...)
>> };
>>
>> extern const struct nla_policy policy[OTHER_ATTR + 1];
>>
>> op = {
>> .policy = policy,
>> .max_attr = ARRAY_SIZE(policy) - 1,
>> }
>>
>> Since it's harder to forget to update the op (you don't have to update
>> op, and compiler will complain about the extern out of sync).
>
> Yeah.
>
> I was thinking the third way ;-)
>
> const struct nla_policy policy[] = {
> [HEADER] = NLA_POLICY(...)
> [OTHER_ATTR] = NLA_POLICY(...)
> };
>
> op = {
> .policy = policy,
> .maxattr = ARRAY_SIZE(policy) - 1,
> };
>
>
> Now you can freely add any attributes, and, due to strict validation,
> anything not specified in the policy will be rejected, whether by being
> out of range (> maxattr) or not specified (NLA_UNSPEC).
>
> johannes
>
This is what I was thinking of as well.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 20:12 ` Johannes Berg
@ 2020-10-05 22:21 ` Jakub Kicinski
2020-10-06 6:37 ` Johannes Berg
0 siblings, 1 reply; 34+ messages in thread
From: Jakub Kicinski @ 2020-10-05 22:21 UTC (permalink / raw)
To: Johannes Berg
Cc: Michal Kubecek, davem, netdev, kernel-team, jiri, andrew, dsahern, pablo
On Mon, 05 Oct 2020 22:12:25 +0200 Johannes Berg wrote:
> On Mon, 2020-10-05 at 21:53 +0200, Johannes Berg wrote:
> > Hm. I like that idea.
> >
> > If we have NLMSGERR_ATTR_OFFS we could accompany that with the sub-
> > policy for that particular attribute, something like
> >
> > [NLMSGERR_ATTR_POLICY] = nested {
> > [NL_POLICY_TYPE_ATTR_TYPE] = ...
> > [NL_POLICY_TYPE_ATTR_MASK] = ...
> > }
> >
> > which we could basically do by factoring out the inner portion of
> > netlink_policy_dump_write():
> >
> > attr = nla_nest_start(skb, state->attr_idx);
> > if (!attr)
> > goto nla_put_failure;
> > ...
> > nla_nest_end(skb, attr);
> >
> > from there into a separate function, give it the pt and the nested
> > attribute (what's "state->attr_idx" here) as arguments, and then we call
> > it with NLMSGERR_ATTR_POLICY from here, and with "state->attr_idx" from
> > netlink_policy_dump_write() :-)
> >
> > Nice, easy & useful, maybe I'll code it up tomorrow.
>
> OK I thought about it a bit more and looked at the code, and it's not
> actually possible to do easily right now, because we can't actually
> point to the bad attribute from the general lib/nlattr.c code ...
>
> Why? Because we don't know right now, e.g. for nla_validate(), where in
> the message we started validation, i.e. the offset of the "head" inside
> the particular message.
>
> For nlmsg_parse() and friends that's a bit easier, but it needs more
> rejiggering than I'm willing to do tonight ;)
I thought we'd record the const struct nla_policy *tp for the failing
attr in struct netlink_ext_ack and output based on that.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-05 22:21 ` Jakub Kicinski
@ 2020-10-06 6:37 ` Johannes Berg
2020-10-06 11:52 ` Johannes Berg
0 siblings, 1 reply; 34+ messages in thread
From: Johannes Berg @ 2020-10-06 6:37 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Michal Kubecek, davem, netdev, kernel-team, jiri, andrew, dsahern, pablo
On Mon, 2020-10-05 at 15:21 -0700, Jakub Kicinski wrote:
> > > Nice, easy & useful, maybe I'll code it up tomorrow.
> >
> > OK I thought about it a bit more and looked at the code, and it's not
> > actually possible to do easily right now, because we can't actually
> > point to the bad attribute from the general lib/nlattr.c code ...
> >
> > Why? Because we don't know right now, e.g. for nla_validate(), where in
> > the message we started validation, i.e. the offset of the "head" inside
> > the particular message.
> >
> > For nlmsg_parse() and friends that's a bit easier, but it needs more
> > rejiggering than I'm willing to do tonight ;)
>
> I thought we'd record the const struct nla_policy *tp for the failing
> attr in struct netlink_ext_ack and output based on that.
We could, but it's a bit useless if you know "which" attribute caused
the issue, but you don't know where it was in the message? That way you
wouldn't know the nesting level etc.
I mean, we actually have that problem today - the generic lib/nlattr.c
policy violation doesn't tell you where exactly the problem occurred, so
it'd be good to fix that regardless.
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 5/6] netlink: add mask validation
2020-10-06 6:37 ` Johannes Berg
@ 2020-10-06 11:52 ` Johannes Berg
0 siblings, 0 replies; 34+ messages in thread
From: Johannes Berg @ 2020-10-06 11:52 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Michal Kubecek, davem, netdev, kernel-team, jiri, andrew, dsahern, pablo
On Tue, 2020-10-06 at 08:37 +0200, Johannes Berg wrote:
> On Mon, 2020-10-05 at 15:21 -0700, Jakub Kicinski wrote:
>
> > > > Nice, easy & useful, maybe I'll code it up tomorrow.
> > >
> > > OK I thought about it a bit more and looked at the code, and it's not
> > > actually possible to do easily right now, because we can't actually
> > > point to the bad attribute from the general lib/nlattr.c code ...
> > >
> > > Why? Because we don't know right now, e.g. for nla_validate(), where in
> > > the message we started validation, i.e. the offset of the "head" inside
> > > the particular message.
> > >
> > > For nlmsg_parse() and friends that's a bit easier, but it needs more
> > > rejiggering than I'm willing to do tonight ;)
> >
> > I thought we'd record the const struct nla_policy *tp for the failing
> > attr in struct netlink_ext_ack and output based on that.
>
> We could, but it's a bit useless if you know "which" attribute caused
> the issue, but you don't know where it was in the message? That way you
> wouldn't know the nesting level etc.
>
> I mean, we actually have that problem today - the generic lib/nlattr.c
> policy violation doesn't tell you where exactly the problem occurred, so
> it'd be good to fix that regardless.
Just for the record: I'm obviously *completely* confused, of course we
do say which attribute was bad, I was just looking for the offset, but
we carry around a pointer and calculate the offset later,
with NL_SET_ERR_MSG_ATTR() or NL_SET_BAD_ATTR().
Which means this isn't so hard ...
johannes
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2020-10-06 11:52 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 15:57 [PATCH net-next 0/6] ethtool: allow dumping policies to user space Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 1/6] ethtool: wire up get policies to ops Jakub Kicinski
2020-10-05 18:56 ` Johannes Berg
2020-10-05 19:16 ` Jakub Kicinski
2020-10-05 19:21 ` Johannes Berg
2020-10-05 19:31 ` Jakub Kicinski
2020-10-05 19:33 ` Johannes Berg
2020-10-05 19:41 ` Jakub Kicinski
2020-10-05 19:46 ` Johannes Berg
2020-10-05 19:51 ` Jakub Kicinski
2020-10-05 21:52 ` Jacob Keller
2020-10-05 21:33 ` Jacob Keller
2020-10-05 15:57 ` [PATCH net-next 2/6] ethtool: use the attributes parsed by the core in get commands Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 3/6] ethtool: wire up set policies to ops Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 4/6] ethtool: link up ethnl_header_policy as a nested policy Jakub Kicinski
2020-10-05 15:57 ` [PATCH net-next 5/6] netlink: add mask validation Jakub Kicinski
2020-10-05 19:05 ` Johannes Berg
2020-10-05 19:22 ` Jakub Kicinski
2020-10-05 19:25 ` Johannes Berg
2020-10-05 19:34 ` Jakub Kicinski
2020-10-05 19:37 ` Johannes Berg
2020-10-05 19:47 ` Michal Kubecek
2020-10-05 19:28 ` Michal Kubecek
2020-10-05 19:31 ` Johannes Berg
2020-10-05 19:40 ` Jakub Kicinski
2020-10-05 19:53 ` Johannes Berg
2020-10-05 20:12 ` Johannes Berg
2020-10-05 22:21 ` Jakub Kicinski
2020-10-06 6:37 ` Johannes Berg
2020-10-06 11:52 ` Johannes Berg
2020-10-05 15:57 ` [PATCH net-next 6/6] ethtool: specify which header flags are supported per command Jakub Kicinski
2020-10-05 18:58 ` Johannes Berg
2020-10-05 19:25 ` Jakub Kicinski
2020-10-05 19:28 ` Johannes Berg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.