* [PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting @ 2017-06-27 20:47 Matthias Schiffer 2017-06-27 20:47 ` [PATCH net-next 2/2] vxlan: add back error messages to vxlan_config_validate() as extended netlink acks Matthias Schiffer 2017-06-28 17:18 ` [PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting Jiri Benc 0 siblings, 2 replies; 4+ messages in thread From: Matthias Schiffer @ 2017-06-27 20:47 UTC (permalink / raw) To: davem, jbenc, roopa, pshelar; +Cc: netdev, linux-kernel The kernel log is not where users expect error messages for netlink requests; as we have extended acks now, we can replace pr_debug() with NL_SET_ERR_MSG_ATTR(). While we're at it, also fix the !is_valid_ether_addr() error message (as it not only rejects the all-zero address, but also multicast addresses), and add messages for the remaining attributes. Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> --- drivers/net/vxlan.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index fd0ff97e3d81..01957e39f2cd 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -2716,12 +2716,14 @@ static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[], { if (tb[IFLA_ADDRESS]) { if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN) { - pr_debug("invalid link address (not ethernet)\n"); + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_ADDRESS], + "invalid link address (not ethernet)"); return -EINVAL; } if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS]))) { - pr_debug("invalid all zero ethernet address\n"); + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_ADDRESS], + "invalid ethernet address"); return -EADDRNOTAVAIL; } } @@ -2729,8 +2731,11 @@ static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[], if (tb[IFLA_MTU]) { u32 mtu = nla_get_u32(tb[IFLA_MTU]); - if (mtu < ETH_MIN_MTU || mtu > ETH_MAX_MTU) + if (mtu < ETH_MIN_MTU || mtu > ETH_MAX_MTU) { + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_MTU], + "invalid MTU"); return -EINVAL; + } } if (!data) @@ -2739,8 +2744,11 @@ static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[], if (data[IFLA_VXLAN_ID]) { u32 id = nla_get_u32(data[IFLA_VXLAN_ID]); - if (id >= VXLAN_N_VID) + if (id >= VXLAN_N_VID) { + NL_SET_ERR_MSG_ATTR(extack, data[IFLA_VXLAN_ID], + "invalid VXLAN ID"); return -ERANGE; + } } if (data[IFLA_VXLAN_PORT_RANGE]) { @@ -2748,8 +2756,8 @@ static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[], = nla_data(data[IFLA_VXLAN_PORT_RANGE]); if (ntohs(p->high) < ntohs(p->low)) { - pr_debug("port range %u .. %u not valid\n", - ntohs(p->low), ntohs(p->high)); + NL_SET_ERR_MSG_ATTR(extack, data[IFLA_VXLAN_PORT_RANGE], + "port range not valid"); return -EINVAL; } } -- 2.13.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH net-next 2/2] vxlan: add back error messages to vxlan_config_validate() as extended netlink acks 2017-06-27 20:47 [PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting Matthias Schiffer @ 2017-06-27 20:47 ` Matthias Schiffer 2017-06-28 17:25 ` Jiri Benc 2017-06-28 17:18 ` [PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting Jiri Benc 1 sibling, 1 reply; 4+ messages in thread From: Matthias Schiffer @ 2017-06-27 20:47 UTC (permalink / raw) To: davem, jbenc, roopa, pshelar; +Cc: netdev, linux-kernel When refactoring the vxlan config validation, some kernel log messages were removed. This brings them back using the new netlink_ext_ack support, and adds some more in the recently added code handling link-local IPv6 addresses. Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> --- drivers/net/vxlan.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 01957e39f2cd..8d4248ab09c2 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -2909,7 +2909,8 @@ static int vxlan_sock_add(struct vxlan_dev *vxlan) static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, struct net_device **lower, - struct vxlan_dev *old) + struct vxlan_dev *old, + struct netlink_ext_ack *extack) { struct vxlan_net *vn = net_generic(src_net, vxlan_net_id); struct vxlan_dev *tmp; @@ -2923,6 +2924,8 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, */ if ((conf->flags & ~VXLAN_F_ALLOWED_GPE) || !(conf->flags & VXLAN_F_COLLECT_METADATA)) { + NL_SET_ERR_MSG(extack, + "unsupported combination of extensions"); return -EINVAL; } } @@ -2957,14 +2960,20 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, if (local_type & IPV6_ADDR_LINKLOCAL) { if (!(remote_type & IPV6_ADDR_LINKLOCAL) && - (remote_type != IPV6_ADDR_ANY)) + (remote_type != IPV6_ADDR_ANY)) { + NL_SET_ERR_MSG(extack, + "invalid combination of address scopes"); return -EINVAL; + } conf->flags |= VXLAN_F_IPV6_LINKLOCAL; } else { if (remote_type == - (IPV6_ADDR_UNICAST | IPV6_ADDR_LINKLOCAL)) + (IPV6_ADDR_UNICAST | IPV6_ADDR_LINKLOCAL)) { + NL_SET_ERR_MSG(extack, + "invalid combination of address scopes"); return -EINVAL; + } conf->flags &= ~VXLAN_F_IPV6_LINKLOCAL; } @@ -2991,12 +3000,18 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, *lower = lowerdev; } else { - if (vxlan_addr_multicast(&conf->remote_ip)) + if (vxlan_addr_multicast(&conf->remote_ip)) { + NL_SET_ERR_MSG(extack, + "multicast destination requires interface to be specified"); return -EINVAL; + } #if IS_ENABLED(CONFIG_IPV6) - if (conf->flags & VXLAN_F_IPV6_LINKLOCAL) + if (conf->flags & VXLAN_F_IPV6_LINKLOCAL) { + NL_SET_ERR_MSG(extack, + "link-local local/remote addresses require interface to be specified"); return -EINVAL; + } #endif *lower = NULL; @@ -3028,6 +3043,7 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, tmp->cfg.remote_ifindex != conf->remote_ifindex) continue; + NL_SET_ERR_MSG(extack, "duplicate VNI"); return -EEXIST; } @@ -3083,14 +3099,14 @@ static void vxlan_config_apply(struct net_device *dev, } static int vxlan_dev_configure(struct net *src_net, struct net_device *dev, - struct vxlan_config *conf, - bool changelink) + struct vxlan_config *conf, bool changelink, + struct netlink_ext_ack *extack) { struct vxlan_dev *vxlan = netdev_priv(dev); struct net_device *lowerdev; int ret; - ret = vxlan_config_validate(src_net, conf, &lowerdev, vxlan); + ret = vxlan_config_validate(src_net, conf, &lowerdev, vxlan, extack); if (ret) return ret; @@ -3100,13 +3116,14 @@ static int vxlan_dev_configure(struct net *src_net, struct net_device *dev, } static int __vxlan_dev_create(struct net *net, struct net_device *dev, - struct vxlan_config *conf) + struct vxlan_config *conf, + struct netlink_ext_ack *extack) { struct vxlan_net *vn = net_generic(net, vxlan_net_id); struct vxlan_dev *vxlan = netdev_priv(dev); int err; - err = vxlan_dev_configure(net, dev, conf, false); + err = vxlan_dev_configure(net, dev, conf, false, extack); if (err) return err; @@ -3352,7 +3369,7 @@ static int vxlan_newlink(struct net *src_net, struct net_device *dev, if (err) return err; - return __vxlan_dev_create(src_net, dev, &conf); + return __vxlan_dev_create(src_net, dev, &conf, extack); } static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[], @@ -3372,7 +3389,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[], memcpy(&old_dst, dst, sizeof(struct vxlan_rdst)); - err = vxlan_dev_configure(vxlan->net, dev, &conf, true); + err = vxlan_dev_configure(vxlan->net, dev, &conf, true, extack); if (err) return err; @@ -3578,7 +3595,7 @@ struct net_device *vxlan_dev_create(struct net *net, const char *name, if (IS_ERR(dev)) return dev; - err = __vxlan_dev_create(net, dev, conf); + err = __vxlan_dev_create(net, dev, conf, NULL); if (err < 0) { free_netdev(dev); return ERR_PTR(err); -- 2.13.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next 2/2] vxlan: add back error messages to vxlan_config_validate() as extended netlink acks 2017-06-27 20:47 ` [PATCH net-next 2/2] vxlan: add back error messages to vxlan_config_validate() as extended netlink acks Matthias Schiffer @ 2017-06-28 17:25 ` Jiri Benc 0 siblings, 0 replies; 4+ messages in thread From: Jiri Benc @ 2017-06-28 17:25 UTC (permalink / raw) To: Matthias Schiffer; +Cc: davem, roopa, pshelar, netdev, linux-kernel On Tue, 27 Jun 2017 22:47:58 +0200, Matthias Schiffer wrote: > if ((conf->flags & ~VXLAN_F_ALLOWED_GPE) || > !(conf->flags & VXLAN_F_COLLECT_METADATA)) { > + NL_SET_ERR_MSG(extack, > + "unsupported combination of extensions"); Since we're redesigning this, let's be more helpful to the user. There's probably not going to be tremendous improvements here but let's try at least a bit. "VXLAN GPE does not support this combination of extensions" > if (local_type & IPV6_ADDR_LINKLOCAL) { > if (!(remote_type & IPV6_ADDR_LINKLOCAL) && > - (remote_type != IPV6_ADDR_ANY)) > + (remote_type != IPV6_ADDR_ANY)) { > + NL_SET_ERR_MSG(extack, > + "invalid combination of address scopes"); "invalid combination of local and remote address scopes" > return -EINVAL; > + } > > conf->flags |= VXLAN_F_IPV6_LINKLOCAL; > } else { > if (remote_type == > - (IPV6_ADDR_UNICAST | IPV6_ADDR_LINKLOCAL)) > + (IPV6_ADDR_UNICAST | IPV6_ADDR_LINKLOCAL)) { > + NL_SET_ERR_MSG(extack, > + "invalid combination of address scopes"); ditto The rest looks good to me. Thanks a lot for doing the work, Matthias! Jiri ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting 2017-06-27 20:47 [PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting Matthias Schiffer 2017-06-27 20:47 ` [PATCH net-next 2/2] vxlan: add back error messages to vxlan_config_validate() as extended netlink acks Matthias Schiffer @ 2017-06-28 17:18 ` Jiri Benc 1 sibling, 0 replies; 4+ messages in thread From: Jiri Benc @ 2017-06-28 17:18 UTC (permalink / raw) To: Matthias Schiffer; +Cc: davem, roopa, pshelar, netdev, linux-kernel On Tue, 27 Jun 2017 22:47:57 +0200, Matthias Schiffer wrote: > if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS]))) { > - pr_debug("invalid all zero ethernet address\n"); > + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_ADDRESS], > + "invalid ethernet address"); Could we be more specific here? This is better than nothing but still not as helpful to the user as it could be. What about something like "the provided ethernet address is not unicast"? > - if (mtu < ETH_MIN_MTU || mtu > ETH_MAX_MTU) > + if (mtu < ETH_MIN_MTU || mtu > ETH_MAX_MTU) { > + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_MTU], > + "invalid MTU"); "MTU must be between 68 and 65535" > - if (id >= VXLAN_N_VID) > + if (id >= VXLAN_N_VID) { > + NL_SET_ERR_MSG_ATTR(extack, data[IFLA_VXLAN_ID], > + "invalid VXLAN ID"); "VXLAN ID must be lower than 16777216" > if (ntohs(p->high) < ntohs(p->low)) { > - pr_debug("port range %u .. %u not valid\n", > - ntohs(p->low), ntohs(p->high)); > + NL_SET_ERR_MSG_ATTR(extack, data[IFLA_VXLAN_PORT_RANGE], > + "port range not valid"); Since you're getting rid of the values output, I'd rather suggest more explicit "the first value of the port range must not be higher than the second value" or so. Shorter wording is welcome :-) Thanks, Jiri ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-28 17:25 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-27 20:47 [PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting Matthias Schiffer 2017-06-27 20:47 ` [PATCH net-next 2/2] vxlan: add back error messages to vxlan_config_validate() as extended netlink acks Matthias Schiffer 2017-06-28 17:25 ` Jiri Benc 2017-06-28 17:18 ` [PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting Jiri Benc
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).