From 99132b6eb7927a549351f57638a1d560039f06f9 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Wed, 25 Jan 2023 15:05:18 -0800 Subject: [PATCH] ethtool: netlink: handle SET intro/outro in the common code Most ethtool SET callbacks follow the same general structure. ethnl_parse_header_dev_get() rtnl_lock() ethnl_ops_begin() ... do stuff ... ethtool_notify() ethnl_ops_complete() rtnl_unlock() ethnl_parse_header_dev_put() This leads to a lot of copy / pasted code an bugs when people mis-handle the error path. Add a generic implementation of this pattern with a .set callback in struct ethnl_request_ops called to "do stuff". Also add an optional .set_validate which is called before ethnl_ops_begin() -- a lot of implementations do basic request capability / sanity checking at that point. Because we want to avoid generating the notification when no change happened - adopt a slightly hairy return values: - 0 means nothing to do (no notification) - 1 means done / continue - negative error codes on error Reuse .hdr_attr from struct ethnl_request_ops, GET and SET use the same attr spaces in all cases. Convert pause as an example (and to avoid unused function warnings). Signed-off-by: Jakub Kicinski Signed-off-by: David S. Miller --- net/ethtool/netlink.c | 49 +++++++++++++++++++- net/ethtool/netlink.h | 22 ++++++++- net/ethtool/pause.c | 103 ++++++++++++++++++------------------------ 3 files changed, 112 insertions(+), 62 deletions(-) diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index 6412c4dc663d..e90ac1f0a1d7 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c @@ -279,6 +279,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = { [ETHTOOL_MSG_CHANNELS_GET] = ðnl_channels_request_ops, [ETHTOOL_MSG_COALESCE_GET] = ðnl_coalesce_request_ops, [ETHTOOL_MSG_PAUSE_GET] = ðnl_pause_request_ops, + [ETHTOOL_MSG_PAUSE_SET] = ðnl_pause_request_ops, [ETHTOOL_MSG_EEE_GET] = ðnl_eee_request_ops, [ETHTOOL_MSG_FEC_GET] = ðnl_fec_request_ops, [ETHTOOL_MSG_TSINFO_GET] = ðnl_tsinfo_request_ops, @@ -591,6 +592,52 @@ static int ethnl_default_done(struct netlink_callback *cb) return 0; } +static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info) +{ + const struct ethnl_request_ops *ops; + struct ethnl_req_info req_info = {}; + const u8 cmd = info->genlhdr->cmd; + int ret; + + ops = ethnl_default_requests[cmd]; + if (WARN_ONCE(!ops, "cmd %u has no ethnl_request_ops\n", cmd)) + return -EOPNOTSUPP; + if (GENL_REQ_ATTR_CHECK(info, ops->hdr_attr)) + return -EINVAL; + + ret = ethnl_parse_header_dev_get(&req_info, info->attrs[ops->hdr_attr], + genl_info_net(info), info->extack, + true); + if (ret < 0) + return ret; + + if (ops->set_validate) { + ret = ops->set_validate(&req_info, info); + /* 0 means nothing to do */ + if (ret <= 0) + goto out_dev; + } + + rtnl_lock(); + ret = ethnl_ops_begin(req_info.dev); + if (ret < 0) + goto out_rtnl; + + ret = ops->set(&req_info, info); + if (ret <= 0) + goto out_ops; + ethtool_notify(req_info.dev, ops->set_ntf_cmd, NULL); + + ret = 0; +out_ops: + ethnl_ops_complete(req_info.dev); +out_rtnl: + rtnl_unlock(); +out_dev: + ethnl_parse_header_dev_put(&req_info); + return ret; +} + static const struct ethnl_request_ops * ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = { [ETHTOOL_MSG_LINKINFO_NTF] = ðnl_linkinfo_request_ops, @@ -921,7 +968,7 @@ static const struct genl_ops ethtool_genl_ops[] = { { .cmd = ETHTOOL_MSG_PAUSE_SET, .flags = GENL_UNS_ADMIN_PERM, - .doit = ethnl_set_pause, + .doit = ethnl_default_set_doit, .policy = ethnl_pause_set_policy, .maxattr = ARRAY_SIZE(ethnl_pause_set_policy) - 1, }, diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index b01f7cd542c4..aca4bdb637a6 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -284,13 +284,14 @@ int ethnl_ops_begin(struct net_device *dev); void ethnl_ops_complete(struct net_device *dev); /** - * struct ethnl_request_ops - unified handling of GET requests + * struct ethnl_request_ops - unified handling of GET and SET requests * @request_cmd: command id for request (GET) * @reply_cmd: command id for reply (GET_REPLY) * @hdr_attr: attribute type for request header * @req_info_size: size of request info * @reply_data_size: size of reply data * @allow_nodev_do: allow non-dump request with no device identification + * @set_ntf_cmd: notification to generate on changes (SET) * @parse_request: * Parse request except common header (struct ethnl_req_info). Common * header is already filled on entry, the rest up to @repdata_offset @@ -319,6 +320,18 @@ void ethnl_ops_complete(struct net_device *dev); * used e.g. to free any additional data structures outside the main * structure which were allocated by ->prepare_data(). When processing * dump requests, ->cleanup() is called for each message. + * @set_validate: + * Check if set operation is supported for a given device, and perform + * extra input checks. Expected return values: + * - 0 if the operation is a noop for the device (rare) + * - 1 if operation should proceed to calling @set + * - negative errno on errors + * Called without any locks, just a reference on the netdev. + * @set: + * Execute the set operation. The implementation should return + * - 0 if no configuration has changed + * - 1 if configuration changed and notification should be generated + * - negative errno on errors * * Description of variable parts of GET request handling when using the * unified infrastructure. When used, a pointer to an instance of this @@ -335,6 +348,7 @@ struct ethnl_request_ops { unsigned int req_info_size; unsigned int reply_data_size; bool allow_nodev_do; + u8 set_ntf_cmd; int (*parse_request)(struct ethnl_req_info *req_info, struct nlattr **tb, @@ -348,6 +362,11 @@ struct ethnl_request_ops { const struct ethnl_req_info *req_info, const struct ethnl_reply_data *reply_data); void (*cleanup_data)(struct ethnl_reply_data *reply_data); + + int (*set_validate)(struct ethnl_req_info *req_info, + struct genl_info *info); + int (*set)(struct ethnl_req_info *req_info, + struct genl_info *info); }; /* request handlers */ @@ -432,7 +451,6 @@ int ethnl_set_privflags(struct sk_buff *skb, struct genl_info *info); int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info); int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info); int ethnl_set_coalesce(struct sk_buff *skb, struct genl_info *info); -int ethnl_set_pause(struct sk_buff *skb, struct genl_info *info); int ethnl_set_eee(struct sk_buff *skb, struct genl_info *info); int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info); int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info); diff --git a/net/ethtool/pause.c b/net/ethtool/pause.c index dcd34b9a849f..6657d0b888d8 100644 --- a/net/ethtool/pause.c +++ b/net/ethtool/pause.c @@ -161,6 +161,46 @@ static int pause_fill_reply(struct sk_buff *skb, return 0; } +/* PAUSE_SET */ + +const struct nla_policy ethnl_pause_set_policy[] = { + [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 }, +}; + +static int +ethnl_set_pause_validate(struct ethnl_req_info *req_info, + struct genl_info *info) +{ + const struct ethtool_ops *ops = req_info->dev->ethtool_ops; + + return ops->get_pauseparam && ops->set_pauseparam ? 1 : -EOPNOTSUPP; +} + +static int +ethnl_set_pause(struct ethnl_req_info *req_info, struct genl_info *info) +{ + struct net_device *dev = req_info->dev; + struct ethtool_pauseparam params = {}; + struct nlattr **tb = info->attrs; + bool mod = false; + int ret; + + dev->ethtool_ops->get_pauseparam(dev, ¶ms); + + ethnl_update_bool32(¶ms.autoneg, tb[ETHTOOL_A_PAUSE_AUTONEG], &mod); + ethnl_update_bool32(¶ms.rx_pause, tb[ETHTOOL_A_PAUSE_RX], &mod); + ethnl_update_bool32(¶ms.tx_pause, tb[ETHTOOL_A_PAUSE_TX], &mod); + if (!mod) + return 0; + + ret = dev->ethtool_ops->set_pauseparam(dev, ¶ms); + return ret < 0 ? ret : 1; +} + const struct ethnl_request_ops ethnl_pause_request_ops = { .request_cmd = ETHTOOL_MSG_PAUSE_GET, .reply_cmd = ETHTOOL_MSG_PAUSE_GET_REPLY, @@ -172,63 +212,8 @@ const struct ethnl_request_ops ethnl_pause_request_ops = { .prepare_data = pause_prepare_data, .reply_size = pause_reply_size, .fill_reply = pause_fill_reply, + + .set_validate = ethnl_set_pause_validate, + .set = ethnl_set_pause, + .set_ntf_cmd = ETHTOOL_MSG_PAUSE_NTF, }; - -/* PAUSE_SET */ - -const struct nla_policy ethnl_pause_set_policy[] = { - [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 }, -}; - -int ethnl_set_pause(struct sk_buff *skb, struct genl_info *info) -{ - 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 = ethnl_parse_header_dev_get(&req_info, - tb[ETHTOOL_A_PAUSE_HEADER], - genl_info_net(info), info->extack, - true); - if (ret < 0) - return ret; - dev = req_info.dev; - ops = dev->ethtool_ops; - ret = -EOPNOTSUPP; - if (!ops->get_pauseparam || !ops->set_pauseparam) - goto out_dev; - - rtnl_lock(); - ret = ethnl_ops_begin(dev); - if (ret < 0) - goto out_rtnl; - ops->get_pauseparam(dev, ¶ms); - - ethnl_update_bool32(¶ms.autoneg, tb[ETHTOOL_A_PAUSE_AUTONEG], &mod); - ethnl_update_bool32(¶ms.rx_pause, tb[ETHTOOL_A_PAUSE_RX], &mod); - ethnl_update_bool32(¶ms.tx_pause, tb[ETHTOOL_A_PAUSE_TX], &mod); - ret = 0; - if (!mod) - goto out_ops; - - ret = dev->ethtool_ops->set_pauseparam(dev, ¶ms); - if (ret < 0) - goto out_ops; - ethtool_notify(dev, ETHTOOL_MSG_PAUSE_NTF, NULL); - -out_ops: - ethnl_ops_complete(dev); -out_rtnl: - rtnl_unlock(); -out_dev: - ethnl_parse_header_dev_put(&req_info); - return ret; -}