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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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@Ed-Cashins-MacBook-Pro vblade-21$ 

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

  https://github.com/ecashin/vblade

On May 19, 2014, at 4:06 PM, Catalin Salgau <csalgau@xxxxxxxxxxxxxxxxxxxxx> wrote:

> 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 <csalgau@xxxxxxxxxxxxxxxxxxxxx> 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
>>> Aoetools-discuss@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.sourceforge.net/lists/listinfo/aoetools-discuss
>> 
>> 
> 
> ------------------------------------------------------------------------------
> "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
> Aoetools-discuss@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/aoetools-discuss


------------------------------------------------------------------------------
"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
Aoetools-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/aoetools-discuss




[Index of Archives]     [Linux ARM Kernel]     [Linux SCSI]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux