[PATCH] netfilter: nft_meta: Extend support for NFT_META_TSTAMP_NS

[PATCH] netfilter: nft_meta: Extend support for NFT_META_TSTAMP_NS

[Karuna Grewal]

Meta evaluation function is extended to suport NFT_META_TSTAMP_NS option
by exposing the 64 bit timestamp of the packet to two 32 bit registers.

Signed-off-by: Karuna Grewal <karunagrewal98@gmail.com>
---
 include/uapi/linux/netfilter/nf_tables.h | 2 ++
 net/netfilter/nft_meta.c                 | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index a66c8de006cc..61f8f604c614 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -793,6 +793,7 @@ enum nft_exthdr_attributes {
  * @NFT_META_SECPATH: boolean, secpath_exists (!!skb->sp)
  * @NFT_META_IIFKIND: packet input interface kind name (dev->rtnl_link_ops->kind)
  * @NFT_META_OIFKIND: packet output interface kind name (dev->rtnl_link_ops->kind)
+ * @NFT_META_TSTAMP_NS: packet arriavl time (skb->tstamp)
  */
 enum nft_meta_keys {
 	NFT_META_LEN,
@@ -823,6 +824,7 @@ enum nft_meta_keys {
 	NFT_META_SECPATH,
 	NFT_META_IIFKIND,
 	NFT_META_OIFKIND,
+	NFT_META_TSTAMP_NS
 };
 
 /**
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 987d2d6ce624..4eeda9ae8369 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -254,6 +254,13 @@ void nft_meta_get_eval(const struct nft_expr *expr,
 			goto err;
 		strncpy((char *)dest, out->rtnl_link_ops->kind, IFNAMSIZ);
 		break;
+	case NFT_META_TSTAMP_NS:
+		if (skb->tstamp == 0)
+			__net_timestamp((struct sk_buff *)skb);
+		u64 timestamp = ktime_to_ns(skb->tstamp);
+		*dest = (u32) timestamp >> 32;
+		*(dest + 1) = (u32) timestamp;
+		break;
 	default:
 		WARN_ON(1);
 		goto err;
@@ -371,6 +378,8 @@ static int nft_meta_get_init(const struct nft_ctx *ctx,
 		len = IFNAMSIZ;
 		break;
 #endif
+	case NFT_META_TSTAMP_NS:
+		len = sizeof(u64);
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.17.1

Re: [PATCH] netfilter: nft_meta: Extend support for NFT_META_TSTAMP_NS

[Karuna Grewal]

On Fri, Mar 1, 2019 at 2:54 PM Karuna Grewal <karunagrewal98@gmail.com> wrote:
>
> Meta evaluation function is extended to suport NFT_META_TSTAMP_NS option
> by exposing the 64 bit timestamp of the packet to two 32 bit registers.
> + * @NFT_META_TSTAMP_NS: packet arriavl time (skb->tstamp)
>   */
Please disregard this patch due to the trivial mistake which I've
fixed in the latest version.

Re: [PATCH] netfilter: nft_meta: Extend support for NFT_META_TSTAMP_NS

[Dan Carpenter]

Hi Karuna,

Thank you for the patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Karuna-Grewal/netfilter-nft_meta-Extend-support-for-NFT_META_TSTAMP_NS/20190301-182752
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git master

smatch warnings:
net/netfilter/nft_meta.c:261 nft_meta_get_eval() warn: right shifting more than type allows 32 vs 32

# https://github.com/0day-ci/linux/commit/02c5bd91b2cb1485eeb226b93c885aaa3f7a2a39
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 02c5bd91b2cb1485eeb226b93c885aaa3f7a2a39
vim +261 net/netfilter/nft_meta.c

0fb4d2195 wenxu             2019-01-16  247  	case NFT_META_IIFKIND:
0fb4d2195 wenxu             2019-01-16  248  		if (in == NULL || in->rtnl_link_ops == NULL)
0fb4d2195 wenxu             2019-01-16  249  			goto err;
0fb4d2195 wenxu             2019-01-16  250  		strncpy((char *)dest, in->rtnl_link_ops->kind, IFNAMSIZ);
0fb4d2195 wenxu             2019-01-16  251  		break;
0fb4d2195 wenxu             2019-01-16  252  	case NFT_META_OIFKIND:
0fb4d2195 wenxu             2019-01-16  253  		if (out == NULL || out->rtnl_link_ops == NULL)
0fb4d2195 wenxu             2019-01-16  254  			goto err;
0fb4d2195 wenxu             2019-01-16  255  		strncpy((char *)dest, out->rtnl_link_ops->kind, IFNAMSIZ);
0fb4d2195 wenxu             2019-01-16  256  		break;
02c5bd91b Karuna Grewal     2019-03-01  257  	case NFT_META_TSTAMP_NS:
02c5bd91b Karuna Grewal     2019-03-01  258  		if (skb->tstamp == 0)
02c5bd91b Karuna Grewal     2019-03-01  259  			__net_timestamp((struct sk_buff *)skb);
02c5bd91b Karuna Grewal     2019-03-01  260  		u64 timestamp = ktime_to_ns(skb->tstamp);
02c5bd91b Karuna Grewal     2019-03-01 @261  		*dest = (u32) timestamp >> 32;
                                                                ^^^^^^^^^^^^^^^
Missing parentheses.  Generally, kernel style is to not put a space
after a cast, because casting is a high precedence operation.

02c5bd91b Karuna Grewal     2019-03-01  262  		*(dest + 1) = (u32) timestamp;
02c5bd91b Karuna Grewal     2019-03-01  263  		break;
96518518c Patrick McHardy   2013-10-14  264  	default:
96518518c Patrick McHardy   2013-10-14  265  		WARN_ON(1);
96518518c Patrick McHardy   2013-10-14  266  		goto err;
96518518c Patrick McHardy   2013-10-14  267  	}
96518518c Patrick McHardy   2013-10-14  268  	return;
96518518c Patrick McHardy   2013-10-14  269  
96518518c Patrick McHardy   2013-10-14  270  err:
a55e22e92 Patrick McHardy   2015-04-11  271  	regs->verdict.code = NFT_BREAK;
96518518c Patrick McHardy   2013-10-14  272  }
96518518c Patrick McHardy   2013-10-14  273  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] netfilter: nft_meta: Extend support for NFT_META_TSTAMP_NS

