Re: [PATCH nf,v2 1/2] netfilter: nftables_offload: set address type in control dissector

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Jakub,

On Wed, Nov 25, 2020 at 03:21:47PM -0800, Jakub Kicinski wrote:
> On Thu, 26 Nov 2020 00:04:45 +0100 Pablo Neira Ayuso wrote:
> > This patch adds nft_flow_rule_set_addr_type() to set the address type
> > from the nft_payload expression accordingly.
> > 
> > If the address type is not set in the control dissector then a rule that
> > matches either source or destination IP address does not work.
> > 
> > After this patch, nft hardware offload generates the flow dissector
> > configuration as tc-flower to match on an IP address.
> > 
> > This patch has been also tested functionally to make sure packets are
> > filtered out by the NIC.
> > 
> > This is also getting the code aligned with the existing netfilter flow
> > offload infrastructure which is also setting the control dissector.
> > 
> > Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > v2: add a nice patch description and remove unnecessary EXPORT_SYMBOL()
> >     per Jakub Kicinski.
> > 
> >  include/net/netfilter/nf_tables_offload.h |  4 ++++
> >  net/netfilter/nf_tables_offload.c         | 17 +++++++++++++++++
> >  net/netfilter/nft_payload.c               |  4 ++++
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/include/net/netfilter/nf_tables_offload.h b/include/net/netfilter/nf_tables_offload.h
> > index ea7d1d78b92d..bddd34c5bd79 100644
> > --- a/include/net/netfilter/nf_tables_offload.h
> > +++ b/include/net/netfilter/nf_tables_offload.h
> > @@ -37,6 +37,7 @@ void nft_offload_update_dependency(struct nft_offload_ctx *ctx,
> >  
> >  struct nft_flow_key {
> >  	struct flow_dissector_key_basic			basic;
> > +	struct flow_dissector_key_control		control;
> >  	union {
> >  		struct flow_dissector_key_ipv4_addrs	ipv4;
> >  		struct flow_dissector_key_ipv6_addrs	ipv6;
> > @@ -62,6 +63,9 @@ struct nft_flow_rule {
> >  
> >  #define NFT_OFFLOAD_F_ACTION	(1 << 0)
> >  
> > +void nft_flow_rule_set_addr_type(struct nft_flow_rule *flow,
> > +				 enum flow_dissector_key_id addr_type);
> > +
> >  struct nft_rule;
> >  struct nft_flow_rule *nft_flow_rule_create(struct net *net, const struct nft_rule *rule);
> >  void nft_flow_rule_destroy(struct nft_flow_rule *flow);
> > diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
> > index 9f625724a20f..9ae14270c543 100644
> > --- a/net/netfilter/nf_tables_offload.c
> > +++ b/net/netfilter/nf_tables_offload.c
> > @@ -28,6 +28,23 @@ static struct nft_flow_rule *nft_flow_rule_alloc(int num_actions)
> >  	return flow;
> >  }
> >  
> > +void nft_flow_rule_set_addr_type(struct nft_flow_rule *flow,
> > +				 enum flow_dissector_key_id addr_type)
> > +{
> > +	struct nft_flow_match *match = &flow->match;
> > +	struct nft_flow_key *mask = &match->mask;
> > +	struct nft_flow_key *key = &match->key;
> > +
> > +	if (match->dissector.used_keys & BIT(FLOW_DISSECTOR_KEY_CONTROL))
> > +		return;
> > +
> > +	key->control.addr_type = addr_type;
> > +	mask->control.addr_type = 0xffff;
> > +	match->dissector.used_keys |= BIT(FLOW_DISSECTOR_KEY_CONTROL);
> > +	match->dissector.offset[FLOW_DISSECTOR_KEY_CONTROL] =
> > +		offsetof(struct nft_flow_key, control);
> > +}
> > +
> >  struct nft_flow_rule *nft_flow_rule_create(struct net *net,
> >  					   const struct nft_rule *rule)
> >  {
> > diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
> > index dcd3c7b8a367..bbf811d030d5 100644
> > --- a/net/netfilter/nft_payload.c
> > +++ b/net/netfilter/nft_payload.c
> > @@ -244,6 +244,7 @@ static int nft_payload_offload_ip(struct nft_offload_ctx *ctx,
> >  
> >  		NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_IPV4_ADDRS, ipv4, src,
> >  				  sizeof(struct in_addr), reg);
> > +		nft_flow_rule_set_addr_type(flow, FLOW_DISSECTOR_KEY_IPV4_ADDRS);
> >  		break;
> >  	case offsetof(struct iphdr, daddr):
> >  		if (priv->len != sizeof(struct in_addr))
> > @@ -251,6 +252,7 @@ static int nft_payload_offload_ip(struct nft_offload_ctx *ctx,
> >  
> >  		NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_IPV4_ADDRS, ipv4, dst,
> >  				  sizeof(struct in_addr), reg);
> > +		nft_flow_rule_set_addr_type(flow, FLOW_DISSECTOR_KEY_IPV4_ADDRS);
> >  		break;
> >  	case offsetof(struct iphdr, protocol):
> >  		if (priv->len != sizeof(__u8))
> > @@ -280,6 +282,7 @@ static int nft_payload_offload_ip6(struct nft_offload_ctx *ctx,
> >  
> >  		NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_IPV6_ADDRS, ipv6, src,
> >  				  sizeof(struct in6_addr), reg);
> > +		nft_flow_rule_set_addr_type(flow, FLOW_DISSECTOR_KEY_IPV6_ADDRS);
> >  		break;
> >  	case offsetof(struct ipv6hdr, daddr):
> >  		if (priv->len != sizeof(struct in6_addr))
> > @@ -287,6 +290,7 @@ static int nft_payload_offload_ip6(struct nft_offload_ctx *ctx,
> >  
> >  		NFT_OFFLOAD_MATCH(FLOW_DISSECTOR_KEY_IPV6_ADDRS, ipv6, dst,
> >  				  sizeof(struct in6_addr), reg);
> > +		nft_flow_rule_set_addr_type(flow, FLOW_DISSECTOR_KEY_IPV6_ADDRS);
> >  		break;
> >  	case offsetof(struct ipv6hdr, nexthdr):
> >  		if (priv->len != sizeof(__u8))
> 
> Still worries me this is done in a response to a match.
> 
> skb_flow_dissector_init() has a straight up BUG_ON() if the dissector
> did not set the CONTROL or BASIC. It says in the comment that both must
> be initialized. But nft does not call skb_flow_dissector_init()?
> 
> Are you 100% sure all cases will set CONTROL and BASIC now?

Enforcing skb_flow_dissector_init() for software make sense, but in
Netfilter this is used for hardware offload only.

All drivers in the tree check for:

        if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL))

before accessing struct flow_match_control.

I can set it on inconditionally, but the driver will get a value 0x0
and mask 0x0, which is the same as leaving it unset.