[PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting

[PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting

[Matthias Schiffer]

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

[PATCH net-next 2/2] vxlan: add back error messages to vxlan_config_validate() as extended netlink acks

[Matthias Schiffer]

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

Re: [PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting

[Jiri Benc]

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

Re: [PATCH net-next 2/2] vxlan: add back error messages to vxlan_config_validate() as extended netlink acks

[Jiri Benc]

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