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

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

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.

On 19/05/2014 10:22 PM, Ed Cashin wrote:
> 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 <cs...@us...> wrote:
>
>> 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
>> Aoe...@li...
>> https://lists.sourceforge.net/lists/listinfo/aoetools-discuss
>
>