[Aoetools-discuss] [PATCH] Incorrect shelf/slot handling in BPF

Discussion:

Catalin Salgau

2014-05-19 19:11:41 UTC

The BPF filter program currently included in vblade requires that the
major and minor fields in a packet header either
- match server's major and minor addresses or
- be both all ones (0xffff and 0xff respectively)
This is against the AoE specification that requires that the two fields
be tested separately. (as seen in aoe.c:368)
Proposed patch corrects this.

diff --git a/vblade/bpf.c b/vblade/bpf.c
--- a/vblade/bpf.c
+++ b/vblade/bpf.c
@@ -82,32 +82,27 @@
{
struct bpf_program *bpf_program;
struct bpf_insn insns[] = {
- /* Load the type into register */
+ /* CHECKTYPE: Load the type into register */
BPF_STMT(BPF_LD+BPF_H+BPF_ABS, 12),
/* Does it match AoE Type (0x88a2)? No, goto INVALID */
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0x88a2, 0, 12),
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0x88a2, 0, 10),
/* Load the flags into register */
BPF_STMT(BPF_LD+BPF_B+BPF_ABS, 14),
/* Check to see if the Resp flag is set */
BPF_STMT(BPF_ALU+BPF_AND+BPF_K, Resp),
/* Yes, goto INVALID */
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0, 0, 9),
- /* Load the shelf number into register */
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0, 0, 7),
+ /* CHECKSHELF: Load the shelf number into register */
BPF_STMT(BPF_LD+BPF_H+BPF_ABS, 16),
- /* Does it match shelf number? No, goto CHECKBROADCAST */
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, shelf, 0, 2),
- /* Load the slot number into register */
+ /* Does it match shelf number? Yes, goto CHECKSLOT */
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, shelf, 1, 0),
+ /* Does it match broadcast? No, goto INVALID */
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0xffff, 0, 4),
+ /* CHECKSLOT: Load the slot number into register */
BPF_STMT(BPF_LD+BPF_B+BPF_ABS, 18),
/* Does it match shelf number? Yes, goto VALID */
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, slot, 4, 0),
- /* CHECKBROADCAST: is (shelf, slot) == (0xffff, 0xff)? */
- /* Load the shelf number into register */
- BPF_STMT(BPF_LD+BPF_H+BPF_ABS, 16),
- /* Is it 0xffff? No, goto INVALID */
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0xffff, 0, 3),
- /* Load the slot number into register */
- BPF_STMT(BPF_LD+BPF_B+BPF_ABS, 18),
- /* Is it 0xff? No, goto INVALID */
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, slot, 1, 0),
+ /* Does it match broadcast? No, goto INVALID */
BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0xff, 0, 1),
/* VALID: return -1 (allow the packet to be read) */
BPF_STMT(BPF_RET+BPF_K, -1),

Ed Cashin

2014-05-19 19:22:57 UTC

Permalink

I think you mean that a vblade that's AoE 11.22 should answer AoE commands sent to 0xffff.2 or 11.0xff but does not currently.

It would be very nice if we had a collection of tests, so that you could also include a test that showed what this fix corrects and validated the fix itself. Alas. Still, it would be good to hear about how the fix was tested if you don't want to take a stab at adding such a test.

