Naga Narayanaswamy wrote:
>
> 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.
interesting.. I looked in RELENG_3. it must have been changed after 3.4
I'm also surprised that with the change in MINCLSIZE
the drivers don't do the right thing.. :-)
>
> > 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.
OOPS!
I guess we should just reassign them.
to the new packet..
> 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...
GOOD WORK!
so the total patched code looks like:
> > }
> > }
> > }
> > 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);
> > }
/* re-set the variables that pointed within
the mbuf */
> wh = mtod(m, struct pppoe_full_hdr *);
> ph = &wh->ph;
> session = ntohs(wh->ph.sid);
> length = ntohs(wh->ph.length);
> code = wh->ph.code;
> > }
> > switch(code) {
> > case PADI_CODE:
>
> 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?
The "other place" doesn't care if it's fragmented as it gets sent to user-space
(the ppp daemon). Only the ETHERTYPE_PPPOE_DISC case needs the data to be
contiguous.
It's a waste of cpu to make a cluster if it is not ETHERTYPE_PPPOE_DISC.
> 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.
definitly..
Can you send me a diff of your patch(s)? (just so I can be sure of
what is tested), then I'll check it in..
>
> Thanks!
> Naga Narayanaswamy
> [EMAIL PROTECTED]
--
__--_|\ Julian Elischer
/ \ [EMAIL PROTECTED]
( OZ ) World tour 2000
---> X_.---._/ from Perth, presently in: Budapest
v
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-net" in the body of the message
