Re: [2/2]: ppp_mppe inclusion

[Oleg Makarenko <mole@quadra.ru>]

[-- Attachment #1: Type: text/plain, Size: 3057 bytes --]

Matt Domsch wrote:

>On Tue, Jul 20, 2004 at 03:45:40PM -0500, Matt Domsch wrote:
>  
>
>>This mail will be followed by two patches, for comment on proposed
>>solution.  I've compiled this, but am not able to test this week.
>>    
>>

Great work, Matt. Thank you for your patches. Any chances to see them 
integrated in 2.6 any time soon?

The only problem is that they do not work for me and never did :) I get 
kernel panic with your patches and wonder how others don't get the same 
result?

Please find attached somewhat (I hope) improved patches that finaly do 
work for me

The problems with your original patches:

1. setup_sg(). Do you really need to split the data this way? The 
documentation on crypto api and scatterlists is not very helpful so I 
could be wrong here but in my reading  of crypto/digest.c/update() (for 
ex)  you may just have a single sg[0] even if the data doesn't fit into 
a single page.  All pages just need to be contiguous. It probably 
doesn't hirt but is it really needed?  I have removed this split from 
your patch just to test it. Seems to work fine.

2.  For some reason you can not use non GFP_KERNEL memory and scatter 
lists or at least mix them in crypto_digest().  That is why sha_pad is 
now in struct state {}.

3.  In get_new_key_from_sha() the code like 

   crypto_digest_update(state->sha1, sg, state->keylen)

looks suspicious as the last parameter (in my reading of digest.c) 
should be the number of elements in sg[] array not the data length. That 
seems to be the reason for my kernel panics. With your setup_sg() the 
last parameter should be 1 or 2. Or may be you could replace all four 
digest_update calls with a single call with properly initilaized sg[4] 
(to make some use from scatterlist) . See the modified patch for details.

4. in ppp_generic.c/pad_compress_skb() the following code:

       } else if (len == 0) {
               /* didn't compress, or CCP not up yet */
               kfree_skb(new_skb);
               new_skb = NULL;
               ...
        return new_skb;

also looks suspicious as later I read

               skb = pad_compress_skb(ppp, skb);
               if (!skb)
                       goto drop;

I think you don't want to drop packets with len == 0.  At least the 
previous mppe enabled ppp_generic.c code don't do that.  I've changed 
new_skb = NULL above to new_skb = skb, see the patch. IPCP can not be 
established without this modification.

Please try these patches and let me know your results. Hope they can be 
useful for you.

And btw,
What do think on crypto api in kernel? It looks like a performance 
killer for the small size block ciphers and especially for the arc4 
(with its single byte block), doesn't it? Or have I missed something?
Why do we need so many calls just to encrypt a single byte (see the for 
loop in crypto/cipher.c/crypt())?
Does it make any sense to cond_resched() for every encrypted (i.e. for 
every sent) byte (as crypt() does for arc4)?
Some benchmarks against this new patches would be very interesting.

=oleg


[-- Attachment #2: ppp_mppe1-moleg.patch.gz --]
[-- Type: application/x-gzip, Size: 1565 bytes --]

[-- Attachment #3: ppp_mppe2-moleg.patch.gz --]
[-- Type: application/x-gzip, Size: 8035 bytes --]