Thanks Julian for that quick reply.
My follow up on your untested patch follows.
Please see my observations inline.
> They should be comaring the packet size with
> #define MINCLSIZE (MHLEN + 1) /* smallest amount to put in
cluster */
First, I found that MINCLSIZE was (MHLEN+MLEN) on FreeBsd-3.4-RELASE code
on my machine, and I changed it to (MHLEN+1) as it in 4.0+ release for the
"untested"
patch to work.
> Of the options, it looks to me like
> allocating a cluster page (2KB) and puting the packet in there
> is the best, as it can then be linked to the mbufs and
> nothing else needs to change.
>
> Or you could try this (untested) patch.
> }
> }
> }
> if (m->m_len != m->m_pkthdr.len) {
> /*
> * It's not all in one piece.
> * We need to do extra work.
> * Put it into a cluster.
> */
> struct mbuf *n;
> n = m_dup(m, M_DONTWAIT);
> m_freem(m);
> m = n;
> if (m) {
> /* just check we got a cluster */
> if (m->m_len != m->m_pkthdr.len) {
> m_freem(m);
> m = NULL;
> }
> }
> id (m == NULL) {
> printf("packet fragmented\n");
> LEAVE(EMSGSIZE);
> }
> }
> switch(code) {
> case PADI_CODE:
After the patch, the cluster gets the correct length (106 in my case), and
data is
there in it, but the PPPoE header, and full header (ph, wh)
were assigned from the mbuf BEFORE we made the cluster.
So the PADO processing still was using old mbuf contents.
See the PADR constructed by the pppoe client having a partial AC-Name.
07:30:41.687833 PPPoE PADR v1, type 1, sess 0 len 61 [Service-Name
mindspring.
com] [AC-Name
34313032313033303034363031312d7265646261636bd3010000000084e421]
[Host-Uniq 008daec0]
1119 0000 003d 0101 000e 6d69 6e64 7370
7269 6e67 2e63 6f6d 0102 001f 3431 3032
3130 3330 3034 3630 3131 2d72 6564 6261
636b d301 0000 0000 84e4 2101 0300 0400 /* Note:
NULL chars in AC-Name */
8dae c0
In fact AC-Name, was 41021030046011-redback \0\0\0 ....,
and tcpdump is confused I guess.
So, I copied the code to cast pppoe_full_hdr and "assign values after
making the cluster" also and then the PADR response is correct.
To be on the safe side to test, I copied all 5 lines from a previous code
segment.
wh = mtod(m, struct pppoe_full_hdr *);
ph = &wh->ph;
session = ntohs(wh->ph.sid);
length = ntohs(wh->ph.length);
code = wh->ph.code;
Then it is ok. Whoo...
09:25:39.856769 PPPoE PADR v1, type 1, sess 0 len 61 [Service-Name
mindspring.c
om] [AC-Name 41021030046011-redback-1.norva3] [Host-Uniq 8002adc0]
1119 0000 003d 0101 000e 6d69 6e64 7370
7269 6e67 2e63 6f6d 0102 001f 3431 3032
3130 3330 3034 3630 3131 2d72 6564 6261
636b 2d31 2e6e 6f72 7661 3301 0300 0480
02ad c0
So my question is :
If I put the code to make a cluster in the outer level if statement
if (hook->private == &privp->ethernet_hook) { ... }
and then assign wh and ph, will it be ok? I am not exactly sure of the code
flow.
If the ether_type is not ETHERTYPE_PPPOE_DISC it goes to another place
and I dont know all ramifications. Basically the question is when should we
make
the cluster if required?
I feel this solution is a better in long term than relying on the MBUF size
of 256 as in 4.0 as some ISP's may send a PADO response greater than 256.
Thanks!
Naga Narayanaswamy
[EMAIL PROTECTED]
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
