From: Petr Machata <petrm@mellanox.com>
To: Roopa Prabhu <roopa@cumulusnetworks.com>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"dsa@cumulusnetworks.com" <dsa@cumulusnetworks.com>,
"sd@queasysnail.net" <sd@queasysnail.net>,
"johannes@sipsolutions.net" <johannes@sipsolutions.net>
Subject: Re: [PATCH net-next 1/2] vxlan: add extack support for create and changelink
Date: Tue, 26 Feb 2019 13:51:13 +0000 [thread overview]
Message-ID: <87imx6fyk1.fsf@mellanox.com> (raw)
In-Reply-To: <1551160982-32685-2-git-send-email-roopa@cumulusnetworks.com>
Roopa Prabhu <roopa@cumulusnetworks.com> writes:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> This patch adds extack coverage in vxlan link
> create and changelink paths. Introduces a new helper
> vxlan_nl2flags to consolidate flag attribute validation.
>
> thanks to Johannes Berg for some tips to construct the
> generic vxlan flag extack strings.
>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
> drivers/net/vxlan.c | 208 +++++++++++++++++++++++++++++++++++-----------------
> include/net/vxlan.h | 31 ++++++++
> 2 files changed, 172 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 577201c..a3c46d7 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -3583,11 +3583,40 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> return err;
> }
>
> +/* Set/clear flags based on attribute */
> +static int vxlan_nl2flag(struct vxlan_config *conf, struct nlattr *tb[],
> + int attrtype, unsigned long mask, bool changelink,
> + bool changelink_supported,
> + struct netlink_ext_ack *extack)
> +{
> + unsigned long flags;
> +
> + if (!tb[attrtype])
> + return 0;
> +
> + if (changelink && !changelink_supported) {
> + vxlan_flag_attr_error(attrtype, extack);
> + return -EOPNOTSUPP;
> + }
> +
> + if (vxlan_policy[attrtype].type == NLA_FLAG)
> + flags = conf->flags | mask;
> + else if (nla_get_u8(tb[attrtype]))
> + flags = conf->flags | mask;
> + else
> + flags = conf->flags & ~mask;
Many of the flags for which you call this don't actually have the else
branch. However I suspect there are no good reasons not to allow
resetting a flag.
Reviewed-by: Petr Machata <petrm@mellanox.com>
> +
> + conf->flags = flags;
> +
> + return 0;
> +}
> +
> static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> struct net_device *dev, struct vxlan_config *conf,
> - bool changelink)
> + bool changelink, struct netlink_ext_ack *extack)
> {
> struct vxlan_dev *vxlan = netdev_priv(dev);
> + int err = 0;
>
> memset(conf, 0, sizeof(*conf));
>
> @@ -3598,40 +3627,54 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> if (data[IFLA_VXLAN_ID]) {
> __be32 vni = cpu_to_be32(nla_get_u32(data[IFLA_VXLAN_ID]));
>
> - if (changelink && (vni != conf->vni))
> + if (changelink && (vni != conf->vni)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_ID], "Cannot change VNI");
> return -EOPNOTSUPP;
> + }
> conf->vni = cpu_to_be32(nla_get_u32(data[IFLA_VXLAN_ID]));
> }
>
> if (data[IFLA_VXLAN_GROUP]) {
> - if (changelink && (conf->remote_ip.sa.sa_family != AF_INET))
> + if (changelink && (conf->remote_ip.sa.sa_family != AF_INET)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP], "New group address family does not match old group");
> return -EOPNOTSUPP;
> + }
>
> conf->remote_ip.sin.sin_addr.s_addr = nla_get_in_addr(data[IFLA_VXLAN_GROUP]);
> conf->remote_ip.sa.sa_family = AF_INET;
> } else if (data[IFLA_VXLAN_GROUP6]) {
> - if (!IS_ENABLED(CONFIG_IPV6))
> + if (!IS_ENABLED(CONFIG_IPV6)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP6], "IPv6 support not enabled in the kernel");
> return -EPFNOSUPPORT;
> + }
>
> - if (changelink && (conf->remote_ip.sa.sa_family != AF_INET6))
> + if (changelink && (conf->remote_ip.sa.sa_family != AF_INET6)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP6], "New group address family does not match old group");
> return -EOPNOTSUPP;
> + }
>
> conf->remote_ip.sin6.sin6_addr = nla_get_in6_addr(data[IFLA_VXLAN_GROUP6]);
> conf->remote_ip.sa.sa_family = AF_INET6;
> }
>
> if (data[IFLA_VXLAN_LOCAL]) {
> - if (changelink && (conf->saddr.sa.sa_family != AF_INET))
> + if (changelink && (conf->saddr.sa.sa_family != AF_INET)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL], "New local address family does not match old");
> return -EOPNOTSUPP;
> + }
>
> conf->saddr.sin.sin_addr.s_addr = nla_get_in_addr(data[IFLA_VXLAN_LOCAL]);
> conf->saddr.sa.sa_family = AF_INET;
> } else if (data[IFLA_VXLAN_LOCAL6]) {
> - if (!IS_ENABLED(CONFIG_IPV6))
> + if (!IS_ENABLED(CONFIG_IPV6)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL6], "IPv6 support not enabled in the kernel");
> return -EPFNOSUPPORT;
> + }
>
> - if (changelink && (conf->saddr.sa.sa_family != AF_INET6))
> + if (changelink && (conf->saddr.sa.sa_family != AF_INET6)) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL6], "New local address family does not match old");
> return -EOPNOTSUPP;
> + }
>
> /* TODO: respect scope id */
> conf->saddr.sin6.sin6_addr = nla_get_in6_addr(data[IFLA_VXLAN_LOCAL6]);
> @@ -3648,9 +3691,12 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]);
>
> if (data[IFLA_VXLAN_TTL_INHERIT]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - conf->flags |= VXLAN_F_TTL_INHERIT;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_TTL_INHERIT,
> + VXLAN_F_TTL_INHERIT, changelink, false,
> + extack);
> + if (err)
> + return err;
> +
> }
>
> if (data[IFLA_VXLAN_LABEL])
> @@ -3658,10 +3704,11 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> IPV6_FLOWLABEL_MASK;
>
> if (data[IFLA_VXLAN_LEARNING]) {
> - if (nla_get_u8(data[IFLA_VXLAN_LEARNING]))
> - conf->flags |= VXLAN_F_LEARN;
> - else
> - conf->flags &= ~VXLAN_F_LEARN;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_LEARNING,
> + VXLAN_F_LEARN, changelink, true,
> + extack);
> + if (err)
> + return err;
> } else if (!changelink) {
> /* default to learn on a new device */
> conf->flags |= VXLAN_F_LEARN;
> @@ -3671,44 +3718,52 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> conf->age_interval = nla_get_u32(data[IFLA_VXLAN_AGEING]);
>
> if (data[IFLA_VXLAN_PROXY]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_PROXY]))
> - conf->flags |= VXLAN_F_PROXY;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_PROXY,
> + VXLAN_F_PROXY, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_RSC]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_RSC]))
> - conf->flags |= VXLAN_F_RSC;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_RSC,
> + VXLAN_F_RSC, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_L2MISS]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_L2MISS]))
> - conf->flags |= VXLAN_F_L2MISS;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_L2MISS,
> + VXLAN_F_L2MISS, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_L3MISS]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_L3MISS]))
> - conf->flags |= VXLAN_F_L3MISS;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_L3MISS,
> + VXLAN_F_L3MISS, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_LIMIT]) {
> - if (changelink)
> + if (changelink) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LIMIT],
> + "Cannot change limit");
> return -EOPNOTSUPP;
> + }
> conf->addrmax = nla_get_u32(data[IFLA_VXLAN_LIMIT]);
> }
>
> if (data[IFLA_VXLAN_COLLECT_METADATA]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_COLLECT_METADATA]))
> - conf->flags |= VXLAN_F_COLLECT_METADATA;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_COLLECT_METADATA,
> + VXLAN_F_COLLECT_METADATA, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_PORT_RANGE]) {
> @@ -3718,72 +3773,92 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> conf->port_min = ntohs(p->low);
> conf->port_max = ntohs(p->high);
> } else {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_PORT_RANGE],
> + "Cannot change port range");
> return -EOPNOTSUPP;
> }
> }
>
> if (data[IFLA_VXLAN_PORT]) {
> - if (changelink)
> + if (changelink) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_PORT],
> + "Cannot change port");
> return -EOPNOTSUPP;
> + }
> conf->dst_port = nla_get_be16(data[IFLA_VXLAN_PORT]);
> }
>
> if (data[IFLA_VXLAN_UDP_CSUM]) {
> - if (changelink)
> + if (changelink) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_UDP_CSUM],
> + "Cannot change UDP_CSUM flag");
> return -EOPNOTSUPP;
> + }
> if (!nla_get_u8(data[IFLA_VXLAN_UDP_CSUM]))
> conf->flags |= VXLAN_F_UDP_ZERO_CSUM_TX;
> }
>
> if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]))
> - conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_TX;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_UDP_ZERO_CSUM6_TX,
> + VXLAN_F_UDP_ZERO_CSUM6_TX, changelink,
> + false, extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]))
> - conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_RX;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_UDP_ZERO_CSUM6_RX,
> + VXLAN_F_UDP_ZERO_CSUM6_RX, changelink,
> + false, extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_REMCSUM_TX]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_TX]))
> - conf->flags |= VXLAN_F_REMCSUM_TX;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_TX,
> + VXLAN_F_REMCSUM_TX, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_REMCSUM_RX]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_RX]))
> - conf->flags |= VXLAN_F_REMCSUM_RX;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_RX,
> + VXLAN_F_REMCSUM_RX, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_GBP]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - conf->flags |= VXLAN_F_GBP;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_GBP,
> + VXLAN_F_GBP, changelink, false, extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_GPE]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - conf->flags |= VXLAN_F_GPE;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_GPE,
> + VXLAN_F_GPE, changelink, false,
> + extack);
> + if (err)
> + return err;
> }
>
> if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> - conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
> + err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_NOPARTIAL,
> + VXLAN_F_REMCSUM_NOPARTIAL, changelink,
> + false, extack);
> + if (err)
> + return err;
> }
>
> if (tb[IFLA_MTU]) {
> - if (changelink)
> + if (changelink) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_MTU],
> + "Cannot change mtu");
> return -EOPNOTSUPP;
> + }
> conf->mtu = nla_get_u32(tb[IFLA_MTU]);
> }
>
> @@ -3800,7 +3875,7 @@ static int vxlan_newlink(struct net *src_net, struct net_device *dev,
> struct vxlan_config conf;
> int err;
>
> - err = vxlan_nl2conf(tb, data, dev, &conf, false);
> + err = vxlan_nl2conf(tb, data, dev, &conf, false, extack);
> if (err)
> return err;
>
> @@ -3817,8 +3892,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> struct vxlan_config conf;
> int err;
>
> - err = vxlan_nl2conf(tb, data,
> - dev, &conf, true);
> + err = vxlan_nl2conf(tb, data, dev, &conf, true, extack);
> if (err)
> return err;
>
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 0976781..00254a5 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -453,4 +453,35 @@ vxlan_fdb_clear_offload(const struct net_device *dev, __be32 vni)
> }
> #endif
>
> +static inline void vxlan_flag_attr_error(int attrtype,
> + struct netlink_ext_ack *extack)
> +{
> +#define VXLAN_FLAG(flg) \
> + case IFLA_VXLAN_##flg: \
> + NL_SET_ERR_MSG_MOD(extack, \
> + "cannot change " #flg " flag"); \
> + break
> + switch (attrtype) {
> + VXLAN_FLAG(TTL_INHERIT);
> + VXLAN_FLAG(LEARNING);
> + VXLAN_FLAG(PROXY);
> + VXLAN_FLAG(RSC);
> + VXLAN_FLAG(L2MISS);
> + VXLAN_FLAG(L3MISS);
> + VXLAN_FLAG(COLLECT_METADATA);
> + VXLAN_FLAG(UDP_ZERO_CSUM6_TX);
> + VXLAN_FLAG(UDP_ZERO_CSUM6_RX);
> + VXLAN_FLAG(REMCSUM_TX);
> + VXLAN_FLAG(REMCSUM_RX);
> + VXLAN_FLAG(GBP);
> + VXLAN_FLAG(GPE);
> + VXLAN_FLAG(REMCSUM_NOPARTIAL);
> + default:
> + NL_SET_ERR_MSG_MOD(extack, \
> + "cannot change flag");
> + break;
> + }
> +#undef VXLAN_FLAG
> +}
> +
> #endif
next prev parent reply other threads:[~2019-02-26 13:51 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-26 6:03 [PATCH net-next 0/2] vxlan: create and changelink extack support Roopa Prabhu
2019-02-26 6:03 ` [PATCH net-next 1/2] vxlan: add extack support for create and changelink Roopa Prabhu
2019-02-26 13:51 ` Petr Machata [this message]
2019-02-26 14:06 ` Roopa Prabhu
2019-02-26 17:45 ` Petr Machata
2019-02-27 1:05 ` Roopa Prabhu
2019-02-26 6:03 ` [PATCH net-next 2/2] tools: selftests: rtnetlink: add testcases for vxlan flag sets Roopa Prabhu
2019-02-26 17:01 ` [PATCH net-next 0/2] vxlan: create and changelink extack support David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87imx6fyk1.fsf@mellanox.com \
--to=petrm@mellanox.com \
--cc=davem@davemloft.net \
--cc=dsa@cumulusnetworks.com \
--cc=johannes@sipsolutions.net \
--cc=netdev@vger.kernel.org \
--cc=roopa@cumulusnetworks.com \
--cc=sd@queasysnail.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).