netdev - Re: [PATCH 3/3] gianfar: remove faulty filer optimizer

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]
Message-ID: <20150811165109.0601035b@north>
Date:	Tue, 11 Aug 2015 16:51:09 +0200
From:	Jakub Kiciński <moorray3@...pl>
To:	Manoil Claudiu <claudiu.manoil@...escale.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Jakub Kicinski <kubakici@...pl>
Subject: Re: [PATCH 3/3] gianfar: remove faulty filer optimizer

On Tue, 11 Aug 2015 14:00:23 +0000, Manoil Claudiu wrote:
> >-----Original Message-----
> >From: Jakub Kicinski [mailto:moorray3@...pl]
> >Sent: Monday, August 10, 2015 11:12 PM
> >To: David S. Miller; Manoil Claudiu-B08782
> >Cc: netdev@...r.kernel.org; Jakub Kicinski
> >Subject: [PATCH 3/3] gianfar: remove faulty filer optimizer
> >
> >From: Jakub Kicinski <kubakici@...pl>
> >
> >Current filer rule optimization is broken in several ways:
> > (1) It destroys rule ordering.
> > (2) It performs reads/writes beyond end of allocated tables.
> > (3) It breaks badly for rules with more than 2 specifiers
> >     (e.g. matching ip, port, tos).
> > (4) We observed that the masking rules it generates do not
> >     play well with clustering on P2020.  Only first rule
> >     of the cluster would ever fire.  Given that optimizer
> >     relies heavily on masking this is very hard to fix.
> >
> >The fact that nobody noticed (1), (3) or (4) makes me think
> >that this feature is not very widely used and we should just
> >remove it.
> 
> I'm not familiar with this filer classification code and its
> author is no longer active apparently.   There is not much of a
> choice here since this optimization feature is too complex and
> poorly documented to be reviewed and validated in a reasonable
> time span.
> An example, a simple use case showing expected behavior vs.
> actual behavior would help.

Sure, sorry, should be part of the submission quite honestly...
Quick note: I'm using virtual queues hence the high action numbers.

(1) Is actually false, I misread the code.

(2) To my eye calculation at gianfar_ethtool.c:1326 should be:
    tab->index - begin + 1, haven't tested this though.

(3) Example looks like this:

# ethtool -N eth2 flow-type udp4 dst-ip 10.0.0.1 dst-port 1 tos 1 action 1
Added rule with ID 254
# ethtool -N eth2 flow-type udp4 dst-ip 10.0.0.2 dst-port 2 tos 2 action 9
Added rule with ID 253
# ethtool -N eth2 flow-type udp4 dst-ip 10.0.0.3 dst-port 3 tos 3 action 17
Added rule with ID 252
# ./filer_decode /sys/kernel/debug/gfar1/filer_raw 
00: MASK == 00000210 AND         Q:00           ctrl:00000080 prop:00000210
01: FPR  == 00000210 AND CLE     Q:00           ctrl:00000281 prop:00000210
02: MASK == ffffffff AND         Q:00           ctrl:00000080 prop:ffffffff
03: DPT  == 00000003 AND         Q:00           ctrl:0000008e prop:00000003
04: TOS  == 00000003 AND         Q:00           ctrl:0000008a prop:00000003
05: DIA  == 0a000003 AND         Q:11           ctrl:0000448c prop:0a000003
06: DPT  == 00000002 AND         Q:00           ctrl:0000008e prop:00000002
07: TOS  == 00000002 AND         Q:00           ctrl:0000008a prop:00000002
08: DIA  == 0a000002 AND         Q:09           ctrl:0000248c prop:0a000002
09: DIA  == 0a000001 AND         Q:00           ctrl:0000008c prop:0a000001
0a: DPT  == 00000001 AND         Q:00           ctrl:0000008e prop:00000001
0b: TOS  == 00000001     CLE     Q:01           ctrl:0000060a prop:00000001
ff: MASK >= 00000000             Q:00           ctrl:00000020 prop:00000000

You can see that all rules are AND-ed together for no reason.  I found
this one during code inspections but can't recall where the bug was now...

(4) Example:

# ethtool -N eth2 flow-type udp4 dst-ip 10.0.0.1 dst-port 1  action 1
Added rule with ID 254
# ethtool -N eth2 flow-type udp4 dst-ip 10.0.0.2 dst-port 2  action 9
Added rule with ID 253
# ethtool -N eth2 flow-type udp4 dst-ip 10.0.0.3 dst-port 3  action 17
Added rule with ID 252
# ./filer_decode /sys/kernel/debug/gfar1/filer_raw 
00: MASK == 00000210 AND         Q:00           ctrl:00000080 prop:00000210
01: FPR  == 00000210 AND CLE     Q:00           ctrl:00000281 prop:00000210
02: MASK == ffffffff AND         Q:00           ctrl:00000080 prop:ffffffff
03: DPT  == 00000003 AND         Q:00           ctrl:0000008e prop:00000003
04: DIA  == 0a000003             Q:11           ctrl:0000440c prop:0a000003
05: DPT  == 00000002 AND         Q:00           ctrl:0000008e prop:00000002
06: DIA  == 0a000002             Q:09           ctrl:0000240c prop:0a000002
07: DIA  == 0a000001 AND         Q:00           ctrl:0000008c prop:0a000001
08: DPT  == 00000001     CLE     Q:01           ctrl:0000060e prop:00000001
ff: MASK >= 00000000             Q:00           ctrl:00000020 prop:00000000

Which looks correct according to the spec but only the first (eth id 252)/
last added rule for 10.0.0.3 will ever trigger.  As if filer did not treat
the AND CLE as cluster start but also kept AND-ing the rules.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html