[PATCHv2 nf-next 0/5] netfilter: nft_tunnel: a bunch of fixes and improvements

[PATCHv2 nf-next 0/5] netfilter: nft_tunnel: a bunch of fixes and improvements

[Xin Long]

This patchset adds some fixes and improvements for nft_tunnel.

Note the patch for adding support for geneve opts in nft_tunnel
will be posted in another patch after this one.

Xin Long (5):
  netfilter: nft_tunnel: no need to call htons() when dumping ports
  netfilter: nft_tunnel: add the missing ERSPAN_VERSION nla_policy
  netfilter: nft_tunnel: also dump ERSPAN_VERSION
  netfilter: nft_tunnel: also dump OPTS_ERSPAN/VXLAN
  netfilter: nft_tunnel: add the missing nla_nest_cancel()

 net/netfilter/nft_tunnel.c | 52 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 13 deletions(-)

-- 
2.1.0

[PATCHv2 nf-next 1/5] netfilter: nft_tunnel: no need to call htons() when dumping ports

[Xin Long]

info->key.tp_src and tp_dst are __be16, when using nla_put_be16()
to dump them, htons() is not needed, so remove it in this patch.

v1->v2:
  - add Fixes tag.

Fixes: af308b94a2a4 ("netfilter: nf_tables: add tunnel support")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_tunnel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nft_tunnel.c b/net/netfilter/nft_tunnel.c
index 3d4c2ae..ef2065dd 100644
--- a/net/netfilter/nft_tunnel.c
+++ b/net/netfilter/nft_tunnel.c
@@ -501,8 +501,8 @@ static int nft_tunnel_opts_dump(struct sk_buff *skb,
 static int nft_tunnel_ports_dump(struct sk_buff *skb,
 				 struct ip_tunnel_info *info)
 {
-	if (nla_put_be16(skb, NFTA_TUNNEL_KEY_SPORT, htons(info->key.tp_src)) < 0 ||
-	    nla_put_be16(skb, NFTA_TUNNEL_KEY_DPORT, htons(info->key.tp_dst)) < 0)
+	if (nla_put_be16(skb, NFTA_TUNNEL_KEY_SPORT, info->key.tp_src) < 0 ||
+	    nla_put_be16(skb, NFTA_TUNNEL_KEY_DPORT, info->key.tp_dst) < 0)
 		return -1;
 
 	return 0;
-- 
2.1.0

Re: [PATCHv2 nf-next 1/5] netfilter: nft_tunnel: no need to call htons() when dumping ports

[Simon Horman]

On Fri, Dec 13, 2019 at 04:53:05PM +0800, Xin Long wrote:
> info->key.tp_src and tp_dst are __be16, when using nla_put_be16()
> to dump them, htons() is not needed, so remove it in this patch.
> 
> v1->v2:
>   - add Fixes tag.
> 
> Fixes: af308b94a2a4 ("netfilter: nf_tables: add tunnel support")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Reviewed-by: Simon Horman <simon.horman@netronome.com>

> ---
>  net/netfilter/nft_tunnel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/nft_tunnel.c b/net/netfilter/nft_tunnel.c
> index 3d4c2ae..ef2065dd 100644
> --- a/net/netfilter/nft_tunnel.c
> +++ b/net/netfilter/nft_tunnel.c
> @@ -501,8 +501,8 @@ static int nft_tunnel_opts_dump(struct sk_buff *skb,
>  static int nft_tunnel_ports_dump(struct sk_buff *skb,
>  				 struct ip_tunnel_info *info)
>  {
> -	if (nla_put_be16(skb, NFTA_TUNNEL_KEY_SPORT, htons(info->key.tp_src)) < 0 ||
> -	    nla_put_be16(skb, NFTA_TUNNEL_KEY_DPORT, htons(info->key.tp_dst)) < 0)
> +	if (nla_put_be16(skb, NFTA_TUNNEL_KEY_SPORT, info->key.tp_src) < 0 ||
> +	    nla_put_be16(skb, NFTA_TUNNEL_KEY_DPORT, info->key.tp_dst) < 0)
>  		return -1;
>  
>  	return 0;
> -- 
> 2.1.0
> 

[PATCHv2 nf-next 2/5] netfilter: nft_tunnel: add the missing ERSPAN_VERSION nla_policy

[Xin Long]

ERSPAN_VERSION is an attribute parsed in kernel side, nla_policy
type should be added for it, like other attributes.

Fixes: af308b94a2a4 ("netfilter: nf_tables: add tunnel support")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_tunnel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nft_tunnel.c b/net/netfilter/nft_tunnel.c
index ef2065dd..6538895 100644
--- a/net/netfilter/nft_tunnel.c
+++ b/net/netfilter/nft_tunnel.c
@@ -248,8 +248,9 @@ static int nft_tunnel_obj_vxlan_init(const struct nlattr *attr,
 }
 
 static const struct nla_policy nft_tunnel_opts_erspan_policy[NFTA_TUNNEL_KEY_ERSPAN_MAX + 1] = {
+	[NFTA_TUNNEL_KEY_ERSPAN_VERSION]	= { .type = NLA_U32 },
 	[NFTA_TUNNEL_KEY_ERSPAN_V1_INDEX]	= { .type = NLA_U32 },
-	[NFTA_TUNNEL_KEY_ERSPAN_V2_DIR]	= { .type = NLA_U8 },
+	[NFTA_TUNNEL_KEY_ERSPAN_V2_DIR]		= { .type = NLA_U8 },
 	[NFTA_TUNNEL_KEY_ERSPAN_V2_HWID]	= { .type = NLA_U8 },
 };
 
-- 
2.1.0

Re: [PATCHv2 nf-next 2/5] netfilter: nft_tunnel: add the missing ERSPAN_VERSION nla_policy

[Simon Horman]

On Fri, Dec 13, 2019 at 04:53:06PM +0800, Xin Long wrote:
> ERSPAN_VERSION is an attribute parsed in kernel side, nla_policy
> type should be added for it, like other attributes.
> 
> Fixes: af308b94a2a4 ("netfilter: nf_tables: add tunnel support")

Is this really a fix?

> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Reviewed-by: Simon Horman <simon.horman@netronome.com>

> ---
>  net/netfilter/nft_tunnel.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nft_tunnel.c b/net/netfilter/nft_tunnel.c
> index ef2065dd..6538895 100644
> --- a/net/netfilter/nft_tunnel.c
> +++ b/net/netfilter/nft_tunnel.c
> @@ -248,8 +248,9 @@ static int nft_tunnel_obj_vxlan_init(const struct nlattr *attr,
>  }
>  
>  static const struct nla_policy nft_tunnel_opts_erspan_policy[NFTA_TUNNEL_KEY_ERSPAN_MAX + 1] = {
> +	[NFTA_TUNNEL_KEY_ERSPAN_VERSION]	= { .type = NLA_U32 },
>  	[NFTA_TUNNEL_KEY_ERSPAN_V1_INDEX]	= { .type = NLA_U32 },
> -	[NFTA_TUNNEL_KEY_ERSPAN_V2_DIR]	= { .type = NLA_U8 },
> +	[NFTA_TUNNEL_KEY_ERSPAN_V2_DIR]		= { .type = NLA_U8 },
>  	[NFTA_TUNNEL_KEY_ERSPAN_V2_HWID]	= { .type = NLA_U8 },
>  };
>  
> -- 
> 2.1.0
> 

Re: [PATCHv2 nf-next 2/5] netfilter: nft_tunnel: add the missing ERSPAN_VERSION nla_policy

[Pablo Neira Ayuso]

On Sat, Dec 14, 2019 at 09:26:31AM +0100, Simon Horman wrote:
> On Fri, Dec 13, 2019 at 04:53:06PM +0800, Xin Long wrote:
> > ERSPAN_VERSION is an attribute parsed in kernel side, nla_policy
> > type should be added for it, like other attributes.
> > 
> > Fixes: af308b94a2a4 ("netfilter: nf_tables: add tunnel support")
> 
> Is this really a fix?

I think so. Netlink attribute validation for
NFTA_TUNNEL_KEY_ERSPAN_VERSION is missing.

Re: [PATCHv2 nf-next 2/5] netfilter: nft_tunnel: add the missing ERSPAN_VERSION nla_policy

[Simon Horman]

On Tue, Dec 17, 2019 at 10:39:45PM +0100, Pablo Neira Ayuso wrote:
> On Sat, Dec 14, 2019 at 09:26:31AM +0100, Simon Horman wrote:
> > On Fri, Dec 13, 2019 at 04:53:06PM +0800, Xin Long wrote:
> > > ERSPAN_VERSION is an attribute parsed in kernel side, nla_policy
> > > type should be added for it, like other attributes.
> > > 
> > > Fixes: af308b94a2a4 ("netfilter: nf_tables: add tunnel support")
> > 
> > Is this really a fix?
> 
> I think so. Netlink attribute validation for
> NFTA_TUNNEL_KEY_ERSPAN_VERSION is missing.

Ok, I accept that reasoning.

[PATCHv2 nf-next 3/5] netfilter: nft_tunnel: also dump ERSPAN_VERSION

[Xin Long]

This is not necessary, but it'll be easier to parse in userspace,
also given that other places like act_tunnel_key, cls_flower and
ip_tunnel_core are also doing so.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_tunnel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/nft_tunnel.c b/net/netfilter/nft_tunnel.c
index 6538895..b3a9b10 100644
--- a/net/netfilter/nft_tunnel.c
+++ b/net/netfilter/nft_tunnel.c
@@ -479,6 +479,9 @@ static int nft_tunnel_opts_dump(struct sk_buff *skb,
 				 htonl(opts->u.vxlan.gbp)))
 			return -1;
 	} else if (opts->flags & TUNNEL_ERSPAN_OPT) {
+		if (nla_put_be32(skb, NFTA_TUNNEL_KEY_ERSPAN_VERSION,
+				 htonl(opts->u.erspan.version)))
+			return -1;
 		switch (opts->u.erspan.version) {
 		case ERSPAN_VERSION:
 			if (nla_put_be32(skb, NFTA_TUNNEL_KEY_ERSPAN_V1_INDEX,
-- 
2.1.0

Re: [PATCHv2 nf-next 3/5] netfilter: nft_tunnel: also dump ERSPAN_VERSION

[Simon Horman]

On Fri, Dec 13, 2019 at 04:53:07PM +0800, Xin Long wrote:
> This is not necessary, but it'll be easier to parse in userspace,
> also given that other places like act_tunnel_key, cls_flower and
> ip_tunnel_core are also doing so.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Reviewed-by: Simon Horman <simon.horman@netronome.com>

> ---
>  net/netfilter/nft_tunnel.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/netfilter/nft_tunnel.c b/net/netfilter/nft_tunnel.c
> index 6538895..b3a9b10 100644
> --- a/net/netfilter/nft_tunnel.c
> +++ b/net/netfilter/nft_tunnel.c
> @@ -479,6 +479,9 @@ static int nft_tunnel_opts_dump(struct sk_buff *skb,
>  				 htonl(opts->u.vxlan.gbp)))
>  			return -1;
>  	} else if (opts->flags & TUNNEL_ERSPAN_OPT) {
> +		if (nla_put_be32(skb, NFTA_TUNNEL_KEY_ERSPAN_VERSION,
> +				 htonl(opts->u.erspan.version)))
> +			return -1;
>  		switch (opts->u.erspan.version) {
>  		case ERSPAN_VERSION:
>  			if (nla_put_be32(skb, NFTA_TUNNEL_KEY_ERSPAN_V1_INDEX,
> -- 
> 2.1.0
> 

[PATCHv2 nf-next 4/5] netfilter: nft_tunnel: also dump OPTS_ERSPAN/VXLAN

[Xin Long]

This patch is to add the nest attr OPTS_ERSPAN/VXLAN when dumping
KEY_OPTS, and it would be helpful when parsing in userpace. Also,
this is needed for supporting multiple geneve opts in the future
patches.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_tunnel.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nft_tunnel.c b/net/netfilter/nft_tunnel.c
index b3a9b10..eb17402 100644
--- a/net/netfilter/nft_tunnel.c
+++ b/net/netfilter/nft_tunnel.c
@@ -468,17 +468,24 @@ static int nft_tunnel_opts_dump(struct sk_buff *skb,
 				struct nft_tunnel_obj *priv)
 {
 	struct nft_tunnel_opts *opts = &priv->opts;
-	struct nlattr *nest;
+	struct nlattr *nest, *inner;
 
 	nest = nla_nest_start_noflag(skb, NFTA_TUNNEL_KEY_OPTS);
 	if (!nest)
 		return -1;
 
 	if (opts->flags & TUNNEL_VXLAN_OPT) {
+		inner = nla_nest_start_noflag(skb, NFTA_TUNNEL_KEY_OPTS_VXLAN);
+		if (!inner)
+			return -1;
 		if (nla_put_be32(skb, NFTA_TUNNEL_KEY_VXLAN_GBP,
 				 htonl(opts->u.vxlan.gbp)))
 			return -1;
+		nla_nest_end(skb, inner);
 	} else if (opts->flags & TUNNEL_ERSPAN_OPT) {
+		inner = nla_nest_start_noflag(skb, NFTA_TUNNEL_KEY_OPTS_ERSPAN);
+		if (!inner)
+			return -1;
 		if (nla_put_be32(skb, NFTA_TUNNEL_KEY_ERSPAN_VERSION,
 				 htonl(opts->u.erspan.version)))
 			return -1;
@@ -496,6 +503,7 @@ static int nft_tunnel_opts_dump(struct sk_buff *skb,
 				return -1;
 			break;
 		}
+		nla_nest_end(skb, inner);
 	}
 	nla_nest_end(skb, nest);
 
-- 
2.1.0

Re: [PATCHv2 nf-next 4/5] netfilter: nft_tunnel: also dump OPTS_ERSPAN/VXLAN

[Simon Horman]

On Fri, Dec 13, 2019 at 04:53:08PM +0800, Xin Long wrote:
> This patch is to add the nest attr OPTS_ERSPAN/VXLAN when dumping
> KEY_OPTS, and it would be helpful when parsing in userpace. Also,
> this is needed for supporting multiple geneve opts in the future
> patches.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Reviewed-by: Simon Horman <simon.horman@netronome.com>

[PATCHv2 nf-next 5/5] netfilter: nft_tunnel: add the missing nla_nest_cancel()

[Xin Long]

When nla_put_xxx() fails under nla_nest_start_noflag(),
nla_nest_cancel() should be called, so that the skb can
be trimmed properly.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_tunnel.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/net/netfilter/nft_tunnel.c b/net/netfilter/nft_tunnel.c
index eb17402..23cd163 100644
--- a/net/netfilter/nft_tunnel.c
+++ b/net/netfilter/nft_tunnel.c
@@ -443,10 +443,15 @@ static int nft_tunnel_ip_dump(struct sk_buff *skb, struct ip_tunnel_info *info)
 		if (!nest)
 			return -1;
 
-		if (nla_put_in6_addr(skb, NFTA_TUNNEL_KEY_IP6_SRC, &info->key.u.ipv6.src) < 0 ||
-		    nla_put_in6_addr(skb, NFTA_TUNNEL_KEY_IP6_DST, &info->key.u.ipv6.dst) < 0 ||
-		    nla_put_be32(skb, NFTA_TUNNEL_KEY_IP6_FLOWLABEL, info->key.label))
+		if (nla_put_in6_addr(skb, NFTA_TUNNEL_KEY_IP6_SRC,
+				     &info->key.u.ipv6.src) < 0 ||
+		    nla_put_in6_addr(skb, NFTA_TUNNEL_KEY_IP6_DST,
+				     &info->key.u.ipv6.dst) < 0 ||
+		    nla_put_be32(skb, NFTA_TUNNEL_KEY_IP6_FLOWLABEL,
+				 info->key.label)) {
+			nla_nest_cancel(skb, nest);
 			return -1;
+		}
 
 		nla_nest_end(skb, nest);
 	} else {
@@ -454,9 +459,13 @@ static int nft_tunnel_ip_dump(struct sk_buff *skb, struct ip_tunnel_info *info)
 		if (!nest)
 			return -1;
 
-		if (nla_put_in_addr(skb, NFTA_TUNNEL_KEY_IP_SRC, info->key.u.ipv4.src) < 0 ||
-		    nla_put_in_addr(skb, NFTA_TUNNEL_KEY_IP_DST, info->key.u.ipv4.dst) < 0)
+		if (nla_put_in_addr(skb, NFTA_TUNNEL_KEY_IP_SRC,
+				    info->key.u.ipv4.src) < 0 ||
+		    nla_put_in_addr(skb, NFTA_TUNNEL_KEY_IP_DST,
+				    info->key.u.ipv4.dst) < 0) {
+			nla_nest_cancel(skb, nest);
 			return -1;
+		}
 
 		nla_nest_end(skb, nest);
 	}
@@ -477,37 +486,42 @@ static int nft_tunnel_opts_dump(struct sk_buff *skb,
 	if (opts->flags & TUNNEL_VXLAN_OPT) {
 		inner = nla_nest_start_noflag(skb, NFTA_TUNNEL_KEY_OPTS_VXLAN);
 		if (!inner)
-			return -1;
+			goto failure;
 		if (nla_put_be32(skb, NFTA_TUNNEL_KEY_VXLAN_GBP,
 				 htonl(opts->u.vxlan.gbp)))
-			return -1;
+			goto inner_failure;
 		nla_nest_end(skb, inner);
 	} else if (opts->flags & TUNNEL_ERSPAN_OPT) {
 		inner = nla_nest_start_noflag(skb, NFTA_TUNNEL_KEY_OPTS_ERSPAN);
 		if (!inner)
-			return -1;
+			goto failure;
 		if (nla_put_be32(skb, NFTA_TUNNEL_KEY_ERSPAN_VERSION,
 				 htonl(opts->u.erspan.version)))
-			return -1;
+			goto inner_failure;
 		switch (opts->u.erspan.version) {
 		case ERSPAN_VERSION:
 			if (nla_put_be32(skb, NFTA_TUNNEL_KEY_ERSPAN_V1_INDEX,
 					 opts->u.erspan.u.index))
-				return -1;
+				goto inner_failure;
 			break;
 		case ERSPAN_VERSION2:
 			if (nla_put_u8(skb, NFTA_TUNNEL_KEY_ERSPAN_V2_HWID,
 				       get_hwid(&opts->u.erspan.u.md2)) ||
 			    nla_put_u8(skb, NFTA_TUNNEL_KEY_ERSPAN_V2_DIR,
 				       opts->u.erspan.u.md2.dir))
-				return -1;
+				goto inner_failure;
 			break;
 		}
 		nla_nest_end(skb, inner);
 	}
 	nla_nest_end(skb, nest);
-
 	return 0;
+
+inner_failure:
+	nla_nest_cancel(skb, inner);
+failure:
+	nla_nest_cancel(skb, nest);
+	return -1;
 }
 
 static int nft_tunnel_ports_dump(struct sk_buff *skb,
-- 
2.1.0

Re: [PATCHv2 nf-next 5/5] netfilter: nft_tunnel: add the missing nla_nest_cancel()

[Simon Horman]

On Fri, Dec 13, 2019 at 04:53:09PM +0800, Xin Long wrote:
> When nla_put_xxx() fails under nla_nest_start_noflag(),
> nla_nest_cancel() should be called, so that the skb can
> be trimmed properly.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Reviewed-by: Simon Horman <simon.horman@netronome.com>

Re: [PATCHv2 nf-next 0/5] netfilter: nft_tunnel: a bunch of fixes and improvements

[Pablo Neira Ayuso]

On Fri, Dec 13, 2019 at 04:53:04PM +0800, Xin Long wrote:
> This patchset adds some fixes and improvements for nft_tunnel.
> 
> Note the patch for adding support for geneve opts in nft_tunnel
> will be posted in another patch after this one.

Series applied, thanks.