Post by Catalin Salgau
The BPF filter program currently included in vblade requires that the
major and minor fields in a packet header either
- match server's major and minor addresses or
- be both all ones (0xffff and 0xff respectively)
This is against the AoE specification that requires that the two fields
be tested separately. (as seen in aoe.c:368)
Proposed patch corrects this.
diff --git a/vblade/bpf.c b/vblade/bpf.c
--- a/vblade/bpf.c
+++ b/vblade/bpf.c
@@ -82,32 +82,27 @@
{
struct bpf_program *bpf_program;
struct bpf_insn insns[] = {
- /* Load the type into register */
+ /* CHECKTYPE: Load the type into register */
BPF_STMT(BPF_LD+BPF_H+BPF_ABS, 12),
/* Does it match AoE Type (0x88a2)? No, goto INVALID */
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0x88a2, 0, 12),
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0x88a2, 0, 10),
/* Load the flags into register */
BPF_STMT(BPF_LD+BPF_B+BPF_ABS, 14),
/* Check to see if the Resp flag is set */
BPF_STMT(BPF_ALU+BPF_AND+BPF_K, Resp),
/* Yes, goto INVALID */
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0, 0, 9),
- /* Load the shelf number into register */
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0, 0, 7),
+ /* CHECKSHELF: Load the shelf number into register */
BPF_STMT(BPF_LD+BPF_H+BPF_ABS, 16),
- /* Does it match shelf number? No, goto CHECKBROADCAST */
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, shelf, 0, 2),
- /* Load the slot number into register */
+ /* Does it match shelf number? Yes, goto CHECKSLOT */
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, shelf, 1, 0),
+ /* Does it match broadcast? No, goto INVALID */
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0xffff, 0, 4),
+ /* CHECKSLOT: Load the slot number into register */
BPF_STMT(BPF_LD+BPF_B+BPF_ABS, 18),
/* Does it match shelf number? Yes, goto VALID */
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, slot, 4, 0),
- /* CHECKBROADCAST: is (shelf, slot) == (0xffff, 0xff)? */
- /* Load the shelf number into register */
- BPF_STMT(BPF_LD+BPF_H+BPF_ABS, 16),
- /* Is it 0xffff? No, goto INVALID */
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0xffff, 0, 3),
- /* Load the slot number into register */
- BPF_STMT(BPF_LD+BPF_B+BPF_ABS, 18),
- /* Is it 0xff? No, goto INVALID */
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, slot, 1, 0),
+ /* Does it match broadcast? No, goto INVALID */
BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, 0xff, 0, 1),
/* VALID: return -1 (allow the packet to be read) */
BPF_STMT(BPF_RET+BPF_K, -1),
------------------------------------------------------------------------------
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.
Get unparalleled scalability from the best Selenium testing platform available
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
_______________________________________________
Aoetools-discuss mailing list
https://lists.sourceforge.net/lists/listinfo/aoetools-discuss

Catalin Salgau

2014-05-19 20:27:29 UTC

Permalink

And yes. I am aware that testing a patch and not covering the actual
problem should not qualify as testing the patch.
My argument for the validity of the patch is that since the new code
touches and treats all packets equally, having it work in general covers
the corner case that was rejected in the original code.
I'll admit that I'm not entirely certain under what circumstances one
would use this behaviour (maybe an environment where multiple hosts
serve the same image?) or if there are Coraid appliances or HBAs that
actually support this.

I'm not exactly sure how this can be tested without relying on a
second system or packet capture. I'm thinking on the lines of
attaching vblade to lo0 and writing another BPF based program to make
requests, but I believe that would not be very reliable under some
conditions.
Back to the patch. I do not have a testing suite for this. I noticed
the incorrect behaviour when tweaking the code in another direction.
I've taken several looks at the BPF filter over the past two years and
have been running with this change in 'production' for about one, but
I do not actually use the corner case covered by it. Since I'm
isolating patches to post to the mailing list, I have tested this
change against vblade-21 with a full boot-shutdown cycle of Windows 8.
By reusing the same accumulator for both comparisons, the code is
slightly shorter(and marginally faster) and this changes a few offsets.
The present code is roughly equivalent to
goto ACCEPT
goto REJECT
goto REJECT
The patch changes it to something like
goto CHECKSLOT
goto REJECT
goto ACCEPT
goto REJECT
I would encourage anyone wishing to validate the changes to read the
FILTER_MACHINE section in bpf(4) in the FreeBSD man pages. Linux LSF
is supposed to be compatible.

Post by Ed Cashin
I think you mean that a vblade that's AoE 11.22 should answer AoE
commands sent to 0xffff.2 or 11.0xff but does not currently.
It would be very nice if we had a collection of tests, so that you
could also include a test that showed what this fix corrects and
validated the fix itself. Alas. Still, it would be good to hear
about how the fix was tested if you don't want to take a stab at
adding such a test.
On May 19, 2014, at 3:11 PM, Catalin Salgau

Catalin Salgau

2014-05-19 20:06:20 UTC

Permalink

I'm not exactly sure how this can be tested without relying on a second
system or packet capture. I'm thinking on the lines of attaching vblade
to lo0 and writing another BPF based program to make requests, but I
believe that would not be very reliable under some conditions.

