| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 11 Aug 2017 09:19:34 -0700
From: Roopa Prabhu <roopa@...ulusnetworks.com>
To: Girish Moodalbail <girish.moodalbail@...cle.com>
Cc: pravin shelar <pshelar@....org>,
"davem@...emloft.net" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Matthias Schiffer <mschiffer@...verse-factory.net>,
Jiri Benc <jbenc@...hat.com>
Subject: Re: [PATCH net-next v2] vxlan: change vxlan_[config_]validate() to
use netlink_ext_ack for error reporting
On Thu, Aug 10, 2017 at 2:16 PM, Girish Moodalbail
<girish.moodalbail@...cle.com> wrote:
> 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().
>
> Signed-off-by: Matthias Schiffer <mschiffer@...verse-factory.net>
> Signed-off-by: Girish Moodalbail <girish.moodalbail@...cle.com>
>
> ---
> v1 -> v2:
> - addressed, error messages rewording, comments from Jiri Benc
> - started off with what Matthias had, and I covered error reporting
> for all of the unsuccessful returns
I have a similar patch in my tree, so, i am tempted to suggest
alternate wordings for some of the msgs below :)
> ---
> drivers/net/vxlan.c | 98 +++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 72 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 35e84a9e..ec302cd7 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -2729,12 +2729,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],
> + "Provided link layer address is not Ethernet");
> return -EINVAL;
> }
keep it simple and closer to the original msg: "invalid link layer address"
>
> 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],
> + "Provided Ethernet address is not unicast");
> return -EADDRNOTAVAIL;
keep it simple and closer to the original msg: "invalid all zero
ethernet address"
> }
> }
> @@ -2742,18 +2744,27 @@ 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],
> + "MTU must be between 68 and 65535");
> return -EINVAL;
> + }
> }
>
> - if (!data)
> + if (!data) {
> + NL_SET_ERR_MSG(extack,
> + "Not enough attributes provided to perform the operation");
> return -EINVAL;
> + }
"not enough attributes"
>
> 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, tb[IFLA_VXLAN_ID],
> + "VXLAN ID must be lower than 16777216");
> return -ERANGE;
"invalid vxlan-id"
> + }
> }
>
> if (data[IFLA_VXLAN_PORT_RANGE]) {
> @@ -2761,8 +2772,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, tb[IFLA_VXLAN_PORT_RANGE],
> + "Provided source port range bounds is invalid");
> return -EINVAL;
> }
"invalid source port range"
> }
> @@ -2919,7 +2930,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;
> @@ -2933,6 +2945,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,
> + "VXLAN GPE does not support this combination of attributes");
> return -EINVAL;
> }
"collect metadata not supported with vxlan gpe"
> }
> @@ -2947,15 +2961,23 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf,
> conf->saddr.sa.sa_family = conf->remote_ip.sa.sa_family;
> }
>
> - if (conf->saddr.sa.sa_family != conf->remote_ip.sa.sa_family)
> + if (conf->saddr.sa.sa_family != conf->remote_ip.sa.sa_family) {
> + NL_SET_ERR_MSG(extack,
> + "Local and remote address must be from the same family");
> return -EINVAL;
> + }
>
> - if (vxlan_addr_multicast(&conf->saddr))
> + if (vxlan_addr_multicast(&conf->saddr)) {
> + NL_SET_ERR_MSG(extack, "Local address cannot be multicast");
> return -EINVAL;
"invalid local address. multicast not supported"
> + }
>
> if (conf->saddr.sa.sa_family == AF_INET6) {
> - if (!IS_ENABLED(CONFIG_IPV6))
> + if (!IS_ENABLED(CONFIG_IPV6)) {
> + NL_SET_ERR_MSG(extack,
> + "IPv6 support not enabled in the kernel");
"invalid address family. ipv6 not enabled"
> return -EPFNOSUPPORT;
> + }
> use_ipv6 = true;
> conf->flags |= VXLAN_F_IPV6;
>
> @@ -2967,46 +2989,68 @@ 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 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 local and remote address scopes");
> return -EINVAL;
> + }
>
> conf->flags &= ~VXLAN_F_IPV6_LINKLOCAL;
> }
> }
> }
>
> - if (conf->label && !use_ipv6)
> + if (conf->label && !use_ipv6) {
> + NL_SET_ERR_MSG(extack,
> + "Label attribute only applies for IPv6 VXLAN devices");
> return -EINVAL;
> + }
>
> if (conf->remote_ifindex) {
> struct net_device *lowerdev;
>
> lowerdev = __dev_get_by_index(src_net, conf->remote_ifindex);
> - if (!lowerdev)
> + if (!lowerdev) {
> + NL_SET_ERR_MSG(extack,
> + "Specified interface for tunnel endpoint communications not found");
> return -ENODEV;
"invalid vxlan remote link interface, device not found"
> + }
>
> #if IS_ENABLED(CONFIG_IPV6)
> if (use_ipv6) {
> struct inet6_dev *idev = __in6_dev_get(lowerdev);
> - if (idev && idev->cnf.disable_ipv6)
> + if (idev && idev->cnf.disable_ipv6) {
> + NL_SET_ERR_MSG(extack,
> + "IPv6 support disabled by administrator");
> return -EPERM;
> + }
> }
> #endif
>
> *lower = lowerdev;
> } else {
> - if (vxlan_addr_multicast(&conf->remote_ip))
> + if (vxlan_addr_multicast(&conf->remote_ip)) {
> + NL_SET_ERR_MSG(extack,
> + "Interface need to be specified for multicast destination");
"vxlan remote link interface required for multicast remote destination"
> +
> 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,
> + "Interface need to be specified for link-local local/remote addresses");
> return -EINVAL;
"vxlan link interface required for link-local local/remote addresses"
> + }
> #endif
>
> *lower = NULL;
> @@ -3038,6 +3082,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, "Specified VNI is duplicate");
"duplicate vni. vxlan device with vni exists."
> return -EEXIST;
> }
>
> @@ -3097,14 +3142,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;
>
> @@ -3114,13 +3159,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;
>
> @@ -3366,7 +3412,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[],
> @@ -3386,7 +3432,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;
>
> @@ -3592,7 +3638,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);
> --
> 1.8.3.1
>
Powered by blists - more mailing lists