[Karuna Grewal]

On Wed, Mar 20, 2019 at 11:16 PM Karuna Grewal <karunagrewal98@gmail.com> wrote:
>
> On Tue, Mar 5, 2019 at 3:42 PM Florian Westphal <fw@strlen.de> wrote:
> >
> > Karuna Grewal <karunagrewal98@gmail.com> wrote:
> > > I've a doubt in the nftables implementation for implementing the `-m
> > > time` support.
> >
> > Full -m time is complicated, do not worry about this yet.
> >
> > > I'm unable to get a proper idea of where the start and
> > > stop time comparison with the dest register's value takes place.
> >
> > Via the nftables evaluation loop.  You only need to worry about placing
> > the value (timestamp) in the dst register (on kernel side), so your
> > patch looks pretty complete aside from the missing 'break'.
> >
> > Its userspace (nftables) responsibility to tell kernel to do something
> > with the register, such as a compare or range.
> >
> > Have a look at
> > http://git.netfilter.org/nftables/commit/src/meta.c?id=512795a673f999fb04b84dbbbe41174e9c581430
> >
> > It should be enough to follow this approach, adding e.g.
> > META_TEMPLATE("timestamp", ..
> >
> > we have TYPE_TIME already, even though its a relative one, it
> > would/should work for a quick prototype.
>
> I had sent the patch for this basic case of simply comparing one timestamp.
> As the startdate and stopdate options need to implemented completely,
> should I make use of an interval
> or is using two different tokens viz. START_TSTAMP and STOP_TSTAMP the
> preferred option.
Sorry, I had mistaken the way intervals were scanned in the rule. This
above doubt is cleared.
Moving on to the part of not using relative times would it be required
to first implement it in the nftables datatypes or is there another
available workaround using the existing nftables code?

Re: [PATCH] netfilter: nft_meta: Extend support for NFT_META_TSTAMP_NS

[Karuna Grewal]

On Tue, Mar 5, 2019 at 3:42 PM Florian Westphal <fw@strlen.de> wrote:
>
> Karuna Grewal <karunagrewal98@gmail.com> wrote:
> > I've a doubt in the nftables implementation for implementing the `-m
> > time` support.
>
> Full -m time is complicated, do not worry about this yet.
>
> > I'm unable to get a proper idea of where the start and
> > stop time comparison with the dest register's value takes place.
>
> Via the nftables evaluation loop.  You only need to worry about placing
> the value (timestamp) in the dst register (on kernel side), so your
> patch looks pretty complete aside from the missing 'break'.
>
> Its userspace (nftables) responsibility to tell kernel to do something
> with the register, such as a compare or range.
>
> Have a look at
> http://git.netfilter.org/nftables/commit/src/meta.c?id=512795a673f999fb04b84dbbbe41174e9c581430
>
> It should be enough to follow this approach, adding e.g.
> META_TEMPLATE("timestamp", ..
>
> we have TYPE_TIME already, even though its a relative one, it
> would/should work for a quick prototype.

I had sent the patch for this basic case of simply comparing one timestamp.
As the startdate and stopdate options need to implemented completely,
should I make use of an interval
or is using two different tokens viz. START_TSTAMP and STOP_TSTAMP the
preferred option.

Re: [PATCH] netfilter: nft_meta: Extend support for NFT_META_TSTAMP_NS

[Florian Westphal]

Karuna Grewal <karunagrewal98@gmail.com> wrote:
> I've a doubt in the nftables implementation for implementing the `-m
> time` support.

Full -m time is complicated, do not worry about this yet.

> I'm unable to get a proper idea of where the start and
> stop time comparison with the dest register's value takes place.

Via the nftables evaluation loop.  You only need to worry about placing
the value (timestamp) in the dst register (on kernel side), so your
patch looks pretty complete aside from the missing 'break'.

Its userspace (nftables) responsibility to tell kernel to do something
with the register, such as a compare or range.

Have a look at
http://git.netfilter.org/nftables/commit/src/meta.c?id=512795a673f999fb04b84dbbbe41174e9c581430

It should be enough to follow this approach, adding e.g.
META_TEMPLATE("timestamp", ..

we have TYPE_TIME already, even though its a relative one, it
would/should work for a quick prototype.

> From my understanding of implementation, I've noticed that after
> parsing the rule and the meta expression is allocated, expression's
> primary evaluation function is invoked.
> Meanwhile, the kernel has the nft_meta_get_eval  function setting the
> register with the relevant field and in nf_tables_core.h  the
> registering of different nft_expr_types is accomplished.
> Could someone please give me some pointers about where this processing
> of the data set in the meta registers takes place?

For instance in nft_cmp.c, but thats just one possibility, there
are other consumers, such as nft_range.c, nft_lookup.c, and so on.

Re: [PATCH] netfilter: nft_meta: Extend support for NFT_META_TSTAMP_NS

[Karuna Grewal]

On Fri, Mar 1, 2019 at 4:30 PM Florian Westphal <fw@strlen.de> wrote:
>
> Karuna Grewal <karunagrewal98@gmail.com> wrote:
> > Meta evaluation function is extended to suport NFT_META_TSTAMP_NS option
> > by exposing the 64 bit timestamp of the packet to two 32 bit registers.

> Other than this, this patch looks good.  Please consider sending v3 once
> you have nftables patches ready as well.
I've a doubt in the nftables implementation for implementing the `-m
time` support. I'm unable to get a proper idea of where the start and
stop time comparison with the dest register's value takes place.
From my understanding of implementation, I've noticed that after
parsing the rule and the meta expression is allocated, expression's
primary evaluation function is invoked.
Meanwhile, the kernel has the nft_meta_get_eval  function setting the
register with the relevant field and in nf_tables_core.h  the
registering of different nft_expr_types is accomplished.
Could someone please give me some pointers about where this processing
of the data set in the meta registers takes place?
Thanks
Karuna

Re: [PATCH] netfilter: nft_meta: Extend support for NFT_META_TSTAMP_NS

[Florian Westphal]

Karuna Grewal <karunagrewal98@gmail.com> wrote:
> Meta evaluation function is extended to suport NFT_META_TSTAMP_NS option
> by exposing the 64 bit timestamp of the packet to two 32 bit registers.

Please test patches first, see below.

> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index 987d2d6ce624..adfa1f221946 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -254,6 +254,13 @@ void nft_meta_get_eval(const struct nft_expr *expr,
>  			goto err;
>  		strncpy((char *)dest, out->rtnl_link_ops->kind, IFNAMSIZ);
>  		break;
> +	case NFT_META_TSTAMP_NS:
> +		if (skb->tstamp == 0)
> +			__net_timestamp((struct sk_buff *)skb);
> +		u64 timestamp = ktime_to_ns(skb->tstamp);
> +		*dest = (u32)(timestamp >> 32);
> +		*(dest + 1) = (u32) timestamp;
> +		break;

Please consider using u64 *dest64 or similar to avoid the need for
shifting.

>  	default:
>  		WARN_ON(1);
>  		goto err;
> @@ -371,6 +378,8 @@ static int nft_meta_get_init(const struct nft_ctx *ctx,
>  		len = IFNAMSIZ;
>  		break;
>  #endif
> +	case NFT_META_TSTAMP_NS:
> +		len = sizeof(u64);
>  	default:
>  		return -EOPNOTSUPP;

This is missing a break statement after len assignment.

Other than this, this patch looks good.  Please consider sending v3 once
you have nftables patches ready as well.

[PATCH] netfilter: nft_meta: Extend support for NFT_META_TSTAMP_NS

[Karuna Grewal]

Meta evaluation function is extended to suport NFT_META_TSTAMP_NS option
by exposing the 64 bit timestamp of the packet to two 32 bit registers.

Signed-off-by: Karuna Grewal <karunagrewal98@gmail.com>
---
 include/uapi/linux/netfilter/nf_tables.h | 2 ++
 net/netfilter/nft_meta.c                 | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index a66c8de006cc..61f8f604c614 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -793,6 +793,7 @@ enum nft_exthdr_attributes {
  * @NFT_META_SECPATH: boolean, secpath_exists (!!skb->sp)
  * @NFT_META_IIFKIND: packet input interface kind name (dev->rtnl_link_ops->kind)
  * @NFT_META_OIFKIND: packet output interface kind name (dev->rtnl_link_ops->kind)
+ * @NFT_META_TSTAMP_NS: packet arrival time (skb->tstamp)
  */
 enum nft_meta_keys {
 	NFT_META_LEN,
@@ -823,6 +824,7 @@ enum nft_meta_keys {
 	NFT_META_SECPATH,
 	NFT_META_IIFKIND,
 	NFT_META_OIFKIND,
+	NFT_META_TSTAMP_NS
 };

 /**
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 987d2d6ce624..adfa1f221946 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -254,6 +254,13 @@ void nft_meta_get_eval(const struct nft_expr *expr,
 			goto err;
 		strncpy((char *)dest, out->rtnl_link_ops->kind, IFNAMSIZ);
 		break;
+	case NFT_META_TSTAMP_NS:
+		if (skb->tstamp == 0)
+			__net_timestamp((struct sk_buff *)skb);
+		u64 timestamp = ktime_to_ns(skb->tstamp);
+		*dest = (u32)(timestamp >> 32);
+		*(dest + 1) = (u32) timestamp;
+		break;
 	default:
 		WARN_ON(1);
 		goto err;
@@ -371,6 +378,8 @@ static int nft_meta_get_init(const struct nft_ctx *ctx,
 		len = IFNAMSIZ;
 		break;
 #endif
+	case NFT_META_TSTAMP_NS:
+		len = sizeof(u64);
 	default:
 		return -EOPNOTSUPP;
 	}
--
2.17.1