netdev - Re: [PATCH net-next 0/7] nfp: flower vxlan tunnel offload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 Sep 2017 16:50:10 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Or Gerlitz <gerlitz.or@...il.com>, Jiri Benc <jbenc@...hat.com>
Cc:     Simon Horman <simon.horman@...ronome.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Linux Netdev List <netdev@...r.kernel.org>,
        oss-drivers@...ronome.com, John Hurley <john.hurley@...ronome.com>,
        Paul Blakey <paulb@...lanox.com>,
        Jiri Pirko <jiri@...lanox.com>, Roi Dayan <roid@...lanox.com>
Subject: Re: [PATCH net-next 0/7] nfp: flower vxlan tunnel offload

On Tue, 2017-09-26 at 17:17 +0300, Or Gerlitz wrote:
> On Tue, Sep 26, 2017 at 3:51 PM, Jiri Benc <jbenc@...hat.com> wrote:
> > On Tue, 26 Sep 2017 15:41:37 +0300, Or Gerlitz wrote:
> > > Please note that the way the rule is being set to the HW driver is by delegation
> > > done in flower, see these commits (specifically "Add offload support
> > > using egress Hardware device")
> > 
> > It's very well possible the bug is somewhere in net/sched.
> 
> maybe before/instead you call it a bug, take a look on the design
> there and maybe
> tell us how to possibly do that otherwise?

The problem, AFAICT, is in the API between flower and NIC implementing
the offload, because in the above example the kernel will call the
offload hook with exactly the same arguments with the 'bad' rule and
the 'good' one - but the 'bad' rule should never match any packets.

I think that can be fixed changing the flower code to invoke the
offload hook for filters with tunnel-based match only if the device
specified in such match has the appropriate type, e.g. given that
currently only vxlan is supported with something like the code below
(very rough and untested, just to give the idea):

Cheers,

Paolo

---
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index d230cb4c8094..ff8476e56d4e 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -243,10 +243,11 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
                                struct fl_flow_key *mask,
                                struct cls_fl_filter *f)
 {
-       struct net_device *dev = tp->q->dev_queue->dev;
+       struct net_device *ingress_dev, *dev = tp->q->dev_queue->dev;
        struct tc_cls_flower_offload cls_flower = {};
        int err;
 
+       ingress_dev = dev;
        if (!tc_can_offload(dev)) {
                if (tcf_exts_get_dev(dev, &f->exts, &f->hw_dev) ||
                    (f->hw_dev && !tc_can_offload(f->hw_dev))) {
@@ -259,6 +260,12 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
                f->hw_dev = dev;
        }
 
+       if ((dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_ENC_KEYID) ||
+            dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_ENC_PORTS) ||
+             // ... list all the others tunnel based keys ...
+             ) && strcmp(ingress_dev->rtnl_link_ops->kind, "vxlan"))
+               return tc_skip_sw(f->flags) ? -EINVAL : 0;
+