| Message ID | 043188fe4776b9420a11116af5e6bad8925b9ee7.1553532199.git.mkubecek@suse.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | ethtool netlink interface, part 1 | expand |
Mon, Mar 25, 2019 at 06:08:12PM CET, mkubecek@suse.cz wrote: >Various helpers used by ethtool netlink code. > >Signed-off-by: Michal Kubecek <mkubecek@suse.cz> >--- > include/uapi/linux/ethtool_netlink.h | 11 ++ > net/ethtool/netlink.c | 144 +++++++++++++++++++++++++++ > net/ethtool/netlink.h | 144 +++++++++++++++++++++++++++ > 3 files changed, 299 insertions(+) > >diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h >index 6aa267451542..59240a2cda56 100644 >--- a/include/uapi/linux/ethtool_netlink.h >+++ b/include/uapi/linux/ethtool_netlink.h >@@ -18,6 +18,17 @@ enum { > ETHNL_CMD_MAX = (__ETHNL_CMD_CNT - 1) > }; > >+/* device specification */ >+ >+enum { >+ ETHA_DEV_UNSPEC, ETHNL/ETHA looks odd. I would prefer prefix "ETHTOOL_* for everything in uapi: ETHTOOL_CMD_* for commands. ETHTOOL_ATTR_* for attributes. Looks much nicer. >+ ETHA_DEV_INDEX, /* u32 */ >+ ETHA_DEV_NAME, /* string */ >+ >+ __ETHA_DEV_CNT, >+ ETHA_DEV_MAX = (__ETHA_DEV_CNT - 1) >+}; >+ > /* generic netlink info */ > #define ETHTOOL_GENL_NAME "ethtool" > #define ETHTOOL_GENL_VERSION 1 >diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c >index 85dd6dac71a2..cc6829ba4331 100644 >--- a/net/ethtool/netlink.c >+++ b/net/ethtool/netlink.c >@@ -1,8 +1,152 @@ > // SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note > >+#include <net/sock.h> > #include <linux/ethtool_netlink.h> > #include "netlink.h" > >+static const struct nla_policy dev_policy[ETHA_DEV_MAX + 1] = { ethnl_dev_policy. Please maintain the namespace if possible. "dev_policy" looks way too generic if anyone sees it in the code. >+ [ETHA_DEV_UNSPEC] = { .type = NLA_REJECT }, >+ [ETHA_DEV_INDEX] = { .type = NLA_U32 }, >+ [ETHA_DEV_NAME] = { .type = NLA_NUL_STRING, >+ .len = IFNAMSIZ - 1 }, >+}; >+ >+/** >+ * ethnl_dev_get() - get device identified by nested attribute >+ * @info: genetlink info (also used for extack error reporting) >+ * @nest: nest attribute with device identification >+ * >+ * Finds the network device identified by ETHA_DEV_INDEX (ifindex) or >+ * ETHA_DEV_NAME (name) attributes in a nested attribute @nest. If both >+ * are supplied, they must identify the same device. If successful, takes >+ * a reference to the device which is to be released by caller. >+ * >+ * Return: pointer to the device if successful, ERR_PTR(err) on error >+ */ >+struct net_device *ethnl_dev_get(struct genl_info *info, struct nlattr *nest) I wonder, why you have this in nested attr? Aren't ifindex/ifname always used as a handle for commands/notifications? Would be just in toplevel (see devlink/nl80211) >+{ >+ struct net *net = genl_info_net(info); >+ struct nlattr *tb[ETHA_DEV_MAX + 1]; >+ struct net_device *dev; >+ int ret; >+ >+ if (!nest) { >+ ETHNL_SET_ERRMSG(info, >+ "mandatory device identification missing"); No need to wrap. >+ return ERR_PTR(-EINVAL); >+ } >+ ret = nla_parse_nested_strict(tb, ETHA_DEV_MAX, nest, dev_policy, >+ info->extack); >+ if (ret < 0) "if (ret)" is enough. "Returns 0 on success or a negative error code." >+ return ERR_PTR(ret); >+ >+ if (tb[ETHA_DEV_INDEX]) { >+ dev = dev_get_by_index(net, nla_get_u32(tb[ETHA_DEV_INDEX])); >+ if (!dev) >+ return ERR_PTR(-ENODEV); >+ /* if both ifindex and ifname are passed, they must match */ >+ if (tb[ETHA_DEV_NAME]) { >+ const char *nl_name = nla_data(tb[ETHA_DEV_NAME]); >+ >+ if (strncmp(dev->name, nl_name, IFNAMSIZ)) { >+ dev_put(dev); >+ ETHNL_SET_ERRMSG(info, >+ "ifindex and ifname do not match"); No need to wrap. >+ return ERR_PTR(-ENODEV); >+ } >+ } >+ return dev; >+ } else if (tb[ETHA_DEV_NAME]) { >+ dev = dev_get_by_name(net, nla_data(tb[ETHA_DEV_NAME])); >+ if (!dev) >+ return ERR_PTR(-ENODEV); >+ } else { >+ ETHNL_SET_ERRMSG(info, "either ifindex or ifname required"); >+ return ERR_PTR(-EINVAL); >+ } >+ >+ if (!netif_device_present(dev)) { >+ dev_put(dev); >+ ETHNL_SET_ERRMSG(info, "device not present"); >+ return ERR_PTR(-ENODEV); >+ } >+ return dev; >+} >+ >+/** >+ * ethnl_fill_dev() - Put device identification nest into a message >+ * @msg: skb with the message >+ * @dev: network device to describe >+ * @attrtype: attribute type to use for the nest >+ * >+ * Create a nested attribute with attributes describing given network device. >+ * Clean up on error. >+ * >+ * Return: 0 on success, error value (-EMSGSIZE only) on error >+ */ >+int ethnl_fill_dev(struct sk_buff *msg, struct net_device *dev, u16 attrtype) >+{ >+ struct nlattr *nest; >+ int ret = -EMSGSIZE; >+ >+ nest = ethnl_nest_start(msg, attrtype); >+ if (!nest) >+ return -EMSGSIZE; >+ >+ if (nla_put_u32(msg, ETHA_DEV_INDEX, (u32)dev->ifindex)) >+ goto err; >+ if (nla_put_string(msg, ETHA_DEV_NAME, dev->name)) >+ goto err; >+ >+ nla_nest_end(msg, nest); >+ return 0; >+err: Usually this label is called "nla_put_failure". Same for the rest. "err" could be confused with error return variable. have "ret = -EMSGSIZE" here. Easier to follow the error path. >+ nla_nest_cancel(msg, nest); >+ return ret; >+} >+ >+/** >+ * ethnl_reply_init() - Create skb for a reply and fill device identification >+ * @payload: payload length (without netlink and genetlink header) >+ * @dev: device the reply is about (may be null) >+ * @cmd: ETHNL_CMD_* command for reply >+ * @info: genetlink info of the received packet we respond to >+ * @ehdrp: place to store payload pointer returned by genlmsg_new() >+ * >+ * Return: pointer to allocated skb on success, NULL on error >+ */ >+struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd, >+ u16 dev_attrtype, struct genl_info *info, >+ void **ehdrp) >+{ >+ struct sk_buff *rskb; Could be just "skb", or "msg". You have "msg" in ethnl_fill_dev(). Please have it consistent. >+ void *ehdr; >+ >+ rskb = genlmsg_new(payload, GFP_KERNEL); >+ if (!rskb) { >+ ETHNL_SET_ERRMSG(info, >+ "failed to allocate reply message"); No need to wrap. >+ return NULL; >+ } >+ >+ ehdr = genlmsg_put_reply(rskb, info, ðtool_genl_family, 0, cmd); >+ if (!ehdr) >+ goto err; >+ if (ehdrp) >+ *ehdrp = ehdr; >+ if (dev) { >+ int ret = ethnl_fill_dev(rskb, dev, dev_attrtype); >+ >+ if (ret < 0) "if (ret)" is enough. >+ goto err; >+ } >+ >+ return rskb; >+err: >+ nlmsg_free(rskb); >+ return NULL; >+} >+ > /* genetlink setup */ > > static const struct genl_ops ethtool_genl_ops[] = { >diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h >index 63063b582ca2..db90d95410b1 100644 >--- a/net/ethtool/netlink.h >+++ b/net/ethtool/netlink.h >@@ -6,7 +6,151 @@ > #include <linux/ethtool_netlink.h> > #include <linux/netdevice.h> > #include <net/genetlink.h> >+#include <net/sock.h> >+ >+#define ETHNL_SET_ERRMSG(info, msg) \ >+ do { if (info) GENL_SET_ERR_MSG(info, msg); } while (0) Why do you need this macro? Can info be null? In general, macros like this should be avoided. > > extern struct genl_family ethtool_genl_family; > >+struct net_device *ethnl_dev_get(struct genl_info *info, struct nlattr *nest); >+int ethnl_fill_dev(struct sk_buff *msg, struct net_device *dev, u16 attrtype); >+ >+struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd, >+ u16 dev_attrtype, struct genl_info *info, >+ void **ehdrp); >+ >+static inline int ethnl_str_size(const char *s) >+{ >+ return nla_total_size(strlen(s) + 1); This looks like more generic helper, not tight to ethtool. >+} >+ >+static inline int ethnl_str_ifne_size(const char *s) Eh? "ifne"? What is this good for? >+{ >+ return s[0] ? ethnl_str_size(s) : 0; >+} >+ >+static inline int ethnl_put_str_ifne(struct sk_buff *skb, int attrtype, >+ const char *s) >+{ >+ if (!s[0]) >+ return 0; >+ return nla_put_string(skb, attrtype, s); I don't like helpers like this. Do the check in the caller and put or not put the string there. It's a single if. >+} >+ >+static inline struct nlattr *ethnl_nest_start(struct sk_buff *skb, >+ int attrtype) >+{ >+ return nla_nest_start(skb, attrtype | NLA_F_NESTED); Please use nla_nest_start directly and avoid helpers like this. >+} >+ >+static inline int ethnlmsg_parse(const struct nlmsghdr *nlh, >+ struct nlattr *tb[], int maxtype, >+ const struct nla_policy *policy, >+ struct genl_info *info) >+{ >+ return nlmsg_parse_strict(nlh, GENL_HDRLEN, tb, maxtype, policy, >+ info ? info->extack : NULL); Same thing, please use nlmsg_parse_strict directly. >+} >+ >+/* ethnl_update_* return true if the value is changed */ >+static inline bool ethnl_update_u32(u32 *dst, struct nlattr *attr) >+{ >+ u32 val; >+ >+ if (!attr) >+ return false; >+ val = nla_get_u32(attr); >+ if (*dst == val) >+ return false; >+ >+ *dst = val; >+ return true; >+} >+ >+static inline bool ethnl_update_u8(u8 *dst, struct nlattr *attr) >+{ >+ u8 val; >+ >+ if (!attr) >+ return false; >+ val = nla_get_u8(attr); >+ if (*dst == val) >+ return false; >+ >+ *dst = val; >+ return true; >+} >+ >+/* update u32 value used as bool from NLA_U8 */ >+static inline bool ethnl_update_bool32(u32 *dst, struct nlattr *attr) >+{ >+ u8 val; >+ >+ if (!attr) >+ return false; >+ val = !!nla_get_u8(attr); >+ if (!!*dst == val) >+ return false; >+ >+ *dst = val; >+ return true; >+} >+ >+static inline bool ethnl_update_binary(u8 *dst, unsigned int len, >+ struct nlattr *attr) >+{ >+ if (!attr) >+ return false; >+ if (nla_len(attr) < len) >+ len = nla_len(attr); >+ if (!memcmp(dst, nla_data(attr), len)) >+ return false; >+ >+ memcpy(dst, nla_data(attr), len); >+ return true; >+} >+ >+static inline bool ethnl_update_bitfield32(u32 *dst, struct nlattr *attr) >+{ >+ struct nla_bitfield32 change; >+ u32 newval; >+ >+ if (!attr) >+ return false; >+ change = nla_get_bitfield32(attr); >+ newval = (*dst & ~change.selector) | (change.value & change.selector); >+ if (*dst == newval) >+ return false; >+ >+ *dst = newval; >+ return true; >+} I don't understand puspose of these "update" helper functions. Try to avoid them. In general, please try to avoid wrappers around netlink api. >+ >+static inline void warn_partial_info(struct genl_info *info) >+{ >+ ETHNL_SET_ERRMSG(info, "not all requested data could be retrieved"); >+} >+ >+/* Check user privileges explicitly to allow finer access control based on >+ * context of the request or hiding part of the information from unprivileged >+ * users >+ */ >+static inline bool ethnl_is_privileged(struct sk_buff *skb) >+{ >+ struct net *net = sock_net(skb->sk); >+ >+ return netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN); Same here. >+} >+ >+/* total size of ETHA_*_DEV nested attribute; this is an upper estimate so that >+ * we do not need to hold RTNL longer than necessary to prevent rename between >+ * estimating the size and composing the message >+ */ >+static inline unsigned int dev_ident_size(void) You are missing "ethnl_" prefix. >+{ >+ return nla_total_size(nla_total_size(sizeof(u32)) + >+ nla_total_size(IFNAMSIZ)); >+} >+ > #endif /* _NET_ETHTOOL_NETLINK_H */ >-- >2.21.0 >
On Tue, Mar 26, 2019 at 01:38:15PM +0100, Jiri Pirko wrote: > Mon, Mar 25, 2019 at 06:08:12PM CET, mkubecek@suse.cz wrote: > >+/* device specification */ > >+ > >+enum { > >+ ETHA_DEV_UNSPEC, > > ETHNL/ETHA looks odd. > I would prefer prefix "ETHTOOL_* for everything in uapi: > ETHTOOL_CMD_* for commands. > ETHTOOL_ATTR_* for attributes. > > Looks much nicer. Even with ETHA_ prefix, there are places where having to fit into 80 columns makes the code hard to read. ETHTOOL_ATTR_ would make it even harder. > >+static const struct nla_policy dev_policy[ETHA_DEV_MAX + 1] = { > > ethnl_dev_policy. Please maintain the namespace if possible. > "dev_policy" looks way too generic if anyone sees it in the code. OK > >+ [ETHA_DEV_UNSPEC] = { .type = NLA_REJECT }, > >+ [ETHA_DEV_INDEX] = { .type = NLA_U32 }, > >+ [ETHA_DEV_NAME] = { .type = NLA_NUL_STRING, > >+ .len = IFNAMSIZ - 1 }, > >+}; > >+ > >+/** > >+ * ethnl_dev_get() - get device identified by nested attribute > >+ * @info: genetlink info (also used for extack error reporting) > >+ * @nest: nest attribute with device identification > >+ * > >+ * Finds the network device identified by ETHA_DEV_INDEX (ifindex) or > >+ * ETHA_DEV_NAME (name) attributes in a nested attribute @nest. If both > >+ * are supplied, they must identify the same device. If successful, takes > >+ * a reference to the device which is to be released by caller. > >+ * > >+ * Return: pointer to the device if successful, ERR_PTR(err) on error > >+ */ > >+struct net_device *ethnl_dev_get(struct genl_info *info, struct nlattr *nest) > > I wonder, why you have this in nested attr? Aren't ifindex/ifname always > used as a handle for commands/notifications? Would be just in toplevel > (see devlink/nl80211) It makes things only slightly easier now but I would like to make the API as future proof as possible. If something needs to be added later (e.g. netns id) or something else changes (e.g. 64-bit ifindex), only one enum and these two helpers would need to be adjusted. > >+ if (!nest) { > >+ ETHNL_SET_ERRMSG(info, > >+ "mandatory device identification missing"); > > No need to wrap. OK > >+ ret = nla_parse_nested_strict(tb, ETHA_DEV_MAX, nest, dev_policy, > >+ info->extack); > >+ if (ret < 0) > > "if (ret)" is enough. "Returns 0 on success or a negative error code." OK > >+ if (strncmp(dev->name, nl_name, IFNAMSIZ)) { > >+ dev_put(dev); > >+ ETHNL_SET_ERRMSG(info, > >+ "ifindex and ifname do not match"); > > No need to wrap. OK > >+/** > >+ * ethnl_fill_dev() - Put device identification nest into a message > >+ * @msg: skb with the message > >+ * @dev: network device to describe > >+ * @attrtype: attribute type to use for the nest > >+ * > >+ * Create a nested attribute with attributes describing given network device. > >+ * Clean up on error. > >+ * > >+ * Return: 0 on success, error value (-EMSGSIZE only) on error > >+ */ > >+int ethnl_fill_dev(struct sk_buff *msg, struct net_device *dev, u16 attrtype) > >+{ > >+ struct nlattr *nest; > >+ int ret = -EMSGSIZE; > >+ > >+ nest = ethnl_nest_start(msg, attrtype); > >+ if (!nest) > >+ return -EMSGSIZE; > >+ > >+ if (nla_put_u32(msg, ETHA_DEV_INDEX, (u32)dev->ifindex)) > >+ goto err; > >+ if (nla_put_string(msg, ETHA_DEV_NAME, dev->name)) > >+ goto err; > >+ > >+ nla_nest_end(msg, nest); > >+ return 0; > >+err: > Usually this label is called "nla_put_failure". Same for the rest. > "err" could be confused with error return variable. > > have "ret = -EMSGSIZE" here. Easier to follow the error path. OK > >+ nla_nest_cancel(msg, nest); > >+ return ret; > >+} > >+ > >+/** > >+ * ethnl_reply_init() - Create skb for a reply and fill device identification > >+ * @payload: payload length (without netlink and genetlink header) > >+ * @dev: device the reply is about (may be null) > >+ * @cmd: ETHNL_CMD_* command for reply > >+ * @info: genetlink info of the received packet we respond to > >+ * @ehdrp: place to store payload pointer returned by genlmsg_new() > >+ * > >+ * Return: pointer to allocated skb on success, NULL on error > >+ */ > >+struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd, > >+ u16 dev_attrtype, struct genl_info *info, > >+ void **ehdrp) > >+{ > >+ struct sk_buff *rskb; > > Could be just "skb", or "msg". You have "msg" in ethnl_fill_dev(). > Please have it consistent. OK > >+ rskb = genlmsg_new(payload, GFP_KERNEL); > >+ if (!rskb) { > >+ ETHNL_SET_ERRMSG(info, > >+ "failed to allocate reply message"); > > No need to wrap. OK > >+ int ret = ethnl_fill_dev(rskb, dev, dev_attrtype); > >+ > >+ if (ret < 0) > > "if (ret)" is enough. OK > >+ > >+#define ETHNL_SET_ERRMSG(info, msg) \ > >+ do { if (info) GENL_SET_ERR_MSG(info, msg); } while (0) > > Why do you need this macro? Can info be null? The same functions are used for generating replies to GET requests (info is not null) and notifications (info is null, no extack). > In general, macros like this should be avoided. At the expense of repeating the same patterns. In this case it would be acceptable but it would mean one more indentation level for lines with the error/warning messages. > >+static inline int ethnl_str_size(const char *s) > >+{ > >+ return nla_total_size(strlen(s) + 1); > > This looks like more generic helper, not tight to ethtool. OK > >+static inline int ethnl_str_ifne_size(const char *s) > > Eh? "ifne"? What is this good for? It's for "if not empty". Structures like ethtool_drvinfo use fixed size char arrays for strings where empty string means the information is not available. So if the string is empty, netlink attribute is omitted. > >+{ > >+ return s[0] ? ethnl_str_size(s) : 0; > >+} > >+ > >+static inline int ethnl_put_str_ifne(struct sk_buff *skb, int attrtype, > >+ const char *s) > >+{ > >+ if (!s[0]) > >+ return 0; > >+ return nla_put_string(skb, attrtype, s); > > I don't like helpers like this. Do the check in the caller and put or > not put the string there. It's a single if. OK > >+static inline struct nlattr *ethnl_nest_start(struct sk_buff *skb, > >+ int attrtype) > >+{ > >+ return nla_nest_start(skb, attrtype | NLA_F_NESTED); > > Please use nla_nest_start directly and avoid helpers like this. OK. It's one more thing to check (and forget) in many places, though. > >+static inline int ethnlmsg_parse(const struct nlmsghdr *nlh, > >+ struct nlattr *tb[], int maxtype, > >+ const struct nla_policy *policy, > >+ struct genl_info *info) > >+{ > >+ return nlmsg_parse_strict(nlh, GENL_HDRLEN, tb, maxtype, policy, > >+ info ? info->extack : NULL); > > Same thing, please use nlmsg_parse_strict directly. OK > >+/* ethnl_update_* return true if the value is changed */ > >+static inline bool ethnl_update_u32(u32 *dst, struct nlattr *attr) > >+{ > >+ u32 val; > >+ > >+ if (!attr) > >+ return false; > >+ val = nla_get_u32(attr); > >+ if (*dst == val) > >+ return false; > >+ > >+ *dst = val; > >+ return true; > >+} > >+ > >+static inline bool ethnl_update_u8(u8 *dst, struct nlattr *attr) > >+{ > >+ u8 val; > >+ > >+ if (!attr) > >+ return false; > >+ val = nla_get_u8(attr); > >+ if (*dst == val) > >+ return false; > >+ > >+ *dst = val; > >+ return true; > >+} > >+ > >+/* update u32 value used as bool from NLA_U8 */ > >+static inline bool ethnl_update_bool32(u32 *dst, struct nlattr *attr) > >+{ > >+ u8 val; > >+ > >+ if (!attr) > >+ return false; > >+ val = !!nla_get_u8(attr); > >+ if (!!*dst == val) > >+ return false; > >+ > >+ *dst = val; > >+ return true; > >+} > >+ > >+static inline bool ethnl_update_binary(u8 *dst, unsigned int len, > >+ struct nlattr *attr) > >+{ > >+ if (!attr) > >+ return false; > >+ if (nla_len(attr) < len) > >+ len = nla_len(attr); > >+ if (!memcmp(dst, nla_data(attr), len)) > >+ return false; > >+ > >+ memcpy(dst, nla_data(attr), len); > >+ return true; > >+} > >+ > >+static inline bool ethnl_update_bitfield32(u32 *dst, struct nlattr *attr) > >+{ > >+ struct nla_bitfield32 change; > >+ u32 newval; > >+ > >+ if (!attr) > >+ return false; > >+ change = nla_get_bitfield32(attr); > >+ newval = (*dst & ~change.selector) | (change.value & change.selector); > >+ if (*dst == newval) > >+ return false; > >+ > >+ *dst = newval; > >+ return true; > >+} > > I don't understand puspose of these "update" helper functions. Try to > avoid them. In general, please try to avoid wrappers around netlink api. The purpose is to update a value according to a request attribute (if present) and tell caller if the value was actually modified. They are used like e.g. mod = false; if (ethnl_update_u32(&data.rx_pending, tb[ETHA_RING_RX_PENDING])) mod = true; if (ethnl_update_u32(&data.rx_mini_pending, tb[ETHA_RING_RX_MINI_PENDING])) mod = true; if (ethnl_update_u32(&data.rx_jumbo_pending, tb[ETHA_RING_RX_JUMBO_PENDING])) mod = true; if (ethnl_update_u32(&data.tx_pending, tb[ETHA_RING_TX_PENDING])) mod = true; if (!mod) return 0; /* call ethtool_ops handler and send notification */ so that we can omit calling the handler and sending a notification if the request was no-op. Expanding the helpers here would make the code needlessly complicated and error prone. > >+static inline void warn_partial_info(struct genl_info *info) > >+{ > >+ ETHNL_SET_ERRMSG(info, "not all requested data could be retrieved"); > >+} > >+ > >+/* Check user privileges explicitly to allow finer access control based on > >+ * context of the request or hiding part of the information from unprivileged > >+ * users > >+ */ > >+static inline bool ethnl_is_privileged(struct sk_buff *skb) > >+{ > >+ struct net *net = sock_net(skb->sk); > >+ > >+ return netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN); > > Same here. It's still the same, helpers allow replacing repeating code patterns with just saying what you are doing. But this one is called only from one place so I can live without it. > >+} > >+ > >+/* total size of ETHA_*_DEV nested attribute; this is an upper estimate so that > >+ * we do not need to hold RTNL longer than necessary to prevent rename between > >+ * estimating the size and composing the message > >+ */ > >+static inline unsigned int dev_ident_size(void) > > You are missing "ethnl_" prefix. OK Michal
Tue, Mar 26, 2019 at 03:19:55PM CET, mkubecek@suse.cz wrote: >On Tue, Mar 26, 2019 at 01:38:15PM +0100, Jiri Pirko wrote: >> Mon, Mar 25, 2019 at 06:08:12PM CET, mkubecek@suse.cz wrote: >> >+/* device specification */ >> >+ >> >+enum { >> >+ ETHA_DEV_UNSPEC, >> >> ETHNL/ETHA looks odd. >> I would prefer prefix "ETHTOOL_* for everything in uapi: >> ETHTOOL_CMD_* for commands. >> ETHTOOL_ATTR_* for attributes. >> >> Looks much nicer. > >Even with ETHA_ prefix, there are places where having to fit into >80 columns makes the code hard to read. ETHTOOL_ATTR_ would make it >even harder. So cut the line. UAPI consistency is more important than to fit on a 80cols line. ETH/ETHA is wrong prefix. This is "ETHTOOL". I can live with ETHTOOL_A_* for attributes. > >> >+static const struct nla_policy dev_policy[ETHA_DEV_MAX + 1] = { >> >> ethnl_dev_policy. Please maintain the namespace if possible. >> "dev_policy" looks way too generic if anyone sees it in the code. > >OK >> >+ [ETHA_DEV_UNSPEC] = { .type = NLA_REJECT }, >> >+ [ETHA_DEV_INDEX] = { .type = NLA_U32 }, >> >+ [ETHA_DEV_NAME] = { .type = NLA_NUL_STRING, >> >+ .len = IFNAMSIZ - 1 }, >> >+}; >> >+ >> >+/** >> >+ * ethnl_dev_get() - get device identified by nested attribute >> >+ * @info: genetlink info (also used for extack error reporting) >> >+ * @nest: nest attribute with device identification >> >+ * >> >+ * Finds the network device identified by ETHA_DEV_INDEX (ifindex) or >> >+ * ETHA_DEV_NAME (name) attributes in a nested attribute @nest. If both >> >+ * are supplied, they must identify the same device. If successful, takes >> >+ * a reference to the device which is to be released by caller. >> >+ * >> >+ * Return: pointer to the device if successful, ERR_PTR(err) on error >> >+ */ >> >+struct net_device *ethnl_dev_get(struct genl_info *info, struct nlattr *nest) >> >> I wonder, why you have this in nested attr? Aren't ifindex/ifname always >> used as a handle for commands/notifications? Would be just in toplevel >> (see devlink/nl80211) > >It makes things only slightly easier now but I would like to make the >API as future proof as possible. If something needs to be added later >(e.g. netns id) or something else changes (e.g. 64-bit ifindex), only >one enum and these two helpers would need to be adjusted. You can still have the helper without the nest. So if you don't nest this, everything is the same, both helper and extra attr in enum. > >> >+ if (!nest) { >> >+ ETHNL_SET_ERRMSG(info, >> >+ "mandatory device identification missing"); >> >> No need to wrap. > >OK > >> >+ ret = nla_parse_nested_strict(tb, ETHA_DEV_MAX, nest, dev_policy, >> >+ info->extack); >> >+ if (ret < 0) >> >> "if (ret)" is enough. "Returns 0 on success or a negative error code." > >OK > >> >+ if (strncmp(dev->name, nl_name, IFNAMSIZ)) { >> >+ dev_put(dev); >> >+ ETHNL_SET_ERRMSG(info, >> >+ "ifindex and ifname do not match"); >> >> No need to wrap. > >OK > >> >+/** >> >+ * ethnl_fill_dev() - Put device identification nest into a message >> >+ * @msg: skb with the message >> >+ * @dev: network device to describe >> >+ * @attrtype: attribute type to use for the nest >> >+ * >> >+ * Create a nested attribute with attributes describing given network device. >> >+ * Clean up on error. >> >+ * >> >+ * Return: 0 on success, error value (-EMSGSIZE only) on error >> >+ */ >> >+int ethnl_fill_dev(struct sk_buff *msg, struct net_device *dev, u16 attrtype) >> >+{ >> >+ struct nlattr *nest; >> >+ int ret = -EMSGSIZE; >> >+ >> >+ nest = ethnl_nest_start(msg, attrtype); >> >+ if (!nest) >> >+ return -EMSGSIZE; >> >+ >> >+ if (nla_put_u32(msg, ETHA_DEV_INDEX, (u32)dev->ifindex)) >> >+ goto err; >> >+ if (nla_put_string(msg, ETHA_DEV_NAME, dev->name)) >> >+ goto err; >> >+ >> >+ nla_nest_end(msg, nest); >> >+ return 0; >> >+err: >> Usually this label is called "nla_put_failure". Same for the rest. >> "err" could be confused with error return variable. >> >> have "ret = -EMSGSIZE" here. Easier to follow the error path. > >OK > >> >+ nla_nest_cancel(msg, nest); >> >+ return ret; >> >+} >> >+ >> >+/** >> >+ * ethnl_reply_init() - Create skb for a reply and fill device identification >> >+ * @payload: payload length (without netlink and genetlink header) >> >+ * @dev: device the reply is about (may be null) >> >+ * @cmd: ETHNL_CMD_* command for reply >> >+ * @info: genetlink info of the received packet we respond to >> >+ * @ehdrp: place to store payload pointer returned by genlmsg_new() >> >+ * >> >+ * Return: pointer to allocated skb on success, NULL on error >> >+ */ >> >+struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd, >> >+ u16 dev_attrtype, struct genl_info *info, >> >+ void **ehdrp) >> >+{ >> >+ struct sk_buff *rskb; >> >> Could be just "skb", or "msg". You have "msg" in ethnl_fill_dev(). >> Please have it consistent. > >OK > >> >+ rskb = genlmsg_new(payload, GFP_KERNEL); >> >+ if (!rskb) { >> >+ ETHNL_SET_ERRMSG(info, >> >+ "failed to allocate reply message"); >> >> No need to wrap. > >OK > >> >+ int ret = ethnl_fill_dev(rskb, dev, dev_attrtype); >> >+ >> >+ if (ret < 0) >> >> "if (ret)" is enough. > >OK > >> >+ >> >+#define ETHNL_SET_ERRMSG(info, msg) \ >> >+ do { if (info) GENL_SET_ERR_MSG(info, msg); } while (0) >> >> Why do you need this macro? Can info be null? > >The same functions are used for generating replies to GET requests (info >is not null) and notifications (info is null, no extack). So again, could this be generic? GENL_SET_ERR_MSG() could accept null and ignore. > >> In general, macros like this should be avoided. > >At the expense of repeating the same patterns. In this case it would be >acceptable but it would mean one more indentation level for lines with >the error/warning messages. No if you move the check to lower layer. > >> >+static inline int ethnl_str_size(const char *s) >> >+{ >> >+ return nla_total_size(strlen(s) + 1); >> >> This looks like more generic helper, not tight to ethtool. > >OK > >> >+static inline int ethnl_str_ifne_size(const char *s) >> >> Eh? "ifne"? What is this good for? > >It's for "if not empty". Structures like ethtool_drvinfo use fixed size >char arrays for strings where empty string means the information is not >available. So if the string is empty, netlink attribute is omitted. > >> >+{ >> >+ return s[0] ? ethnl_str_size(s) : 0; >> >+} >> >+ >> >+static inline int ethnl_put_str_ifne(struct sk_buff *skb, int attrtype, >> >+ const char *s) >> >+{ >> >+ if (!s[0]) >> >+ return 0; >> >+ return nla_put_string(skb, attrtype, s); >> >> I don't like helpers like this. Do the check in the caller and put or >> not put the string there. It's a single if. > >OK > > >> >+static inline struct nlattr *ethnl_nest_start(struct sk_buff *skb, >> >+ int attrtype) >> >+{ >> >+ return nla_nest_start(skb, attrtype | NLA_F_NESTED); >> >> Please use nla_nest_start directly and avoid helpers like this. > >OK. It's one more thing to check (and forget) in many places, though. > >> >+static inline int ethnlmsg_parse(const struct nlmsghdr *nlh, >> >+ struct nlattr *tb[], int maxtype, >> >+ const struct nla_policy *policy, >> >+ struct genl_info *info) >> >+{ >> >+ return nlmsg_parse_strict(nlh, GENL_HDRLEN, tb, maxtype, policy, >> >+ info ? info->extack : NULL); >> >> Same thing, please use nlmsg_parse_strict directly. > >OK > >> >+/* ethnl_update_* return true if the value is changed */ >> >+static inline bool ethnl_update_u32(u32 *dst, struct nlattr *attr) >> >+{ >> >+ u32 val; >> >+ >> >+ if (!attr) >> >+ return false; >> >+ val = nla_get_u32(attr); >> >+ if (*dst == val) >> >+ return false; >> >+ >> >+ *dst = val; >> >+ return true; >> >+} >> >+ >> >+static inline bool ethnl_update_u8(u8 *dst, struct nlattr *attr) >> >+{ >> >+ u8 val; >> >+ >> >+ if (!attr) >> >+ return false; >> >+ val = nla_get_u8(attr); >> >+ if (*dst == val) >> >+ return false; >> >+ >> >+ *dst = val; >> >+ return true; >> >+} >> >+ >> >+/* update u32 value used as bool from NLA_U8 */ >> >+static inline bool ethnl_update_bool32(u32 *dst, struct nlattr *attr) >> >+{ >> >+ u8 val; >> >+ >> >+ if (!attr) >> >+ return false; >> >+ val = !!nla_get_u8(attr); >> >+ if (!!*dst == val) >> >+ return false; >> >+ >> >+ *dst = val; >> >+ return true; >> >+} >> >+ >> >+static inline bool ethnl_update_binary(u8 *dst, unsigned int len, >> >+ struct nlattr *attr) >> >+{ >> >+ if (!attr) >> >+ return false; >> >+ if (nla_len(attr) < len) >> >+ len = nla_len(attr); >> >+ if (!memcmp(dst, nla_data(attr), len)) >> >+ return false; >> >+ >> >+ memcpy(dst, nla_data(attr), len); >> >+ return true; >> >+} >> >+ >> >+static inline bool ethnl_update_bitfield32(u32 *dst, struct nlattr *attr) >> >+{ >> >+ struct nla_bitfield32 change; >> >+ u32 newval; >> >+ >> >+ if (!attr) >> >+ return false; >> >+ change = nla_get_bitfield32(attr); >> >+ newval = (*dst & ~change.selector) | (change.value & change.selector); >> >+ if (*dst == newval) >> >+ return false; >> >+ >> >+ *dst = newval; >> >+ return true; >> >+} >> >> I don't understand puspose of these "update" helper functions. Try to >> avoid them. In general, please try to avoid wrappers around netlink api. > >The purpose is to update a value according to a request attribute (if >present) and tell caller if the value was actually modified. They are >used like e.g. > > mod = false; > if (ethnl_update_u32(&data.rx_pending, tb[ETHA_RING_RX_PENDING])) > mod = true; > if (ethnl_update_u32(&data.rx_mini_pending, > tb[ETHA_RING_RX_MINI_PENDING])) > mod = true; > if (ethnl_update_u32(&data.rx_jumbo_pending, > tb[ETHA_RING_RX_JUMBO_PENDING])) > mod = true; > if (ethnl_update_u32(&data.tx_pending, tb[ETHA_RING_TX_PENDING])) > mod = true; > > if (!mod) > return 0; > /* call ethtool_ops handler and send notification */ > >so that we can omit calling the handler and sending a notification if >the request was no-op. Expanding the helpers here would make the code >needlessly complicated and error prone. Got it. Makes sense. > >> >+static inline void warn_partial_info(struct genl_info *info) >> >+{ >> >+ ETHNL_SET_ERRMSG(info, "not all requested data could be retrieved"); >> >+} >> >+ >> >+/* Check user privileges explicitly to allow finer access control based on >> >+ * context of the request or hiding part of the information from unprivileged >> >+ * users >> >+ */ >> >+static inline bool ethnl_is_privileged(struct sk_buff *skb) >> >+{ >> >+ struct net *net = sock_net(skb->sk); >> >+ >> >+ return netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN); >> >> Same here. > >It's still the same, helpers allow replacing repeating code patterns >with just saying what you are doing. But this one is called only from >one place so I can live without it. It is always a tradeoff. We have to be careful not to abuse it. > >> >+} >> >+ >> >+/* total size of ETHA_*_DEV nested attribute; this is an upper estimate so that >> >+ * we do not need to hold RTNL longer than necessary to prevent rename between >> >+ * estimating the size and composing the message >> >+ */ >> >+static inline unsigned int dev_ident_size(void) >> >> You are missing "ethnl_" prefix. > >OK > >Michal
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h index 6aa267451542..59240a2cda56 100644 --- a/include/uapi/linux/ethtool_netlink.h +++ b/include/uapi/linux/ethtool_netlink.h @@ -18,6 +18,17 @@ enum { ETHNL_CMD_MAX = (__ETHNL_CMD_CNT - 1) }; +/* device specification */ + +enum { + ETHA_DEV_UNSPEC, + ETHA_DEV_INDEX, /* u32 */ + ETHA_DEV_NAME, /* string */ + + __ETHA_DEV_CNT, + ETHA_DEV_MAX = (__ETHA_DEV_CNT - 1) +}; + /* generic netlink info */ #define ETHTOOL_GENL_NAME "ethtool" #define ETHTOOL_GENL_VERSION 1 diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index 85dd6dac71a2..cc6829ba4331 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c @@ -1,8 +1,152 @@ // SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note +#include <net/sock.h> #include <linux/ethtool_netlink.h> #include "netlink.h" +static const struct nla_policy dev_policy[ETHA_DEV_MAX + 1] = { + [ETHA_DEV_UNSPEC] = { .type = NLA_REJECT }, + [ETHA_DEV_INDEX] = { .type = NLA_U32 }, + [ETHA_DEV_NAME] = { .type = NLA_NUL_STRING, + .len = IFNAMSIZ - 1 }, +}; + +/** + * ethnl_dev_get() - get device identified by nested attribute + * @info: genetlink info (also used for extack error reporting) + * @nest: nest attribute with device identification + * + * Finds the network device identified by ETHA_DEV_INDEX (ifindex) or + * ETHA_DEV_NAME (name) attributes in a nested attribute @nest. If both + * are supplied, they must identify the same device. If successful, takes + * a reference to the device which is to be released by caller. + * + * Return: pointer to the device if successful, ERR_PTR(err) on error + */ +struct net_device *ethnl_dev_get(struct genl_info *info, struct nlattr *nest) +{ + struct net *net = genl_info_net(info); + struct nlattr *tb[ETHA_DEV_MAX + 1]; + struct net_device *dev; + int ret; + + if (!nest) { + ETHNL_SET_ERRMSG(info, + "mandatory device identification missing"); + return ERR_PTR(-EINVAL); + } + ret = nla_parse_nested_strict(tb, ETHA_DEV_MAX, nest, dev_policy, + info->extack); + if (ret < 0) + return ERR_PTR(ret); + + if (tb[ETHA_DEV_INDEX]) { + dev = dev_get_by_index(net, nla_get_u32(tb[ETHA_DEV_INDEX])); + if (!dev) + return ERR_PTR(-ENODEV); + /* if both ifindex and ifname are passed, they must match */ + if (tb[ETHA_DEV_NAME]) { + const char *nl_name = nla_data(tb[ETHA_DEV_NAME]); + + if (strncmp(dev->name, nl_name, IFNAMSIZ)) { + dev_put(dev); + ETHNL_SET_ERRMSG(info, + "ifindex and ifname do not match"); + return ERR_PTR(-ENODEV); + } + } + return dev; + } else if (tb[ETHA_DEV_NAME]) { + dev = dev_get_by_name(net, nla_data(tb[ETHA_DEV_NAME])); + if (!dev) + return ERR_PTR(-ENODEV); + } else { + ETHNL_SET_ERRMSG(info, "either ifindex or ifname required"); + return ERR_PTR(-EINVAL); + } + + if (!netif_device_present(dev)) { + dev_put(dev); + ETHNL_SET_ERRMSG(info, "device not present"); + return ERR_PTR(-ENODEV); + } + return dev; +} + +/** + * ethnl_fill_dev() - Put device identification nest into a message + * @msg: skb with the message + * @dev: network device to describe + * @attrtype: attribute type to use for the nest + * + * Create a nested attribute with attributes describing given network device. + * Clean up on error. + * + * Return: 0 on success, error value (-EMSGSIZE only) on error + */ +int ethnl_fill_dev(struct sk_buff *msg, struct net_device *dev, u16 attrtype) +{ + struct nlattr *nest; + int ret = -EMSGSIZE; + + nest = ethnl_nest_start(msg, attrtype); + if (!nest) + return -EMSGSIZE; + + if (nla_put_u32(msg, ETHA_DEV_INDEX, (u32)dev->ifindex)) + goto err; + if (nla_put_string(msg, ETHA_DEV_NAME, dev->name)) + goto err; + + nla_nest_end(msg, nest); + return 0; +err: + nla_nest_cancel(msg, nest); + return ret; +} + +/** + * ethnl_reply_init() - Create skb for a reply and fill device identification + * @payload: payload length (without netlink and genetlink header) + * @dev: device the reply is about (may be null) + * @cmd: ETHNL_CMD_* command for reply + * @info: genetlink info of the received packet we respond to + * @ehdrp: place to store payload pointer returned by genlmsg_new() + * + * Return: pointer to allocated skb on success, NULL on error + */ +struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd, + u16 dev_attrtype, struct genl_info *info, + void **ehdrp) +{ + struct sk_buff *rskb; + void *ehdr; + + rskb = genlmsg_new(payload, GFP_KERNEL); + if (!rskb) { + ETHNL_SET_ERRMSG(info, + "failed to allocate reply message"); + return NULL; + } + + ehdr = genlmsg_put_reply(rskb, info, ðtool_genl_family, 0, cmd); + if (!ehdr) + goto err; + if (ehdrp) + *ehdrp = ehdr; + if (dev) { + int ret = ethnl_fill_dev(rskb, dev, dev_attrtype); + + if (ret < 0) + goto err; + } + + return rskb; +err: + nlmsg_free(rskb); + return NULL; +} + /* genetlink setup */ static const struct genl_ops ethtool_genl_ops[] = { diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index 63063b582ca2..db90d95410b1 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -6,7 +6,151 @@ #include <linux/ethtool_netlink.h> #include <linux/netdevice.h> #include <net/genetlink.h> +#include <net/sock.h> + +#define ETHNL_SET_ERRMSG(info, msg) \ + do { if (info) GENL_SET_ERR_MSG(info, msg); } while (0) extern struct genl_family ethtool_genl_family; +struct net_device *ethnl_dev_get(struct genl_info *info, struct nlattr *nest); +int ethnl_fill_dev(struct sk_buff *msg, struct net_device *dev, u16 attrtype); + +struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd, + u16 dev_attrtype, struct genl_info *info, + void **ehdrp); + +static inline int ethnl_str_size(const char *s) +{ + return nla_total_size(strlen(s) + 1); +} + +static inline int ethnl_str_ifne_size(const char *s) +{ + return s[0] ? ethnl_str_size(s) : 0; +} + +static inline int ethnl_put_str_ifne(struct sk_buff *skb, int attrtype, + const char *s) +{ + if (!s[0]) + return 0; + return nla_put_string(skb, attrtype, s); +} + +static inline struct nlattr *ethnl_nest_start(struct sk_buff *skb, + int attrtype) +{ + return nla_nest_start(skb, attrtype | NLA_F_NESTED); +} + +static inline int ethnlmsg_parse(const struct nlmsghdr *nlh, + struct nlattr *tb[], int maxtype, + const struct nla_policy *policy, + struct genl_info *info) +{ + return nlmsg_parse_strict(nlh, GENL_HDRLEN, tb, maxtype, policy, + info ? info->extack : NULL); +} + +/* ethnl_update_* return true if the value is changed */ +static inline bool ethnl_update_u32(u32 *dst, struct nlattr *attr) +{ + u32 val; + + if (!attr) + return false; + val = nla_get_u32(attr); + if (*dst == val) + return false; + + *dst = val; + return true; +} + +static inline bool ethnl_update_u8(u8 *dst, struct nlattr *attr) +{ + u8 val; + + if (!attr) + return false; + val = nla_get_u8(attr); + if (*dst == val) + return false; + + *dst = val; + return true; +} + +/* update u32 value used as bool from NLA_U8 */ +static inline bool ethnl_update_bool32(u32 *dst, struct nlattr *attr) +{ + u8 val; + + if (!attr) + return false; + val = !!nla_get_u8(attr); + if (!!*dst == val) + return false; + + *dst = val; + return true; +} + +static inline bool ethnl_update_binary(u8 *dst, unsigned int len, + struct nlattr *attr) +{ + if (!attr) + return false; + if (nla_len(attr) < len) + len = nla_len(attr); + if (!memcmp(dst, nla_data(attr), len)) + return false; + + memcpy(dst, nla_data(attr), len); + return true; +} + +static inline bool ethnl_update_bitfield32(u32 *dst, struct nlattr *attr) +{ + struct nla_bitfield32 change; + u32 newval; + + if (!attr) + return false; + change = nla_get_bitfield32(attr); + newval = (*dst & ~change.selector) | (change.value & change.selector); + if (*dst == newval) + return false; + + *dst = newval; + return true; +} + +static inline void warn_partial_info(struct genl_info *info) +{ + ETHNL_SET_ERRMSG(info, "not all requested data could be retrieved"); +} + +/* Check user privileges explicitly to allow finer access control based on + * context of the request or hiding part of the information from unprivileged + * users + */ +static inline bool ethnl_is_privileged(struct sk_buff *skb) +{ + struct net *net = sock_net(skb->sk); + + return netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN); +} + +/* total size of ETHA_*_DEV nested attribute; this is an upper estimate so that + * we do not need to hold RTNL longer than necessary to prevent rename between + * estimating the size and composing the message + */ +static inline unsigned int dev_ident_size(void) +{ + return nla_total_size(nla_total_size(sizeof(u32)) + + nla_total_size(IFNAMSIZ)); +} + #endif /* _NET_ETHTOOL_NETLINK_H */
Various helpers used by ethtool netlink code. Signed-off-by: Michal Kubecek <mkubecek@suse.cz> --- include/uapi/linux/ethtool_netlink.h | 11 ++ net/ethtool/netlink.c | 144 +++++++++++++++++++++++++++ net/ethtool/netlink.h | 144 +++++++++++++++++++++++++++ 3 files changed, 299 insertions(+)