Back to the patch. I do not have a testing suite for this. I noticed the
incorrect behaviour when tweaking the code in another direction. I've
taken several looks at the BPF filter over the past two years and have
been running with this change in 'production' for about one, but I do
not actually use the corner case covered by it. Since I'm isolating
patches to post to the mailing list, I have tested this change against
vblade-21 with a full boot-shutdown cycle of Windows 8.
By reusing the same accumulator for both comparisons, the code is
slightly shorter(and marginally faster) and this changes a few offsets.

The present code is roughly equivalent to
if P[16:2] == SHELF:
if P[18] == SLOT:
goto ACCEPT
if P[16:2] != '\xFF\xFF':
goto REJECT
if P[18] != '\xFF':
goto REJECT

The patch changes it to something like
if P[16:2] == SHELF:
goto CHECKSLOT
if P[16:2] != 0xffff:
goto REJECT
if P[18] == SLOT:
goto ACCEPT
if P[18] != 0xff:
goto REJECT

I would encourage anyone wishing to validate the changes to read the
FILTER_MACHINE section in bpf(4) in the FreeBSD man pages. Linux LSF is
supposed to be compatible.

Post by Ed Cashin
I think you mean that a vblade that's AoE 11.22 should answer AoE commands sent to 0xffff.2 or 11.0xff but does not currently.
It would be very nice if we had a collection of tests, so that you could also include a test that showed what this fix corrects and validated the fix itself. Alas. Still, it would be good to hear about how the fix was tested if you don't want to take a stab at adding such a test.

Ed Cashin

2014-05-20 01:54:26 UTC

Permalink

You're right that it's not exactly straightforward to use existing tools to test this case.

I suppose the easiest way I can think of is to use a version of aoeping that has been modified to burp out the right kind of ethernet packet. You could even check for the response with tcpdump if you didn't want to use aoeping itself.

I tried to apply it, but your emailer might have mangled the patch.

***@Ed-Cashins-MacBook-Pro vblade-21$ patch -p2 < ~/tmp/bpf.eml
(Stripping trailing CRs from patch.)
patching file bpf.c
Hunk #1 FAILED at 82.
1 out of 1 hunk FAILED -- saving rejects to file bpf.c.rej
***@Ed-Cashins-MacBook-Pro vblade-21$

Can you use github and send a pull request against my git tree?

https://github.com/ecashin/vblade

Post by Catalin Salgau
I'm not exactly sure how this can be tested without relying on a second
system or packet capture. I'm thinking on the lines of attaching vblade
to lo0 and writing another BPF based program to make requests, but I
believe that would not be very reliable under some conditions.
Back to the patch. I do not have a testing suite for this. I noticed the
incorrect behaviour when tweaking the code in another direction. I've
taken several looks at the BPF filter over the past two years and have
been running with this change in 'production' for about one, but I do
not actually use the corner case covered by it. Since I'm isolating
patches to post to the mailing list, I have tested this change against
vblade-21 with a full boot-shutdown cycle of Windows 8.
By reusing the same accumulator for both comparisons, the code is
slightly shorter(and marginally faster) and this changes a few offsets.
The present code is roughly equivalent to
goto ACCEPT
goto REJECT
goto REJECT
The patch changes it to something like
goto CHECKSLOT
goto REJECT
goto ACCEPT
goto REJECT
I would encourage anyone wishing to validate the changes to read the
FILTER_MACHINE section in bpf(4) in the FreeBSD man pages. Linux LSF is
supposed to be compatible.

Post by Ed Cashin
I think you mean that a vblade that's AoE 11.22 should answer AoE commands sent to 0xffff.2 or 11.0xff but does not currently.
It would be very nice if we had a collection of tests, so that you could also include a test that showed what this fix corrects and validated the fix itself. Alas. Still, it would be good to hear about how the fix was tested if you don't want to take a stab at adding such a test.

------------------------------------------------------------------------------
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.
Get unparalleled scalability from the best Selenium testing platform available
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
_______________________________________________
Aoetools-discuss mailing list
https://lists.sourceforge.net/lists/listinfo/aoetools-discuss