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@xxxxxxxxxxxxx>
> > ---
> > 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.
